Discussion:
[libav-devel] [PATCH] avcodec/vda_h264: use av_buffer to manage buffers
Sebastien Zwickert
2013-05-28 07:15:36 UTC
Permalink
From: Xidorn Quan <***@gmail.com>

This patch fixes a leak of buffer when seeking occurs.
It adds a flag in struct vda_context for compatibility with apps which
currently use it. If the flag is not set, the hwaccel will behave like
before.
---
libavcodec/vda.h | 11 +++++++++++
libavcodec/vda_h264.c | 27 +++++++++++++++++++++++++++
2 files changed, 38 insertions(+)

diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..13e4fc5 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
* The reference size used for fast reallocation.
*/
int priv_allocated_size;
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
+ *
+ * encoding: unused
+ * decoding: Set by user.
+ */
+ int use_ref_buffer;
};

/** Create the video decoder. */
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..a183afa 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -28,6 +28,10 @@
#include "h264.h"
#include "vda.h"

+struct vda_buffer {
+ CVPixelBufferRef cv_buffer;
+};
+
/* Decoder callback that adds the VDA frame to the queue in display order. */
static void vda_decoder_callback(void *vda_hw_ctx,
CFDictionaryRef user_info,
@@ -107,11 +111,20 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
return 0;
}

+static void vda_h264_release_buffer(void *opaque, uint8_t *data)
+{
+ struct vda_buffer *context = opaque;
+ CVPixelBufferRelease(context->cv_buffer);
+ av_free(context);
+}
+
static int vda_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
AVFrame *frame = &h->cur_pic_ptr->f;
+ struct vda_buffer *context;
+ AVBufferRef *buffer;
int status;

if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
@@ -123,6 +136,20 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
if (status)
av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);

+ if (!vda_ctx->use_ref_buffer || status)
+ return status;
+
+ context = av_mallocz(sizeof(*context));
+ buffer = av_buffer_create(NULL, 0, vda_h264_release_buffer, context, 0);
+ if (!context || !buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ av_free(context);
+ return -1;
+ }
+
+ context->cv_buffer = vda_ctx->cv_buffer;
+ frame->buf[3] = buffer;
+
return status;
}
--
1.7.9.6 (Apple Git-31.1)
Luca Barbato
2013-05-28 10:10:27 UTC
Permalink
Post by Sebastien Zwickert
It adds a flag in struct vda_context for compatibility with apps which
It can do w/out the additional level of indirection I guess.

Beside that seems ok.

