465458 - Media selection algorithm changed in r2403 of HTML5 spec

Status: VERIFIED FIXED

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

Description

[Matthew Gregan [:kinetik]]

We need to update nsHTMLMediaElement to use the new algorithm.

There are also related changes in r2404 that need to be made: http://html5.org/tools/web-apps-tracker?from=2403&to=2404

Comment 1

[Damon Sicore (:damons)]

Progress on this?

Comment 2

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

Matthew's away. I don't think he's made any progress on this, he's been working on a Wave-specific issue for a while now. I might be able to get Chris Pearce to take it when he gets back, which should be any day now.

Comment 3

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

Attachment: Patch v1

* Add resource candidate list and iteration to nsMediaLoad.
* AbortExistingLoads() no longer returns false causing bail out of Load().
* Load algorithm made asynchronous. It returns after constructing the candidate list, and the "candidate loop" continues in a callback. This means that networkState changes after a load, but it's only when the JS stack returns that the load/fetch/error events actually run.
* Added nsMediaEvent, which provides functionality to check if we should cancel the event, due to a new load starting. nsAsyncEventRunner now uses that.
* nsOggDecoder::LoadOggHeaders() now takes the nsChannelReader which it's loading from as a parameter. This is because when we're in LoadOggHeaders() we can move into shutdown state which causes the decoders ref to the reader to disappear, causing a null pointer to be returned by mDecoder->GetReader() in LoadOggHeaders().
* Tests adapted to new load() behaviour.
* Built and tested on Windows and Linux.

Comment 4

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

   class nsMediaLoadListener;
+  class nsLoadNextCandidateEvent;

These don't need the ugly ns prefix.

AbortExistingLoads fires synchronous events. If those handlers do a load(), you need to bail out of the current load. I think you need to save the new load object that AbortExistingLoads creates, and after firing those events, check that it's still the current load. So you probably still want a boolean return value to indicate if the caller should bail out. You'll want a test for this too.

+    if (NS_SUCCEEDED(res))
+      mCurrentLoad->AddCandidate(uri);

{} around the statement

+    if (NS_SUCCEEDED(res))
+      mCurrentLoad->AddCandidate(uri);

Ditto

Can you add a test that verifies that new sources added during an asynchronous load aren't used?

Other than that, this looks really great!

Comment 5

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

Attachment: Patch v2 [Checkin: Comment 6]

* Removed "ns" prefix from inner class names in nsHTMLMediaElement.
* AbortExistingLoads() now returns PR_TRUE if the load was aborted in event handlers posted while it was running.
* Added test for "only uses sources added before a load is triggered in an async load" case.
* Added test for "bail out of a load if we do another load in an abort handler" case.
* Made nsMediaLoad::mPosition signed to remove compile warning.
* Compiles and test pass on Windows and Linux.

Comment 6

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/b71b0ebd08ed

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 362785 [details] [diff] [review]
Patch v2
[Checkin: Comment 6]

Does not apply "at all" to 1.9.1 :-(

Comment 8

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ece99f1ebc45

Comment 9

[Tony Chung [:tchung]]

making verified, code update.