| Message ID | 20170406153643.647-1-nfxjfg@googlemail.com |
|---|---|
| State | Accepted |
| Commit | 15a23a83326d70e470a6b5c8d71e55257bffd97b |
| Headers | show |
Hi, On Thu, Apr 6, 2017 at 11:36 AM, wm4 <nfxjfg@googlemail.com> wrote: > Consider the following sequence of events: > > - open a codec without AV_CODEC_CAP_DELAY > - decode call fails with an error > - ff_thread_flush() is called > - drain packet is sent > > Then the last step would make ff_thread_decode_frame() return an error, > because p->result can still be set to an error value. This is because > submit_packet returns immediately if AV_CODEC_CAP_DELAY is not set, and > no worker thread gets the chance to reset p->result, yet its value is > trusted by ff_thread_decode_frame(). > > Fix this by clearing the error fields on flush. > --- > libavcodec/pthread_frame.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > index 9a6b83ac45..7586f00bec 100644 > --- a/libavcodec/pthread_frame.c > +++ b/libavcodec/pthread_frame.c > @@ -859,6 +859,7 @@ void ff_thread_flush(AVCodecContext *avctx) > // Make sure decode flush calls with size=0 won't return old > frames > p->got_frame = 0; > av_frame_unref(p->frame); > + p->result = 0; > > release_delayed_buffers(p); > > -- > 2.11.0 Nice catch - I think that looks good. Ronald
On Thu, 6 Apr 2017 11:38:55 -0400 "Ronald S. Bultje" <rsbultje@gmail.com> wrote: > Hi, > > On Thu, Apr 6, 2017 at 11:36 AM, wm4 <nfxjfg@googlemail.com> wrote: > > > Consider the following sequence of events: > > > > - open a codec without AV_CODEC_CAP_DELAY > > - decode call fails with an error > > - ff_thread_flush() is called > > - drain packet is sent > > > > Then the last step would make ff_thread_decode_frame() return an error, > > because p->result can still be set to an error value. This is because > > submit_packet returns immediately if AV_CODEC_CAP_DELAY is not set, and > > no worker thread gets the chance to reset p->result, yet its value is > > trusted by ff_thread_decode_frame(). > > > > Fix this by clearing the error fields on flush. > > --- > > libavcodec/pthread_frame.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > > index 9a6b83ac45..7586f00bec 100644 > > --- a/libavcodec/pthread_frame.c > > +++ b/libavcodec/pthread_frame.c > > @@ -859,6 +859,7 @@ void ff_thread_flush(AVCodecContext *avctx) > > // Make sure decode flush calls with size=0 won't return old > > frames > > p->got_frame = 0; > > av_frame_unref(p->frame); > > + p->result = 0; > > > > release_delayed_buffers(p); > > > > -- > > 2.11.0 > > > Nice catch - I think that looks good. > > Ronald Pushed.
On Thu, 2017-04-06 at 18:18 +0200, wm4 wrote: > > > p->got_frame = 0; > > > av_frame_unref(p->frame); > > > + p->result = 0; Shouldn't p->result be similarly reset together with p->got_frame also in ff_thread_decode_frame()? A similar problem seems possible: - a normal decode call returns an error due to p->result being negative - drain packet is sent before cycling through all threads - the loop in ff_thread_decode_frame hits "if (p->result < 0)" Thus incorrectly returning the same error again from the drain packet.
On Thu, 06 Apr 2017 23:09:41 +0300 Uoti Urpala <uoti.urpala@pp1.inet.fi> wrote: > On Thu, 2017-04-06 at 18:18 +0200, wm4 wrote: > > > > p->got_frame = 0; > > > > av_frame_unref(p->frame); > > > > + p->result = 0; > > Shouldn't p->result be similarly reset together with p->got_frame also > in ff_thread_decode_frame()? A similar problem seems possible: > - a normal decode call returns an error due to p->result being negative > - drain packet is sent before cycling through all threads > - the loop in ff_thread_decode_frame hits "if (p->result < 0)" > Thus incorrectly returning the same error again from the drain packet. Please send a patch.
diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index 9a6b83ac45..7586f00bec 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -859,6 +859,7 @@ void ff_thread_flush(AVCodecContext *avctx) // Make sure decode flush calls with size=0 won't return old frames p->got_frame = 0; av_frame_unref(p->frame); + p->result = 0; release_delayed_buffers(p);