lu
Sebastien Zwickert
2013-05-29 06:52:17 UTC
Permalink
Post by Luca Barbato
Post by Sebastien Zwickert
It adds a flag in struct vda_context for compatibility with apps which
It can do w/out the additional level of indirection I guess.
W/out this it could cause double free issue on existing applications which
use vda hwaccel.
Anton Khirnov
2013-05-28 18:09:16 UTC
Permalink
Post by Sebastien Zwickert
This patch fixes a leak of buffer when seeking occurs.
It adds a flag in struct vda_context for compatibility with apps which
currently use it. If the flag is not set, the hwaccel will behave like
before.
---
libavcodec/vda.h | 11 +++++++++++
libavcodec/vda_h264.c | 27 +++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..13e4fc5 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
* The reference size used for fast reallocation.
*/
int priv_allocated_size;
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
+ *
+ * encoding: unused
+ * decoding: Set by user.
+ */
+ int use_ref_buffer;
};
/** Create the video decoder. */
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..a183afa 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -28,6 +28,10 @@
#include "h264.h"
#include "vda.h"
+struct vda_buffer {
+ CVPixelBufferRef cv_buffer;
+};
+
/* Decoder callback that adds the VDA frame to the queue in display order. */
static void vda_decoder_callback(void *vda_hw_ctx,
CFDictionaryRef user_info,
@@ -107,11 +111,20 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
return 0;
}
+static void vda_h264_release_buffer(void *opaque, uint8_t *data)
+{
+ struct vda_buffer *context = opaque;
+ CVPixelBufferRelease(context->cv_buffer);
+ av_free(context);
+}
+
static int vda_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
AVFrame *frame = &h->cur_pic_ptr->f;
+ struct vda_buffer *context;
+ AVBufferRef *buffer;
int status;
if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
@@ -123,6 +136,20 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
if (status)
av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);
+ if (!vda_ctx->use_ref_buffer || status)
+ return status;
+
+ context = av_mallocz(sizeof(*context));
+ buffer = av_buffer_create(NULL, 0, vda_h264_release_buffer, context, 0);
+ if (!context || !buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ av_free(context);
+ return -1;
+ }
+
+ context->cv_buffer = vda_ctx->cv_buffer;
+ frame->buf[3] = buffer;
Are the previous 3 entries always filled? I think the code currently assumes
the buffers are filled continuously.
--
Anton Khirnov
Sebastien Zwickert
2013-05-29 06:57:13 UTC
Permalink
Post by Anton Khirnov
Post by Sebastien Zwickert
This patch fixes a leak of buffer when seeking occurs.
It adds a flag in struct vda_context for compatibility with apps which
currently use it. If the flag is not set, the hwaccel will behave like
before.
---
libavcodec/vda.h | 11 +++++++++++
libavcodec/vda_h264.c | 27 +++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..13e4fc5 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
* The reference size used for fast reallocation.
*/
int priv_allocated_size;
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
+ *
+ * encoding: unused
+ * decoding: Set by user.
+ */
+ int use_ref_buffer;
};
/** Create the video decoder. */
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..a183afa 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -28,6 +28,10 @@
#include "h264.h"
#include "vda.h"
+struct vda_buffer {
+ CVPixelBufferRef cv_buffer;
+};
+
/* Decoder callback that adds the VDA frame to the queue in display order. */
static void vda_decoder_callback(void *vda_hw_ctx,
CFDictionaryRef user_info,
@@ -107,11 +111,20 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
return 0;
}
+static void vda_h264_release_buffer(void *opaque, uint8_t *data)
+{
+ struct vda_buffer *context = opaque;
+ CVPixelBufferRelease(context->cv_buffer);
+ av_free(context);
+}
+
static int vda_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
AVFrame *frame = &h->cur_pic_ptr->f;
+ struct vda_buffer *context;
+ AVBufferRef *buffer;
int status;
if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
@@ -123,6 +136,20 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
if (status)
av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);
+ if (!vda_ctx->use_ref_buffer || status)
+ return status;
+
+ context = av_mallocz(sizeof(*context));
+ buffer = av_buffer_create(NULL, 0, vda_h264_release_buffer, context, 0);
+ if (!context || !buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ av_free(context);
+ return -1;
+ }
+
+ context->cv_buffer = vda_ctx->cv_buffer;
+ frame->buf[3] = buffer;
Are the previous 3 entries always filled? I think the code currently assumes
the buffers are filled continuously.
It depends on the applications get_buffer/get_buffer2 custom implementations.
However, there is no reason to have previously filled these entries.
Anton Khirnov
2013-05-29 08:05:40 UTC
Permalink
Post by Sebastien Zwickert
Post by Anton Khirnov
Post by Sebastien Zwickert
This patch fixes a leak of buffer when seeking occurs.
It adds a flag in struct vda_context for compatibility with apps which
currently use it. If the flag is not set, the hwaccel will behave like
before.
---
libavcodec/vda.h | 11 +++++++++++
libavcodec/vda_h264.c | 27 +++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..13e4fc5 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
* The reference size used for fast reallocation.
*/
int priv_allocated_size;
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
+ *
+ * encoding: unused
+ * decoding: Set by user.
+ */
+ int use_ref_buffer;
};
/** Create the video decoder. */
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..a183afa 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -28,6 +28,10 @@
#include "h264.h"
#include "vda.h"
+struct vda_buffer {
+ CVPixelBufferRef cv_buffer;
+};
+
/* Decoder callback that adds the VDA frame to the queue in display order. */
static void vda_decoder_callback(void *vda_hw_ctx,
CFDictionaryRef user_info,
@@ -107,11 +111,20 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
return 0;
}
+static void vda_h264_release_buffer(void *opaque, uint8_t *data)
+{
+ struct vda_buffer *context = opaque;
+ CVPixelBufferRelease(context->cv_buffer);
+ av_free(context);
+}
+
static int vda_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
AVFrame *frame = &h->cur_pic_ptr->f;
+ struct vda_buffer *context;
+ AVBufferRef *buffer;
int status;
if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
@@ -123,6 +136,20 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
if (status)
av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);
+ if (!vda_ctx->use_ref_buffer || status)
+ return status;
+
+ context = av_mallocz(sizeof(*context));
+ buffer = av_buffer_create(NULL, 0, vda_h264_release_buffer, context, 0);
+ if (!context || !buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ av_free(context);
+ return -1;
+ }
+
+ context->cv_buffer = vda_ctx->cv_buffer;
+ frame->buf[3] = buffer;
Are the previous 3 entries always filled? I think the code currently assumes
the buffers are filled continuously.
It depends on the applications get_buffer/get_buffer2 custom implementations.
However, there is no reason to have previously filled these entries.
Then this patch should check what buffers are set and append after the last one.

