Bug 1516011 - Part 2. Deny recycling for frames used in blob recordings. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Thu, 10 Jan 2019 07:42:12 -0500
changeset 510881 c5f982e028923c2465f2d47303a9e9769912ea77
parent 510880 7ed5d740825007c2a3e04a749e7965de6874d2cb
child 510882 c15d04c80f5b966cd0cb5bee952eb28b7c96d9ee
push id10547
push userffxbld-merge
push dateMon, 21 Jan 2019 13:03:58 +0000
treeherdermozilla-beta@24ec1916bffe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1516011
milestone66.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 1516011 - Part 2. Deny recycling for frames used in blob recordings. r=tnikkel Given the crash resolved in part 1, it is possible for the blob rasterizer in the compositor process to still be using surfaces after the animation has advanced to the next frame. With recycling this can be problematic as the recycled surface will be reused for a future frame. In an ideal world, the blob recording would use the animation's image key instead, but the rasterizer doesn't have easy access to the mapping table. As such, for any frames used in a blob recording, we now explicitly mark them as non-recyclable and we will be forced to allocate a new frame instead. Differential Revision: https://phabricator.services.mozilla.com/D16192
image/AnimationFrameBuffer.cpp
image/Decoder.cpp
image/imgFrame.cpp
image/imgFrame.h
image/test/gtest/TestAnimationFrameBuffer.cpp
--- a/image/AnimationFrameBuffer.cpp
+++ b/image/AnimationFrameBuffer.cpp
@@ -347,19 +347,17 @@ void AnimationFrameRecyclingQueue::Advan
   }
 
   RefPtr<imgFrame>& front = mDisplay.front();
   RecycleEntry newEntry(mForceUseFirstFrameRefreshArea ? mFirstFrameRefreshArea
                                                        : front->GetDirtyRect());
 
   // If we are allowed to recycle the frame, then we should save it before the
   // base class's AdvanceInternal discards it.
-  if (front->ShouldRecycle()) {
-    newEntry.mFrame = std::move(front);
-  }
+  newEntry.mFrame = std::move(front);
 
   // Even if the frame itself isn't saved, we want the dirty rect to calculate
   // the recycle rect for future recycled frames.
   mRecycle.push_back(std::move(newEntry));
   mDisplay.pop_front();
   MOZ_ASSERT(!mDisplay.empty());
   MOZ_ASSERT(mDisplay.front());
 
