[Media] Implement loading attribute for video and audio elements [chromium/src : main]
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
I haven't reviewed yet, but a couple of high level notes:
- You probably want to send out an Intent to Ship for this
- The feature needs to be behind a flag
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 1 comment![]( "Open in Gerrit")
bool poster_deferred_for_lazy_load_ : 1;You probably also want to make sure https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/parser/html_preload_scanner.cc;l=579?q=HTMLPreloadScanner&ss=chromium%2Fchromium%2Fsrc is aware of the loading attribute. Otherwise, the resource will be loaded anyway if the tag is part of the initial HTML.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 2 comments![]( "Open in Gerrit")
// Copyright 2025 The Chromium Authors2026..
return settings->GetLazyLoadingMediaMarginPx2G();Can you help me understand where this is defined?
Helmut Januschka (Gerrit)
Helmut Januschka added 4 comments![]( "Open in Gerrit")
You probably also want to make sure https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/parser/html_preload_scanner.cc;l=579?q=HTMLPreloadScanner&ss=chromium%2Fchromium%2Fsrc is aware of the loading attribute. Otherwise, the resource will be loaded anyway if the tag is part of the initial HTML.
done, thanks for the hint! also added a WPT to validate
// Copyright 2025 The Chromium AuthorsHelmut Januschka2026..
Done
return settings->GetLazyLoadingMediaMarginPx2G();Can you help me understand where this is defined?
they are defined in `settings.json5`. getters are auto generated, following the same pattern as the existing lazy-loading image margins here (thought it would make sense to have own settings)
Related details
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 10 comments![]( "Open in Gerrit")
Are these values matching what we're doing for images/iframes?
void LoadDeferredMediaInternal();The Internal one should be protected/private, right?
// If the loading attribute is changed to eager while deferred, load nowNit: period (.) at the end of the comment.
LoadDeferredMedia();I'm not super familiar with media element loading. Can you describe the high level flow here? Who loads the media when there's no loading attribute, or when the flag is disabled?
LoadDeferredMedia();The name `LoadDeferredMedia` is not ideal here, as the media is not deferred..
GetDocument().GetAgent().event_loop()->EnqueueMicrotask(BindOnce(Is this specified?
bool poster_deferred_for_lazy_load_ : 1;Can we appease ClangTidy here? (I'm not sure we can, given the `: 1` and our language support limitations)
return;The bots claim that this is not covered by tests. Is it?
status: "test",I think it's fair to make this "experimental", to make it easier for interested developers to test this
"prefix": "lazy-load-media",I don't think the virtual test suite is really needed: https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/RuntimeEnabledFeatures.md#:~:text=test,Yes
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 10 comments![]( "Open in Gerrit")
Are these values matching what we're doing for images/iframes?
yes they match the images ones, keeping them separate still makes sense i guess to be able to separatly tune in the future!
The Internal one should be protected/private, right?
Done
// If the loading attribute is changed to eager while deferred, load nowNit: period (.) at the end of the comment.
Done
LoadDeferredMedia();The name `LoadDeferredMedia` is not ideal here, as the media is not deferred..
how about EnsureMediaLoading?
I'm not super familiar with media element loading. Can you describe the high level flow here? Who loads the media when there's no loading attribute, or when the flag is disabled?
**Normal flow (flag disabled or no loading attr):**
`load()` -> `LoadInternal()` -> `SelectMediaResource()` proceeds immediately with network loading.
**With lazy loading enabled + loading="lazy":**
At the start of `SelectMediaResource()`, we check `LazyMediaHelper::ShouldDeferMediaLoad()`. If true, we set state to `kDeferred`, start IntersectionObserver monitoring, and return early (no network request yet).
When the element becomes visible (or loading attr changes to "eager"), `EnsureMediaLoading()` resumes `SelectMediaResource()` to do the actual load.
GetDocument().GetAgent().event_loop()->EnqueueMicrotask(BindOnce(Is this specified?
The microtask posting follows the same pattern as lazy-loaded images (to avoid re-entrancy when called from IntersectionObserver callbacks or during attribute parsing). havent done a spec update so far, where should i add this? or should i ping scott about it?
Can we appease ClangTidy here? (I'm not sure we can, given the `: 1` and our language support limitations)
well it does not work! also it would break existign patterns with default initialized fields. so guess we'd need to keep it as is.
The bots claim that this is not covered by tests. Is it?
added tests! should be covered now
I think it's fair to make this "experimental", to make it easier for interested developers to test this
Done
I don't think the virtual test suite is really needed: https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/RuntimeEnabledFeatures.md#:~:text=test,Yes
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 2 comments![]( "Open in Gerrit")
LoadDeferredMedia();Helmut JanuschkaThe name `LoadDeferredMedia` is not ideal here, as the media is not deferred..
how about EnsureMediaLoading?
Your flow explanation actually convinced me that `LoadDeferredMedia` (or `LoadDeferredMediaIfNeeded`) is actually better 😊
Apologies for the noise!
GetDocument().GetAgent().event_loop()->EnqueueMicrotask(BindOnce(Helmut JanuschkaIs this specified?
The microtask posting follows the same pattern as lazy-loaded images (to avoid re-entrancy when called from IntersectionObserver callbacks or during attribute parsing). havent done a spec update so far, where should i add this? or should i ping scott about it?
This should be part of the spec that Scott is pushing.
If this is needed here, it's probably needed in the specification as well (As I'm guessing the spec also relies on IntersectionObserver concepts). Also, this would be observable, so we should make sure it's as interoperable as it can be.
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 3 comments![]( "Open in Gerrit")
also synced the wpt tests from PR
LoadDeferredMedia();Helmut JanuschkaThe name `LoadDeferredMedia` is not ideal here, as the media is not deferred..
Yoav Weiss (@Shopify)how about EnsureMediaLoading?
Your flow explanation actually convinced me that `LoadDeferredMedia` (or `LoadDeferredMediaIfNeeded`) is actually better 😊
Apologies for the noise!
Done
GetDocument().GetAgent().event_loop()->EnqueueMicrotask(BindOnce(Helmut JanuschkaIs this specified?
Yoav Weiss (@Shopify)The microtask posting follows the same pattern as lazy-loaded images (to avoid re-entrancy when called from IntersectionObserver callbacks or during attribute parsing). havent done a spec update so far, where should i add this? or should i ping scott about it?
This should be part of the spec that Scott is pushing.
If this is needed here, it's probably needed in the specification as well (As I'm guessing the spec also relies on IntersectionObserver concepts). Also, this would be observable, so we should make sure it's as interoperable as it can be.
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yoav Weiss (@Shopify) (Gerrit)
Yoav Weiss (@Shopify) added 4 comments![]( "Open in Gerrit")
Dale - the overall approach seems reasonable to me, but can you take a closer look at the media loading bits? I think you're more familiar with them than me 😊
return;Is this covered by tests? The bots are complaining..
poster_deferred_for_lazy_load_ = true;Is this covered by tests?
return nullptr;- Dale Curtis
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 8 comments![]( "Open in Gerrit")
if (loading == LoadingAttributeValue::kEager) {Did you want to check that the old_value wasn't already eager?
// Check for lazy loading - defer resource selection if loading=lazyThis could use some more explanation. The header suggest that LazyMediaLoadState::kNone means lazy loading isn't used, so some notes on why we're switching to lazy loading here would be helpful.
if (lazy_media_load_state_ != LazyMediaLoadState::kDeferred) {DCHECK runtime feature is enabled?
// Check that we're still in the right state (element may have been removedDCHECk feature enabled.
bool poster_deferred_for_lazy_load_ : 1;Please fix this WARNING reported by ClangTidy: check: modernize-use-default-member-init
use default member initializer for 'po...
check: modernize-use-default-member-init
use default member initializer for 'poster_deferred_for_lazy_load_' (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-default-member-init` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
!FastHasAttribute(html_names::kSrcAttr) && GetDocument().GetFrame()) {Do you also need to check that it has no source children?
if (!FastHasAttribute(html_names::kSrcAttr) && GetDocument().GetFrame()) {Ditto on source elements
if (!lazy_load_intersection_observer_) {We already have a couple other intersection observers attached to the video element. I'm wondering if we should be sharing these instead of adding new ones. This does use params the others don't though.
https://source.chromium.org/search?ss=chromium&q=IntersectionObserver::Create%20file:media%2F
We even already have one called lazy load intersection observer:
We should at least differentiate between that last one.
IIRC, each of these has a non-trivial cost. @bk...@google.com for his thoughts since he implemented MediaVideoVisibilityTracker.
Related details
- Benjamin Keen
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benjamin Keen (Gerrit)
Benjamin Keen added 1 comment![]( "Open in Gerrit")
if (!lazy_load_intersection_observer_) {We already have a couple other intersection observers attached to the video element. I'm wondering if we should be sharing these instead of adding new ones. This does use params the others don't though.
https://source.chromium.org/search?ss=chromium&q=IntersectionObserver::Create%20file:media%2F
We even already have one called lazy load intersection observer:
We should at least differentiate between that last one.
IIRC, each of these has a non-trivial cost. @bk...@google.com for his thoughts since he implemented MediaVideoVisibilityTracker.
I am not too concerned here. The cost increases significantly when tracking visibility, and this observer is not created using the `track_visibility` param.
I also share Dale's concern regarding the various intersection observers we have. For the scope of this CL, I wonder if we could just have one observer for image/media lazy load, since the [lazy_load_image_observer](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/lazy_load_image_observer.h;drc=899b3f1c8d928409caee3a12d17c62691fec793c) is very similar to this one.
Related details
- Helmut Januschka
Helmut Januschka (Gerrit)
Helmut Januschka voted and added 11 comments![]( "Open in Gerrit")
Votes added by Helmut Januschka
| Commit-Queue | +1 |
11 comments
Did you want to check that the old_value wasn't already eager?
Done. Added a check that the old value wasn't already eager before calling LoadDeferredMediaIfNeeded().
// Check for lazy loading - defer resource selection if loading=lazyThis could use some more explanation. The header suggest that LazyMediaLoadState::kNone means lazy loading isn't used, so some notes on why we're switching to lazy loading here would be helpful.
Done. Expanded the comment to explain that kNone is the initial state before any lazy loading decision.
if (lazy_media_load_state_ != LazyMediaLoadState::kDeferred) {DCHECK runtime feature is enabled?
Done
// Check that we're still in the right state (element may have been removedHelmut JanuschkaDCHECk feature enabled.
Done
Please fix this WARNING reported by ClangTidy: check: modernize-use-default-member-init
use default member initializer for 'po...
check: modernize-use-default-member-init
use default member initializer for 'poster_deferred_for_lazy_load_' (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-default-member-init` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Done
!FastHasAttribute(html_names::kSrcAttr) && GetDocument().GetFrame()) {Do you also need to check that it has no source children?
Done.
Is this covered by tests? The bots are complaining..
Done. The new `PosterDeferredViaUpdatePosterImageInternal` test in html_video_element_test.cc covers this path.
Is this covered by tests?
Done. The `PosterDeferredViaUpdatePosterImageInternal` test covers `UpdatePosterImageInternal()`
if (!FastHasAttribute(html_names::kSrcAttr) && GetDocument().GetFrame()) {Helmut JanuschkaDitto on source elements
Done
Benjamin KeenWe already have a couple other intersection observers attached to the video element. I'm wondering if we should be sharing these instead of adding new ones. This does use params the others don't though.
https://source.chromium.org/search?ss=chromium&q=IntersectionObserver::Create%20file:media%2F
We even already have one called lazy load intersection observer:
We should at least differentiate between that last one.
IIRC, each of these has a non-trivial cost. @bk...@google.com for his thoughts since he implemented MediaVideoVisibilityTracker.
I am not too concerned here. The cost increases significantly when tracking visibility, and this observer is not created using the `track_visibility` param.
I also share Dale's concern regarding the various intersection observers we have. For the scope of this CL, I wonder if we could just have one observer for image/media lazy load, since the [lazy_load_image_observer](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/lazy_load_image_observer.h;drc=899b3f1c8d928409caee3a12d17c62691fec793c) is very similar to this one.
good idea, merged them into LazyLoadMediaObserver with one IntersectionObserver per document, handling images, video, and audio. Deleted LazyLoadImageObserver entirely. Reusing the existing image margin settings for all media types.
Is this covered by tests?
Done. Added `LazyLoadVideoPoster` test in `html_preload_scanner_test.cc` that verifies the preload scanner skips video poster preloads when `loading="lazy"` is set, and allows them for eager/auto/no-attribute cases.
Related details
- Benjamin Keen
- Dale Curtis
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 4 comments![]( "Open in Gerrit")
bool poster_deferred_for_lazy_load_ : 1;Helmut JanuschkaPlease fix this WARNING reported by ClangTidy: check: modernize-use-default-member-init
use default member initializer for 'po...
check: modernize-use-default-member-init
use default member initializer for 'poster_deferred_for_lazy_load_' (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-default-member-init` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Done
I'd go ahead and fix all the nearby ones too, but up to you.
!FastHasAttribute(html_names::kSrcAttr) && GetDocument().GetFrame()) {Helmut JanuschkaDo you also need to check that it has no source children?
Done.
I'm surprised we don't have a HasSources() type method already. Since you use this in a couple places, a helper function to share this logic would be helpful.
if (!lazy_load_intersection_observer_) {Benjamin KeenWe already have a couple other intersection observers attached to the video element. I'm wondering if we should be sharing these instead of adding new ones. This does use params the others don't though.
https://source.chromium.org/search?ss=chromium&q=IntersectionObserver::Create%20file:media%2F
We even already have one called lazy load intersection observer:
We should at least differentiate between that last one.
IIRC, each of these has a non-trivial cost. @bk...@google.com for his thoughts since he implemented MediaVideoVisibilityTracker.
Helmut JanuschkaI am not too concerned here. The cost increases significantly when tracking visibility, and this observer is not created using the `track_visibility` param.
I also share Dale's concern regarding the various intersection observers we have. For the scope of this CL, I wonder if we could just have one observer for image/media lazy load, since the [lazy_load_image_observer](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/lazy_load_image_observer.h;drc=899b3f1c8d928409caee3a12d17c62691fec793c) is very similar to this one.
good idea, merged them into LazyLoadMediaObserver with one IntersectionObserver per document, handling images, video, and audio. Deleted LazyLoadImageObserver entirely. Reusing the existing image margin settings for all media types.
This is mostly done, but https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/media/html_video_element.cc;l=439;drc=56fe0f6dc9d67cc5fef1036a7e58aea477aa26a0 still needs to be differentiated somehow.
bool IsElementInInvisibleSubTree(const Element& element) {Does `Element::checkVisibility` do what you want to do here?
Related details
- Benjamin Keen
- Helmut Januschka
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 4 comments![]( "Open in Gerrit")
bool poster_deferred_for_lazy_load_ : 1;Helmut JanuschkaPlease fix this WARNING reported by ClangTidy: check: modernize-use-default-member-init
use default member initializer for 'po...
check: modernize-use-default-member-init
use default member initializer for 'poster_deferred_for_lazy_load_' (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)
(Note: You can add `Skip-Clang-Tidy-Checks: modernize-use-default-member-init` footer to the CL description to skip the check)
(Lint observed on `android-clang-tidy-rel`, but not on `linux-clang-tidy-rel`)
Dale CurtisDone
I'd go ahead and fix all the nearby ones too, but up to you.
Done. Moved `is_persistent_`, `is_auto_picture_in_picture_`, `is_effectively_fullscreen_`, `video_has_played_`, and `mostly_filling_viewport_` to use default member initializers in the header and removed them from the constructor initializer list.
!FastHasAttribute(html_names::kSrcAttr) && GetDocument().GetFrame()) {Helmut JanuschkaDo you also need to check that it has no source children?
Dale CurtisDone.
I'm surprised we don't have a HasSources() type method already. Since you use this in a couple places, a helper function to share this logic would be helpful.
Done. Added `HasMediaSources()` to `HTMLMediaElement` that checks for src attribute, srcObject, or `<source>` child elements. Replaced the two occurrences in `html_video_element.cc`.
if (!lazy_load_intersection_observer_) {Benjamin KeenWe already have a couple other intersection observers attached to the video element. I'm wondering if we should be sharing these instead of adding new ones. This does use params the others don't though.
https://source.chromium.org/search?ss=chromium&q=IntersectionObserver::Create%20file:media%2F
We even already have one called lazy load intersection observer:
We should at least differentiate between that last one.
IIRC, each of these has a non-trivial cost. @bk...@google.com for his thoughts since he implemented MediaVideoVisibilityTracker.
Helmut JanuschkaI am not too concerned here. The cost increases significantly when tracking visibility, and this observer is not created using the `track_visibility` param.
I also share Dale's concern regarding the various intersection observers we have. For the scope of this CL, I wonder if we could just have one observer for image/media lazy load, since the [lazy_load_image_observer](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/lazy_load_image_observer.h;drc=899b3f1c8d928409caee3a12d17c62691fec793c) is very similar to this one.
Dale Curtisgood idea, merged them into LazyLoadMediaObserver with one IntersectionObserver per document, handling images, video, and audio. Deleted LazyLoadImageObserver entirely. Reusing the existing image margin settings for all media types.
This is mostly done, but https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/html/media/html_video_element.cc;l=439;drc=56fe0f6dc9d67cc5fef1036a7e58aea477aa26a0 still needs to be differentiated somehow.
Done
bool IsElementInInvisibleSubTree(const Element& element) {Does `Element::checkVisibility` do what you want to do here?
Replaced `IsElementInInvisibleSubTree` with `Element::checkVisibility()` and removed the custom function entirely.
Related details
- Benjamin Keen
- Dale Curtis
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis voted and added 4 comments![]( "Open in Gerrit")
Votes added by Dale Curtis
| Code-Review | +1 |
4 comments
// We only defer when lazy_media_load_state_ is kNone, which is the initialback ticks around lazy_media_load_state_
bool in_overlay_fullscreen_video_ : 1;Give this a default value of `false` while you're here?
remoting_interstitial_(nullptr),Remove these nullptr since that's the default.
Related details
- Benjamin Keen
- Helmut Januschka
- Yoav Weiss (@Shopify)
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |