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

Muhammad Faiz mfcc64 at gmail.com
Thu Apr 27 20:07:49 EEST 2017


On Thu, Apr 27, 2017 at 2:59 AM, wm4 <nfxjfg at googlemail.com> wrote:
> On Thu, 27 Apr 2017 01:20:53 +0700
> 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.
>
> The old public decode API, or the new API? The latter would be about
> the wrapper. I remember adding a hack to avoid the situation that
> decoders can get "stuck" during draining, not sure if it was
> overwritten in a recent merge.

New API, of course. For old api, it depends on who interprets return
value and got_frame value.


More information about the ffmpeg-devel mailing list