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

[Michael Niedermayer]

On Tue, Feb 25, 2014 at 01:46:21AM +0100, Lukasz Marek wrote:
> On 25.02.2014 01:19, Michael Niedermayer wrote:
> >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
> >
> >Thread B cannot call ref if it has no reference, thats violating
> >the API
> 
> If so then OK, just haven't found it in docs (I did not read all tho)

i dont know if its documented.


> 
> 
> >>  buffer.c |    2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>802258605a7bfb649be51b57b10830b2c7b35bda  0002-lavu-buffer-do-not-touch-refcount-directly.patch
> >> From 220010aa7b2971f33346fdb0a78bd95b5f91be25 Mon Sep 17 00:00:00 2001
> >>From: Lukasz Marek <lukasz.m.luki at gmail.com>
> >>Date: Tue, 25 Feb 2014 00:38:20 +0100
> >>Subject: [PATCH 2/2] lavu/buffer: do not touch refcount directly
> >>
> >>Signed-off-by: Lukasz Marek <lukasz.m.luki at gmail.com>
> >>---
> >>  libavutil/buffer.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >>diff --git a/libavutil/buffer.c b/libavutil/buffer.c
> >>index 76582f2..2a41150 100644
> >>--- a/libavutil/buffer.c
> >>+++ b/libavutil/buffer.c
> >>@@ -161,7 +161,7 @@ void *av_buffer_get_opaque(const AVBufferRef *buf)
> >>
> >>  int av_buffer_get_ref_count(const AVBufferRef *buf)
> >>  {
> >>-    return buf->buffer->refcount;
> >>+    return avpriv_atomic_int_get(&buf->buffer->refcount);
> >
> >do you know of a case where this needs to be atomic ?
> 
> OK, but why it is used in other places? If no use case in here than
> what's the use case in av_buffer_is_writable for example?

> If you are
> sure it is not required then feel free to ignore this patch.
> Probably OK, I haven't investigate it before, just guessed from
> other uses of this function.

Iam not sure, which is why i asked
and also why ive not suggested to remove it from
av_buffer_is_writable()
I dont immedeatly see why it would be needed, this does not mean its
not needed
but i would suggest to leave it on all functions that are not speed
critical

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

it is not once nor twice but times without number that the same ideas make
their appearance in the world. -- Aristotle
-------------- 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/c3bcf830/attachment.asc>