Bug 1120023 - Clean up semantics of SourceBufferResource reading. r=cpearce

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

@@ -48,28 +48,46 @@ 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;
 
-  while (blockingRead &&
+  return ReadInternal(aBuffer, aCount, aBytes, /* aMayBlock = */ !aBytes);
+}
+
+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;
 }
 
@@ -79,11 +97,20 @@ SourceBufferResource::ReadAt(int64_t aOffset, char* aBuffer, uint32_t aCount, ui
   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
@@ -134,11 +161,14 @@ SourceBufferResource::ReadFromCache(char* aBuffer, int64_t aOffset, uint32_t aCo
   SBR_DEBUG("SourceBufferResource(%p)::ReadFromCache(aBuffer=%p, aOffset=%lld, aCount=%u)",
             this, aBuffer, aOffset, aCount);
   ReentrantMonitorAutoEnter mon(mMonitor);
-  int64_t oldOffset = mOffset;
   uint32_t bytesRead;
-  nsresult rv = ReadAt(aOffset, aBuffer, aCount, &bytesRead);
-  mOffset = oldOffset;
-  return rv;
+  int64_t oldOffset = mOffset;
+  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

dom/media/mediasource/SourceBufferResource.h

@@ -135,6 +135,8 @@ public:
 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;