nsHTMLMediaElement::PickMediaElement does not do media type switching

RESOLVED FIXED in mozilla1.9.1

Status

()

Core
Audio/Video
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: kinetik, Assigned: kinetik)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.1
fixed1.9.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

9 years ago
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.
Flags: blocking1.9.1?
(Assignee)

Updated

9 years ago
Target Milestone: mozilla1.9.1b2 → mozilla1.9.1
(Assignee)

Comment 1

9 years ago
Created attachment 347706 [details] [diff] [review]
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.
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+
(Assignee)

Comment 4

9 years ago
Created attachment 349904 [details] [diff] [review]
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.
Attachment #347706 - Attachment is obsolete: true
Attachment #349904 - Flags: superreview?(roc)
Attachment #349904 - Flags: review?(roc)
(Assignee)

Updated

9 years ago
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?
(Assignee)

Comment 6

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

Updated

9 years ago
Attachment #349904 - Flags: review?(chris.double) → review+
(Assignee)

Comment 7

9 years ago
Created attachment 349912 [details] [diff] [review]
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).
Attachment #349904 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Updated

9 years ago
Blocks: 463162
Priority: -- → P2
Whiteboard: [needs landing]
Pushed to trunk as 3f5a6da199fc
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs 191 landing]
Keywords: checkin-needed
Flags: in-testsuite+
Backed out. This cased test_videocontrols.html to fail:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1228256186.1228261066.4385.gz
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Whiteboard: [needs 191 landing] → [needs landing]
(Assignee)

Comment 10

9 years ago
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]
(Assignee)

Comment 11

9 years ago
Created attachment 352665 [details] [diff] [review]
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.
Attachment #349912 - Attachment is obsolete: true
Attachment #352665 - Flags: superreview?(roc)
Attachment #352665 - Flags: review?(chris.double)
(Assignee)

Comment 12

9 years ago
> 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.
(Assignee)

Comment 13

9 years ago
Created attachment 352820 [details] [diff] [review]
patch v2.2

Same patch, but rebased against current trunk since it doesn't merge cleanly and needs a bunch of manual fixup.
Attachment #352665 - Attachment is obsolete: true
Attachment #352820 - Flags: superreview?(roc)
Attachment #352820 - Flags: review?(chris.double)
Attachment #352665 - Flags: superreview?(roc)
Attachment #352665 - Flags: review?(chris.double)
(Assignee)

Comment 14

9 years ago
Created attachment 352877 [details] [diff] [review]
patch v2.3

Rebased.
Attachment #352820 - Attachment is obsolete: true
Attachment #352877 - Flags: superreview?(roc)
Attachment #352877 - Flags: review?(chris.double)
Attachment #352820 - Flags: superreview?(roc)
Attachment #352820 - Flags: review?(chris.double)
(Assignee)

Comment 15

9 years ago
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.
(Assignee)

Comment 16

9 years ago
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+

Updated

9 years ago
Attachment #352877 - Flags: review?(chris.double) → review+
(Assignee)

Comment 17

9 years ago
Created attachment 353143 [details] [diff] [review]
patch v2.4

Rebased on top of current trunk now that bug 462570 has landed.
Attachment #352877 - Attachment is obsolete: true
(Assignee)

Comment 18

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

Obsoleting.  Forgot to clean something up.
Attachment #353143 - Attachment is obsolete: true
(Assignee)

Comment 19

9 years ago
Created attachment 353146 [details] [diff] [review]
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.
Attachment #353146 - Flags: superreview?(roc)
Attachment #353146 - Flags: review?(chris.double)

Updated

9 years ago
Attachment #353146 - Flags: review?(chris.double) → review+
Attachment #353146 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
Whiteboard: [needs landing]

Comment 20

9 years ago
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/73c3f66ec28f
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing] → [needs landing][baking for 1.9.1]
(Assignee)

Updated

9 years ago
Whiteboard: [needs landing][baking for 1.9.1] → [needs landing][baking for 1.9.1][needs 1.9.1 landing]
(Assignee)

Updated

9 years ago
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
Keywords: fixed1.9.1
Whiteboard: [baking for 1.9.1][needs 191 landing]
(Assignee)

Updated

9 years ago
Depends on: 470662
(Assignee)

Updated

9 years ago
Depends on: 471024
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.

Updated

9 years ago
Blocks: 489830
Depends on: 496832
You need to log in before you can comment on or make changes to this bug.