Closed Bug 1125581 Opened 5 years ago Closed 4 years ago

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

Categories

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

x86
macOS
defect

Tracking

()

RESOLVED WONTFIX

People

(Reporter: jya, Assigned: jya)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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..
Blocks: 1118589
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.
Attachment #8554332 - Flags: review?(matt.woodrow)
Assignee: nobody → jyavenard
Status: NEW → ASSIGNED
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 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.
Attachment #8554332 - Flags: review?(matt.woodrow) → review+
(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.
rebase. Following bug 1102642, we use a SourceBufferResource outside a SourceBufferDecoder, so can't protect the various appenddata and eviction members
Attachment #8554332 - Attachment is obsolete: true
I backed it out, if caused failures on some mochitest.
I believe this is due to Ended not clearing the cache.
Priority: P2 → P3
This problem is resolved in the new MSE stack
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.