[FFmpeg-devel] pthread_frame: ignore errors during draining

Submitted by wm4 on April 24, 2017, 6:57 p.m.

Details

Message ID 20170424185707.18647-1-nfxjfg@googlemail.com
State New
Headers show

Commit Message

wm4 April 24, 2017, 6:57 p.m.
This is needed to get compatibility with the behavior before the
recent decode.c restructuring merge, and fixes fate failures with
this:

  make fate-h264-attachment-631 THREADS=32

For every 4 threads, a frame is dropped at the point at which
draining is initialized. The problem starts at THREADS=4.

This patch "fixes" it by ignoring errors during draining (if there
is a frame to be returned), but there is probably a deeper cause
and/or a better fix for this. I haven't looked closer at this, but
was asked to send this patch for discussion.
---
 libavcodec/pthread_frame.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Muhammad Faiz April 25, 2017, 2:32 a.m.
On Tue, Apr 25, 2017 at 1:57 AM, wm4 <nfxjfg@googlemail.com> wrote:
> This is needed to get compatibility with the behavior before the
> recent decode.c restructuring merge, and fixes fate failures with
> this:
>
>   make fate-h264-attachment-631 THREADS=32
>
> For every 4 threads, a frame is dropped at the point at which
> draining is initialized. The problem starts at THREADS=4.
>
> This patch "fixes" it by ignoring errors during draining (if there
> is a frame to be returned), but there is probably a deeper cause
> and/or a better fix for this. I haven't looked closer at this, but
> was asked to send this patch for discussion.
> ---
>  libavcodec/pthread_frame.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> index 13d682842d..e7fa503adf 100644
> --- a/libavcodec/pthread_frame.c
> +++ b/libavcodec/pthread_frame.c
> @@ -548,7 +548,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
>      fctx->next_finished = finished;
>
>      /* return the size of the consumed packet if no error occurred */
> -    if (err >= 0)
> +    if (err >= 0 || (!avpkt->size && *got_picture_ptr))

Probably (*got_picture_ptr && p->result >= 0)
So error is not ignored.


>          err = avpkt->size;
>  finish:
>      async_lock(fctx);
> --
> 2.11.0
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
Hendrik Leppkes April 25, 2017, 9:09 a.m.
On Tue, Apr 25, 2017 at 4:32 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
> On Tue, Apr 25, 2017 at 1:57 AM, wm4 <nfxjfg@googlemail.com> wrote:
>> This is needed to get compatibility with the behavior before the
>> recent decode.c restructuring merge, and fixes fate failures with
>> this:
>>
>>   make fate-h264-attachment-631 THREADS=32
>>
>> For every 4 threads, a frame is dropped at the point at which
>> draining is initialized. The problem starts at THREADS=4.
>>
>> This patch "fixes" it by ignoring errors during draining (if there
>> is a frame to be returned), but there is probably a deeper cause
>> and/or a better fix for this. I haven't looked closer at this, but
>> was asked to send this patch for discussion.
>> ---
>>  libavcodec/pthread_frame.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
>> index 13d682842d..e7fa503adf 100644
>> --- a/libavcodec/pthread_frame.c
>> +++ b/libavcodec/pthread_frame.c
>> @@ -548,7 +548,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
>>      fctx->next_finished = finished;
>>
>>      /* return the size of the consumed packet if no error occurred */
>> -    if (err >= 0)
>> +    if (err >= 0 || (!avpkt->size && *got_picture_ptr))
>
> Probably (*got_picture_ptr && p->result >= 0)
> So error is not ignored.
>

Isnt the entire point of this to ignore errors during draining?

- Hendrik
Muhammad Faiz April 25, 2017, 10:29 a.m.
On Tue, Apr 25, 2017 at 4:09 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> On Tue, Apr 25, 2017 at 4:32 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>> On Tue, Apr 25, 2017 at 1:57 AM, wm4 <nfxjfg@googlemail.com> wrote:
>>> This is needed to get compatibility with the behavior before the
>>> recent decode.c restructuring merge, and fixes fate failures with
>>> this:
>>>
>>>   make fate-h264-attachment-631 THREADS=32
>>>
>>> For every 4 threads, a frame is dropped at the point at which
>>> draining is initialized. The problem starts at THREADS=4.
>>>
>>> This patch "fixes" it by ignoring errors during draining (if there
>>> is a frame to be returned), but there is probably a deeper cause
>>> and/or a better fix for this. I haven't looked closer at this, but
>>> was asked to send this patch for discussion.
>>> ---
>>>  libavcodec/pthread_frame.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
>>> index 13d682842d..e7fa503adf 100644
>>> --- a/libavcodec/pthread_frame.c
>>> +++ b/libavcodec/pthread_frame.c
>>> @@ -548,7 +548,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
>>>      fctx->next_finished = finished;
>>>
>>>      /* return the size of the consumed packet if no error occurred */
>>> -    if (err >= 0)
>>> +    if (err >= 0 || (!avpkt->size && *got_picture_ptr))
>>
>> Probably (*got_picture_ptr && p->result >= 0)
>> So error is not ignored.
>>
>
> Isnt the entire point of this to ignore errors during draining?

