From 23da824e1d61e37fbe0cc1c0f4d32052022a50ba Mon Sep 17 00:00:00 2001 From: "Steinar H. Gunderson" Date: Mon, 18 Mar 2019 22:53:21 +0100 Subject: [PATCH] Fix some memory leaks in the VA-API upload code (also make it more RAII-ish to be fail safe). --- nageru/mixer.cpp | 4 +--- nageru/mjpeg_encoder.cpp | 20 ++++++-------------- nageru/mjpeg_encoder.h | 6 ------ nageru/pbo_frame_allocator.cpp | 15 +++++++++++++++ 4 files changed, 22 insertions(+), 23 deletions(-) diff --git a/nageru/mixer.cpp b/nageru/mixer.cpp index e0aa8b1..32c1984 100644 --- a/nageru/mixer.cpp +++ b/nageru/mixer.cpp @@ -1083,9 +1083,7 @@ void Mixer::thread_func() if (new_frame->frame->data_copy != nullptr) { int mjpeg_card_index = mjpeg_encoder->get_mjpeg_stream_for_card(card_index); - if (mjpeg_card_index == -1) { - mjpeg_encoder->finish_frame(new_frame->frame); - } else { + if (mjpeg_card_index != -1) { mjpeg_encoder->upload_frame(pts_int, mjpeg_card_index, new_frame->frame, new_frame->video_format, new_frame->y_offset, new_frame->cbcr_offset); } } diff --git a/nageru/mjpeg_encoder.cpp b/nageru/mjpeg_encoder.cpp index d66ef14..24aa0eb 100644 --- a/nageru/mjpeg_encoder.cpp +++ b/nageru/mjpeg_encoder.cpp @@ -304,19 +304,6 @@ void MJPEGEncoder::upload_frame(int64_t pts, unsigned card_index, RefCountedFram any_frames_to_be_encoded.notify_all(); } -void MJPEGEncoder::finish_frame(RefCountedFrame frame) -{ - PBOFrameAllocator::Userdata *userdata = (PBOFrameAllocator::Userdata *)frame->userdata; - - if (userdata->data_copy_current_src == PBOFrameAllocator::Userdata::FROM_VA_API) { - VAResources resources __attribute__((unused)) = move(userdata->va_resources); - ReleaseVAResources release = move(userdata->va_resources_release); - - VAStatus va_status = vaUnmapBuffer(va_dpy->va_dpy, resources.image.buf); - CHECK_VASTATUS(va_status, "vaUnmapBuffer"); - } -} - int MJPEGEncoder::get_mjpeg_stream_for_card(unsigned card_index) { // Only bother doing MJPEG encoding if there are any connected clients @@ -456,6 +443,9 @@ void MJPEGEncoder::release_va_resources(MJPEGEncoder::VAResources resources) va_status = vaDestroySurfaces(va_dpy->va_dpy, &it->surface, 1); CHECK_VASTATUS(va_status, "vaDestroySurfaces"); + va_status = vaDestroyImage(va_dpy->va_dpy, it->image.image_id); + CHECK_VASTATUS(va_status, "vaDestroyImage"); + va_resources_freelist.erase(it); } @@ -677,9 +667,9 @@ void MJPEGEncoder::encode_jpeg_va(QueuedFrame &&qf) VABufferDestroyer destroy_slice_param(va_dpy->va_dpy, slice_param_buffer); if (userdata->data_copy_current_src == PBOFrameAllocator::Userdata::FROM_VA_API) { + // The pixel data is already put into the image by the caller. va_status = vaUnmapBuffer(va_dpy->va_dpy, resources.image.buf); CHECK_VASTATUS(va_status, "vaUnmapBuffer"); - // The pixel data is already put into the image by the caller. } else { assert(userdata->data_copy_current_src == PBOFrameAllocator::Userdata::FROM_MALLOC); @@ -700,6 +690,8 @@ void MJPEGEncoder::encode_jpeg_va(QueuedFrame &&qf) CHECK_VASTATUS(va_status, "vaUnmapBuffer"); } + qf.frame->data_copy = nullptr; + // Seemingly vaPutImage() (which triggers a GPU copy) is much nicer to the // CPU than vaDeriveImage() and copying directly into the GPU's buffers. // Exactly why is unclear, but it seems to involve L3 cache usage when there diff --git a/nageru/mjpeg_encoder.h b/nageru/mjpeg_encoder.h index aee1b9b..6e0357f 100644 --- a/nageru/mjpeg_encoder.h +++ b/nageru/mjpeg_encoder.h @@ -39,12 +39,6 @@ public: ~MJPEGEncoder(); void stop(); void upload_frame(int64_t pts, unsigned card_index, RefCountedFrame frame, const bmusb::VideoFormat &video_format, size_t y_offset, size_t cbcr_offset); - - // If the frame was started (data_copy != nullptr) but will not be finished - // (MJPEG decoding was turned off in the meantime), you'll need to call finish_frame() - // to release any VA-API resources. - void finish_frame(RefCountedFrame frame); - bool using_vaapi() const { return va_dpy != nullptr; } // Returns -1 for inactive (ie., don't encode frames for this card right now). diff --git a/nageru/pbo_frame_allocator.cpp b/nageru/pbo_frame_allocator.cpp index 54182a8..7fb06f8 100644 --- a/nageru/pbo_frame_allocator.cpp +++ b/nageru/pbo_frame_allocator.cpp @@ -370,6 +370,21 @@ void PBOFrameAllocator::release_frame(Frame frame) } #endif + { + // In case we never got to upload the frame to MJPEGEncoder. + Userdata *userdata = (Userdata *)frame.userdata; + MJPEGEncoder::VAResources resources __attribute__((unused)) = move(userdata->va_resources); + MJPEGEncoder::ReleaseVAResources release = move(userdata->va_resources_release); + + if (frame.data_copy != nullptr && userdata->data_copy_current_src == Userdata::FROM_VA_API) { + VADisplay va_dpy = mjpeg_encoder->va_dpy->va_dpy; + VAStatus va_status = vaUnmapBuffer(va_dpy, resources.image.buf); + CHECK_VASTATUS(va_status, "vaUnmapBuffer"); + + frame.data_copy = nullptr; + } + } + lock_guard lock(freelist_mutex); freelist.push(frame); //--sumsum; -- 2.39.2