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

[Ronald S. Bultje]

Hi,

On Wed, Apr 26, 2017 at 2:20 PM, Muhammad Faiz <mfcc64 at gmail.com> wrote:

> On Wed, Apr 26, 2017 at 10:34 PM, 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?
>
> The problem is that return<0 and *got_picture_ptr==0 is also
> considered as eof when avpkt->size==0.


This will probably count as an API change then, but my thinking is that we
should add a new API that "fixes" the above. The old API can then skip
error-packets-on-flush (similar to how your patch does it).

Or do people dislike this?

Ronald