Closed
Bug 703379
Opened 13 years ago
Closed 13 years ago
Share media resources automatically when playing the same URL
Categories
(Core :: Audio/Video, defect)
Core
Audio/Video
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: roc, Assigned: roc)
References
(Depends on 1 open bug)
Details
Attachments
(4 files, 4 obsolete files)
27.70 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
5.31 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
4.78 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
40.88 KB,
patch
|
cajbir
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•13 years ago
|
||
Otherwise it sounds like the stream for a decoder could change, which would break the following patch horribly.
Attachment #575371 -
Flags: review?
Assignee | ||
Updated•13 years ago
|
Attachment #575371 -
Flags: review? → review?(chris.double)
Assignee | ||
Comment 2•13 years ago
|
||
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.
Attachment #575372 -
Flags: review?(chris.double)
Assignee | ||
Comment 3•13 years ago
|
||
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.
Updated•13 years ago
|
Attachment #575371 -
Flags: review?(chris.double) → review+
Updated•13 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
Comment 4•13 years ago
|
||
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•13 years ago
|
||
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'?
Assignee | ||
Comment 6•13 years ago
|
||
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.
Assignee | ||
Comment 7•13 years ago
|
||
OK, mLoadingSrc is not set along the LoadWithChannel -> InitializeDecoderForChannel path. I'll fix that.
Assignee | ||
Comment 8•13 years ago
|
||
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.
Attachment #575798 -
Flags: review?(chris.double)
Assignee | ||
Comment 9•13 years ago
|
||
Sorry, this is better. We can make mLoadingSrc always be the original (pre-redirect) URI.
Attachment #575372 -
Attachment is obsolete: true
Attachment #575798 -
Attachment is obsolete: true
Attachment #575372 -
Flags: review?(chris.double)
Attachment #575798 -
Flags: review?(chris.double)
Attachment #575800 -
Flags: review?(chris.double)
Assignee | ||
Comment 10•13 years ago
|
||
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.
Attachment #575968 -
Flags: review?(chris.double)
Comment 11•13 years ago
|
||
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'.
Assignee | ||
Updated•13 years ago
|
Attachment #575800 -
Attachment is obsolete: true
Attachment #575800 -
Flags: review?(chris.double)
Assignee | ||
Comment 12•13 years ago
|
||
I think I can see the seek failure intermittently. I can't see the others.
Assignee | ||
Comment 13•13 years ago
|
||
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.
Attachment #576078 -
Flags: review?(chris.double)
Assignee | ||
Comment 14•13 years ago
|
||
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.
Attachment #576079 -
Flags: review?(chris.double)
Updated•13 years ago
|
Attachment #576078 -
Flags: review?(chris.double) → review+
Updated•13 years ago
|
Attachment #575968 -
Flags: review?(chris.double) → review+
Updated•13 years ago
|
Attachment #576079 -
Flags: review?(chris.double) → review+
Assignee | ||
Comment 15•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/10f58db28c81
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d4b3e893fe4
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e96c5b62115
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ea9c8656500
Whiteboard: [inbound]
Assignee | ||
Comment 16•13 years ago
|
||
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.
Assignee | ||
Comment 17•13 years ago
|
||
The backed-out changesets were ea58921ed9ea and fe83d9a8de5d.
Comment 18•13 years ago
|
||
The backout also fixed timeouts in test_decoder_disable.html (followed by timeouts in other tests) in Windows and Mac opt builds.
Assignee | ||
Comment 19•13 years ago
|
||
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.
Attachment #575968 -
Attachment is obsolete: true
Attachment #576693 -
Flags: review?(chris.double)
Comment 20•13 years ago
|
||
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.
Updated•13 years ago
|
Attachment #576693 -
Flags: review?(chris.double) → review+
Assignee | ||
Comment 21•13 years ago
|
||
Comment 22•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1d9de4178e98
https://hg.mozilla.org/mozilla-central/rev/09d5d6e6dd28
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
Comment 23•13 years ago
|
||
Should we backout while we investigate bug 687972?
Assignee | ||
Comment 24•13 years ago
|
||
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•13 years ago
|
||
> 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•13 years ago
|
||
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•13 years ago
|
||
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•13 years ago
|
||
Please open a new bug.
Updated•13 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•