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

[Michael Niedermayer]

On Sun, Mar 17, 2013 at 07:16:56PM +0100, Hendrik Leppkes wrote:
> 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.

applied

thanks


> This whole atomic thing is sometimes confusing me, personally i
> probably would've used a mutex for the whole buffer pool.

yes, i wonder which way is actually faster, these atomic operations
are not fast and they need additional code ...

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

Those who are too smart to engage in politics are punished by being
governed by those who are dumber. -- Plato 
-------------- 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/20130317/82bcc299/attachment.asc>