Bug 1120023 - Clean up semantics of SourceBufferResource reading. r=cpearce
authorBobby Holley <bobbyholley@gmail.com>
Sat, 10 Jan 2015 02:05:28 -0800
changeset 248938 8ce96306c2d339d94a49a442d5c56320b13b8169
parent 248937 6dab2820469ae49c71d3bd7045d44a915810f7b5
child 248939 677f21c77f7e092f4187342f43694c55463c164d
push id4489
push userraliiev@mozilla.com
push dateMon, 23 Feb 2015 15:17:55 +0000
treeherdermozilla-beta@fd7c3dc24146 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscpearce
bugs1120023
milestone37.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
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;