Attachment 520044 Details for Bug 639391

[patch] Patch 5: Ensure WebM GetBuffered() is threadsafe

639391-5-webm-buffered-threadsafe.patch (text/plain), 10.00 KB, created by Chris Pearce [:cpearce (Not reading bugmail)]

(hide)

Description:

Filename:

MIME Type:

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

Size: 10.00 KB

patch

obsolete

># HG changeset patch
># Parent c1d55fe78dabdbea755cb0fcba026c543f6a070c
>Bug 639391 - Ensure WebM GetBuffered() is threadsafe. r=?
>
>diff --git a/content/media/webm/nsWebMBufferedParser.cpp b/content/media/webm/nsWebMBufferedParser.cpp
>--- a/content/media/webm/nsWebMBufferedParser.cpp
>+++ b/content/media/webm/nsWebMBufferedParser.cpp
>@@ -34,16 +34,19 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsAlgorithm.h"
> #include "nsWebMBufferedParser.h"
> #include "nsTimeRanges.h"
>+#include "nsThreadUtils.h"
>+
>+using mozilla::MonitorAutoEnter;
> 
> static const double NS_PER_S = 1e9;
> static const double MS_PER_S = 1e3;
> 
> static PRUint32
> VIntLength(unsigned char aFirstByte, PRUint32* aMask)
> {
>   PRUint32 count = 1;
>@@ -58,18 +61,21 @@ VIntLength(unsigned char aFirstByte, PRU
>   if (aMask) {
>     *aMask = mask;
>   }
>   NS_ASSERTION(count >= 1 && count <= 8, "Insane VInt length.");
>   return count;
> }
> 
> void nsWebMBufferedParser::Append(const unsigned char* aBuffer, PRUint32 aLength,
>-                                  nsTArray<nsWebMTimeDataOffset>& aMapping)
>+                                  nsTArray<nsWebMTimeDataOffset>& aMapping,
>+                                  Monitor& aMonitor)
> {
>+  NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>+
>   static const unsigned char CLUSTER_ID[] = { 0x1f, 0x43, 0xb6, 0x75 };
>   static const unsigned char TIMECODE_ID = 0xe7;
>   static const unsigned char BLOCKGROUP_ID = 0xa0;
>   static const unsigned char BLOCK_ID = 0xa1;
>   static const unsigned char SIMPLEBLOCK_ID = 0xa3;
> 
>   const unsigned char* p = aBuffer;
> 
>@@ -162,20 +168,23 @@ void nsWebMBufferedParser::Append(const 
>     case READ_BLOCK_TIMECODE:
>       if (mBlockTimecodeLength) {
>         mBlockTimecode <<= 8;
>         mBlockTimecode |= *p++;
>         mBlockTimecodeLength -= 1;
>       } else {
>         // It's possible we've parsed this data before, so avoid inserting
>         // duplicate nsWebMTimeDataOffset entries.
>-        PRUint32 idx;
>-        if (!aMapping.GreatestIndexLtEq(mBlockOffset, idx)) {
>-          nsWebMTimeDataOffset entry(mBlockOffset, mClusterTimecode + mBlockTimecode);
>-          aMapping.InsertElementAt(idx, entry);
>+        {
>+          MonitorAutoEnter mon(aMonitor);
>+          PRUint32 idx;
>+          if (!aMapping.GreatestIndexLtEq(mBlockOffset, idx)) {
>+            nsWebMTimeDataOffset entry(mBlockOffset, mClusterTimecode + mBlockTimecode);
>+            aMapping.InsertElementAt(idx, entry);
>+          }
>         }
> 
>         // Skip rest of block header and the block's payload.
>         mBlockSize -= mVIntLength;
>         mBlockSize -= 2;
>         mSkipBytes = PRUint32(mBlockSize);
>         mState = SKIP_DATA;
>         mNextState = ANY_BLOCK_SYNC;
>@@ -203,16 +212,18 @@ void nsWebMBufferedParser::Append(const 
>   mCurrentOffset += aLength;
> }
> 
> void nsWebMBufferedState::CalculateBufferedForRange(nsTimeRanges* aBuffered,
>                                                     PRInt64 aStartOffset, PRInt64 aEndOffset,
>                                                     PRUint64 aTimecodeScale,
>                                                     PRInt64 aStartTimeOffsetNS)
> {
>+  MonitorAutoEnter mon(mMonitor);
>+
>   // Find the first nsWebMTimeDataOffset at or after aStartOffset.
>   PRUint32 start;
>   mTimeMapping.GreatestIndexLtEq(aStartOffset, start);
>   if (start == mTimeMapping.Length()) {
>     return;
>   }
> 
>   // Find the first nsWebMTimeDataOffset at or before aEndOffset.
>@@ -246,16 +257,17 @@ void nsWebMBufferedState::CalculateBuffe
> 
>   double startTime = (mTimeMapping[start].mTimecode * aTimecodeScale - aStartTimeOffsetNS) / NS_PER_S;
>   double endTime = (mTimeMapping[end].mTimecode * aTimecodeScale - aStartTimeOffsetNS) / NS_PER_S;
>   aBuffered->Add(startTime, endTime);
> }
> 
> void nsWebMBufferedState::NotifyDataArrived(const char* aBuffer, PRUint32 aLength, PRUint32 aOffset)
> {
>+  NS_ASSERTION(NS_IsMainThread(), "Should be on main thread.");
>   PRUint32 idx;
>   if (!mRangeParsers.GreatestIndexLtEq(aOffset, idx)) {
>     // If the incoming data overlaps an already parsed range, adjust the
>     // buffer so that we only reparse the new data.  It's also possible to
>     // have an overlap where the end of the incoming data is within an
>     // already parsed range, but we don't bother handling that other than by
>     // avoiding storing duplicate timecodes when the parser runs.
>     if (idx != mRangeParsers.Length() && mRangeParsers[idx].mStartOffset <= aOffset) {
>@@ -269,17 +281,20 @@ void nsWebMBufferedState::NotifyDataArri
>       NS_ASSERTION(adjust >= 0, "Overlap detection bug.");
>       aBuffer += adjust;
>       aLength -= PRUint32(adjust);
>     } else {
>       mRangeParsers.InsertElementAt(idx, nsWebMBufferedParser(aOffset));
>     }
>   }
> 
>-  mRangeParsers[idx].Append(reinterpret_cast<const unsigned char*>(aBuffer), aLength, mTimeMapping);
>+  mRangeParsers[idx].Append(reinterpret_cast<const unsigned char*>(aBuffer),
>+                            aLength,
>+                            mTimeMapping,
>+                            mMonitor);
> 
>   // Merge parsers with overlapping regions and clean up the remnants.
>   PRUint32 i = 0;
>   while (i + 1 < mRangeParsers.Length()) {
>     if (mRangeParsers[i].mCurrentOffset >= mRangeParsers[i + 1].mStartOffset) {
>       mRangeParsers[i + 1].mStartOffset = mRangeParsers[i].mStartOffset;
>       mRangeParsers.RemoveElementAt(i);
>     } else {
>diff --git a/content/media/webm/nsWebMBufferedParser.h b/content/media/webm/nsWebMBufferedParser.h
>--- a/content/media/webm/nsWebMBufferedParser.h
>+++ b/content/media/webm/nsWebMBufferedParser.h
>@@ -35,18 +35,20 @@
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> #if !defined(nsWebMBufferedParser_h_)
> #define nsWebMBufferedParser_h_
> 
> #include "nsISupportsImpl.h"
> #include "nsTArray.h"
>+#include "mozilla/Monitor.h"
> 
> class nsTimeRanges;
>+using mozilla::Monitor;
> 
> // Stores a stream byte offset and the scaled timecode of the block at
> // that offset.  The timecode must be scaled by the stream's timecode
> // scale before use.
> struct nsWebMTimeDataOffset
> {
>   nsWebMTimeDataOffset(PRInt64 aOffset, PRUint64 aTimecode)
>     : mOffset(aOffset), mTimecode(aTimecode)
>@@ -72,19 +74,21 @@ struct nsWebMTimeDataOffset
> // within the stream.
> struct nsWebMBufferedParser
> {
>   nsWebMBufferedParser(PRInt64 aOffset)
>     : mStartOffset(aOffset), mCurrentOffset(aOffset), mState(CLUSTER_SYNC), mClusterIDPos(0)
>   {}
> 
>   // Steps the parser through aLength bytes of data.  Always consumes
>-  // aLength bytes.  Updates mCurrentOffset before returning.
>+  // aLength bytes.  Updates mCurrentOffset before returning.  Acquires
>+  // aMonitor before using aMapping.
>   void Append(const unsigned char* aBuffer, PRUint32 aLength,
>-              nsTArray<nsWebMTimeDataOffset>& aMapping);
>+              nsTArray<nsWebMTimeDataOffset>& aMapping,
>+              Monitor& aMonitor);
> 
>   bool operator==(PRInt64 aOffset) const {
>     return mCurrentOffset == aOffset;
>   }
> 
>   bool operator<(PRInt64 aOffset) const {
>     return mCurrentOffset < aOffset;
>   }
>@@ -207,31 +211,34 @@ private:
>   PRUint32 mSkipBytes;
> };
> 
> class nsWebMBufferedState
> {
>   NS_INLINE_DECL_REFCOUNTING(nsWebMBufferedState)
> 
> public:
>-  nsWebMBufferedState() {
>+  nsWebMBufferedState() : mMonitor("nsWebMBufferedState") {
>     MOZ_COUNT_CTOR(nsWebMBufferedState);
>   }
> 
>   ~nsWebMBufferedState() {
>     MOZ_COUNT_DTOR(nsWebMBufferedState);
>   }
> 
>   void NotifyDataArrived(const char* aBuffer, PRUint32 aLength, PRUint32 aOffset);
>   void CalculateBufferedForRange(nsTimeRanges* aBuffered,
>                                  PRInt64 aStartOffset, PRInt64 aEndOffset,
>                                  PRUint64 aTimecodeScale,
>                                  PRInt64 aStartTimeOffsetNS);
> 
> private:
>+  // Synchronizes access to the mTimeMapping array.
>+  Monitor mMonitor;
>+
>   // Sorted (by offset) map of data offsets to timecodes.  Populated
>   // on the main thread as data is received and parsed by nsWebMBufferedParsers.
>   nsTArray<nsWebMTimeDataOffset> mTimeMapping;
> 
>   // Sorted (by offset) live parser instances.  Main thread only.
>   nsTArray<nsWebMBufferedParser> mRangeParsers;
> };
> 
>diff --git a/content/media/webm/nsWebMReader.cpp b/content/media/webm/nsWebMReader.cpp
>--- a/content/media/webm/nsWebMReader.cpp
>+++ b/content/media/webm/nsWebMReader.cpp
>@@ -791,32 +791,28 @@ nsresult nsWebMReader::GetBuffered(nsTim
> 
>   // Special case completely cached files.  This also handles local files.
>   if (stream->IsDataCachedToEndOfStream(0)) {
>     uint64_t duration = 0;
>     if (nestegg_duration(mContext, &duration) == 0) {
>       aBuffered->Add(0, duration / NS_PER_S);
>     }
>   } else {
>-    PRInt64 startOffset = stream->GetNextCachedData(0);
>+    nsMediaStream* stream = mDecoder->GetCurrentStream();
>+    nsTArray<nsByteRange> ranges;
>+    nsresult res = stream->GetCachedRanges(ranges);
>+    NS_ENSURE_SUCCESS(res, res);
>+
>     PRInt64 startTimeOffsetNS = aStartTime * NS_PER_MS;
>-    while (startOffset >= 0) {
>-      PRInt64 endOffset = stream->GetCachedDataEnd(startOffset);
>-      NS_ASSERTION(startOffset < endOffset, "Cached range invalid");
>-
>+    for (PRUint32 index = 0; index < ranges.Length(); index++) {
>       mBufferedState->CalculateBufferedForRange(aBuffered,
>-                                                startOffset,
>-                                                endOffset,
>+                                                ranges[index].mStart,
>+                                                ranges[index].mEnd,
>                                                 timecodeScale,
>                                                 startTimeOffsetNS);
>-
>-      // Advance to the next cached data range.
>-      startOffset = stream->GetNextCachedData(endOffset);
>-      NS_ASSERTION(startOffset == -1 || startOffset > endOffset,
>-                   "Next cached range invalid");
>     }
>   }
> 
>   return NS_OK;
> }
> 
> void nsWebMReader::NotifyDataArrived(const char* aBuffer, PRUint32 aLength, PRUint32 aOffset)
> {

Flags:

kinetik: review+

Actions: View | Diff | Review

Attachments on bug 639391: 520039 | 520040 | 520041 | 520042 | 520044 | 520045