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

[Muhammad Faiz]

On Fri, Apr 28, 2017 at 12:16 AM, Muhammad Faiz <mfcc64 at gmail.com> wrote:
> 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?
>

A Patch is sent.

Thx.