Last Comment Bug 548523 - HTML 5 media attribute 'autobuffer' has been renamed to 'preload'
: HTML 5 media attribute 'autobuffer' has been renamed to 'preload'
Status: RESOLVED FIXED
[parity-webkit]
: dev-doc-complete, html5
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
: 548538 554767 (view as bug list)
Depends on:
Blocks: 569993
  Show dependency treegraph
 
Reported: 2010-02-25 01:29 PST by cajbir (:cajbir)
Modified: 2010-08-31 09:42 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
betaN+


Attachments
Patch attempt 1 (27.05 KB, patch)
2010-05-09 21:11 PDT, Rich Dougherty
no flags Details | Diff | Review
Patch 1 v2: preload attribute (52.88 KB, patch)
2010-07-26 16:59 PDT, Chris Pearce (:cpearce)
roc: review+
Details | Diff | Review
Patch 2 v1: Don't show video controls throbber when not loading resource (2.37 KB, patch)
2010-07-26 17:04 PDT, Chris Pearce (:cpearce)
dolske: review-
Details | Diff | Review
Patch 1 v3: preload attribute, with review comments addressed (53.25 KB, patch)
2010-07-29 16:03 PDT, Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Review
Patch 2 v2: Don't show status overlay when not loading resource (2.97 KB, patch)
2010-07-29 21:09 PDT, Chris Pearce (:cpearce)
dolske: review+
Details | Diff | Review
Patch disable test_preload_actions case 9 (1.99 KB, patch)
2010-08-12 22:07 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Review
Patch 4: Replace all usage of autobuffer with preload='auto' (15.88 KB, patch)
2010-08-18 17:34 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Review

Description cajbir (:cajbir) 2010-02-25 01:29:26 PST
See:

http://html5.org/tools/web-apps-tracker?from=4810&to=4811
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-media-preload
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#attr-media-preload

It has also changed behavior. It now has four options rather than being an HTML boolean attribute.

We need to remove 'autobuffer' and implement 'preload'. This will probably affect existing sites that use autobuffer.
Comment 1 Paul Rouget [:paul] 2010-02-25 06:59:20 PST
*** Bug 548538 has been marked as a duplicate of this bug. ***
Comment 2 Boris Zbarsky [:bz] 2010-02-25 07:51:55 PST
Do we actually want to remove autobuffer, or keep it for now for backwards compat and implement preload alongside?
Comment 3 Christopher Blizzard (:blizzard) 2010-02-25 08:12:22 PST
Keep it plz.
Comment 4 :Ms2ger 2010-03-03 09:51:19 PST
I'd say drop it, like WebKit did.
Comment 5 Anne (:annevk) 2010-03-04 03:49:24 PST
Since the default should now be buffering, removing autobuffer="" support should be simple and safe.
Comment 6 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-24 15:11:25 PDT
*** Bug 554767 has been marked as a duplicate of this bug. ***
Comment 7 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-24 15:19:14 PDT
I agree with Anne.

However, making buffering be the default is a bit of a problem. There are existing pages with lots of media elements with no attributes.

"Hints to the user agent that the user agent can put the user's needs first without risk to the server, up to and including optimistically downloading the entire resource." doesn't seem like something that should be the default for authors who don't know what they're doing. I'll bring this up on the list.
Comment 8 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-24 15:33:38 PDT
I started a thread on public-html. Maybe I should have filed a bug instead, oh well.

The spec bug where 'preload' was discussed is here:
http://www.w3.org/Bugs/Public/show_bug.cgi?id=8731

I think for now we should just implement the spec except make the default be "metadata". I think we can drop "autobuffer". We'll need to update our tests.
Comment 9 Rich Dougherty 2010-05-09 21:11:37 PDT
Created attachment 444340 [details] [diff] [review]
Patch attempt 1

The attached patch is a first step toward supporting the 'preload' hint. I'd really appreciate getting a bit of feedback before I continue. (Chris: Rob said you might be able to help here!)

The updated spec changes the attribute name from 'autobuffer' to 'preload'. The spec introduces different values for the attribute, including a way to avoid preloading entirely. Here's a quick summary of how the changes relate to the patch.

The spec defines behaviour for five different states of the 'preload' attribute. These states are:
1. no attribute present,
2. attribute set to "",
3. attribute set to "none",
4. attribute set to "metadata", or
5. attribute set to "auto".

State 1 is simply represented by an element without a 'preload' attribute. States 2-5 are represented by attributes with one of the values from the PreloadAttrValue enumeration.

Each state drives a certain preloading behaviour. The unpatched code supports two types of behaviour: loading metadata only and loading "enough" content to support uninterrupted playback. To support preload="none" we need to introduce another preloading behaviour: loading no content at all.

These different behaviours are represented in the code by the PreloadAction enumeration.

