Skip to content
This repository has been archived by the owner on Feb 12, 2023. It is now read-only.

Commit

Permalink
fix(video): guard storeVideoFrame() against freeing in-use memory
Browse files Browse the repository at this point in the history
This commit fixes an issue where storeVideoFrame() can in rare cases
free memory that is still in use. Also adds extra documentation
documenting it's precise usage.
  • Loading branch information
initramfs committed Aug 5, 2016
1 parent f6a698b commit 5b31b5d
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 9 deletions.
35 changes: 27 additions & 8 deletions src/video/videoframe.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ extern "C"{
* directly equal to the width.
*
*
* @var dataAlignment
* @var VideoFrame::dataAlignment
* @brief Data alignment parameter used to populate AVFrame buffers.
*
* This field is public in effort to standardize the data alignment parameter for all AVFrame
Expand Down Expand Up @@ -636,13 +636,26 @@ AVFrame* VideoFrame::generateAVFrame(const QSize& dimensions, const int pixelFor
/**
* @brief Stores a given AVFrame within the frameBuffer map.
*
* As protection against duplicate frames, the storage mechanism will only allow one frame of a
* given type to exist in the frame buffer. Should the given frame type already exist in the frame
* buffer, the given frame will be freed and have it's buffers invalidated. In order to ensure
* correct operation, always replace the frame pointer with the one returned by this function.
*
* As an example:
* @code{.cpp}
* AVFrame* frame = // create AVFrame...
*
* frame = storeAVFrame(frame, dimensions, pixelFormat);
* @endcode
*
* This function is not thread-safe and must be called from a thread-safe context.
*
* @param frame the given frame to store.
* @param dimensions the dimensions of the frame, must be valid.
* @param pixelFormat the pixel format of the frame.
* @return The given AVFrame* or a pre-existing AVFrame* that already exists in the frameBuffer.
*/
void VideoFrame::storeAVFrame(AVFrame* frame, const QSize& dimensions, const int pixelFormat)
AVFrame* VideoFrame::storeAVFrame(AVFrame* frame, const QSize& dimensions, const int pixelFormat)
{
FrameBufferKey frameKey = getFrameKey(dimensions, pixelFormat, frame->linesize[0]);

Expand All @@ -651,13 +664,19 @@ void VideoFrame::storeAVFrame(AVFrame* frame, const QSize& dimensions, const int
{
AVFrame* old_ret = frameBuffer[frameKey];

// Free old frame
av_freep(&old_ret->data[0]);
av_frame_unref(old_ret);
av_frame_free(&old_ret);
// Free new frame
av_freep(&frame->data[0]);
av_frame_unref(frame);
av_frame_free(&frame);

return old_ret;
}
else
{
frameBuffer[frameKey] = frame;

frameBuffer[frameKey] = frame;
return frame;
}
}

/**
Expand Down Expand Up @@ -753,7 +772,7 @@ T VideoFrame::toGenericObject(const QSize& dimensions, const int pixelFormat, co
frameLock.unlock();
frameLock.lockForWrite();

storeAVFrame(frame, dimensions, static_cast<int>(pixelFormat));
frame = storeAVFrame(frame, dimensions, static_cast<int>(pixelFormat));

T ret = objectConstructor(frame);

Expand Down
2 changes: 1 addition & 1 deletion src/video/videoframe.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ class VideoFrame

AVFrame* retrieveAVFrame(const QSize& dimensions, const int pixelFormat, const bool requireAligned);
AVFrame* generateAVFrame(const QSize& dimensions, const int pixelFormat, const bool requireAligned);
void storeAVFrame(AVFrame* frame, const QSize& dimensions, const int pixelFormat);
AVFrame* storeAVFrame(AVFrame* frame, const QSize& dimensions, const int pixelFormat);

void deleteFrameBuffer();

Expand Down

0 comments on commit 5b31b5d

Please sign in to comment.