@@ -388,21 +386,19 @@ void AnimationFrameRecyclingQueue::Advan
 bool AnimationFrameRecyclingQueue::ResetInternal() {
   // We should save any display frames that we can to save on at least the
   // allocation. The first frame refresh area is guaranteed to be the aggregate
   // dirty rect or the entire frame, and so the bare minimum area we can
   // recycle. We don't need to worry about updating the dirty rect for the
   // existing mRecycle entries, because that will happen in RecycleFrame when
   // we try to pull out a frame to redecode the first frame.
   for (RefPtr<imgFrame>& frame : mDisplay) {
-    if (frame->ShouldRecycle()) {
-      RecycleEntry newEntry(mFirstFrameRefreshArea);
-      newEntry.mFrame = std::move(frame);
-      mRecycle.push_back(std::move(newEntry));
-    }
+    RecycleEntry newEntry(mFirstFrameRefreshArea);
+    newEntry.mFrame = std::move(frame);
+    mRecycle.push_back(std::move(newEntry));
   }
 
   return AnimationFrameDiscardingQueue::ResetInternal();
 }
 
 RawAccessFrameRef AnimationFrameRecyclingQueue::RecycleFrame(
     gfx::IntRect& aRecycleRect) {
   if (mInsertIndex == 0) {
--- a/image/Decoder.cpp
+++ b/image/Decoder.cpp
@@ -345,18 +345,17 @@ RawAccessFrameRef Decoder::AllocateFrame
     ref = mFrameRecycler->RecycleFrame(mRecycleRect);
     if (ref) {
       // If the recycled frame is actually the current restore frame, we cannot
       // use it. If the next restore frame is the new frame we are creating, in
       // theory we could reuse it, but we would need to store the restore frame
       // animation parameters elsewhere. For now we just drop it.
       bool blocked = ref.get() == mRestoreFrame.get();
       if (!blocked) {
-        nsresult rv = ref->InitForDecoderRecycle(aAnimParams.ref());
-        blocked = NS_WARN_IF(NS_FAILED(rv));
+        blocked = NS_FAILED(ref->InitForDecoderRecycle(aAnimParams.ref()));
       }
 
       if (blocked) {
         ref.reset();
       }
     }
   }
 
--- a/image/imgFrame.cpp
+++ b/image/imgFrame.cpp
@@ -300,17 +300,22 @@ nsresult imgFrame::InitForDecoder(const 
 nsresult imgFrame::InitForDecoderRecycle(const AnimationParams& aAnimParams) {
   // We want to recycle this frame, but there is no guarantee that consumers are
   // done with it in a timely manner. Let's ensure they are done with it first.
   MonitorAutoLock lock(mMonitor);
 
   MOZ_ASSERT(mIsFullFrame);
   MOZ_ASSERT(mLockCount > 0);
   MOZ_ASSERT(mLockedSurface);
-  MOZ_ASSERT(mShouldRecycle);
+
+  if (!mShouldRecycle) {
+    // This frame either was never marked as recyclable, or the flag was cleared
+    // for a caller which does not support recycling.
+    return NS_ERROR_NOT_AVAILABLE;
+  }
 
   if (mRecycleLockCount > 0) {
     if (NS_IsMainThread()) {
       // We should never be both decoding and recycling on the main thread. Sync
       // decoding can only be used to produce the first set of frames. Those
       // either never use recycling because advancing was blocked (main thread
       // is busy) or we were auto-advancing (to seek to a frame) and the frames
       // were never accessed (and thus cannot have recycle locks).
@@ -619,27 +624,35 @@ bool imgFrame::Draw(gfxContext* aContext
 
     // Most draw targets will just use the surface only during DrawPixelSnapped
     // but captures/recordings will retain a reference outside this stack
     // context. While in theory a decoder thread could be trying to recycle this
     // frame at this very moment, in practice the only way we can get here is if
     // this frame is the current frame of the animation. Since we can only
     // advance on the main thread, we know nothing else will try to use it.
     DrawTarget* drawTarget = aContext->GetDrawTarget();
-    bool temporary = !drawTarget->IsCaptureDT() &&
-                     drawTarget->GetBackendType() != BackendType::RECORDING;
+    bool recording = drawTarget->GetBackendType() == BackendType::RECORDING;
+    bool temporary = !drawTarget->IsCaptureDT() && !recording;
     RefPtr<SourceSurface> surf = GetSourceSurfaceInternal(temporary);
     if (!surf) {
       return false;
     }
 
     bool doTile = !imageRect.Contains(aRegion.Rect()) &&
                   !(aImageFlags & imgIContainer::FLAG_CLAMP);
 
     surfaceResult = SurfaceForDrawing(doPartialDecode, doTile, region, surf);
+
+    // If we are recording, then we cannot recycle the surface. The blob
+    // rasterizer is not properly synchronized for recycling in the compositor
+    // process. The easiest thing to do is just mark the frames it consumes as
+    // non-recyclable.
+    if (recording && surfaceResult.IsValid()) {
+      mShouldRecycle = false;
+    }
   }
 
   if (surfaceResult.IsValid()) {
     gfxUtils::DrawPixelSnapped(aContext, surfaceResult.mDrawable,
                                imageRect.Size(), region, surfaceResult.mFormat,
                                aSamplingFilter, aImageFlags, aOpacity);
   }
 
--- a/image/imgFrame.h
+++ b/image/imgFrame.h
@@ -197,18 +197,16 @@ class imgFrame {
   const IntRect& GetDirtyRect() const { return mDirtyRect; }
   void SetDirtyRect(const IntRect& aDirtyRect) { mDirtyRect = aDirtyRect; }
 
   bool IsFullFrame() const { return mIsFullFrame; }
 
   bool GetCompositingFailed() const;
   void SetCompositingFailed(bool val);
 
-  bool ShouldRecycle() const { return mShouldRecycle; }
-
   void SetOptimizable();
 
   void FinalizeSurface();
   already_AddRefed<SourceSurface> GetSourceSurface();
 
   struct AddSizeOfCbData {
     AddSizeOfCbData()
         : heap(0), nonHeap(0), handles(0), index(0), externalId(0) {}
--- a/image/test/gtest/TestAnimationFrameBuffer.cpp
+++ b/image/test/gtest/TestAnimationFrameBuffer.cpp
@@ -26,16 +26,27 @@ static already_AddRefed<imgFrame> Create
   frame->SetRawAccessOnly();
   // Normally the blend animation filter would set the dirty rect, but since
   // we aren't producing an actual animation here, we need to fake it.
   frame->SetDirtyRect(aFrameRect);
   frame->Finish();
   return frame.forget();
 }
 
+static bool ReinitForRecycle(RawAccessFrameRef& aFrame) {
+  if (!aFrame) {
+    return false;
+  }
+
+  AnimationParams animParams{aFrame->GetRect(), FrameTimeout::Forever(),
+                             /* aFrameNum */ 1, BlendMethod::OVER,
+                             DisposalMethod::NOT_SPECIFIED};
+  return NS_SUCCEEDED(aFrame->InitForDecoderRecycle(animParams));
+}
+
 static void PrepareForDiscardingQueue(AnimationFrameRetainedBuffer& aQueue) {
   ASSERT_EQ(size_t(0), aQueue.Size());
   ASSERT_LT(size_t(1), aQueue.Batch());
 
   AnimationFrameBuffer::InsertStatus status = aQueue.Insert(CreateEmptyFrame());
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::CONTINUE, status);
 
   while (true) {
@@ -95,21 +106,17 @@ static void VerifyAdvance(AnimationFrame
 
   if (aQueue.IsRecycling()) {
     const AnimationFrameRecyclingQueue& queue =
         *static_cast<AnimationFrameRecyclingQueue*>(&aQueue);
     EXPECT_FALSE(queue.Recycle().back().mDirtyRect.IsEmpty());
     EXPECT_TRUE(
         queue.Recycle().back().mDirtyRect.Contains(oldFrame->GetDirtyRect()));
     EXPECT_EQ(totalRecycled + 1, queue.Recycle().size());
-    if (oldFrame->ShouldRecycle()) {
-      EXPECT_EQ(oldFrame.get(), queue.Recycle().back().mFrame.get());
-    } else {
-      EXPECT_EQ(nullptr, queue.Recycle().back().mFrame.get());
-    }
+    EXPECT_EQ(oldFrame.get(), queue.Recycle().back().mFrame.get());
   }
 }
 
 static void VerifyInsertAndAdvance(
     AnimationFrameBuffer& aQueue, size_t aExpectedFrame,
     AnimationFrameBuffer::InsertStatus aExpectedStatus) {
   // Insert the decoded frame.
   RefPtr<imgFrame> frame = CreateEmptyFrame();
@@ -559,23 +566,24 @@ TEST_F(ImageAnimationFrameBuffer, Recycl
     RefPtr<imgFrame> expectedFrame = buffer.Recycle().front().mFrame;
     EXPECT_FALSE(expectedRect.IsEmpty());
     EXPECT_TRUE(expectedFrame.get() != nullptr);
 
     IntRect gotRect;
     RawAccessFrameRef gotFrame = buffer.RecycleFrame(gotRect);
     EXPECT_EQ(expectedFrame.get(), gotFrame.get());
     EXPECT_EQ(expectedRect, gotRect);
+    EXPECT_TRUE(ReinitForRecycle(gotFrame));
   }
 
   // Trying to pull a recycled frame when we have nothing should be safe too.
   IntRect gotRect;
   RawAccessFrameRef gotFrame = buffer.RecycleFrame(gotRect);
   EXPECT_TRUE(gotFrame.get() == nullptr);
-  EXPECT_TRUE(gotRect.IsEmpty());
+  EXPECT_FALSE(ReinitForRecycle(gotFrame));
 }
 
 static void TestDiscardingQueueReset(AnimationFrameDiscardingQueue& aQueue,
                                      const imgFrame* aFirstFrame,
                                      size_t aThreshold, size_t aBatch,
                                      size_t aStartFrame) {
   // We should be advanced right up to the last decoded frame.
   EXPECT_TRUE(aQueue.MayDiscard());
@@ -740,17 +748,23 @@ TEST_F(ImageAnimationFrameBuffer, Recycl
   status = retained.Insert(std::move(frame));
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::DISCARD_YIELD, status);
 
   AnimationFrameRecyclingQueue buffer(std::move(retained));
   bool restartDecoding = buffer.Reset();
   EXPECT_TRUE(restartDecoding);
 
   // None of the buffers were recyclable.
-  EXPECT_TRUE(buffer.Recycle().empty());
+  EXPECT_FALSE(buffer.Recycle().empty());
+  while (!buffer.Recycle().empty()) {
+    IntRect recycleRect;
+    RawAccessFrameRef frameRef = buffer.RecycleFrame(recycleRect);
+    EXPECT_TRUE(frameRef);
+    EXPECT_FALSE(ReinitForRecycle(frameRef));
+  }
 
   // Reinsert the first two frames as recyclable and reset again.
   frame = CreateEmptyFrame(kImageSize, kImageRect, true);
   status = buffer.Insert(std::move(frame));
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::CONTINUE, status);
 
   frame = CreateEmptyFrame(kImageSize, IntRect(IntPoint(10, 10), IntSize(1, 1)),
                            true);
@@ -768,86 +782,83 @@ TEST_F(ImageAnimationFrameBuffer, Recycl
   }
 }
 
 TEST_F(ImageAnimationFrameBuffer, RecyclingRect) {
   const size_t kThreshold = 5;
   const size_t kBatch = 2;
   const size_t kStartFrame = 0;
   const IntSize kImageSize(100, 100);
+  const IntRect kImageRect(IntPoint(0, 0), kImageSize);
   AnimationFrameRetainedBuffer retained(kThreshold, kBatch, kStartFrame);
 
   // Let's get to the recycling state while marking all of the frames as not
   // recyclable, just like AnimationFrameBuffer / the decoders would do.
   RefPtr<imgFrame> frame;
-  frame =
-      CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  frame = CreateEmptyFrame(kImageSize, kImageRect, false);
   AnimationFrameBuffer::InsertStatus status = retained.Insert(std::move(frame));
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::CONTINUE, status);
 
-  frame =
-      CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  frame = CreateEmptyFrame(kImageSize, kImageRect, false);
   status = retained.Insert(std::move(frame));
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::CONTINUE, status);
 
-  frame =
-      CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  frame = CreateEmptyFrame(kImageSize, kImageRect, false);
   status = retained.Insert(std::move(frame));
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::CONTINUE, status);
 
-  frame =
-      CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  frame = CreateEmptyFrame(kImageSize, kImageRect, false);
   status = retained.Insert(std::move(frame));
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
 
   VerifyAdvance(retained, 1, false);
   VerifyAdvance(retained, 2, true);
   VerifyAdvance(retained, 3, false);
 
-  frame =
-      CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  frame = CreateEmptyFrame(kImageSize, kImageRect, false);
   status = retained.Insert(std::move(frame));
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::DISCARD_CONTINUE, status);
 
   AnimationFrameRecyclingQueue buffer(std::move(retained));
 
   // The first frame is now the candidate for recycling. Since it was marked as
   // not recyclable, we should get nothing.
   VerifyAdvance(buffer, 4, false);
 
   IntRect recycleRect;
   EXPECT_FALSE(buffer.Recycle().empty());
   RawAccessFrameRef frameRef = buffer.RecycleFrame(recycleRect);
-  EXPECT_FALSE(frameRef);
-  EXPECT_TRUE(recycleRect.IsEmpty());
+  EXPECT_TRUE(frameRef);
+  EXPECT_FALSE(ReinitForRecycle(frameRef));
   EXPECT_TRUE(buffer.Recycle().empty());
 
   // Insert a recyclable partial frame. Its dirty rect shouldn't matter since
   // the previous frame was not recyclable.
   frame = CreateEmptyFrame(kImageSize, IntRect(0, 0, 25, 25));
   status = buffer.Insert(std::move(frame));
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
 
   VerifyAdvance(buffer, 5, true);
   EXPECT_FALSE(buffer.Recycle().empty());
   frameRef = buffer.RecycleFrame(recycleRect);
-  EXPECT_FALSE(frameRef);
-  EXPECT_TRUE(recycleRect.IsEmpty());
+  EXPECT_TRUE(frameRef);
+  EXPECT_FALSE(ReinitForRecycle(frameRef));
   EXPECT_TRUE(buffer.Recycle().empty());
 
   // Insert a recyclable partial frame. Its dirty rect should match the recycle
   // rect since it is the only frame in the buffer.
   frame = CreateEmptyFrame(kImageSize, IntRect(25, 0, 50, 50));
   status = buffer.Insert(std::move(frame));
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
 
   VerifyAdvance(buffer, 6, true);
   EXPECT_FALSE(buffer.Recycle().empty());
   frameRef = buffer.RecycleFrame(recycleRect);
   EXPECT_TRUE(frameRef);
+  EXPECT_TRUE(ReinitForRecycle(frameRef));
   EXPECT_EQ(IntRect(25, 0, 50, 50), recycleRect);
   EXPECT_TRUE(buffer.Recycle().empty());
 
   // Insert the last frame and mark us as complete. The next recycled frame is
   // producing the first frame again, so we should use the first frame refresh
   // area instead of its dirty rect.
   frame = CreateEmptyFrame(kImageSize, IntRect(10, 10, 60, 10));
   status = buffer.Insert(std::move(frame));
@@ -855,26 +866,27 @@ TEST_F(ImageAnimationFrameBuffer, Recycl
 
   bool continueDecoding = buffer.MarkComplete(IntRect(0, 0, 75, 50));
   EXPECT_FALSE(continueDecoding);
 
   VerifyAdvance(buffer, 7, true);
   EXPECT_FALSE(buffer.Recycle().empty());
   frameRef = buffer.RecycleFrame(recycleRect);
   EXPECT_TRUE(frameRef);
+  EXPECT_TRUE(ReinitForRecycle(frameRef));
   EXPECT_EQ(IntRect(0, 0, 75, 50), recycleRect);
   EXPECT_TRUE(buffer.Recycle().empty());
 
   // Now let's reinsert the first frame. The recycle rect should still be the
   // first frame refresh area instead of the dirty rect of the first frame (e.g.
   // the full frame).
-  frame =
-      CreateEmptyFrame(kImageSize, IntRect(IntPoint(0, 0), kImageSize), false);
+  frame = CreateEmptyFrame(kImageSize, kImageRect, false);
   status = buffer.Insert(std::move(frame));
   EXPECT_EQ(AnimationFrameBuffer::InsertStatus::YIELD, status);
 
   VerifyAdvance(buffer, 0, true);
   EXPECT_FALSE(buffer.Recycle().empty());
   frameRef = buffer.RecycleFrame(recycleRect);
   EXPECT_TRUE(frameRef);
+  EXPECT_TRUE(ReinitForRecycle(frameRef));
   EXPECT_EQ(IntRect(0, 0, 75, 50), recycleRect);
   EXPECT_TRUE(buffer.Recycle().empty());
 }