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

[Ronald S. Bultje]

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?

Ronald