[FFmpeg-devel] [PATCH] pthread_frame: set err from the thread that return frame

[wm4]

On Wed, 26 Apr 2017 11:34:22 -0400
"Ronald S. Bultje" <rsbultje at gmail.com> wrote:

> Hi,
> 
> On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfcc64 at gmail.com> wrote:
> 
> > On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg at googlemail.com> wrote:  
> > > On Tue, 25 Apr 2017 23:52:04 +0700
> > > Muhammad Faiz <mfcc64 at gmail.com> wrote:
> > >  
> > >> when frame is received, not from other threads.
> > >>
> > >> Should fix fate failure with THREADS>=4:
> > >>   make fate-h264-attachment-631 THREADS=4
> > >>
> > >> Signed-off-by: Muhammad Faiz <mfcc64 at gmail.com>
> > >> ---
> > >>  libavcodec/pthread_frame.c | 4 ++++
> > >>  1 file changed, 4 insertions(+)
> > >>
> > >> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> > >> index 13d6828..c452ed7 100644
> > >> --- a/libavcodec/pthread_frame.c
> > >> +++ b/libavcodec/pthread_frame.c
> > >> @@ -547,6 +547,10 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
> > >>
> > >>      fctx->next_finished = finished;
> > >>
> > >> +    /* if frame is returned, properly set err from the thread that  
> > return frame */  
> > >> +    if (*got_picture_ptr)
> > >> +        err = p->result;
> > >> +
> > >>      /* return the size of the consumed packet if no error occurred */
> > >>      if (err >= 0)
> > >>          err = avpkt->size;  
> > >
> > > Well, the logic confuses me. Does this override an earlier set err
> > > value?  
> >
> > Yes, because an earlier set err value may be from a different thread.
> >  
> > >Could err be set to the correct value in the first place (inside
> > > of the loop)?  
> >
> > No, it was intended on 32a5b631267  
> 
> 
> Thanks for working on this. It's good to get more people familiar with this
> code.
> 
> So, I'm looking at understanding this, you're trying to fix the case where
> during draining, we may iterate over >1 worker thread cases where the first
> returned an error code without having decoded a frame, and the second
> decoded a frame without returning an error code, right? The current code
> would return a frame with an error return code, which I believe is then
> ignored by the user thread.
> 
> So, you're basically trying to say that instead, we should ignore the
> error. I agree that fixes the issue of md5 mismatch w/ vs. w/o threads, but
> I doubt that it's fundamentally more correct, because the user thread still
> misses out on error codes from the worker threads. Wouldn't you agree that
> we should - even during draining - not iterate over N threads, but just the
> next thread, and return either an error or a decoded frame, and keep doing
> that until all worker threads are flushed, which we can then signal e.g. by
> return=0 and *got_picture_ptr=0?

That actually sounds good to me (although this issue is currently a
distraction to me so I never thought too deeply about it, not looked
too closely at the code).