Hoist the logic to keep an AVFrame reference up to Pacer

app/streaming/video/ffmpeg-renderers/drm.cpp

@@ -1396,7 +1396,7 @@ void DrmRenderer::notifyOverlayUpdated(Overlay::OverlayType type)
         // Queue the plane flip with the new FB
         //
         // NB: This takes ownership of the FB and dumb buffer, even on failure
-        m_PropSetter.flipPlane(m_OverlayPlanes[type], fbId, dumbBuffer, nullptr);
+        m_PropSetter.flipPlane(m_OverlayPlanes[type], fbId, dumbBuffer);
 
         SDL_FreeSurface(newSurface);
     }
@@ -1657,11 +1657,10 @@ void DrmRenderer::renderFrame(AVFrame* frame)
     //
     // NB: This takes ownership of fbId, even on failure
     //
-    // NB2: We reference the AVFrame (which also references the AVBuffers backing the frame itself
+    // NB2: Pacer references the AVFrame (which also references the AVBuffers backing the frame
     // and the opaque_ref which may store our DRM-PRIME mapping) for frames backed by DMA-BUFs in
     // order to keep those from being reused by the decoder while they're still being scanned out.
-    m_PropSetter.flipPlane(m_VideoPlane, fbId, 0,
+    m_PropSetter.flipPlane(m_VideoPlane, fbId, 0);
-                           (m_DrmPrimeBackend || frame->format == AV_PIX_FMT_DRM_PRIME) ? frame : nullptr);
 
     // Apply pending atomic transaction (if in atomic mode)
     m_PropSetter.apply();

app/streaming/video/ffmpeg-renderers/drm.h

