New issue
Advanced search Search tips
Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 73609
issue 658313

Blocking:
issue 694855



Sign in to add a comment

The default <video> preload value should be "metadata"

Reported by phil...@opera.com, Oct 23 2013

Issue description

IE11 and Opera Presto use "metadata". Gecko says "" but I believe the internal value is metadata.

Blink uses "auto", which is allowed per spec, but this comment in HTMLMediaElement::parseAttribute is out of sync with the spec:

            // The spec does not define an "invalid value default" but "auto" is suggested as the
            // "missing value default", so use it for everything except "none" and "metadata"

The best fix is to align with the other browsers, but at the very least the comment should be fixed.

This issue was found as part of https://codereview.chromium.org/35803004/
 

Comment 1 by phil...@opera.com, Oct 23 2013

Aaron, Andrew, I'd like your input on this before I do anything. Does switching to "metadata" sound OK, or is "auto" used deliberately?
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 23 2013

The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=160318

------------------------------------------------------------------------
r160318 | philipj@opera.com | 2013-10-23T08:24:10.684417Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLMediaElement.idl?r1=160318&r2=160317&pathrev=160318
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLSourceElement.idl?r1=160318&r2=160317&pathrev=160318
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/dom/element-attribute-js-null-expected.txt?r1=160318&r2=160317&pathrev=160318
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLTrackElement.idl?r1=160318&r2=160317&pathrev=160318
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLVideoElement.idl?r1=160318&r2=160317&pathrev=160318
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/fast/dom/element-attribute-js-null.html?r1=160318&r2=160317&pathrev=160318

Remove TreatNullAs=NullString for media interfaces

The spec doesn't have special null handling here:
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html

The updated test was run in Firefox Nightly (with media.webvtt.enabled),
IE11 Release Preview and Opera 12.16 (Presto 2.12.388). Everything
passes, with these exceptions:

No other browser supports the mediaGroup property.

The preload default value is "metadata" in IE11 and Presto, and "" in
Firefox. The spec suggests "metadata". Bugs filed:
https://code.google.com/p/chromium/issues/detail?id=310450
https://bugzilla.mozilla.org/show_bug.cgi?id=929890

BUG= 310298 

Review URL: https://codereview.chromium.org/35803004
------------------------------------------------------------------------
Project Member

Comment 3 by bugdroid1@chromium.org, Dec 18 2013

Labels: merge-merged-1700
The following revision refers to this bug:
    http://src.chromium.org/viewvc/blink?view=rev&rev=164124

------------------------------------------------------------------------
r164124 | haraken@chromium.org | 2013-12-18T23:40:29.144039Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/branches/chromium/1700/Source/core/html/HTMLVideoElement.idl?r1=164124&r2=164123&pathrev=164124
   M http://src.chromium.org/viewvc/blink/branches/chromium/1700/LayoutTests/fast/dom/element-attribute-js-null.html?r1=164124&r2=164123&pathrev=164124
   M http://src.chromium.org/viewvc/blink/branches/chromium/1700/Source/core/html/HTMLMediaElement.idl?r1=164124&r2=164123&pathrev=164124
   M http://src.chromium.org/viewvc/blink/branches/chromium/1700/Source/core/html/HTMLSourceElement.idl?r1=164124&r2=164123&pathrev=164124
   M http://src.chromium.org/viewvc/blink/branches/chromium/1700/LayoutTests/fast/dom/element-attribute-js-null-expected.txt?r1=164124&r2=164123&pathrev=164124
   M http://src.chromium.org/viewvc/blink/branches/chromium/1700/Source/core/html/HTMLTrackElement.idl?r1=164124&r2=164123&pathrev=164124

Revert 160318 "Remove TreatNullAs=NullString for media interfaces"

> Remove TreatNullAs=NullString for media interfaces
> 
> The spec doesn't have special null handling here:
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-video-element.html
> 
> The updated test was run in Firefox Nightly (with media.webvtt.enabled),
> IE11 Release Preview and Opera 12.16 (Presto 2.12.388). Everything
> passes, with these exceptions:
> 
> No other browser supports the mediaGroup property.
> 
> The preload default value is "metadata" in IE11 and Presto, and "" in
> Firefox. The spec suggests "metadata". Bugs filed:
> https://code.google.com/p/chromium/issues/detail?id=310450
> https://bugzilla.mozilla.org/show_bug.cgi?id=929890
> 
> BUG= 310298 
> 
> Review URL: https://codereview.chromium.org/35803004

TBR=philipj@opera.com

Review URL: https://codereview.chromium.org/98793007
------------------------------------------------------------------------
Cc: -scherkus@chromium.org

