1125581 - SourceBuffer.Buffered result should be cached rather than computed each time.

Status: RESOLVED WONTFIX

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

Description

[Jean-Yves Avenard [:jya]]

Right now, for every audio or video frame demuxed, we loop through the list of decoders in each trackbuffer, and compute the data range buffered ; which in itself loops through each interval and calculate the union of those time ranges.

This is done up to 60 times per second for video, and several thousand times per second for audio.

However, the buffered time range will only ever change when the duration is changed (by modifying the duration attribute), when appendBuffer is called or sourcebuffer.remove() is called.

As such, we should only calculate the buffered range after those operations, and use a cache value in MediaSourceReader::Request*Data.

Comment 1

[Jean-Yves Avenard [:jya]]

Some stats..

Watching a 144p video on YouTube.
6 video SourceBufferDecoder
1 audio SourceBufferDecoder.

1007489024[137ff7eb0]: SourceBufferDecoder[audio/mp4]: Called 7697 in 123.008119, 64/s
1007489024[137ff7eb0]: SourceBufferDecoder[video/mp4]: Called 3934 in 123.153848, 32/s
1007489024[137ff7eb0]: SourceBufferDecoder[video/mp4]: Called 3657 in 118.942579, 31/s
1007489024[137ff7eb0]: SourceBufferDecoder[video/mp4]: Called 4233 in 108.457366, 39/s
1007489024[137ff7eb0]: SourceBufferDecoder[video/mp4]: Called 3100 in 77.822097, 40/s
1007489024[137ff7eb0]: SourceBufferDecoder[video/mp4]: Called 2272 in 55.561781, 41/s
1007489024[137ff7eb0]: SourceBufferDecoder[video/mp4]: Called 1997 in 39.839030, 50/s

The calculation (two loops) runs near 300 times per seconds..

Comment 2

[Jean-Yves Avenard [:jya]]

Attachment: Cache buffered TimeRanges and only recalculate as necessary

Cache GetBuffered result. This also fixes a potential race, which appears to be the reason test_eme_playback.html failed earlier on. Data is to be removed from the SourceBufferResource via the owning SourceBufferDecoder.

Comment 3

[Jean-Yves Avenard [:jya]]

With: https://treeherder.mozilla.org/#/jobs?repo=try&revision=800e10d61705
without: https://treeherder.mozilla.org/#/jobs?repo=try&revision=629eb53ec2fd

surprising result!

Comment 4

[Jean-Yves Avenard [:jya]]

consistent result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fae16d97243

Comment 5

[Jean-Yves Avenard [:jya]]

Actually, upon further look, I think it's the late change to MediaSourceReader::Request*Data
http://mxr.mozilla.org/comm-central/source/mozilla/dom/media/mediasource/MediaSourceReader.cpp#276

added by bug 1096089, that is the actual reason. Just happened to coincide when I was working on this patch.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8554332 [details] [diff] [review]
Cache buffered TimeRanges and only recalculate as necessary

Review of attachment 8554332 [details] [diff] [review]:
-----------------------------------------------------------------

This seems fine, but do we have any evidence that this is actually a performance issue?

I know it's called a lot, but if it's still fast then adding the complexity of a cache might not be worth it.

::: dom/media/mediasource/SourceBufferDecoder.cpp
@@ +210,5 @@
> +}
> +
> +void SourceBufferDecoder::AppendData(const uint8_t* aData, uint32_t aLength)
> +{
> +  mCacheMonitor.Lock();

Use an anonymous scope and MonitorAutoLock instead of manual locking.

Comment 7

[Jean-Yves Avenard [:jya]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> This seems fine, but do we have any evidence that this is actually a
> performance issue?

I can time that call.

> 
> I know it's called a lot, but if it's still fast then adding the complexity
> of a cache might not be worth it.

actually, the code reason for this patch other than performance was to streamline all access to the mediasource resource via its decoder. Right now, it's very unclear and a bit all over the place. Data is accessed from the decoder, but trackbuffer adds it to the resource directly. and all use that one giant lock for everything

> 
> ::: dom/media/mediasource/SourceBufferDecoder.cpp
> @@ +210,5 @@
> > +}
> > +
> > +void SourceBufferDecoder::AppendData(const uint8_t* aData, uint32_t aLength)
> > +{
> > +  mCacheMonitor.Lock();
> 
> Use an anonymous scope and MonitorAutoLock instead of manual locking.

I thought it was more explicit to Lock/Unlock exactly where I wanted it to, rather that using AutoLock/AutoUnlock which here would add an extra lock/unlock unecessary sequence.

Comment 8

[Jean-Yves Avenard [:jya]]

Attachment: Cache buffered TimeRanges and only recalculate as necessary

rebase. Following bug 1102642, we use a SourceBufferResource outside a SourceBufferDecoder, so can't protect the various appenddata and eviction members

Comment 9

[Jean-Yves Avenard [:jya]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6dbc7775c83

Comment 10

[Jean-Yves Avenard [:jya]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b7b87042f254
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0e9b37fd47c6

Comment 11

[Jean-Yves Avenard [:jya]]

I backed it out, if caused failures on some mochitest.
I believe this is due to Ended not clearing the cache.

Comment 12

[Jean-Yves Avenard [:jya]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/982c4dbb6927

Comment 13

[Jean-Yves Avenard [:jya]]

This problem is resolved in the new MSE stack