Here are the mappings that the spec gives from attribute state to each preloading behaviour.
1. (no attr)    -> preload metadata
2. ("")         -> preload enough data
3. ("none")     -> preload no data
4. ("metadata") -> preload metadata
5. ("auto")     -> preload enough data

The mapping from attribute state to preload behaviour is handled by the PreloadAttributeUpdated() method. At the moment the method doesn't actually implement all these behaviours. I will handle them in a future version of the patch. It does handle one case---loading "enough" data---because code for doing this had already be implemented.

Next steps:

1. Stopping load when preload="none": Split LoadResource() into two parts so that we can suspend before actually starting any load actions. We would stash the current URI into a member variable so that we can restart the load later on.

2. Resuming load: maybe generalising mSuspendedAfterFirstFrame, mAllowSuspendAfterFirstFrame and StopSuspendingAfterFirstFrame() to support resuming from PRELOAD_NONE in addition to PRELOAD_METADATA. As an alternative to generalising this, I could introduce duplicate variables and methods. i.e. mSuspendedBeforeLoad, mAllowSuspendBeforeLoad, StopSuspendingBeforeLoad().

3. By this stage we will have support for all preload values and behaviours. However there is a question about whether we support all preload behaviour *transitions*. For example the existing (unpatched) code supports the transition from "metadata" to "enough" but ignores transitions in the other direction. In other words, it supports *increases* in the amount of preloading, but not *decreases*. I discussed this with Rob and the reason is that the use case for decreasing the amount of preloading doesn't seem very important, when compared to the extra complexity of the code that would need to be written to support it. At the moment I'm planning on taking the same approach (i.e. only supporting increases), but I'd welcome comments.

4. Add tests for new attribute values and preload behaviours. I've already patched the existing tests where it was easy to do so. But I haven't written any tests for the new values and behaviours.
Comment 10 Chris Pearce (:cpearce) 2010-05-09 23:14:09 PDT
Comment on attachment 444340 [details] [diff] [review]
Patch attempt 1

