On Fri, Apr 28, 2017 at 12:16 AM, Muhammad Faiz <mfc...@gmail.com> wrote: > On Thu, Apr 27, 2017 at 5:30 AM, Marton Balint <c...@passwd.hu> wrote: >> >> >> On Wed, 26 Apr 2017, Ronald S. Bultje wrote: >> >>> Hi, >>> >>> On Wed, Apr 26, 2017 at 2:20 PM, Muhammad Faiz <mfc...@gmail.com> wrote: >>> >>>> On Wed, Apr 26, 2017 at 10:34 PM, Ronald S. Bultje <rsbul...@gmail.com> >>>> wrote: >>>> > Hi, >>>> > >>>> > On Wed, Apr 26, 2017 at 1:21 AM, Muhammad Faiz <mfc...@gmail.com> >>>> > wrote: >>>> > >>>> >> On Wed, Apr 26, 2017 at 4:09 AM, wm4 <nfx...@googlemail.com> wrote: >>>> >> > On Tue, 25 Apr 2017 23:52:04 +0700 >>>> >> > Muhammad Faiz <mfc...@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 <mfc...@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. _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
