[FFmpeg-devel] [PATCH 1/3] avutil/get_pool: Remove redundant initial atomic operation

[Hendrik Leppkes]

On Sun, Mar 17, 2013 at 7:06 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
> On Sun, Mar 17, 2013 at 06:54:35PM +0100, Hendrik Leppkes wrote:
>> On Sun, Mar 17, 2013 at 6:46 PM, Michael Niedermayer <michaelni at gmx.at> wrote:
>> > 602->442 dezicycles
>> >
>> > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
>> > ---
>> >  libavutil/buffer.c |    6 +++---
>> >  1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/libavutil/buffer.c b/libavutil/buffer.c
>> > index d268a7f..3475e57 100644
>> > --- a/libavutil/buffer.c
>> > +++ b/libavutil/buffer.c
>> > @@ -239,14 +239,14 @@ void av_buffer_pool_uninit(AVBufferPool **ppool)
>> >  /* remove the whole buffer list from the pool and return it */
>> >  static BufferPoolEntry *get_pool(AVBufferPool *pool)
>> >  {
>> > -    BufferPoolEntry *cur = NULL, *last = NULL;
>> > +    BufferPoolEntry *cur = *(void * volatile *)&pool->pool, *last = NULL;
>> >
>> > -    do {
>> > +    while (cur != last) {
>> >          FFSWAP(BufferPoolEntry*, cur, last);
>> >          cur = avpriv_atomic_ptr_cas((void * volatile *)&pool->pool, last, NULL);
>> >          if (!cur)
>> >              return NULL;
>> > -    } while (cur != last);
>> > +    }
>> >
>> >      return cur;
>> >  }
>> > --
>> > 1.7.9.5
>> >
>>
>>
>> I don't think you can safely assume that the initial get is atomic
>> because there is no memory barrier, and considering this function is
>> called only a handful of times per frame, i would stay on the safe
>> side.
>
> It should be safe because the initial value of cur doesnt matter
>
> consider cur is set to NULL, this can already happen as get_pool()
> can always switch the whole buffer chain out making it NULL until it
> gets reinserted
>
> now consider its non NULL but wrong
> while (cur != last) will be true
>
> cur = avpriv_atomic_ptr_cas((void * volatile *)&pool->pool, last, NULL);
> will compare it against the actual correct value and just do an extra
> iteration
>
> but iam not insisting on this, it just looked inefficient to me how it
> was done
>
> [...]

You're right, it would work out as well.
This whole atomic thing is sometimes confusing me, personally i
probably would've used a mutex for the whole buffer pool.