Closed Bug 464158 Opened 13 years ago Closed 13 years ago
HTMLMedia Element::Pick Media Element does not do media type switching
nsHTMLMediaElement::PickMediaElement needs to be taught to do media type switching so that <audio src="a.wav"> will work. Right now the Ogg decoder would be instantiated unconditionally.
Target Milestone: mozilla1.9.1b2 → mozilla1.9.1
This is pretty much the minimal set of changes required to get this to work. All existing tests pass. This also includes the fixes for bug 463162 and the part of bug 463999 (which we should do for the reason outlined in bug 444931 comment 17) that isn't covered by bug 462878 already. It's not ideal to combine them but this patch is going to bitrot both of those changes. Need to write some new tests. Test the simple case of <video src=?> with different types, but also test <video> with a bunch of <source> children, some of which we can't decode. Currently untested but would be broken by this WIP patch. For now, if the element has a <source> child with a type= and src=, we go through the old code path that lets the decoder open the channel. It'd be cleaner to always open the channel in the element and pass it down, since that'd allow a bunch of code in the decoder/media stream to be simplified or removed. Also need to deal with attempting to load each <source> child element if the prior one fails. As mentioned, this is untested but would be broken by this WIP patch.
Looks reasonable. I wonder if we should do the work in your last two paragraphs here in this bug.
Maybe we should open an nsMediaStream in the media element, and pass that down to the decoder? Rather than passing either an open channel, or a URI with nsMediaDecoder::Load()? We're leaking the abstraction nsMediaStream makes. It should provide a seamless interface to the underlying resource, and ideally hide all the channel stuff underneath. But we're creating channels in four places: in the media element (with this patch), in nsMediaStream::Open(), when seeking in HTTP, and wherever we create a channel for a video document. Why can't we just create an nsMediaStream from a URI, and pass that nsMediaStream to our decoder? Doing so would also allow us to isolate the security checks in there too. It would require a bit of reorganization to nsChannelReader etc, but would make things more centralized an simpler. The channel we pass into nsMediaDecoder::Load(_,channel,_) also won't last long if HTTP seeks are required anyway, so it makes more sense for nsMediaDecoder to logically load an nsMediaStream, as it has a longer lifetime. Also, http://www.whatwg.org/specs/web-apps/current-work/#pick-a should be http://www.whatwg.org/specs/web-apps/current-work/#pick-a-media-resource
Flags: blocking1.9.1? → blocking1.9.1+
Complete version of the patch, with tests. The basic approach is the same as v0, but I've renamed some methods and fixed a couple of bugs found during testing. I also fixed the HTML5 spec URL references that cpearce pointed out. The included test should cover all existing working cases, plus the new ones intended to be fixed by this patch (e.g. src="foo.wav"). There are a number of subtests commented out that depend on bug 462455 (updated/clarified load algorithm) being fixed, so I'll enable those as part of the patch for that bug. This patch also passes the rest of the media mochitests. The nsMediaDecoder::Load method still takes a URI, but nsHTMLMediaElement always passes nsnull. This is used internally by nsOggDecoder and nsWaveDecoder to restart the stream by calling Load(mURI, nsnull, nsnull). This isn't ideal, but having the decoder request a new channel from the element doesn't seem any better--in fact that doesn't work in the case where an existing channel is passed in via the nsVideoDocument code path, since we still need code somewhere that finds the channel's URI and creates a new channel from it. Actually, if we kept a copy of the URI in the element, and moved the code that fetches the URI from the channel from ns*Decoder::Load to the element, we could remove the channel creation code from nsMediaStream, and the element's GetCurrentSrc method could use the element's copy of the URI rather than having to call down to the decoder to fetch it. That would address cpearce's point about being multiple places to create a channel in the code, at least by removing the number of places, not by centralizing them. Note that we can't use an nsMediaStream in the case where an nsVideoDocument is passing us an existing channel, since the higher-level code where the channel is created is not specific to the media support. I looked at using the nsMediaStream in the element directly, but I'm not convinced that it'd be an improvement. It does mean we can centralize the channel creation code (except for the nsVideoDocument case), but it requires large changes to the nsMediaStream (it assumes a decoder instance is available to fire the loaded and error events and set the totalBytes and bytesDownloaded attributes) and makes the lifetime management somewhat more complicated (nsMediaStream isn't ref counted at the moment). It'd also still need to be able to be instantiated from both a URI and an existing channel because of the nsVideoDocument case. It's quite possible that I'm wrong and using the nsMediaStream would be cleaner, but right now I don't think it will be for the reasons mentioned here, and I also want to keep this patch small so it can be reviewed quickly and land--we really want src="foo.wav" working in 3.1.
Attachment #349904 - Flags: review?(roc) → review?(chris.double)
Attachment #349904 - Flags: superreview?(roc) → superreview+
Comment on attachment 349904 [details] [diff] [review] patch v1 + // The listener holds a strong reference to us. This creates a reference + // cycle which is manually broken in the listener's OnStartRequest method + // after it is finished with the element. Are we guaranteed to get an OnStartRequest if AsyncOpen succeeds, even if there's an error after that?
That's right. The documentation in nsIChannel.idl states: "If asyncOpen returns successfully, the channel promises to call at least onStartRequest and onStopRequest.".
Trivial changes, so this can carry the r/sr forward. I added a comment to the code to cover the question roc raised, and included the mochitest I wrote for bug 463162, since this patch includes the same change (and thus fixes that bug).
Attachment #349904 - Attachment is obsolete: true
Priority: -- → P2
Whiteboard: [needs landing]
Pushed to trunk as 3f5a6da199fc
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Backed out. This cased test_videocontrols.html to fail: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228256186.1228261066.4385.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [needs 191 landing] → [needs landing]
Oops, thought I had commented here already. The patch is broken: if a play request comes in before the decoder is created, the play request is thrown away. This never used to happen because the synchronous loading would always create a decoder before returning. With async loading, the channel may not yet have called us back to set up the decoder, so we need to queue up the play request and handle it once the decoder has been created. Working on a revised patch now.
Whiteboard: [needs landing]
Fix bug discussed in comment 10. Also remove CanDecode, since we can just use CanHandleMediaType instead. Note that we should really deal with the requested play stuff when we reach CAN_PLAY, but we don't have a complete implementation of that yet, and I don't want to do that as part of this bug. I'll file another bug (if there isn't one already) to cover that.
> Note that we should really deal with the requested play stuff when we reach > CAN_PLAY, but we don't have a complete implementation of that yet, and I don't > want to do that as part of this bug. I'll file another bug (if there isn't one > already) to cover that. Bug 469272.
Same patch, but rebased against current trunk since it doesn't merge cleanly and needs a bunch of manual fixup.
Note that it applies on top of bug 469255, since the chain of bugs leading up to that one are already reviewed and awaiting landing.
Bug 469255 and its dependencies are in the tree now. I'll need to rebase this on top of cpearce's patch for bug 462570 before adding checkin-needed.
Attachment #352877 - Flags: superreview?(roc) → superreview+
Rebased on top of current trunk now that bug 462570 has landed.
Attachment #352877 - Attachment is obsolete: true
Comment on attachment 353143 [details] [diff] [review] patch v2.4 Obsoleting. Forgot to clean something up.
Attachment #353143 - Attachment is obsolete: true
After merging the patch with current trunk, I forgot to check what to do with the assertion added to LoadWithChannel recently: NS_ASSERTION(mNetworkState >= nsIDOMHTMLMediaElement::NETWORK_IDLE, "Pick-a-media-resource should advance network state or bail."); I ended up just removing it, since it doesn't really make sense in the new code. I'm going to rerequest a quick (hopefully!) review of this, since I moved the nsMediaLoadListener helper from nsHTMLMediaElement.h (where I'd moved it when fixing the problem in comment 10 so it could be a friend of nsHTMLMediaElement) back to nsHTMLMediaElement.cpp and made it a nested class.
Attachment #353146 - Flags: superreview?(roc) → superreview+
Whiteboard: [needs landing]
Pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/73c3f66ec28f
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs landing][baking for 1.9.1]
Whiteboard: [needs landing][baking for 1.9.1] → [needs landing][baking for 1.9.1][needs 1.9.1 landing]
Whiteboard: [needs landing][baking for 1.9.1][needs 1.9.1 landing] → [baking for 1.9.1][needs 191 landing]
Pushed 4df826d5d0b3 to 1.9.1
Whiteboard: [baking for 1.9.1][needs 191 landing]
Comment on attachment 353146 [details] [diff] [review] patch v2.5 >+ dump('subtest ' + nextTest + '\n'); Could you either remove this line or use |ok(true, ...);| or the like, to get a better log ? Thanks.
(In reply to comment #22) > Could you either remove this line Done by http://hg.mozilla.org/mozilla-central/rev/d0de9c05aad3 > Thanks.
You need to log in before you can comment on or make changes to this bug.