639391 - Make nsMediaDecoder::GetBuffered() implementations threadsafe

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

In order to use the GetBuffered() implementations to help us decide when to pause playback in order to buffer, we need to make it threadsafe. Currently it assumes we only call GetBuffered() from the main thread, but it needs to run on the state machine and decode threads for the patches in bug 628665 to work correctly.

Comment 1

[Chris Pearce [:cpearce (Not reading bugmail)]]

To make GetBuffered() threadsafe, we need to ensure that we pin the media stream while determining which ranges are buffered and also while using said ranges. We also need to ensure that new data isn't added to the cache while we're determining what ranges are buffered. We determine what ranges are buffered by looping and calling nsMediaCacheStream::GetNextCachedData() and GetCachedDataEnd(). But if we're unlucky and the range we're on increases in size, we could end up creating a new range for every block of data received. So we need to lock the media cache while determining what byte ranges are buffered.

So I propose we factor out the GetNextCachedData()/GetCachedDataEnd() loop pattern we use in all our GetBuffered() implementations, and put that into nsMediaStream, having it fill a vector of ByteRanges while holding the media cache lock. Outside of the lock (in the GetBuffered()) implementations, we can use the vector of ByteRanges to determine their start and end times, without needing to hold the media cache lock.

Comment 2

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

OK but we will need to protect other structures too, right?

GetBuffered() is only called while we're in the buffering state, right? Maybe we could simply proxy the call to the main thread via an event.

Comment 3

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes, the readers need some changes to make them threadsafe too.

We also use GetBuffered() on the decoder thread in HasLowUndecodedData() (once bug bug 628665 lands) to determine whether we should skip-to-keyframe. We also use it to test whether we should go into buffering (but only if HasLowDecodedData() is true). So proxying to the main thread doesn't seem like a good idea.

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1: Push Ogg specific seeking stuff down into nsOggReader

nsBuiltinDecoderReader contains GetBufferedBytes() and GetSeekRange() functions and a ByteRange class which are only used in nsOggReader for seeking. ByteRange also stores the exact time at the start and end of its byte range. GetBufferedBytes() finds the exact start and end times ranges contain by decoding data, whereas nsOggReader::GetBuffered() finds approximate times using the container format, which is quicker, but not accurate enough to be used during seeking.

Since this stuff is Ogg specific, push it down into nsOggReader, and rename ByteRange to SeekRange, since it represents more than just a range of bytes.

Based on top of patches from bug 580531, bug 638617, and bug 628665.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2: Make nsMediaStream responsible for determining cached media data in a threadsafe manner

Add a GetCachedRanges(nsTArray<nsByteRange>&) function to nsMediaStream, which fills an array with the byte ranges which are cached in a threadsafe manner (it holds the media cache lock while running, so data can't be added or removed from the cache while running). Add a nsByteRange class in nsMediaStream, which represents a [start,end] byte range. We can then ask the nsMediaStream what ranges it has cached, use the cached ranges to determine the times stored in those ranges, and we won't notice (or be affected or care) if the ranges grow while we're determining the times they contain. Provided we pin the nsMediaStream while using the ranges, the ranges won't shrink either.

Based on top of patches from bug 580531, bug 638617, and bug 628665.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3: Pin nsMediaStream while determining buffered ranges

Ensure that while we call nsBuiltinDecoderReader::GetBuffered() implementations, we've pinned the media stream. This ensures that data isn't evicted while we're determining the times that cached ranges contain.

Based on top of patches from bug 580531, bug 638617, and bug 628665.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 4: Ensure Ogg GetBuffered() is threadsafe

Use the nsMediaStream's cached ranges to determine buffered time ranges.

Based on top of patches from bug 580531, bug 638617, and bug 628665.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 5: Ensure WebM GetBuffered() is threadsafe

Use the nsMediaStream's cached ranges to determine buffered time ranges. Use a monitor to protect access to the byte range to time mapping in nsWebMBufferedState, to make it threadsafe.

Based on top of patches from bug 580531, bug 638617, and bug 628665.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 6: Ensure nsWaveReader::GetBuffered() is threadsafe

Use the nsMediaStream's cached ranges to determine buffered time ranges in the new nsWaveReader which uses the nsBuilinDecoder framework (bug 635649).

Based on top of patches from bug 580531, bug 638617, bug 628665 _and bug 635649_.

Comment 10

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

Comment on attachment 520042 [details] [diff] [review]
Patch 4: Ensure Ogg GetBuffered() is threadsafe

Now that nsOggReader::GetSeekRanges calls GetCachedRanges, what ensures that the stream is pinned during GetSeekRanges?

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #10)
> Comment on attachment 520042 [details] [diff] [review]
> Patch 4: Ensure Ogg GetBuffered() is threadsafe
> 
> Now that nsOggReader::GetSeekRanges calls GetCachedRanges, what ensures that
> the stream is pinned during GetSeekRanges?

That's called only while we're seeking, so we should be pinned already. GetCachedRanges() asserts that it's pinned, so we'll hear about it if the stream is not pinned.

Comment 12

[Matthew Gregan [:kinetik]]

+  NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");

This is already asserted higher in the call chain.

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

http://hg.mozilla.org/mozilla-central/rev/9e2d5f8192dc
http://hg.mozilla.org/mozilla-central/rev/40bc6bd8ceb2
http://hg.mozilla.org/mozilla-central/rev/68ddaa1facdb
http://hg.mozilla.org/mozilla-central/rev/d3aeb102da2b
http://hg.mozilla.org/mozilla-central/rev/4902d72f6072

Comment 14

[u279076]

Can someone confirm if this is fixed?