[FFmpeg-devel] [PATCH 1/2] avutil/threadmessage: add av_thread_message_flush()

Clément Bœsch u at pkh.me
Tue Dec 1 17:26:29 CET 2015


On Tue, Dec 01, 2015 at 05:11:06PM +0100, Nicolas George wrote:
> Le decadi 10 frimaire, an CCXXIV, Clement Boesch a écrit :
> > From: Clément Bœsch <clement at stupeflix.com>
> > 
> > ---
> >  libavutil/threadmessage.c | 37 ++++++++++++++++++++++++++++++++++---
> >  libavutil/threadmessage.h | 21 ++++++++++++++++++---
> >  2 files changed, 52 insertions(+), 6 deletions(-)
> > 
> > diff --git a/libavutil/threadmessage.c b/libavutil/threadmessage.c
> > index b7fcbe2..87ce8dc 100644
> > --- a/libavutil/threadmessage.c
> > +++ b/libavutil/threadmessage.c
> > @@ -40,14 +40,16 @@ struct AVThreadMessageQueue {
> >      int err_send;
> >      int err_recv;
> >      unsigned elsize;
> > +    void (*free_func)(void *msg);
> >  #else
> >      int dummy;
> >  #endif
> >  };
> >  
> > -int av_thread_message_queue_alloc(AVThreadMessageQueue **mq,
> > -                                  unsigned nelem,
> > -                                  unsigned elsize)
> 
> > +int av_thread_message_queue_alloc2(AVThreadMessageQueue **mq,
> > +                                   unsigned nelem,
> > +                                   unsigned elsize,
> > +                                   void (*free_func)(void *msg))
> 
> I must say I am not very fond of this aspect of the change, because of
> foobar2 and numerous function arguments.
> 
> What about having the application set queue->free_func with a second
> function if needed, either directly or through
> av_thread_message_queue_set_free_func()?
> 

Yeah, sure, no opinion really. Will send a new patch.

[...]
> > +/**
> > + * Flush the message queue
> > + */
> > +void av_thread_message_flush(AVThreadMessageQueue *mq);
> 
> Can you explain in the doxy (and to me) how it is better than:
> 
> 	while (av_thread_message_queue_recv_locked(mq, msg, AV_THREAD_MESSAGE_NONBLOCK))
> 	    free_func(msg);
> 


current:
    int used, off;

    pthread_mutex_lock(&mq->lock);
    used = av_fifo_size(mq->fifo);
    if (mq->free_func) {
        for (off = 0; off < used; off += mq->elsize) {
            void *msg;
            av_fifo_generic_peek_at(mq->fifo, &msg, off, mq->elsize, NULL);
            mq->free_func(msg);
        }
    }
    av_fifo_drain(mq->fifo, used);
    pthread_cond_broadcast(&mq->cond);
    pthread_mutex_unlock(&mq->lock);

yours:
    void *msg;

    pthread_mutex_lock(&mq->lock);
    while (av_thread_message_queue_recv_locked(mq, msg, AV_THREAD_MESSAGE_NONBLOCK))
        if (mq->free_func)
            mq->free_func(msg);
    pthread_mutex_unlock(&mq->lock);

Your suggestion is twice smaller.  I don't mind that much but I guess I like
saving a few checks and preventing a bunch of signal broadcasting. Also,
passing the AV_THREAD_MESSAGE_NONBLOCK seems a bit like a (smart) hack to me. I
will pick your solution (which I admit haven't thought of initially) if you
insist, but I prefer my version.

[...]

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20151201/4cd29a60/attachment.sig>


More information about the ffmpeg-devel mailing list