(In reply to comment #9)
> Created an attachment (id=444340) [details]
> Patch attempt 1

Looks like you've got a good handle on how to approach this.

> Next steps:
> 
> 1. Stopping load when preload="none": Split LoadResource() into two parts so
> that we can suspend before actually starting any load actions. We would stash
> the current URI into a member variable so that we can restart the load later
> on.

As discussed, when script or the user requests a load, we should just do a complete load, ignoring preload. The preload attribute should only affect the implicit loads which start when a we add a media element to a document. With preload:none we should stop the load once we reach resource fetch algorithm step 2.

> 
> 2. Resuming load: maybe generalising mSuspendedAfterFirstFrame,
> mAllowSuspendAfterFirstFrame and StopSuspendingAfterFirstFrame() to support
> resuming from PRELOAD_NONE in addition to PRELOAD_METADATA. As an alternative
> to generalising this, I could introduce duplicate variables and methods. i.e.
> mSuspendedBeforeLoad, mAllowSuspendBeforeLoad, StopSuspendingBeforeLoad().
> 

Do whichever you feel is cleaner.

> 3. By this stage we will have support for all preload values and behaviours.
> However there is a question about whether we support all preload behaviour
> *transitions*. 

Decreasing values don't seem that important to me; a script should really set the value of preload before starting a load.

> 
> 4. Add tests for new attribute values and preload behaviours. I've already
> patched the existing tests where it was easy to do so. But I haven't written
> any tests for the new values and behaviours.

Backend independent tests please, so that it covers both the wav and ogg backends, similar to the approach taken in test_playback.html.
Comment 11 Chris Pearce (:cpearce) 2010-07-20 19:10:57 PDT
I'll take this and finish it off. Rich, thanks very much for getting us started on this!
Comment 12 Chris Pearce (:cpearce) 2010-07-26 16:59:17 PDT
Created attachment 460371 [details] [diff] [review]
Patch 1 v2: preload attribute

Rich's patch finished up, with tests. We preload:metadata by default, and support preload attribute changes during loads which cause us to preload more data, but not less.
Comment 13 Chris Pearce (:cpearce) 2010-07-26 17:04:12 PDT
Created attachment 460374 [details] [diff] [review]
Patch 2 v1: Don't show video controls throbber when not loading resource

When we're loading a preload:none video, or if a video element with controls is added to a document, we'll show the throbber in the controls. This is misleading, as we're not actively loading the video. This patch changes the controls so that we only show the throbber upon video element bind to document or loadstart if we've got a resource and we're actively loading it.
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-07-27 15:56:00 PDT
Comment on attachment 460371 [details] [diff] [review]
Patch 1 v2: preload attribute

Shouldn't there be a call to UpdatePreloadAction() when there's an attribute change with aName == nsGkAtoms::autoplay? Otherwise looks good.
Comment 15 Chris Pearce (:cpearce) 2010-07-27 15:59:14 PDT
(In reply to comment #14)
> Shouldn't there be a call to UpdatePreloadAction() when there's an attribute
> change with aName == nsGkAtoms::autoplay? Otherwise looks good.

Yeah, I think there needs to be. I'll add a test for this too.

Can this block2.0?
Comment 16 Chris Pearce (:cpearce) 2010-07-29 16:03:15 PDT
Created attachment 461377 [details] [diff] [review]
Patch 1 v3: preload attribute, with review comments addressed

With UpdatePreloadAction() in setattr autoplay case, and with test for this case.
Comment 17 Justin Dolske [:Dolske] 2010-07-29 17:14:29 PDT
Comment on attachment 460374 [details] [diff] [review]
Patch 2 v1: Don't show video controls throbber when not loading resource

Setting the statusIcon type doesn't control its display, just which icon (throbber or error "X") should be displayed... Instead, I think you want to update the logic in setupStatusFader() to suppress display of the statusOverlay (which includes the icon as a child). [The status overlay is a semitransparent black, so I suspect this patch was just breaking the icon and not the overlay display?]
Comment 18 Justin Dolske [:Dolske] 2010-07-29 17:16:09 PDT
Hmm, my first sentence it probably confusing. It's better rephrased as: Setting the statusIcon type doesn't control its display, just which icon is used when the statusOverlay is being shown.
Comment 19 Chris Pearce (:cpearce) 2010-07-29 21:09:23 PDT
Created attachment 461460 [details] [diff] [review]
Patch 2 v2: Don't show status overlay when not loading resource

Patch attempt 2. Changes setupStatusFader() so that we only show the status overlay in the non-seeking and non-error cases when networkState is NETWORK_LOADING. So only show the status overlay if we're actually loading a resource in these cases.

I had to also change the media element so that it goes back into NETWORK_LOADING network state when it resumes a load; the spec and this controls change requires this, I missed it in the previous patch.
Comment 20 Chris Pearce (:cpearce) 2010-08-12 22:07:22 PDT
Created attachment 465551 [details] [diff] [review]
Patch disable test_preload_actions case 9

Whilde developing another patch on top of this one, I noticed that test_preload_actions, testcase 9 often times out because the canplaythrough event doesn't always fire. We have bug 568402 on file for this. This patch disables testcase 9, so that we don't inject a new random orange when this lands. We should re-enable that testcase once bug 568402 is fixed. a=test-fix.
Comment 21 Chris Pearce (:cpearce) 2010-08-12 22:08:34 PDT
We should land 'Patch 1 v3' and 'Patch disable test_preload_actions case 9' ASAP, and follow up with Patch 2 once it gets review.
Comment 22 Justin Dolske [:Dolske] 2010-08-14 21:14:56 PDT
Comment on attachment 461460 [details] [diff] [review]
Patch 2 v2: Don't show status overlay when not loading resource

Well, I feel dumb. Now that I finally sat down to rewrap my head around how setupStatusFader() works, I see that this is actually a fairly simple change. Sorry for the delay. :(
Comment 24 cajbir (:cajbir) 2010-08-17 18:26:38 PDT
+  PRBool wasPreloadNone = mPreloadAction == PRELOAD_NONE;

This appears to be an unused variable:

content/src/nsHTMLMediaElement.cpp:749: warning: unused variable‘wasPreloadNone’

Also an order of constructor warning:

nsHTMLMediaElement.h: In constructor ‘nsHTMLMediaElement::nsHTMLMediaElement(already_AddRefed<nsINodeInfo>, PRUint32)’:
nsHTMLMediaElement.h:632: warning: ‘nsHTMLMediaElement::mShuttingDown’ will be initialized after
nsHTMLMediaElement.h:539: warning:   ‘nsHTMLMediaElement::PreloadAction nsHTMLMediaElement::mPreloadAction’
Comment 26 Chris Pearce (:cpearce) 2010-08-18 17:34:04 PDT
Created attachment 467251 [details] [diff] [review]
Patch 4: Replace all usage of autobuffer with preload='auto'

The reason we had test hangs when I landed was that I hadn't replaced some of the uses of the autobuffer attribute with preload='auto'.

This patch replaces all remaining uses of "autobuffer" with "preload='auto'". I grepped my file local tree, and there are no other uses of autobuffer. a/r=test-fix.
Comment 28 :Ms2ger 2010-08-22 12:42:55 PDT
Updated <https://developer.mozilla.org/En/HTML/Element/Video>, I expect more will need to be done.
Comment 29 Eric Shepherd [:sheppy] 2010-08-22 18:10:04 PDT
Also updated:

https://developer.mozilla.org/En/HTML/Element/Audio
https://developer.mozilla.org/En/XPCOM_Interface_Reference/NsIDOMHTMLMediaElement

And the Firefox 4 for developers page.

Note You need to log in before you can comment on or make changes to this bug.