@@ -209,12 +209,10 @@ class DrmRenderer : public IFFmpegRenderer {
         struct PlaneBuffer {
             uint32_t fbId;
             uint32_t dumbBufferHandle;
-            AVFrame* frame;
 
             // Atomic only
             uint32_t pendingFbId;
             uint32_t pendingDumbBuffer;
-            AVFrame* pendingFrame;
             bool modified;
         };
 
@@ -224,7 +222,6 @@ class DrmRenderer : public IFFmpegRenderer {
             for (auto it = m_PlaneBuffers.begin(); it != m_PlaneBuffers.end(); it++) {
                 SDL_assert(!it->second.fbId);
                 SDL_assert(!it->second.dumbBufferHandle);
-                SDL_assert(!it->second.pendingFrame);
 
                 if (it->second.pendingFbId) {
                     drmModeRmFB(m_Fd, it->second.pendingFbId);
@@ -234,7 +231,6 @@ class DrmRenderer : public IFFmpegRenderer {
                     destroyBuf.handle = it->second.pendingDumbBuffer;
                     drmIoctl(m_Fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroyBuf);
                 }
-                av_frame_free(&it->second.pendingFrame);
             }
 
             if (m_AtomicReq) {
@@ -337,14 +333,9 @@ class DrmRenderer : public IFFmpegRenderer {
 
         // Unconditionally takes ownership of fbId and dumbBufferHandle (if present)
         // If provided, frame is referenced to keep the FB's backing DMA-BUFs around
-        bool flipPlane(const DrmPropertyMap& plane, uint32_t fbId, uint32_t dumbBufferHandle, AVFrame* frame) {
+        bool flipPlane(const DrmPropertyMap& plane, uint32_t fbId, uint32_t dumbBufferHandle) {
             bool ret;
 
-            // If a frame was provided, clone a reference to it to hold until the next flip
-            if (frame) {
-                frame = av_frame_clone(frame);
-            }
-
             if (m_Atomic) {
                 std::lock_guard lg { m_Lock };
 
@@ -355,7 +346,6 @@ class DrmRenderer : public IFFmpegRenderer {
                     auto &pb = m_PlaneBuffers[plane.objectId()];
                     std::swap(fbId, pb.pendingFbId);
                     std::swap(dumbBufferHandle, pb.pendingDumbBuffer);
-                    std::swap(frame, pb.pendingFrame);
                     pb.modified = true;
                 }
             }
@@ -383,7 +373,6 @@ class DrmRenderer : public IFFmpegRenderer {
                     auto &pb = m_PlaneBuffers[plane.objectId()];
                     std::swap(fbId, pb.fbId);
                     std::swap(dumbBufferHandle, pb.dumbBufferHandle);
-                    std::swap(frame, pb.frame);
 
                     ret = true;
                 }
@@ -404,7 +393,6 @@ class DrmRenderer : public IFFmpegRenderer {
                 destroyBuf.handle = dumbBufferHandle;
                 drmIoctl(m_Fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroyBuf);
             }
-            av_frame_free(&frame);
 
             return ret;
         }
@@ -448,7 +436,7 @@ class DrmRenderer : public IFFmpegRenderer {
         void disablePlane(const DrmPropertyMap& plane) {
             if (plane.isValid()) {
                 configurePlane(plane, 0, 0, 0, 0, 0, 0, 0, 0, 0);
-                flipPlane(plane, 0, 0, nullptr);
+                flipPlane(plane, 0, 0);
             }
         }
 
@@ -567,15 +555,13 @@ class DrmRenderer : public IFFmpegRenderer {
                         drmIoctl(m_Fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroyBuf);
                         it->second.dumbBufferHandle = 0;
                     }
-                    av_frame_free(&it->second.frame);
 
                     // The pending buffers become the active buffers for this FB
                     auto &pb = m_PlaneBuffers[it->first];
                     pb.fbId = it->second.pendingFbId;
                     pb.dumbBufferHandle = it->second.pendingDumbBuffer;
-                    pb.frame = it->second.pendingFrame;
                 }
-                else if (err < 0 || it->second.fbId || it->second.dumbBufferHandle || it->second.frame) {
+                else if (err < 0 || it->second.fbId || it->second.dumbBufferHandle) {
                     // Free the old pending buffers on a failed commit
                     if (it->second.pendingFbId) {
                         SDL_assert(err < 0);
@@ -587,13 +573,11 @@ class DrmRenderer : public IFFmpegRenderer {
                         destroyBuf.handle = it->second.pendingDumbBuffer;
                         drmIoctl(m_Fd, DRM_IOCTL_MODE_DESTROY_DUMB, &destroyBuf);
                     }
-                    av_frame_free(&it->second.pendingFrame);
 
                     // This FB wasn't modified in this commit, so the current buffers stay around
                     auto &pb = m_PlaneBuffers[it->first];
                     pb.fbId = it->second.fbId;
                     pb.dumbBufferHandle = it->second.dumbBufferHandle;
-                    pb.frame = it->second.frame;
                 }
 
                 // NB: We swapped in a new plane buffers map which will clear the modified value.

app/streaming/video/ffmpeg-renderers/eglvid.cpp

@@ -65,7 +65,6 @@ EGLRenderer::EGLRenderer(IFFmpegRenderer *backendRenderer)
         m_VideoVAO(0),
         m_BlockingSwapBuffers(false),
         m_LastRenderSync(EGL_NO_SYNC),
-        m_LastFrame(av_frame_alloc()),
         m_glEGLImageTargetTexture2DOES(nullptr),
         m_glGenVertexArraysOES(nullptr),
         m_glBindVertexArrayOES(nullptr),
@@ -116,8 +115,6 @@ EGLRenderer::~EGLRenderer()
     if (m_DummyRenderer) {
         SDL_DestroyRenderer(m_DummyRenderer);
     }
-
-    av_frame_free(&m_LastFrame);
 }
 
 bool EGLRenderer::prepareDecoderContext(AVCodecContext*, AVDictionary**)
@@ -733,18 +730,17 @@ void EGLRenderer::waitToRender()
     // See comment in renderFrame() for more details.
     SDL_GL_MakeCurrent(m_Window, m_Context);
 
-    // Wait for the previous buffer swap to finish before picking the next frame to render.
+    // Our fence will wait until the previous frame is drawn (non-blocking swapbuffers case)
-    // This way we'll get the latest available frame and render it without blocking.
+    // or until the new back buffer is available (blocking swapbuffers case)
-    if (m_BlockingSwapBuffers) {
+    if (m_LastRenderSync != EGL_NO_SYNC) {
-        // Try to use eglClientWaitSync() if the driver supports it
+        SDL_assert(m_eglClientWaitSync != nullptr);
-        if (m_LastRenderSync != EGL_NO_SYNC) {
+        m_eglClientWaitSync(m_EGLDisplay, m_LastRenderSync, EGL_SYNC_FLUSH_COMMANDS_BIT, EGL_FOREVER);
-            SDL_assert(m_eglClientWaitSync != nullptr);
+        m_eglDestroySync(m_EGLDisplay, m_LastRenderSync);
-            m_eglClientWaitSync(m_EGLDisplay, m_LastRenderSync, EGL_SYNC_FLUSH_COMMANDS_BIT, EGL_FOREVER);
+        m_LastRenderSync = EGL_NO_SYNC;
-        }
+    }
-        else {
+    else {
-            // Use glFinish() if fences aren't available
+        // Use glFinish() if fences aren't available
-            glFinish();
+        glFinish();
-        }
     }
 }
 
@@ -847,6 +843,22 @@ void EGLRenderer::renderFrame(AVFrame* frame)
     glDrawArrays(GL_TRIANGLES, 0, 6);
     m_glBindVertexArrayOES(0);
 
+    if (!m_BlockingSwapBuffers) {
+        // If we aren't going to wait on the full swap buffers operation,
+        // insert a fence now to let us know when the memory backing our
+        // video frame is safe for Pacer to free
+        if (m_eglClientWaitSync != nullptr) {
+            SDL_assert(m_LastRenderSync == EGL_NO_SYNC);
+            if (m_eglCreateSync != nullptr) {
+                m_LastRenderSync = m_eglCreateSync(m_EGLDisplay, EGL_SYNC_FENCE, nullptr);
+            }
+            else {
+                SDL_assert(m_eglCreateSyncKHR != nullptr);
+                m_LastRenderSync = m_eglCreateSyncKHR(m_EGLDisplay, EGL_SYNC_FENCE, nullptr);
+            }
+        }
+    }
+
     // Draw overlays on top
     for (int i = 0; i < Overlay::OverlayMax; i++) {
         renderOverlay((Overlay::OverlayType)i, drawableWidth, drawableHeight);
@@ -859,17 +871,8 @@ void EGLRenderer::renderFrame(AVFrame* frame)
         // our eglClientWaitSync() or glFinish() call in waitToRender() will not
         // return before the new buffer is actually ready for rendering.
         glClear(GL_COLOR_BUFFER_BIT);
-
-        // If we this EGL implementation supports fences, use those to delay
-        // rendering the next frame until this one is completed. If not, we'll
-        // have to just use glFinish().
         if (m_eglClientWaitSync != nullptr) {
-            // Delete the sync object from last render
+            SDL_assert(m_LastRenderSync == EGL_NO_SYNC);
-            if (m_LastRenderSync != EGL_NO_SYNC) {
-                m_eglDestroySync(m_EGLDisplay, m_LastRenderSync);
-            }
-
-            // Create a new sync object that will be signalled when the buffer swap is completed
             if (m_eglCreateSync != nullptr) {
                 m_LastRenderSync = m_eglCreateSync(m_EGLDisplay, EGL_SYNC_FENCE, nullptr);
             }
@@ -879,14 +882,6 @@ void EGLRenderer::renderFrame(AVFrame* frame)
             }
         }
     }
-
-    // Free the DMA-BUF backing the last frame now that it is definitely
-    // no longer being used anymore. While the PRIME FD stays around until
-    // EGL is done with it, the memory backing it may be reused by FFmpeg
-    // before the GPU has read it. This is particularly noticeable on the
-    // RK3288-based TinkerBoard when V-Sync is disabled.
-    av_frame_unref(m_LastFrame);
-    av_frame_move_ref(m_LastFrame, frame);
 }
 
 bool EGLRenderer::testRenderFrame(AVFrame* frame)

app/streaming/video/ffmpeg-renderers/eglvid.h

@@ -46,7 +46,6 @@ private:
     unsigned int m_VideoVAO;
     bool m_BlockingSwapBuffers;
     EGLSync m_LastRenderSync;
-    AVFrame* m_LastFrame;
     PFNGLEGLIMAGETARGETTEXTURE2DOESPROC m_glEGLImageTargetTexture2DOES;
     PFNGLGENVERTEXARRAYSOESPROC m_glGenVertexArraysOES;
     PFNGLBINDVERTEXARRAYOESPROC m_glBindVertexArrayOES;

app/streaming/video/ffmpeg-renderers/pacer/pacer.cpp

@@ -31,6 +31,7 @@
 Pacer::Pacer(IFFmpegRenderer* renderer, PVIDEO_STATS videoStats) :
     m_RenderThread(nullptr),
     m_VsyncThread(nullptr),
+    m_DeferredFreeFrame(nullptr),
     m_Stopping(false),
     m_VsyncSource(nullptr),
     m_VsyncRenderer(renderer),
@@ -76,6 +77,7 @@ Pacer::~Pacer()
         AVFrame* frame = m_PacingQueue.dequeue();
         av_frame_free(&frame);
     }
+    av_frame_free(&m_DeferredFreeFrame);
 }
 
 void Pacer::renderOnMainThread()
@@ -342,6 +344,11 @@ void Pacer::renderFrame(AVFrame* frame)
 
     m_VideoStats->totalRenderTimeUs += (afterRender - beforeRender);
     m_VideoStats->renderedFrames++;
+
+    // Wait until after next frame to free this one to ensure the GPU
+    // doesn't stall or read garbage if the backing buffer gets returned
+    // to the pool and the decoder tries to write a new frame into it
+    std::swap(frame, m_DeferredFreeFrame);
     av_frame_free(&frame);
 
     // Drop frames if we have too many queued up for a while

app/streaming/video/ffmpeg-renderers/pacer/pacer.h

@@ -60,6 +60,7 @@ private:
     QWaitCondition m_VsyncSignalled;
     SDL_Thread* m_RenderThread;
     SDL_Thread* m_VsyncThread;
+    AVFrame* m_DeferredFreeFrame;
     bool m_Stopping;
 
     IVsyncSource* m_VsyncSource;