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

Submitted by Muhammad Faiz on April 25, 2017, 4:52 p.m.

Details

Message ID 20170425165204.3061-1-mfcc64@gmail.com
State New
Headers show

Commit Message

Muhammad Faiz April 25, 2017, 4:52 p.m.
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@gmail.com>
---
 libavcodec/pthread_frame.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

wm4 April 25, 2017, 9:09 p.m.
On Tue, 25 Apr 2017 23:52:04 +0700
Muhammad Faiz <mfcc64@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@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? Could err be set to the correct value in the first place (inside
of the loop)?
Muhammad Faiz April 26, 2017, 5:21 a.m.
On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg@googlemail.com> wrote:
> On Tue, 25 Apr 2017 23:52:04 +0700
> Muhammad Faiz <mfcc64@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@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
Ronald S. Bultje April 26, 2017, 3:34 p.m.
Hi,

On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:

> On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg@googlemail.com> wrote:
> > On Tue, 25 Apr 2017 23:52:04 +0700
> > Muhammad Faiz <mfcc64@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@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
wm4 April 26, 2017, 4:27 p.m.
On Wed, 26 Apr 2017 11:34:22 -0400
"Ronald S. Bultje" <rsbultje@gmail.com> wrote:

> Hi,
> 
> On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> 
> > On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg@googlemail.com> wrote:  
> > > On Tue, 25 Apr 2017 23:52:04 +0700
> > > Muhammad Faiz <mfcc64@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@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?

That actually sounds good to me (although this issue is currently a
distraction to me so I never thought too deeply about it, not looked
too closely at the code).
Muhammad Faiz April 26, 2017, 6:20 p.m.
On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> Hi,
>
> On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>
>> On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg@googlemail.com> wrote:
>> > On Tue, 25 Apr 2017 23:52:04 +0700
>> > Muhammad Faiz <mfcc64@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@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.
Ronald S. Bultje April 26, 2017, 7:05 p.m.
Hi,

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

> On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje <rsbultje@gmail.com>
> wrote:
> > Hi,
> >
> > On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> >
> >> On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg@googlemail.com> wrote:
> >> > On Tue, 25 Apr 2017 23:52:04 +0700
> >> > Muhammad Faiz <mfcc64@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@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
wm4 April 26, 2017, 7:59 p.m.
On Thu, 27 Apr 2017 01:20:53 +0700
Muhammad Faiz <mfcc64@gmail.com> wrote:

> On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
> > Hi,
> >
> > On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> >  
> >> On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg@googlemail.com> wrote:  
> >> > On Tue, 25 Apr 2017 23:52:04 +0700
> >> > Muhammad Faiz <mfcc64@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@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.
Marton Balint April 26, 2017, 10:30 p.m.
On Wed, 26 Apr 2017, Ronald S. Bultje wrote:

> Hi,
>
> On Wed, Apr 26, 2017 at 2:20 PM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>
>> On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje <rsbultje@gmail.com>
>> wrote:
>> > Hi,
>> >
>> > On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>> >
>> >> On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg@googlemail.com> wrote:
>> >> > On Tue, 25 Apr 2017 23:52:04 +0700
>> >> > Muhammad Faiz <mfcc64@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@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.

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.

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

Thanks,
Marton
Muhammad Faiz April 27, 2017, 5:07 p.m.
On Thu, Apr 27, 2017 at 2:59 AM, wm4 <nfxjfg@googlemail.com> wrote:
> On Thu, 27 Apr 2017 01:20:53 +0700
> Muhammad Faiz <mfcc64@gmail.com> wrote:
>
>> On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
>> > Hi,
>> >
>> > On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>> >
>> >> On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg@googlemail.com> wrote:
>> >> > On Tue, 25 Apr 2017 23:52:04 +0700
>> >> > Muhammad Faiz <mfcc64@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@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.
Muhammad Faiz April 27, 2017, 5:16 p.m.
On Thu, Apr 27, 2017 at 5:30 AM, Marton Balint <cus@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@gmail.com> wrote:
>>
>>> On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje <rsbultje@gmail.com>
>>> wrote:
>>> > Hi,
>>> >
>>> > On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfcc64@gmail.com>
>>> > wrote:
>>> >
>>> >> On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg@googlemail.com> wrote:
>>> >> > On Tue, 25 Apr 2017 23:52:04 +0700
>>> >> > Muhammad Faiz <mfcc64@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@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? :)
Muhammad Faiz April 27, 2017, 6:33 p.m.
On Fri, Apr 28, 2017 at 12:16 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> On Thu, Apr 27, 2017 at 5:30 AM, Marton Balint <cus@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@gmail.com> wrote:
>>>
>>>> On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje <rsbultje@gmail.com>
>>>> wrote:
>>>> > Hi,
>>>> >
>>>> > On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfcc64@gmail.com>
>>>> > wrote:
>>>> >
>>>> >> On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfxjfg@googlemail.com> wrote:
>>>> >> > On Tue, 25 Apr 2017 23:52:04 +0700
>>>> >> > Muhammad Faiz <mfcc64@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@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.

Patch hide | download patch | download mbox

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;