53.25 KB, patch
|Details | Diff | Splinter Review|
2.97 KB, patch
|Details | Diff | Splinter Review|
1.99 KB, patch
|Details | Diff | Splinter Review|
15.88 KB, patch
|Details | Diff | Splinter Review|
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.
Do we actually want to remove autobuffer, or keep it for now for backwards compat and implement preload alongside?
Keep it plz.
I'd say drop it, like WebKit did.
Since the default should now be buffering, removing autobuffer="" support should be simple and safe.
Duplicate of this bug: 554767
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.
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.
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 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.
I'll take this and finish it off. Rich, thanks very much for getting us started on this!
Assignee: nobody → chris
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.
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.
Attachment #460374 - Flags: review?(dolske)
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.
Attachment #460371 - Flags: review?(roc) → review+
(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?
blocking2.0: ? → betaN+
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 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?]
Attachment #460374 - Flags: review?(dolske) → review-
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.
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.
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.
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.
Whiteboard: [parity-webkit] → [parity-webkit][needs-landing]
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. :(
Attachment #461460 - Flags: review?(dolske) → review+
http://hg.mozilla.org/mozilla-central/rev/42c8cd0e1a65 http://hg.mozilla.org/mozilla-central/rev/f88258e1ddbe http://hg.mozilla.org/mozilla-central/rev/137f0ce0e0ca
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [parity-webkit][needs-landing] → [parity-webkit]
+ 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’
Backed out due to random videocontrols test hang which didn't show up on tinderbox or my local machine. yay. :( http://hg.mozilla.org/mozilla-central/rev/1d4f04d244bc http://hg.mozilla.org/mozilla-central/rev/0cac60a3aee0 http://hg.mozilla.org/mozilla-central/rev/898826f52402 http://hg.mozilla.org/mozilla-central/rev/72d4a91d791d http://hg.mozilla.org/mozilla-central/rev/c59438cb3548 http://hg.mozilla.org/mozilla-central/rev/c9cbe6a36c96
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Once more: http://hg.mozilla.org/mozilla-central/rev/3f7dfabab5e4 http://hg.mozilla.org/mozilla-central/rev/571d2664ead0 http://hg.mozilla.org/mozilla-central/rev/d6e8fddeb95b http://hg.mozilla.org/mozilla-central/rev/c6abfc89215a
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago → 8 years ago
Resolution: --- → FIXED
Updated <https://developer.mozilla.org/En/HTML/Element/Video>, I expect more will need to be done.
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.