| Message ID | 20170424185707.18647-1-nfxjfg@googlemail.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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?
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
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);