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.

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.

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.

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.

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.

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.

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.

Use the nsMediaStream's cached ranges to determine buffered time ranges. Based on top of patches from bug 580531, bug 638617, and bug 628665.

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.

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

(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.

+ NS_ASSERTION(NS_IsMainThread(), "Should be on main thread."); This is already asserted higher in the call chain.

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

Can someone confirm if this is fixed?