Though I don't quite understand how does this work. If get_buffer2 is used, so
the frame data lives in user memory, then why is this additional buffer needed.
--
Anton Khirnov
Sebastien Zwickert
2013-06-08 11:05:09 UTC
Permalink
Post by Anton Khirnov
Post by Sebastien Zwickert
Post by Anton Khirnov
Post by Sebastien Zwickert
This patch fixes a leak of buffer when seeking occurs.
It adds a flag in struct vda_context for compatibility with apps which
currently use it. If the flag is not set, the hwaccel will behave like
before.
---
libavcodec/vda.h | 11 +++++++++++
libavcodec/vda_h264.c | 27 +++++++++++++++++++++++++++
2 files changed, 38 insertions(+)
diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..13e4fc5 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
* The reference size used for fast reallocation.
*/
int priv_allocated_size;
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
+ *
+ * encoding: unused
+ * decoding: Set by user.
+ */
+ int use_ref_buffer;
};
/** Create the video decoder. */
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..a183afa 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -28,6 +28,10 @@
#include "h264.h"
#include "vda.h"
+struct vda_buffer {
+ CVPixelBufferRef cv_buffer;
+};
+
/* Decoder callback that adds the VDA frame to the queue in display order. */
static void vda_decoder_callback(void *vda_hw_ctx,
CFDictionaryRef user_info,
@@ -107,11 +111,20 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
return 0;
}
+static void vda_h264_release_buffer(void *opaque, uint8_t *data)
+{
+ struct vda_buffer *context = opaque;
+ CVPixelBufferRelease(context->cv_buffer);
+ av_free(context);
+}
+
static int vda_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
AVFrame *frame = &h->cur_pic_ptr->f;
+ struct vda_buffer *context;
+ AVBufferRef *buffer;
int status;
if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
@@ -123,6 +136,20 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
if (status)
av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);
+ if (!vda_ctx->use_ref_buffer || status)
+ return status;
+
+ context = av_mallocz(sizeof(*context));
+ buffer = av_buffer_create(NULL, 0, vda_h264_release_buffer, context, 0);
+ if (!context || !buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ av_free(context);
+ return -1;
+ }
+
+ context->cv_buffer = vda_ctx->cv_buffer;
+ frame->buf[3] = buffer;
Are the previous 3 entries always filled? I think the code currently assumes
the buffers are filled continuously.
It depends on the applications get_buffer/get_buffer2 custom implementations.
However, there is no reason to have previously filled these entries.
Then this patch should check what buffers are set and append after the last one.
I've sent an updated patch that checks first free entry to append the vda extra buffer.
There's a workaround here to bypass the ref counted buffer mechanism here to avoid
memory leaks issue when flushing dropped frames (i.e.: while seeking).
I've added some comments before creating the extra buffer to try to make clear
this workaround.
Post by Anton Khirnov
Though I don't quite understand how does this work. If get_buffer2 is used, so
the frame data lives in user memory, then why is this additional buffer needed.
Core video buffers in vda_h264_end_frame may never be got by the user
while seeking for example and this causes some memory leaks.
Sebastien Zwickert
2013-06-08 11:01:51 UTC
Permalink
From: Xidorn Quan <***@gmail.com>

This patch fixes a leak of buffer when seeking occurs.
It adds a flag in struct vda_context for compatibility with apps which
currently use it. If the flag is not set, the hwaccel will behave like
before.
---
libavcodec/vda.h | 11 +++++++++++
libavcodec/vda_h264.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..13e4fc5 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
* The reference size used for fast reallocation.
*/
int priv_allocated_size;
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
+ *
+ * encoding: unused
+ * decoding: Set by user.
+ */
+ int use_ref_buffer;
};

/** Create the video decoder. */
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..564fc81 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -28,6 +28,10 @@
#include "h264.h"
#include "vda.h"

+struct vda_buffer {
+ CVPixelBufferRef cv_buffer;
+};
+
/* Decoder callback that adds the VDA frame to the queue in display order. */
static void vda_decoder_callback(void *vda_hw_ctx,
CFDictionaryRef user_info,
@@ -107,12 +111,21 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
return 0;
}

