[FFmpeg-devel] [PATCH] pthread_frame: minor simplification to error handling

Alexander Strasser eclipse7 at gmx.net
Wed Mar 29 23:25:48 EEST 2017


Hi,

I already saw this on -cvslog ml, so apparently I am too late...

On 2017-03-29 12:19 +0200, wm4 wrote:
> On Mon, 27 Mar 2017 14:28:17 +0200
> wm4 <nfxjfg at googlemail.com> wrote:
> 
> > Get rid of the "ret" variable, and always use err. Report the packet as
> > consumed if err is unset. This should be equivalent to the old code,
> > which obviously required err=0 for p->result>=0 (and otherwise,
> > p->result must have had the value err was last set to). The code block
> > added by commit 32a5b631267 is also not needed anymore, because the new
> > code strictly returns err if it's >=0.
> > ---
> > Not totally sure about this, but it seems to work out? I probably missed
> > something obvious.
> > ---
> >  libavcodec/pthread_frame.c | 19 +++++--------------
> >  1 file changed, 5 insertions(+), 14 deletions(-)
> > 
> > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> > index b618be0bf5..36e4a8affa 100644
> > --- a/libavcodec/pthread_frame.c
> > +++ b/libavcodec/pthread_frame.c
> > @@ -468,7 +468,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
> >      FrameThreadContext *fctx = avctx->internal->thread_ctx;
> >      int finished = fctx->next_finished;
> >      PerThreadContext *p;
> > -    int err, ret = 0;
> > +    int err;

Wouldn't it have been more readable to keep ret instead of err?

Assigning a size to err looks a bit strange. I am not insisting on changing
it now, just wanted to point this out.

> >  
> >      /* release the async lock, permitting blocked hwaccel threads to
> >       * go forward while we are in this function */
> > @@ -496,7 +496,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
> >      if (fctx->delaying) {
> >          *got_picture_ptr=0;
> >          if (avpkt->size) {
> > -            ret = avpkt->size;
> > +            err = avpkt->size;
> >              goto finish;
> >          }
> >      }
> > @@ -542,21 +542,12 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
> >  
> >      fctx->next_finished = finished;
> >  
> > -    /*
> > -     * When no frame was found while flushing, but an error occurred in
> > -     * any thread, return it instead of 0.
> > -     * Otherwise the error can get lost.
> > -     */
> > -    if (!avpkt->size && !*got_picture_ptr)
> > -        goto finish;
> > -
> >      /* return the size of the consumed packet if no error occurred */
> > -    ret = (p->result >= 0) ? avpkt->size : p->result;
> > +    if (err >= 0)
> > +        err = avpkt->size;
> >  finish:
> >      async_lock(fctx);
> > -    if (err < 0)
> > -        return err;
> > -    return ret;
> > +    return err;
> >  }
> >  
> >  void ff_thread_report_progress(ThreadFrame *f, int n, int field)
> 
> Received a review by BBB on IRC yesterday. Pushed.

It's kind of hard to follow these changes and the function in general, but
after looking at the function and this patch a few times it seems alright
and simpler.

  Alexander


More information about the ffmpeg-devel mailing list