Last Comment Bug 703379 - Share media resources automatically when playing the same URL
: Share media resources automatically when playing the same URL
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
: Maire Reavy [:mreavy]
Mentors:
Depends on: 715297 761485 802310 1129121 687972 705614 712836 716417
Blocks: 668449 674080 696354
  Show dependency treegraph
 
Reported: 2011-11-17 13:50 PST by Robert O'Callahan (:roc) (email my personal email if necessary)
Modified: 2015-02-04 02:19 PST (History)
11 users (show)
mounir: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Rename GetCurrentStream to GetStream (27.70 KB, patch)
2011-11-17 19:47 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
cajbir.bugzilla: review+
Details | Diff | Splinter Review
Automatically clone the decoder of an existing element with the same URI (24.37 KB, patch)
2011-11-17 19:49 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
fix v2 (27.11 KB, patch)
2011-11-20 19:49 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
fix v3 (27.00 KB, patch)
2011-11-20 19:53 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
fix v4 (30.50 KB, patch)
2011-11-21 14:00 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
cajbir.bugzilla: review+
Details | Diff | Splinter Review
Part 3: fix media cache bug (5.31 KB, patch)
2011-11-21 20:36 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
cajbir.bugzilla: review+
Details | Diff | Splinter Review
Part 4: fix download-ended notifications for shared resources (4.78 KB, patch)
2011-11-21 20:37 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
cajbir.bugzilla: review+
Details | Diff | Splinter Review
fix v5 (40.88 KB, patch)
2011-11-24 00:34 PST, Robert O'Callahan (:roc) (email my personal email if necessary)
cajbir.bugzilla: review+
Details | Diff | Splinter Review

Description Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-17 13:50:50 PST
http://www.whatwg.org/specs/web-apps/current-work/#fetch step 5 lets us do this. We need to do this to enable fast playback of multiple instances of preloaded sounds.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-17 19:47:14 PST
Created attachment 575371 [details] [diff] [review]
Rename GetCurrentStream to GetStream

Otherwise it sounds like the stream for a decoder could change, which would break the following patch horribly.
Comment 2 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-17 19:49:46 PST
Created attachment 575372 [details] [diff] [review]
Automatically clone the decoder of an existing element with the same URI

This makes mLoadingSrc always be non-null while we have an mDecoder. It makes us register all media elements in a hashtable indexed by mLoadingSrc URI. And when loading a resource, it makes us automatically clone the decoder of an existing element with the same URI and node principal, if there is one.
Comment 3 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-17 19:50:59 PST
Of course, cloning the decoder means that we share the underlying media-cache resource data and will not need to do any network requests if the resource is already fully preloaded.
Comment 4 cajbir (:cajbir) 2011-11-20 18:30:22 PST
The mochitest 'test_error_in_video_document' seems to crash with both these patches applied to trunk with:

###!!! ASSERTION: mLoadingSrc set up: 'mLoadingSrc', file /src/mozilla/mcgit/content/html/content/src/nsHTMLMediaElement.cpp, line 1978
Comment 5 cajbir (:cajbir) 2011-11-20 18:42:23 PST
Comment on attachment 575372 [details] [diff] [review]
Automatically clone the decoder of an existing element with the same URI

>+typedef nsTHashtable<MediaElementSetForURI> MediaElementURITable;
>+// Elements in this table must have non-null mDecoder and mLoadingSrc, and those
>+// can't change while the element is in the table. The table is keyed by
>+// the element's mLoadingSrc. Each entry has a list of all elements with the
>+// same mLoadingSrc.
>+static MediaElementURITable* gElementTable;

How do you prevent the mDecoder and/or mLoadingSrc from changing for elements in the gElementTable? What happens if the original element in the table has its mLoadingSrc changed due to a change in the 'src'?
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-20 19:20:43 PST
Everywhere that sets mDecoder to null calls RemoveMediaElementFromURITable() first.

The only site that sets mDecoder to non-null is FinishDecoderSetup, where mDecoder is already null. We call AddMediaElementToURITable there.

mLoadingSrc should not change while mDecoder is non-null, but maybe there is a bug that lets it do so. I'll look into that.
Comment 7 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-20 19:41:22 PST
OK, mLoadingSrc is not set along the LoadWithChannel -> InitializeDecoderForChannel path. I'll fix that.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-20 19:49:55 PST
Created attachment 575798 [details] [diff] [review]
fix v2

This should fix it. It compiles, but it's still building so I haven't tested it yet. This version requires that all callers of InitializeDecoder* have set mLoadingSrc.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-20 19:53:48 PST
Created attachment 575800 [details] [diff] [review]
fix v3

Sorry, this is better. We can make mLoadingSrc always be the original (pre-redirect) URI.
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-21 14:00:01 PST
Created attachment 575968 [details] [diff] [review]
fix v4

This version actually passes tests.

