464158 - nsHTMLMediaElement::PickMediaElement does not do media type switching

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P2)

Description

[Matthew Gregan [:kinetik]]

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.

Comment 1

[Matthew Gregan [:kinetik]]

Attachment: patch v0 (wip)

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.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Looks reasonable. I wonder if we should do the work in your last two paragraphs here in this bug.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 4

[Matthew Gregan [:kinetik]]

Attachment: patch v1

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.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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?

Comment 6

[Matthew Gregan [:kinetik]]

That's right.  The documentation in nsIChannel.idl states: "If asyncOpen returns successfully, the channel promises to call at least onStartRequest and onStopRequest.".

Comment 7

[Matthew Gregan [:kinetik]]

Attachment: patch v1.1

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).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed to trunk as 3f5a6da199fc

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Backed out. This cased test_videocontrols.html to fail:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228256186.1228261066.4385.gz

Comment 10

[Matthew Gregan [:kinetik]]

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.

Comment 11

[Matthew Gregan [:kinetik]]

Attachment: patch v2.1

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.

Comment 12

[Matthew Gregan [:kinetik]]

> 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.

Comment 13

[Matthew Gregan [:kinetik]]

Attachment: patch v2.2

Same patch, but rebased against current trunk since it doesn't merge cleanly and needs a bunch of manual fixup.

Comment 14

[Matthew Gregan [:kinetik]]

Attachment: patch v2.3

Rebased.

Comment 15

[Matthew Gregan [:kinetik]]

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.

Comment 16

[Matthew Gregan [:kinetik]]

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.

Comment 17

[Matthew Gregan [:kinetik]]

Attachment: patch v2.4

Rebased on top of current trunk now that bug 462570 has landed.

Comment 18

[Matthew Gregan [:kinetik]]

Comment on attachment 353143 [details] [diff] [review]
patch v2.4

Obsoleting.  Forgot to clean something up.

Comment 19

[Matthew Gregan [:kinetik]]

Attachment: patch v2.5

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.

Comment 20

[cajbir (:cajbir)]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/73c3f66ec28f

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Pushed 4df826d5d0b3 to 1.9.1

Comment 22

[Serge Gautherie (:sgautherie)]

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.

Comment 23

[Serge Gautherie (:sgautherie)]

(In reply to comment #22)
> Could you either remove this line

Done by
http://hg.mozilla.org/mozilla-central/rev/d0de9c05aad3

> Thanks.