Bug 1120023 - Clean up semantics of SourceBufferResource reading. r=cpearce, a=sledru
authorBobby Holley <bobbyholley@gmail.com>
Sat, 10 Jan 2015 02:05:28 -0800
changeset 242861 60f6890d84cf
parent 242860 aa8cdb057186
child 242862 e5cc2f8f3f7e
push id4322
push userryanvm@gmail.com
push date2015-01-14 15:18 +0000
treeherdermozilla-beta@82cce51fb174 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscpearce, sledru
bugs1120023
milestone36.0
Bug 1120023 - Clean up semantics of SourceBufferResource reading. r=cpearce, a=sledru This patch refactors things and makes two function changes: (1) ReadFromCache does not block and properly fails if the data is unavailable. (2) Read and ReadAt block if an out-param is _not_ provided, rather than the reverse. Both karlt and I think this is the appropriate thing to do.
dom/media/mediasource/SourceBufferResource.cpp
dom/media/mediasource/SourceBufferResource.h
--- a/dom/media/mediasource/SourceBufferResource.cpp
+++ b/dom/media/mediasource/SourceBufferResource.cpp
@@ -43,52 +43,79 @@ SourceBufferResource::Close()
 }
 
 nsresult
 SourceBufferResource::Read(char* aBuffer, uint32_t aCount, uint32_t* aBytes)
 {
   SBR_DEBUGV("SourceBufferResource(%p)::Read(aBuffer=%p, aCount=%u, aBytes=%p)",
              this, aBytes, aCount, aBytes);
   ReentrantMonitorAutoEnter mon(mMonitor);
-  bool blockingRead = !!aBytes;
+
+  return ReadInternal(aBuffer, aCount, aBytes, /* aMayBlock = */ !aBytes);
+}
 
