From 9ffe5218d558b798a95a154772fc61cad978e30f Mon Sep 17 00:00:00 2001 From: Cameron Gutman Date: Wed, 7 Jan 2026 17:50:43 -0600 Subject: [PATCH] Hoist the logic to keep an AVFrame reference up to Pacer --- app/streaming/video/ffmpeg-renderers/drm.cpp | 7 +-- app/streaming/video/ffmpeg-renderers/drm.h | 22 +------ .../video/ffmpeg-renderers/eglvid.cpp | 61 +++++++++---------- app/streaming/video/ffmpeg-renderers/eglvid.h | 1 - .../video/ffmpeg-renderers/pacer/pacer.cpp | 7 +++ .../video/ffmpeg-renderers/pacer/pacer.h | 1 + 6 files changed, 42 insertions(+), 57 deletions(-) diff --git a/app/streaming/video/ffmpeg-renderers/drm.cpp b/app/streaming/video/ffmpeg-renderers/drm.cpp index f9bf6c36..87515b27 100644 --- a/app/streaming/video/ffmpeg-renderers/drm.cpp +++ b/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_DrmPrimeBackend || frame->format == AV_PIX_FMT_DRM_PRIME) ? frame : nullptr); + m_PropSetter.flipPlane(m_VideoPlane, fbId, 0); // Apply pending atomic transaction (if in atomic mode) m_PropSetter.apply(); diff --git a/app/streaming/video/ffmpeg-renderers/drm.h b/app/streaming/video/ffmpeg-renderers/drm.h index 43a3547b..1fdece9c 100644 --- a/app/streaming/video/ffmpeg-renderers/drm.h +++ b/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. diff --git a/app/streaming/video/ffmpeg-renderers/eglvid.cpp b/app/streaming/video/ffmpeg-renderers/eglvid.cpp index ab9e80e6..9f7d1149 100644 --- a/app/streaming/video/ffmpeg-renderers/eglvid.cpp +++ b/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. - // This way we'll get the latest available frame and render it without blocking. - if (m_BlockingSwapBuffers) { - // Try to use eglClientWaitSync() if the driver supports it - if (m_LastRenderSync != EGL_NO_SYNC) { - SDL_assert(m_eglClientWaitSync != nullptr); - m_eglClientWaitSync(m_EGLDisplay, m_LastRenderSync, EGL_SYNC_FLUSH_COMMANDS_BIT, EGL_FOREVER); - } - else { - // Use glFinish() if fences aren't available - glFinish(); - } + // Our fence will wait until the previous frame is drawn (non-blocking swapbuffers case) + // or until the new back buffer is available (blocking swapbuffers case) + if (m_LastRenderSync != EGL_NO_SYNC) { + SDL_assert(m_eglClientWaitSync != nullptr); + m_eglClientWaitSync(m_EGLDisplay, m_LastRenderSync, EGL_SYNC_FLUSH_COMMANDS_BIT, EGL_FOREVER); + m_eglDestroySync(m_EGLDisplay, m_LastRenderSync); + m_LastRenderSync = EGL_NO_SYNC; + } + else { + // Use glFinish() if fences aren't available + 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 - 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 + SDL_assert(m_LastRenderSync == EGL_NO_SYNC); 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) diff --git a/app/streaming/video/ffmpeg-renderers/eglvid.h b/app/streaming/video/ffmpeg-renderers/eglvid.h index 9c0ef2f4..6ba2b37c 100644 --- a/app/streaming/video/ffmpeg-renderers/eglvid.h +++ b/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; diff --git a/app/streaming/video/ffmpeg-renderers/pacer/pacer.cpp b/app/streaming/video/ffmpeg-renderers/pacer/pacer.cpp index d4052a31..688e45e0 100644 --- a/app/streaming/video/ffmpeg-renderers/pacer/pacer.cpp +++ b/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 diff --git a/app/streaming/video/ffmpeg-renderers/pacer/pacer.h b/app/streaming/video/ffmpeg-renderers/pacer/pacer.h index 11963a5f..0bdf8074 100644 --- a/app/streaming/video/ffmpeg-renderers/pacer/pacer.h +++ b/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;