+static void vda_h264_release_buffer(void *opaque, uint8_t *data)
+{
+ struct vda_buffer *context = opaque;
+ CVPixelBufferRelease(context->cv_buffer);
+ av_free(context);
+}
+
static int vda_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
AVFrame *frame = &h->cur_pic_ptr->f;
- int status;
+ struct vda_buffer *context;
+ AVBufferRef *buffer;
+ int status, i;

if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
return -1;
@@ -123,6 +136,36 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
if (status)
av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);

+ if (!vda_ctx->use_ref_buffer || status)
+ return status;
+
+ buffer = NULL;
+
+ /* VDA workaround to release properly each core video buffers:
+ * we need to create an extra av buffer with a custom freeing callback. This extra buffer
+ * should not be ref counted to avoid potential memory leaks. That's why we put it
+ * after the first free entry of AVFrame.buf to not ref this extra buffer in
+ * AVFrame.av_frame_ref(). */
+ for (i = 1; i < AV_NUM_DATA_POINTERS; i++) {
+ if (!frame->buf[i] && !frame->buf[i-1]) {
+ context = av_mallocz(sizeof(*context));
+ buffer = av_buffer_create(NULL, 0, vda_h264_release_buffer, context, 0);
+ if (!context || !buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ av_free(context);
+ return -1;
+ }
+ context->cv_buffer = vda_ctx->cv_buffer;
+ frame->buf[i] = buffer;
+ break;
+ }
+ }
+
+ if (!buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ return -1;
+ }
+
return status;
}
--
1.7.9.6 (Apple Git-31.1)
Diego Biurrun
2013-06-08 12:48:28 UTC
Permalink
Post by Sebastien Zwickert
This patch fixes a leak of buffer when seeking occurs.
buffer leak
Post by Sebastien Zwickert
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -123,6 +136,36 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
Either "all buffer_s_" or "each buffer_".
Post by Sebastien Zwickert
+ * we need to create an extra av buffer with a custom freeing callback. This extra buffer
av_buffer
Post by Sebastien Zwickert
+ * should not be ref counted to avoid potential memory leaks. That's why we put it
reference-counted
Post by Sebastien Zwickert
+ * after the first free entry of AVFrame.buf to not ref this extra buffer in
reference

