Bug 1551735 - Clearly document the case of the RendererOGL receiving a new WebRenderCompositionRecorder while it has one r=kats
authorBarret Rennie <barret@brennie.ca>
Fri, 31 May 2019 00:31:52 +0000
changeset 476298 63568b2a8178eca237b71cd661e2aaab3b6a360c
parent 476297 0ef19da1d641b06c3238892d7e81ebc27c85b98d
child 476299 909f78f4ebaed6cfd473735f6140a88ee7de3c0c
child 476300 87017ac1c788a9226a8f2d291a65ea974482dc43
child 476384 53af18412fd188c4db1af090874f68d1f6a804cf
push id36090
push usernbeleuzu@mozilla.com
push dateFri, 31 May 2019 03:59:09 +0000
treeherdermozilla-central@63568b2a8178 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskats
bugs1551735
milestone69.0a1
first release with
nightly linux32
63568b2a8178 / 69.0a1 / 20190531035909 / files
nightly linux64
63568b2a8178 / 69.0a1 / 20190531035909 / files
nightly mac
63568b2a8178 / 69.0a1 / 20190531035909 / files
nightly win32
63568b2a8178 / 69.0a1 / 20190531035909 / files
nightly win64
63568b2a8178 / 69.0a1 / 20190531035909 / files
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
releases
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1551735 - Clearly document the case of the RendererOGL receiving a new WebRenderCompositionRecorder while it has one r=kats Differential Revision: https://phabricator.services.mozilla.com/D32356
gfx/layers/CompositionRecorder.cpp
gfx/layers/CompositionRecorder.h
gfx/layers/wr/WebRenderCompositionRecorder.cpp
gfx/layers/wr/WebRenderCompositionRecorder.h
gfx/webrender_bindings/RenderThread.cpp
--- a/gfx/layers/CompositionRecorder.cpp
+++ b/gfx/layers/CompositionRecorder.cpp
@@ -53,10 +53,12 @@ void CompositionRecorder::WriteCollected
                     (frame->GetTimeStamp() - mRecordingStart).ToMilliseconds())
              << ".png";
     gfxUtils::WriteAsPNG(surf, filename.str().c_str());
     i++;
   }
   mCollectedFrames.Clear();
 }
 
+void CompositionRecorder::ClearCollectedFrames() { mCollectedFrames.Clear(); }
+
 }  // namespace layers
 }  // namespace mozilla
--- a/gfx/layers/CompositionRecorder.h
+++ b/gfx/layers/CompositionRecorder.h
@@ -63,16 +63,18 @@ class CompositionRecorder {
   /**
    * Write out the collected frames as a series of timestamped images.
    */
   virtual void WriteCollectedFrames();
 
  protected:
   virtual ~CompositionRecorder() = default;
 
+  void ClearCollectedFrames();
+
  private:
   nsTArray<RefPtr<RecordedFrame>> mCollectedFrames;
   TimeStamp mRecordingStart;
 };
 
 }  // namespace layers
 }  // namespace mozilla
 
--- a/gfx/layers/wr/WebRenderCompositionRecorder.cpp
+++ b/gfx/layers/wr/WebRenderCompositionRecorder.cpp
@@ -55,18 +55,19 @@ bool WebRenderCompositionRecorder::Maybe
     wr::Renderer* aRenderer, wr::WebRenderPipelineInfo* aFrameEpochs) {
   MOZ_ASSERT(wr::RenderThread::IsInRenderThread());
 
   if (!aRenderer || !aFrameEpochs) {
     return false;
   }
 
   if (!mMutex.TryLock()) {
-    // If we cannot lock the mutex, then the |CompositorBridgeParent|
-    // is holding the mutex in |WriteCollectedFrames|.
+    // If we cannot lock the mutex, then either (a) the |CompositorBridgeParent|
+    // is holding the mutex in |WriteCollectedFrames| or (b) the |RenderThread|
+    // is holding the mutex in |ForceFinishRecording|.
     //
     // In either case we do not want to wait to acquire the mutex to record a
     // frame since frames recorded now will not be written to disk.
 
     return false;
   }
 
   auto unlockGuard = MakeScopeExit([&]() { mMutex.Unlock(); });
@@ -101,16 +102,27 @@ void WebRenderCompositionRecorder::Write
       "WebRenderCompositionRecorder: Attempting to write frames from invalid "
       "state.");
 
   CompositionRecorder::WriteCollectedFrames();
 
   mFinishedRecording = true;
 }
 
