[FFmpeg-devel] [PATCH] pthreads: Generic progress lubrication support.

[Michael Niedermayer]

On Mon, Jan 23, 2012 at 08:29:41PM +0100, Reimar Döffinger wrote:
> On Mon, Jan 23, 2012 at 08:21:19PM +0100, Reimar Döffinger wrote:
> > On Mon, Jan 23, 2012 at 04:00:11PM +0100, Michael Niedermayer wrote:
> > > On Mon, Jan 23, 2012 at 08:58:05AM +0100, Reimar Döffinger wrote:
> > > > 
> > > > 
> > > > On 23 Jan 2012, at 07:08, Michael Niedermayer <michaelni at gmx.at> wrote:
> > > > 
> > > > > Fixes bug118, bug120 and bug125 at least
> > > > > 
> > > > > Signed-off-by: Michael Niedermayer <michaelni at gmx.at>
> > > > > ---
> > > > > libavcodec/pthread.c |    7 +++++++
> > > > > 1 files changed, 7 insertions(+), 0 deletions(-)
> > > > > 
> > > > > diff --git a/libavcodec/pthread.c b/libavcodec/pthread.c
> > > > > index 070dbff..6ae763d 100644
> > > > > --- a/libavcodec/pthread.c
> > > > > +++ b/libavcodec/pthread.c
> > > > > @@ -366,6 +366,7 @@ static attribute_align_arg void *frame_worker_thread(void *arg)
> > > > >     AVCodec *codec = avctx->codec;
> > > > > 
> > > > >     while (1) {
> > > > > +        int i;
> > > > >         if (p->state == STATE_INPUT_READY && !fctx->die) {
> > > > >             pthread_mutex_lock(&p->mutex);
> > > > >             while (p->state == STATE_INPUT_READY && !fctx->die)
> > > > > @@ -388,6 +389,12 @@ static attribute_align_arg void *frame_worker_thread(void *arg)
> > > > >         p->state = STATE_INPUT_READY;
> > > > > 
> > > > >         pthread_mutex_lock(&p->progress_mutex);
> > > > > +        for (i = 0; i < MAX_BUFFERS; i++)
> > > > > +            if (p->progress_used[i]) {
> > > > > +                p->progress[i][0] = INT_MAX;
> > > > > +                p->progress[i][1] = INT_MAX;
> > > > > +            }
> > > > > +        pthread_cond_broadcast(&p->progress_cond);
> > > > 
> > > > I think this should add a loud warning when encountering these values since IMO it indicates a bug in the decoder.
> > > 
> > > Iam not sure how to detect that the progress was increased from the
> > > max the decoder would normally set because the code doesnt know the
> > > max.
> > > And it seems overkill to add code for exporting the max just for the
> > > warning IMHO
> > > but ideas are welcome ..
> > 
> > --- a/libavcodec/pthread.c
> > +++ b/libavcodec/pthread.c
> > @@ -391,8 +391,10 @@ static attribute_align_arg void *frame_worker_thread(void *arg)
> >          pthread_mutex_lock(&p->progress_mutex);
> >          for (i = 0; i < MAX_BUFFERS; i++)
> >              if (p->progress_used[i]) {
> > -                p->progress[i][0] = INT_MAX;
> > -                p->progress[i][1] = INT_MAX;
> > +                if (p->progress[i][0] < INT_MAX - 1)
> > +                    p->progress[i][0] = -p->progress[i][0] - 3;
> > +                if (p->progress[i][1] < INT_MAX - 1)
> > +                    p->progress[i][1] = -p->progress[i][1] - 3;
> >              }
> >          pthread_cond_broadcast(&p->progress_cond);
> >          pthread_cond_signal(&p->output_cond);
> > @@ -697,6 +699,12 @@ void ff_thread_await_progress(AVFrame *f, int n, int field)
> >      int *progress = f->thread_opaque;
> >  
> >      if (!progress || progress[field] >= n) return;
> > +    if (progress[field] < -1) {
> > +        int max = -(progress[field] + 3);
> > +        if (max < n)
> > +            av_log(f->owner, AV_LOG_ERROR, "thread awaiting %d field %d from %d"
> > +                   "which will never be reached (max %d)\n", n, field, progress, max);
> > +    }
> 
> And yes, I obviously forgot a "return" here.

this adds races & a deadlock

this code is not under a mutex, progress can change at any time between
these checks.

also i think whoever wants to 50x duplicate the progress update in
every decoder could just add this code to his local tree and once he
is done with the cleanup we can merge it
I dont see much sense in this being in main git before we have some
volunteer to actually do anything about these warnings.

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

During times of universal deceit, telling the truth becomes a
revolutionary act. -- George Orwell
-------------- 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/20120123/c70784d6/attachment.asc>