703379 - Share media resources automatically when playing the same URL

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

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

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)]

Attachment: 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)]

Attachment: 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)]

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)]

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)]

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)]

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)]

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)]

Attachment: 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)]

Attachment: 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)]

Attachment: 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)]

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)]

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)]

Attachment: 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)]

Attachment: 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 15

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

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

Comment 16

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

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)]

The backed-out changesets were ea58921ed9ea and fe83d9a8de5d.

Comment 18

[Matt Brubeck (:mbrubeck)]

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)]

Attachment: 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]]

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)]

Relanded on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d9de4178e98
https://hg.mozilla.org/integration/mozilla-inbound/rev/09d5d6e6dd28

Comment 22

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/1d9de4178e98
https://hg.mozilla.org/mozilla-central/rev/09d5d6e6dd28

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Should we backout while we investigate bug 687972?

Comment 24

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

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] (Away Apr 25 - May 5)]

> 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 (Not reading bugmail)]]

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]

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]]

Please open a new bug.