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

[Lukasz Marek]

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)


>>   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.


-- 
Best Regards,
Lukasz Marek

Royale with Cheese.