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

[Muhammad Faiz]

On Thu, Apr 27, 2017 at 5:30 AM, Marton Balint <cus at passwd.hu> wrote:
>
>
> On Wed, 26 Apr 2017, Ronald S. Bultje wrote:
>
>> 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?
>
>
> I propose the following:
>
> Using the old (and deprecated) public API you should simply get an error.
> Losing more frames in the end if threading is invovled is acceptable IMHO.
> Sliently ignoring an error is not.

Error is not silently ignored. Only reordered, and returned after all
frames are flushed

>
> Using the new public API you should get the error code, then the proper
> frame on the next call. In the new public API only AVERROR_EOF signals EOF,
> so this seems doable.

Sound good. Are all decoders ready for this? I mean, a guarantee that
they don't return error infinitely on eof?


>
> Or do I miss something? Or I just stated the obvious? :)