Diego
Sebastien Zwickert
2013-06-09 09:17:11 UTC
Permalink
Post by Diego Biurrun
Post by Sebastien Zwickert
This patch fixes a leak of buffer when seeking occurs.
buffer leak
Post by Sebastien Zwickert
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -123,6 +136,36 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
Either "all buffer_s_" or "each buffer_".
Post by Sebastien Zwickert
+ * we need to create an extra av buffer with a custom freeing callback. This extra buffer
av_buffer
Post by Sebastien Zwickert
+ * should not be ref counted to avoid potential memory leaks. That's why we put it
reference-counted
Post by Sebastien Zwickert
+ * after the first free entry of AVFrame.buf to not ref this extra buffer in
reference
Fixed.
Thanks!
Luca Barbato
2013-06-08 18:51:48 UTC
Permalink
Post by Sebastien Zwickert
This patch fixes a leak of buffer when seeking occurs.
It adds a flag in struct vda_context for compatibility with apps which
currently use it. If the flag is not set, the hwaccel will behave like
before.
---
libavcodec/vda.h | 11 +++++++++++
libavcodec/vda_h264.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..13e4fc5 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
* The reference size used for fast reallocation.
*/
int priv_allocated_size;
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
+ *
+ * encoding: unused
+ * decoding: Set by user.
+ */
+ int use_ref_buffer;
};
/** Create the video decoder. */
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..564fc81 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -28,6 +28,10 @@
#include "h264.h"
#include "vda.h"
+struct vda_buffer {
+ CVPixelBufferRef cv_buffer;
+};
It is an unneeded level of indirection IMHO.
Post by Sebastien Zwickert
/* Decoder callback that adds the VDA frame to the queue in display order. */
static void vda_decoder_callback(void *vda_hw_ctx,
CFDictionaryRef user_info,
@@ -107,12 +111,21 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
return 0;
}
+static void vda_h264_release_buffer(void *opaque, uint8_t *data)
+{
+ struct vda_buffer *context = opaque;
CVPixelBufferRef *context = opaque
Post by Sebastien Zwickert
+ CVPixelBufferRelease(context->cv_buffer);
*context
Post by Sebastien Zwickert
+ av_free(context);
+}
+
static int vda_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
AVFrame *frame = &h->cur_pic_ptr->f;
- int status;
+ struct vda_buffer *context;
+ AVBufferRef *buffer;
+ int status, i;
if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
return -1;
@@ -123,6 +136,36 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
if (status)
av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);
+ if (!vda_ctx->use_ref_buffer || status)
+ return status;
+
+ buffer = NULL;
+
+ * we need to create an extra av buffer with a custom freeing callback. This extra buffer
+ * should not be ref counted to avoid potential memory leaks. That's why we put it
+ * after the first free entry of AVFrame.buf to not ref this extra buffer in
+ * AVFrame.av_frame_ref(). */
+ for (i = 1; i < AV_NUM_DATA_POINTERS; i++) {
+ if (!frame->buf[i] && !frame->buf[i-1]) {
+ context = av_mallocz(sizeof(*context));
what prevents context from being a CVPixelBufferRef ?
Post by Sebastien Zwickert
+ buffer = av_buffer_create(NULL, 0, vda_h264_release_buffer, context, 0);
+ if (!context || !buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ av_free(context);
+ return -1;
+ }
+ context->cv_buffer = vda_ctx->cv_buffer;
*context = vda_ctx->cv_buffer;

lu
Sebastien Zwickert
2013-06-09 09:15:06 UTC
Permalink
Post by Luca Barbato
Post by Sebastien Zwickert
This patch fixes a leak of buffer when seeking occurs.
It adds a flag in struct vda_context for compatibility with apps which
currently use it. If the flag is not set, the hwaccel will behave like
before.
---
libavcodec/vda.h | 11 +++++++++++
libavcodec/vda_h264.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..13e4fc5 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
* The reference size used for fast reallocation.
*/
int priv_allocated_size;
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
+ *
+ * encoding: unused
+ * decoding: Set by user.
+ */
+ int use_ref_buffer;
};
/** Create the video decoder. */
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..564fc81 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -28,6 +28,10 @@
#include "h264.h"
#include "vda.h"
+struct vda_buffer {
+ CVPixelBufferRef cv_buffer;
+};
It is an unneeded level of indirection IMHO.
You're absolutely right. Sorry I misunderstood your comment in the previous discussion
about this level of indirection.

Thanks!
Post by Luca Barbato
[…]
Sebastien Zwickert
2013-06-09 09:17:43 UTC
Permalink
From: Xidorn Quan <***@gmail.com>

This patch fixes a buffer leak when seeking occurs.
It adds a flag in struct vda_context for compatibility with apps which
currently use it. If the flag is not set, the hwaccel will behave like
before.
---
libavcodec/vda.h | 11 +++++++++++
libavcodec/vda_h264.c | 36 +++++++++++++++++++++++++++++++++++-
2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..13e4fc5 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
* The reference size used for fast reallocation.
*/
int priv_allocated_size;
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
+ *
+ * encoding: unused
+ * decoding: Set by user.
+ */
+ int use_ref_buffer;
};

/** Create the video decoder. */
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..aa6a3a7 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -107,12 +107,19 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
return 0;
}

+static void vda_h264_release_buffer(void *opaque, uint8_t *data)
+{
+ CVPixelBufferRef cv_buffer = opaque;
+ CVPixelBufferRelease(cv_buffer);
+}
+
static int vda_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
AVFrame *frame = &h->cur_pic_ptr->f;
- int status;
+ AVBufferRef *buffer;
+ int status, i;

if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
return -1;
@@ -123,6 +130,33 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
if (status)
av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);