test_preload_actions revealed a bug where a call to .play() on a preload="none" element could set up a cloned decoder during the call to Load() from nsHTMLMediaElement::Play(). Play() assumed that after the Load() call there could not be a decoder, so the cloned decoder would never have Play() called on it. Fixed that by having nsHTMLMediaElement::Play() always check for a decoder.

Also made test_preload_actions run two sets of tests, one set where the preloaded resource is always the same (and the decoder gets cloned) and one set where the preloaded resources are all different.
Comment 11 cajbir (:cajbir) 2011-11-21 17:32:41 PST
I assume 'fix v3' is deprecated? 'fix v4' still gives me multiple test failures. This time it's timeouts. It's most often in 'test_seek' but also intermittent (1 in 3 runs) in 'test_bug495300' and 'test_686952'.
Comment 12 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-21 18:32:19 PST
I think I can see the seek failure intermittently. I can't see the others.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-21 20:36:16 PST
Created attachment 576078 [details] [diff] [review]
Part 3: fix media cache bug

We hit a situation where stream A for a resource had already read to the end and was holding an incomplete block (incomplete blocks aren't committed back to the cache). Another stream B for the resource reads up to the last block, then we note that it's about to read a block that's being read by A, so B stops reading and wait for A to commit its block so B can copy it. But of course A is never going to do that. So a decoder reading B will block indefinitely when it tries to read part of that last block.

The solution is to make Read() try harder to avoid blocking, by looking through all streams for the resource to see if any have partial block data that covers the data range we want to read. This means when a decoder reads the last block of the resource through B, we'll notice that A has the data and hand it over. This makes test_seek not hang.
Comment 14 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-21 20:37:35 PST
Created attachment 576079 [details] [diff] [review]
Part 4: fix download-ended notifications for shared resources

I noticed by reading the code that a cloned stream might fail to fire CacheClientNotifyDataEnded if it's cloned from a stream that has already fired that notification. This fixes that.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-23 15:33:56 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/e6be0cc7a770
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ebbe65a5949

Backed out of inbound due to build failure on Android. The rename patch didn't fix nsRawReader.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-23 15:34:42 PST
The backed-out changesets were ea58921ed9ea and fe83d9a8de5d.
Comment 18 Matt Brubeck (:mbrubeck) 2011-11-23 17:00:50 PST
The backout also fixed timeouts in test_decoder_disable.html (followed by timeouts in other tests) in Windows and Mac opt builds.
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-24 00:34:29 PST
Created attachment 576693 [details] [diff] [review]
fix v5

Some disable-decoder tests failed on tinderbox because we disabled the decoder but were still able to play a file of that type by cloning an existing decoder for the resource.

test_access_control could have a similar problem; in general, when setting preferences that can affect resource loading, we need to make sure resources loaded with non-default prefs are unique. This patch changes test_access_control and test_decoder_disable to do that.

This requires being able to add arbitrary query parameters to the URIs passed to various .sjs modules. The query parameter parsing in those modules is a bit fragile so I rewrote it.

This patch passes video mochitests on tinderbox.
Comment 20 Ed Morley [:emorley] 2011-11-24 08:24:01 PST
The parts not backed out; that have now merged to mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/0e96c5b62115
https://hg.mozilla.org/mozilla-central/rev/3ea9c8656500

Leaving open for the rest to reland.
Comment 21 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-24 18:18:35 PST
Relanded on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d9de4178e98
https://hg.mozilla.org/integration/mozilla-inbound/rev/09d5d6e6dd28
Comment 23 :Ms2ger (⌚ UTC+1/+2) 2011-11-27 23:22:48 PST
Should we backout while we investigate bug 687972?
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-28 00:34:13 PST
Sure. Only the last changeset (09d5d6e6dd28) needs to be backed out.

I'm not sure whether to back out on inbound or central, though.
Comment 25 Marco Bonardo [::mak] 2011-11-28 08:31:52 PST
> I'm not sure whether to back out on inbound or central, though.

Doesn't matter, unless this is something that should at any cost be in the next Nightly. If it can wait 24 hours it can surely just go to inbound.
Comment 26 Chris Pearce (:cpearce) 2011-11-28 16:53:54 PST
I tried for several hours and I can't reproduce bug 687972 locally. It seems to most often occur on PGO and opt builds, so leaving this landed will make fixing bug 687972 easier.
Comment 27 scientes 2012-05-19 13:41:13 PDT
https://crash-stats.mozilla.com/report/index/dec2542c-91e9-405b-bb65-b9b6c2120515
https://crash-stats.mozilla.com/report/index/5160848c-9156-456d-acd4-0e3d32120515

we've got some crashes with null mLoadingSrc's coming in via nsHTMLMediaElement::SelectResource
Comment 28 Matthew Gregan [:kinetik] 2012-05-20 19:38:36 PDT
Please open a new bug.

Note You need to log in before you can comment on or make changes to this bug.