wm4 stated that probably there is a better fix, and not ignoring error
is a better fix.
My point is that when a frame is retuned, propagate error from the
thread that return it,
not from another thread. More clearly
if (*got_picture_ptr) err = p->result
wm4 April 25, 2017, 11:58 a.m.
On Tue, 25 Apr 2017 17:29:10 +0700
Muhammad Faiz <mfcc64@gmail.com> wrote:

> On Tue, Apr 25, 2017 at 4:09 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
> > On Tue, Apr 25, 2017 at 4:32 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:  
> >> On Tue, Apr 25, 2017 at 1:57 AM, wm4 <nfxjfg@googlemail.com> wrote:  
> >>> This is needed to get compatibility with the behavior before the
> >>> recent decode.c restructuring merge, and fixes fate failures with
> >>> this:
> >>>
> >>>   make fate-h264-attachment-631 THREADS=32
> >>>
> >>> For every 4 threads, a frame is dropped at the point at which
> >>> draining is initialized. The problem starts at THREADS=4.
> >>>
> >>> This patch "fixes" it by ignoring errors during draining (if there
> >>> is a frame to be returned), but there is probably a deeper cause
> >>> and/or a better fix for this. I haven't looked closer at this, but
> >>> was asked to send this patch for discussion.
> >>> ---
> >>>  libavcodec/pthread_frame.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
> >>> index 13d682842d..e7fa503adf 100644
> >>> --- a/libavcodec/pthread_frame.c
> >>> +++ b/libavcodec/pthread_frame.c
> >>> @@ -548,7 +548,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
> >>>      fctx->next_finished = finished;
> >>>
> >>>      /* return the size of the consumed packet if no error occurred */
> >>> -    if (err >= 0)
> >>> +    if (err >= 0 || (!avpkt->size && *got_picture_ptr))  
> >>
> >> Probably (*got_picture_ptr && p->result >= 0)
> >> So error is not ignored.
> >>  
> >
> > Isnt the entire point of this to ignore errors during draining?  
> 
> wm4 stated that probably there is a better fix, and not ignoring error
> is a better fix.
> My point is that when a frame is retuned, propagate error from the
> thread that return it,
> not from another thread. More clearly
> if (*got_picture_ptr) err = p->result

Can you send a patch for this?
Muhammad Faiz April 25, 2017, 4:52 p.m.
On Tue, Apr 25, 2017 at 6:58 PM, wm4 <nfxjfg@googlemail.com> wrote:
> On Tue, 25 Apr 2017 17:29:10 +0700
> Muhammad Faiz <mfcc64@gmail.com> wrote:
>
>> On Tue, Apr 25, 2017 at 4:09 PM, Hendrik Leppkes <h.leppkes@gmail.com> wrote:
>> > On Tue, Apr 25, 2017 at 4:32 AM, Muhammad Faiz <mfcc64@gmail.com> wrote:
>> >> On Tue, Apr 25, 2017 at 1:57 AM, wm4 <nfxjfg@googlemail.com> wrote:
>> >>> This is needed to get compatibility with the behavior before the
>> >>> recent decode.c restructuring merge, and fixes fate failures with
>> >>> this:
>> >>>
>> >>>   make fate-h264-attachment-631 THREADS=32
>> >>>
>> >>> For every 4 threads, a frame is dropped at the point at which
>> >>> draining is initialized. The problem starts at THREADS=4.
>> >>>
>> >>> This patch "fixes" it by ignoring errors during draining (if there
>> >>> is a frame to be returned), but there is probably a deeper cause
>> >>> and/or a better fix for this. I haven't looked closer at this, but
>> >>> was asked to send this patch for discussion.
>> >>> ---
>> >>>  libavcodec/pthread_frame.c | 2 +-
>> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>>
>> >>> diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
>> >>> index 13d682842d..e7fa503adf 100644
>> >>> --- a/libavcodec/pthread_frame.c
>> >>> +++ b/libavcodec/pthread_frame.c
>> >>> @@ -548,7 +548,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx,
>> >>>      fctx->next_finished = finished;
>> >>>
>> >>>      /* return the size of the consumed packet if no error occurred */
>> >>> -    if (err >= 0)
>> >>> +    if (err >= 0 || (!avpkt->size && *got_picture_ptr))
>> >>
>> >> Probably (*got_picture_ptr && p->result >= 0)
>> >> So error is not ignored.
>> >>
>> >
>> > Isnt the entire point of this to ignore errors during draining?
>>
>> wm4 stated that probably there is a better fix, and not ignoring error
>> is a better fix.
>> My point is that when a frame is retuned, propagate error from the
>> thread that return it,
>> not from another thread. More clearly
>> if (*got_picture_ptr) err = p->result
>
> Can you send a patch for this?

posted

thx

Patch hide | download patch | download mbox

diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c
index 13d682842d..e7fa503adf 100644
--- a/libavcodec/pthread_frame.c
+++ b/libavcodec/pthread_frame.c
@@ -548,7 +548,7 @@  int ff_thread_decode_frame(AVCodecContext *avctx,
     fctx->next_finished = finished;
 
     /* return the size of the consumed packet if no error occurred */
-    if (err >= 0)
+    if (err >= 0 || (!avpkt->size && *got_picture_ptr))
         err = avpkt->size;
 finish:
     async_lock(fctx);