[FFmpeg-devel] [PATCH] lavu/buffer: add release function

[Michael Niedermayer]

On Tue, Feb 25, 2014 at 12:58:12AM +0100, Lukasz Marek wrote:
> On 24.02.2014 02:18, Michael Niedermayer wrote:
> >On Sun, Feb 23, 2014 at 11:19:23PM +0100, Lukasz Marek wrote:
> >>new function allows to unref buffer and obtain its data.
> >>
> >>Signed-off-by: Lukasz Marek <lukasz.m.luki at gmail.com>
> >>---
> >>  libavutil/buffer.c | 26 ++++++++++++++++++++++++++
> >>  libavutil/buffer.h | 12 ++++++++++++
> >>  2 files changed, 38 insertions(+)
> >>
> >>diff --git a/libavutil/buffer.c b/libavutil/buffer.c
> >>index e9bf54b..a68b0be 100644
> >>--- a/libavutil/buffer.c
> >>+++ b/libavutil/buffer.c
> >>@@ -117,6 +117,32 @@ void av_buffer_unref(AVBufferRef **buf)
> >>      }
> >>  }
> >>
> >>+int av_buffer_release(AVBufferRef **buf, uint8_t **data)
> >>+{
> >>+    AVBuffer *b;
> >>+
> >>+    if (!buf || !*buf) {
> >>+        if (data)
> >>+            *data = NULL;
> >>+        return 0;
> >>+    }
> >>+    b = (*buf)->buffer;
> >>+    av_freep(buf);
> >>+
> >>+    if (!avpriv_atomic_int_add_and_fetch(&b->refcount, -1)) {
> >>+        if (data)
> >>+            *data = b->data;
> >>+        else
> >>+            b->free(b->opaque, b->data);
> >>+        av_freep(&b);
> >
> >>+    } else if (data) {
> >>+        *data = av_memdup(b->data, b->size);
> >>+        if (!*data)
> >>+            return AVERROR(ENOMEM);
> >
> >this is not safe
> >
> >you decreased the ref count and afterwards copy
> >but between the 2 the memory could have been deallocated
> 
> Yep,
> I attached updated patach - hopely better, and one more which is not
> relevant to the first one, but kinda trivial, so don't want to spam
> too much.
> 
> I assumed you talked about unref from other thread while memory is
> being copied. It is true it is not safe, but I think there are
> already rece condition in buffer.c.
> 
> For example there is AVBufferRef pointer shared across 2 threads
> Thread A has reference and calls unref
> Thread B has no reference and calls ref
> In case thread A enter buffer release "if" (refcount drops to 0) and
> thread B increases reference during freeing memory then thread B
> will have reference to released buffer if Im not wrong.
> 
> Also av_buffer_is_writable seems to be API pitfall. Other thread may
> increase ref count and buffer will not be writable anymore.
> 
> If I'm not mistaken than maybe I should open ticket for it?
> 
> -- 
> Best Regards,
> Lukasz Marek
> 
> You can avoid reality, but you cannot avoid the consequences of
> avoiding reality. - Ayn Rand

>  buffer.c |   29 +++++++++++++++++++++++++++++
>  buffer.h |   12 ++++++++++++
>  2 files changed, 41 insertions(+)
> 614d96fcb4f74611859a605963c97c5c9c78eeb5  0001-lavu-buffer-add-release-function.patch
> From 01d21fb2e8fc217b46c6c049ae66cd055c81190f Mon Sep 17 00:00:00 2001
> From: Lukasz Marek <lukasz.m.luki at gmail.com>
> Date: Sun, 23 Feb 2014 23:19:23 +0100
> Subject: [PATCH 1/2] lavu/buffer: add release function
> 
> new function allows to unref buffer and obtain its data.
> 
> Signed-off-by: Lukasz Marek <lukasz.m.luki at gmail.com>
> ---
>  libavutil/buffer.c | 29 +++++++++++++++++++++++++++++
>  libavutil/buffer.h | 12 ++++++++++++
>  2 files changed, 41 insertions(+)

patch applied

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Frequently ignored answer#1 FFmpeg bugs should be sent to our bugtracker. User
questions about the command line tools should be sent to the ffmpeg-user ML.
And questions about how to use libav* should be sent to the libav-user ML.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20140225/4b323926/attachment.asc>