Comment 5 by phil...@opera.com, Jul 14 2015

Blockedon: chromium:73609
I'm marking this as blocked on  issue 73609 , because trying to change the default to "metadata" without first fixing the behavior of "metadata" could cause damage by later making it more difficult to change the (new) default behavior.
Project Member

Comment 6 by bugdroid1@chromium.org, Jul 15 2015

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=198955

------------------------------------------------------------------
r198955 | philipj@opera.com | 2015-07-15T13:47:12.333230Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLMediaElement.cpp?r1=198955&r2=198954&pathrev=198955
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/html/HTMLMediaElement.h?r1=198955&r2=198954&pathrev=198955
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/frame/UseCounter.h?r1=198955&r2=198954&pathrev=198955

Measure usage of HTMLMediaElement preload states

Refactoring into a preloadType() is necessary in order to count cases
where the preload attribute is never set, i.e. the missing value default
case. Also, it's nice to have less state in HTMLMediaElement.

BUG= 73609 ,  310450 

Review URL: https://codereview.chromium.org/1227403004
-----------------------------------------------------------------

Comment 7 by sshru...@google.com, Mar 21 2016

Components: -Blink>Video Blink>Media>Video
Renaming Blink>Video to Blink>Media>Video for better characterization
Status: Assigned (was: Untriaged)
Labels: Needs-BlinkMediaTriage
Cc: foolip@chromium.org
Labels: -Needs-BlinkMediaTriage
Owner: ----
Status: Available (was: Assigned)
Blockedon: 658313
Blocking: 694855
Owner: apaci...@chromium.org
Status: Started (was: Available)
Project Member

Comment 15 by bugdroid1@chromium.org, Aug 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82e25c91501c542161f379f4f44343686dd9a3e6

commit 82e25c91501c542161f379f4f44343686dd9a3e6
Author: Jennifer Apacible <apacible@chromium.org>
Date: Mon Aug 07 18:15:27 2017

Add Media.UnderflowDuration2.* histograms.

This histogram replaces the Media.UnderflowDuration.* histograms, which
is deprecated in this change.

The primary difference is that Media.UnderflowDuration2.* will only
account for rebuffers. Today, the Media.UnderflowDuration.* histograms
accounts for initial zero samples, which were previously reported to
compensate for playbacks that never rebuffer.

Bug:  310450 
Change-Id: Ie57a3a618f1ee320c0ae0390f1eb62d3b38df6a3
Reviewed-on: https://chromium-review.googlesource.com/598747
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492363}
[modify] https://crrev.com/82e25c91501c542161f379f4f44343686dd9a3e6/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/82e25c91501c542161f379f4f44343686dd9a3e6/tools/metrics/histograms/histograms.xml

Labels: Merge-Request-61
Project Member

Comment 17 by sheriffbot@chromium.org, Aug 10 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: M61 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
This bug has been open since Oct 23rd, 2013. Doesn't seem like M61 regression. Can this wait until M62?

Please note M61 is already in Beta, so merge bar is VERY high. Thank you.
Cc: dalecur...@chromium.org
+dalecurtis

Thanks govind@, this is not a regression. We would like to gather metrics in M61 and compare beta with a change in canary.
Thank you  apacible@.
Is the change well baked/verified in Canary, having enough automation tests coverage and safe to merge to M61?
Yes, this landed earlier this week and is safe to merge.
Labels: -Merge-Review-61 Merge-Approved-61
Approving merge to M61 branch 3163 based on comment #19 and #21. Please merge ASAP. Thank you.
Project Member

Comment 23 by bugdroid1@chromium.org, Aug 11 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ac01a3bf1db5c90f426e601174040a9f877eb7b1

commit ac01a3bf1db5c90f426e601174040a9f877eb7b1
Author: Jennifer Apacible <apacible@chromium.org>
Date: Fri Aug 11 00:30:18 2017

Add Media.UnderflowDuration2.* histograms.

This histogram replaces the Media.UnderflowDuration.* histograms, which
is deprecated in this change.

The primary difference is that Media.UnderflowDuration2.* will only
account for rebuffers. Today, the Media.UnderflowDuration.* histograms
accounts for initial zero samples, which were previously reported to
compensate for playbacks that never rebuffer.

TBR=apacible@chromium.org

(cherry picked from commit 82e25c91501c542161f379f4f44343686dd9a3e6)

