Closed Bug 703379 Opened 8 years ago Closed 8 years ago

Share media resources automatically when playing the same URL

Categories

(Core :: Audio/Video, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: roc, Assigned: roc)

References

(Depends on 1 open bug)

Details

Attachments

(4 files, 4 obsolete files)

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.
Otherwise it sounds like the stream for a decoder could change, which would break the following patch horribly.
Attachment #575371 - Flags: review?
Attachment #575371 - Flags: review? → review?(chris.double)
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)
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.
Attachment #575371 - Flags: review?(chris.double) → review+
OS: Windows 7 → All
Hardware: x86_64 → All
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 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'?
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.
OK, mLoadingSrc is not set along the LoadWithChannel -> InitializeDecoderForChannel path. I'll fix that.
Attached patch fix v2 (obsolete) — Splinter Review
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)
Attached patch fix v3 (obsolete) — Splinter Review
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)
Blocks: 696354
Attached patch fix v4 (obsolete) — Splinter Review
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)
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'.
Attachment #575800 - Attachment is obsolete: true
Attachment #575800 - Flags: review?(chris.double)
I think I can see the seek failure intermittently. I can't see the others.
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)
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)
Attachment #576078 - Flags: review?(chris.double) → review+
Attachment #575968 - Flags: review?(chris.double) → review+
Attachment #576079 - Flags: review?(chris.double) → review+
The backed-out changesets were ea58921ed9ea and fe83d9a8de5d.
The backout also fixed timeouts in test_decoder_disable.html (followed by timeouts in other tests) in Windows and Mac opt builds.
Attached patch fix v5Splinter Review
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)
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.
Attachment #576693 - Flags: review?(chris.double) → review+
https://hg.mozilla.org/mozilla-central/rev/1d9de4178e98
https://hg.mozilla.org/mozilla-central/rev/09d5d6e6dd28
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla11
Depends on: 687972
Should we backout while we investigate bug 687972?
Sure. Only the last changeset (09d5d6e6dd28) needs to be backed out.

I'm not sure whether to back out on inbound or central, though.
> 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.
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.
Depends on: 705614
No longer depends on: 713381
Please open a new bug.
No longer blocks: 668449, 674080, 696354
No longer depends on: 687972, 715297, 705614, 712836, 716417
Depends on: 761485
Depends on: 802310
Depends on: 1129121
Depends on: 1413108
You need to log in before you can comment on or make changes to this bug.