-  while (blockingRead &&
+nsresult
+SourceBufferResource::ReadInternal(char* aBuffer, uint32_t aCount, uint32_t* aBytes, bool aMayBlock)
+{
+  mMonitor.AssertCurrentThreadIn();
+  MOZ_ASSERT_IF(!aMayBlock, aBytes);
+
+  // Cache the offset for the read in case mOffset changes while waiting on the
+  // monitor below. It's basically impossible to implement these API semantics
+  // sanely. :-(
+  uint64_t readOffset = mOffset;
+
+  while (aMayBlock &&
          !mEnded &&
-         mOffset + aCount > static_cast<uint64_t>(GetLength())) {
-    SBR_DEBUGV("SourceBufferResource(%p)::Read waiting for data", this);
-    mon.Wait();
+         readOffset + aCount > static_cast<uint64_t>(GetLength())) {
+    SBR_DEBUGV("SourceBufferResource(%p)::ReadInternal waiting for data", this);
+    mMonitor.Wait();
   }
 
-  uint32_t available = GetLength() - mOffset;
+  uint32_t available = GetLength() - readOffset;
   uint32_t count = std::min(aCount, available);
-  SBR_DEBUGV("SourceBufferResource(%p)::Read() mOffset=%llu GetLength()=%u available=%u count=%u mEnded=%d",
-             this, mOffset, GetLength(), available, count, mEnded);
+  SBR_DEBUGV("SourceBufferResource(%p)::ReadInternal() readOffset=%llu GetLength()=%u available=%u count=%u mEnded=%d",
+             this, readOffset, GetLength(), available, count, mEnded);
   if (available == 0) {
-    SBR_DEBUGV("SourceBufferResource(%p)::Read() reached EOF", this);
+    SBR_DEBUGV("SourceBufferResource(%p)::ReadInternal() reached EOF", this);
     *aBytes = 0;
     return NS_OK;
   }
 
-  mInputBuffer.CopyData(mOffset, count, aBuffer);
+  mInputBuffer.CopyData(readOffset, count, aBuffer);
   *aBytes = count;
-  mOffset += count;
+
+  // From IRC:
+  // <@cpearce>bholley: *this* is why there should only every be a ReadAt() and
+  // no Read() on a Stream abstraction! there's no good answer, they all suck.
+  mOffset = readOffset + count;
+
   return NS_OK;
 }
 
 nsresult
 SourceBufferResource::ReadAt(int64_t aOffset, char* aBuffer, uint32_t aCount, uint32_t* aBytes)
 {
   SBR_DEBUG("SourceBufferResource(%p)::ReadAt(aOffset=%lld, aBuffer=%p, aCount=%u, aBytes=%p)",
             this, aOffset, aBytes, aCount, aBytes);
   ReentrantMonitorAutoEnter mon(mMonitor);
+  return ReadAtInternal(aOffset, aBuffer, aCount, aBytes, /* aMayBlock = */ !aBytes);
+}
+
+nsresult
+SourceBufferResource::ReadAtInternal(int64_t aOffset, char* aBuffer, uint32_t aCount, uint32_t* aBytes,
+                                     bool aMayBlock)
+{
+  mMonitor.AssertCurrentThreadIn();
   nsresult rv = SeekInternal(aOffset);
   if (NS_FAILED(rv)) {
     return rv;
   }
-  return Read(aBuffer, aCount, aBytes);
+
+  return ReadInternal(aBuffer, aCount, aBytes, aMayBlock);
 }
 
 nsresult
 SourceBufferResource::Seek(int32_t aWhence, int64_t aOffset)
 {
   SBR_DEBUG("SourceBufferResource(%p)::Seek(aWhence=%d, aOffset=%lld)", this, aWhence, aOffset);
   ReentrantMonitorAutoEnter mon(mMonitor);
 
@@ -129,21 +156,24 @@ SourceBufferResource::SeekInternal(int64
 }
 
 nsresult
 SourceBufferResource::ReadFromCache(char* aBuffer, int64_t aOffset, uint32_t aCount)
 {
   SBR_DEBUG("SourceBufferResource(%p)::ReadFromCache(aBuffer=%p, aOffset=%lld, aCount=%u)",
             this, aBuffer, aOffset, aCount);
   ReentrantMonitorAutoEnter mon(mMonitor);
+  uint32_t bytesRead;
   int64_t oldOffset = mOffset;
-  uint32_t bytesRead;
-  nsresult rv = ReadAt(aOffset, aBuffer, aCount, &bytesRead);
-  mOffset = oldOffset;
-  return rv;
+  nsresult rv = ReadAtInternal(aOffset, aBuffer, aCount, &bytesRead, /* aMayBlock = */ false);
+  mOffset = oldOffset; // ReadFromCache isn't supposed to affect the seek position.
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  // ReadFromCache return failure if not all the data is cached.
+  return bytesRead == aCount ? NS_OK : NS_ERROR_FAILURE;
 }
 
 uint32_t
 SourceBufferResource::EvictData(uint32_t aThreshold)
 {
   SBR_DEBUG("SourceBufferResource(%p)::EvictData(aThreshold=%u)", this, aThreshold);
   ReentrantMonitorAutoEnter mon(mMonitor);
   return mInputBuffer.Evict(mOffset, aThreshold);
--- a/dom/media/mediasource/SourceBufferResource.h
+++ b/dom/media/mediasource/SourceBufferResource.h
@@ -130,16 +130,18 @@ public:
   void Dump(const char* aPath) {
     mInputBuffer.Dump(aPath);
   }
 #endif
 
 private:
   ~SourceBufferResource();
   nsresult SeekInternal(int64_t aOffset);
+  nsresult ReadInternal(char* aBuffer, uint32_t aCount, uint32_t* aBytes, bool aMayBlock);
+  nsresult ReadAtInternal(int64_t aOffset, char* aBuffer, uint32_t aCount, uint32_t* aBytes, bool aMayBlock);
 
   const nsCString mType;
 
   // Provides synchronization between SourceBuffers and InputAdapters.
   // Protects all of the member variables below.  Read() will await a
   // Notify() (from Seek, AppendData, Ended, or Close) when insufficient
   // data is available in mData.
   mutable ReentrantMonitor mMonitor;