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

Michael Niedermayer michaelni at gmx.at
Mon Jan 23 22:14:12 CET 2012


On Mon, Jan 23, 2012 at 09:34:32PM +0100, Michael Niedermayer wrote:
> 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.

of course if someone finds a clean way to print this warning then its
a different matter but as is this looks kinda risky and messy for
just a warning

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

The worst form of inequality is to try to make unequal things equal.
-- Aristotle
-------------- 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/4ef1c117/attachment.asc>


More information about the ffmpeg-devel mailing list