+bool WebRenderCompositionRecorder::ForceFinishRecording() {
+  MutexAutoLock guard(mMutex);
+
+  bool wasRecording = !mFinishedRecording;
+  mFinishedRecording = true;
+
+  ClearCollectedFrames();
+
+  return wasRecording;
+}
+
 bool WebRenderCompositionRecorder::DidPaintContent(
     wr::WebRenderPipelineInfo* aFrameEpochs) {
   const wr::WrPipelineInfo& info = aFrameEpochs->Raw();
   bool didPaintContent = false;
 
   for (wr::usize i = 0; i < info.epochs.length; i++) {
     const wr::PipelineId pipelineId = info.epochs.data[i].pipeline_id;
 
--- a/gfx/layers/wr/WebRenderCompositionRecorder.h
+++ b/gfx/layers/wr/WebRenderCompositionRecorder.h
@@ -78,16 +78,29 @@ class WebRenderCompositionRecorder final
    *
    * Returns whether or not the recorder has finished recording frames. If
    * true, it is safe to release both this object and Web Render's composition
    * recorder structures.
    */
   bool MaybeRecordFrame(wr::Renderer* aRenderer,
                         wr::WebRenderPipelineInfo* aFrameEpochs);
 
+  /**
+   * Force the composition recorder to finish recording.
+   *
+   * This should only be called if |WriteCollectedFrames| is not to be called,
+   * since the recorder will be in an invalid state to do so.
+   *
+   * This returns whether or not the recorder was recording before this method
+   * was called.
+   *
+   * Note: This method will block acquiring a lock.
+   */
+  bool ForceFinishRecording();
+
  protected:
   ~WebRenderCompositionRecorder() = default;
 
   /**
    * Determine if any content pipelines updated.
    */
   bool DidPaintContent(wr::WebRenderPipelineInfo* aFrameEpochs);
 
--- a/gfx/webrender_bindings/RenderThread.cpp
+++ b/gfx/webrender_bindings/RenderThread.cpp
@@ -231,16 +231,38 @@ size_t RenderThread::RendererCount() {
 }
 
 void RenderThread::SetCompositionRecorderForWindow(
     wr::WindowId aWindowId,
     RefPtr<layers::WebRenderCompositionRecorder>&& aCompositionRecorder) {
   MOZ_ASSERT(IsInRenderThread());
   MOZ_ASSERT(GetRenderer(aWindowId));
 
+  auto it = mCompositionRecorders.find(aWindowId);
+  if (it != mCompositionRecorders.end() && it->second->ForceFinishRecording()) {
+    // This case should never occur since the |CompositorBridgeParent| will
+    // receive its "EndRecording" IPC message before another "BeginRecording"
+    // IPC message.
+    //
+    // However, if we do hit this case, then we should handle it gracefully.
+    // We free the structures here because any captured frames are not going
+    // to be read back.
+    if (RendererOGL* renderer = GetRenderer(aWindowId)) {
+      wr_renderer_release_composition_recorder_structures(
+          renderer->GetRenderer());
+    }
+  }
+
+  // If we have finished recording, then we have received
+  // |SetCompositionRecorderEvent| after the compositor brige parent finished
+  // writing but before we handled another frame to delete the data structure.
+  //
+  // In this case we do not need to free the |wr::Renderer|'s composition
+  // recorder structures since we can re-use them.
+
   mCompositionRecorders[aWindowId] = std::move(aCompositionRecorder);
 }
 
 void RenderThread::HandleFrame(wr::WindowId aWindowId, bool aRender) {
   if (mHasShutdown) {
     return;
   }