+ if (!vda_ctx->use_ref_buffer || status)
+ return status;
+
+ buffer = NULL;
+
+ /* VDA workaround to release properly each core video buffer:
+ * we need to create an extra av_buffer with a custom freeing callback. This extra buffer
+ * should not be reference-counted to avoid potential memory leaks. That's why we put it
+ * after the first free entry of AVFrame.buf to not reference this extra buffer in
+ * AVFrame.av_frame_ref(). */
+ for (i = 1; i < AV_NUM_DATA_POINTERS; i++) {
+ if (!frame->buf[i] && !frame->buf[i-1]) {
+ buffer = av_buffer_create(NULL, 0, vda_h264_release_buffer, vda_ctx->cv_buffer, 0);
+ if (!buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ return -1;
+ }
+ frame->buf[i] = buffer;
+ break;
+ }
+ }
+
+ if (!buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ return -1;
+ }
+
return status;
}
--
1.7.9.6 (Apple Git-31.1)
Diego Biurrun
2013-06-09 12:43:58 UTC
Permalink
Post by Sebastien Zwickert
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
Either write "memory leak_s_" or "a memory leak_".
Post by Sebastien Zwickert
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -123,6 +130,33 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
+
properly release
Post by Sebastien Zwickert
+ * we need to create an extra av_buffer with a custom freeing callback. This extra buffer
+ * should not be reference-counted to avoid potential memory leaks. That's why we put it
+ * after the first free entry of AVFrame.buf to not reference this extra buffer in
+ * AVFrame.av_frame_ref(). */
* We need to create an extra av_buffer with a custom freeing
callback.
* This extra buffer should not be reference-counted to avoid
potential
* memory leaks. That iss why it is placed after the first free
entry
* of AVFrame.buf to not reference this extra buffer
* in AVFrame.av_frame_ref(). */

Wait for more comments before sending a new patch.

Diego
Anton Khirnov
2013-06-10 19:03:42 UTC
Permalink
Post by Sebastien Zwickert
This patch fixes a buffer leak when seeking occurs.
It adds a flag in struct vda_context for compatibility with apps which
currently use it. If the flag is not set, the hwaccel will behave like
before.
---
libavcodec/vda.h | 11 +++++++++++
libavcodec/vda_h264.c | 36 +++++++++++++++++++++++++++++++++++-
2 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/libavcodec/vda.h b/libavcodec/vda.h
index 987b94f..13e4fc5 100644
--- a/libavcodec/vda.h
+++ b/libavcodec/vda.h
@@ -125,6 +125,17 @@ struct vda_context {
* The reference size used for fast reallocation.
*/
int priv_allocated_size;
+
+ /**
+ * Use av_buffer to manage buffer.
+ * When the flag is set, the CVPixelBuffers returned by the decoder will
+ * be released automatically, so you have to retain them if necessary.
+ * Not setting this flag may cause memory leak.
+ *
+ * encoding: unused
+ * decoding: Set by user.
+ */
+ int use_ref_buffer;
};
/** Create the video decoder. */
diff --git a/libavcodec/vda_h264.c b/libavcodec/vda_h264.c
index 6c1845a..aa6a3a7 100644
--- a/libavcodec/vda_h264.c
+++ b/libavcodec/vda_h264.c
@@ -107,12 +107,19 @@ static int vda_h264_decode_slice(AVCodecContext *avctx,
return 0;
}
+static void vda_h264_release_buffer(void *opaque, uint8_t *data)
+{
+ CVPixelBufferRef cv_buffer = opaque;
+ CVPixelBufferRelease(cv_buffer);
+}
+
static int vda_h264_end_frame(AVCodecContext *avctx)
{
H264Context *h = avctx->priv_data;
struct vda_context *vda_ctx = avctx->hwaccel_context;
AVFrame *frame = &h->cur_pic_ptr->f;
- int status;
+ AVBufferRef *buffer;
+ int status, i;
if (!vda_ctx->decoder || !vda_ctx->priv_bitstream)
return -1;
@@ -123,6 +130,33 @@ static int vda_h264_end_frame(AVCodecContext *avctx)
if (status)
av_log(avctx, AV_LOG_ERROR, "Failed to decode frame (%d)\n", status);
+ if (!vda_ctx->use_ref_buffer || status)
+ return status;
+
+ buffer = NULL;
+
+ * we need to create an extra av_buffer with a custom freeing callback. This extra buffer
+ * should not be reference-counted to avoid potential memory leaks. That's why we put it
+ * after the first free entry of AVFrame.buf to not reference this extra buffer in
+ * AVFrame.av_frame_ref(). */
+ for (i = 1; i < AV_NUM_DATA_POINTERS; i++) {
+ if (!frame->buf[i] && !frame->buf[i-1]) {
+ buffer = av_buffer_create(NULL, 0, vda_h264_release_buffer, vda_ctx->cv_buffer, 0);
+ if (!buffer) {
+ CVPixelBufferRelease(vda_ctx->cv_buffer);
+ return -1;
+ }
+ frame->buf[i] = buffer;
+ break;
+ }
+ }
I must say this looks quite hacky to me.

If this isn't meant to propagate to other references, why is this stored in the
frame at all? Why don't you store it in the private context?
--
Anton Khirnov
Loading...