Bug:  310450 
Change-Id: Ie57a3a618f1ee320c0ae0390f1eb62d3b38df6a3
Reviewed-on: https://chromium-review.googlesource.com/598747
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#492363}
Reviewed-on: https://chromium-review.googlesource.com/611421
Reviewed-by: apacible <apacible@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#479}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/ac01a3bf1db5c90f426e601174040a9f877eb7b1/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/ac01a3bf1db5c90f426e601174040a9f877eb7b1/tools/metrics/histograms/histograms.xml

Project Member

Comment 24 by bugdroid1@chromium.org, Aug 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66a53db908b0cfe26b07c340c9781a7e0af80597

commit 66a53db908b0cfe26b07c340c9781a7e0af80597
Author: Jennifer Apacible <apacible@chromium.org>
Date: Mon Aug 28 20:45:03 2017

Set <video> and <audio> preload= default to metadata.

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/5CDvJkdxyQ8

Bug:  310450 
Change-Id: I67a1c6828e94a6f82688755747f4122ac3f0bf47
Reviewed-on: https://chromium-review.googlesource.com/596623
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Philip J├Ągenstedt <foolip@chromium.org>
Reviewed-by: Dimitri Glazkov <dglazkov@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#497864}
[modify] https://crrev.com/66a53db908b0cfe26b07c340c9781a7e0af80597/content/renderer/render_view_impl.cc
[modify] https://crrev.com/66a53db908b0cfe26b07c340c9781a7e0af80597/media/base/media_switches.cc
[modify] https://crrev.com/66a53db908b0cfe26b07c340c9781a7e0af80597/media/base/media_switches.h
[modify] https://crrev.com/66a53db908b0cfe26b07c340c9781a7e0af80597/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/66a53db908b0cfe26b07c340c9781a7e0af80597/third_party/WebKit/LayoutTests/http/tests/media/preload-conditions.html
[modify] https://crrev.com/66a53db908b0cfe26b07c340c9781a7e0af80597/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
[modify] https://crrev.com/66a53db908b0cfe26b07c340c9781a7e0af80597/third_party/WebKit/Source/core/html/HTMLMediaElementTest.cpp
[modify] https://crrev.com/66a53db908b0cfe26b07c340c9781a7e0af80597/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5
[modify] https://crrev.com/66a53db908b0cfe26b07c340c9781a7e0af80597/third_party/WebKit/Source/platform/exported/WebRuntimeFeatures.cpp
[modify] https://crrev.com/66a53db908b0cfe26b07c340c9781a7e0af80597/third_party/WebKit/public/platform/WebRuntimeFeatures.h

Project Member

Comment 25 by bugdroid1@chromium.org, Oct 26 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/69e12d5a958299ebc8b5eaabae4b32f831f6d93b

commit 69e12d5a958299ebc8b5eaabae4b32f831f6d93b
Author: Jennifer Apacible <apacible@chromium.org>
Date: Thu Oct 26 02:03:54 2017

Enable PreloadDefaultIsMetadata by default.

Starting M64, set <video> and <audio> preload= default to metadata.

Intent to Implement and Ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/5CDvJkdxyQ8

Bug:  310450 
Change-Id: I681f532a688646523028f8cc8578d8da29ea1881
Reviewed-on: https://chromium-review.googlesource.com/731675
Commit-Queue: apacible <apacible@chromium.org>
Reviewed-by: Dimitri Glazkov <dglazkov@chromium.org>
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511696}
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/content/renderer/render_view_impl.cc
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/media/base/media_switches.cc
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/media/base/media_switches.h
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/media/blink/webmediaplayer_impl.cc
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null-expected.txt
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/fast/dom/element-attribute-js-null.html
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/http/tests/media/preload-conditions.html
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/media/W3C/audio/preload/preload_reflects_bogus_value-expected.txt
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/media/W3C/audio/preload/preload_reflects_bogus_value.html
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/media/W3C/audio/preload/preload_reflects_no_value-expected.txt
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/media/W3C/audio/preload/preload_reflects_no_value.html
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/media/W3C/video/preload/preload_reflects_bogus_value-expected.txt
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/media/W3C/video/preload/preload_reflects_bogus_value.html
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/media/W3C/video/preload/preload_reflects_no_value-expected.txt
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/media/W3C/video/preload/preload_reflects_no_value.html
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/media/media-ended.html
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/LayoutTests/media/video-playbackrate.html
[modify] https://crrev.com/69e12d5a958299ebc8b5eaabae4b32f831f6d93b/third_party/WebKit/Source/platform/runtime_enabled_features.json5

Status: Fixed (was: Started)
apacible@ can you summarize the changes in metrics either here or on the mailing list post to clarify just what you were looking for and why what you saw made you confident to enable this by default? It's good to have that information for posterity.

Sign in to add a comment