| Message ID | 20170327122817.10940-1-nfxjfg@googlemail.com |
|---|---|
| State | Accepted |
| Commit | 4cf1f68903cebcf6a6bede970f1b8f1509edf710 |
| Headers | show |
On Mon, 27 Mar 2017 14:28:17 +0200 wm4 <nfxjfg@googlemail.com> wrote: > Get rid of the "ret" variable, and always use err. Report the packet as > consumed if err is unset. This should be equivalent to the old code, > which obviously required err=0 for p->result>=0 (and otherwise, > p->result must have had the value err was last set to). The code block > added by commit 32a5b631267 is also not needed anymore, because the new > code strictly returns err if it's >=0. > --- > Not totally sure about this, but it seems to work out? I probably missed > something obvious. > --- > libavcodec/pthread_frame.c | 19 +++++-------------- > 1 file changed, 5 insertions(+), 14 deletions(-) > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > index b618be0bf5..36e4a8affa 100644 > --- a/libavcodec/pthread_frame.c > +++ b/libavcodec/pthread_frame.c > @@ -468,7 +468,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx, > FrameThreadContext *fctx = avctx->internal->thread_ctx; > int finished = fctx->next_finished; > PerThreadContext *p; > - int err, ret = 0; > + int err; > > /* release the async lock, permitting blocked hwaccel threads to > * go forward while we are in this function */ > @@ -496,7 +496,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx, > if (fctx->delaying) { > *got_picture_ptr=0; > if (avpkt->size) { > - ret = avpkt->size; > + err = avpkt->size; > goto finish; > } > } > @@ -542,21 +542,12 @@ int ff_thread_decode_frame(AVCodecContext *avctx, > > fctx->next_finished = finished; > > - /* > - * When no frame was found while flushing, but an error occurred in > - * any thread, return it instead of 0. > - * Otherwise the error can get lost. > - */ > - if (!avpkt->size && !*got_picture_ptr) > - goto finish; > - > /* return the size of the consumed packet if no error occurred */ > - ret = (p->result >= 0) ? avpkt->size : p->result; > + if (err >= 0) > + err = avpkt->size; > finish: > async_lock(fctx); > - if (err < 0) > - return err; > - return ret; > + return err; > } > > void ff_thread_report_progress(ThreadFrame *f, int n, int field) Received a review by BBB on IRC yesterday. Pushed.
Hi, I already saw this on -cvslog ml, so apparently I am too late... On 2017-03-29 12:19 +0200, wm4 wrote: > On Mon, 27 Mar 2017 14:28:17 +0200 > wm4 <nfxjfg@googlemail.com> wrote: > > > Get rid of the "ret" variable, and always use err. Report the packet as > > consumed if err is unset. This should be equivalent to the old code, > > which obviously required err=0 for p->result>=0 (and otherwise, > > p->result must have had the value err was last set to). The code block > > added by commit 32a5b631267 is also not needed anymore, because the new > > code strictly returns err if it's >=0. > > --- > > Not totally sure about this, but it seems to work out? I probably missed > > something obvious. > > --- > > libavcodec/pthread_frame.c | 19 +++++-------------- > > 1 file changed, 5 insertions(+), 14 deletions(-) > > > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > > index b618be0bf5..36e4a8affa 100644 > > --- a/libavcodec/pthread_frame.c > > +++ b/libavcodec/pthread_frame.c > > @@ -468,7 +468,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx, > > FrameThreadContext *fctx = avctx->internal->thread_ctx; > > int finished = fctx->next_finished; > > PerThreadContext *p; > > - int err, ret = 0; > > + int err; Wouldn't it have been more readable to keep ret instead of err? Assigning a size to err looks a bit strange. I am not insisting on changing it now, just wanted to point this out. > > > > /* release the async lock, permitting blocked hwaccel threads to > > * go forward while we are in this function */ > > @@ -496,7 +496,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx, > > if (fctx->delaying) { > > *got_picture_ptr=0; > > if (avpkt->size) { > > - ret = avpkt->size; > > + err = avpkt->size; > > goto finish; > > } > > } > > @@ -542,21 +542,12 @@ int ff_thread_decode_frame(AVCodecContext *avctx, > > > > fctx->next_finished = finished; > > > > - /* > > - * When no frame was found while flushing, but an error occurred in > > - * any thread, return it instead of 0. > > - * Otherwise the error can get lost. > > - */ > > - if (!avpkt->size && !*got_picture_ptr) > > - goto finish; > > - > > /* return the size of the consumed packet if no error occurred */ > > - ret = (p->result >= 0) ? avpkt->size : p->result; > > + if (err >= 0) > > + err = avpkt->size; > > finish: > > async_lock(fctx); > > - if (err < 0) > > - return err; > > - return ret; > > + return err; > > } > > > > void ff_thread_report_progress(ThreadFrame *f, int n, int field) > > Received a review by BBB on IRC yesterday. Pushed. It's kind of hard to follow these changes and the function in general, but after looking at the function and this patch a few times it seems alright and simpler. Alexander
On Wed, 29 Mar 2017 22:25:48 +0200 Alexander Strasser <eclipse7@gmx.net> wrote: > Hi, > > I already saw this on -cvslog ml, so apparently I am too late... > > On 2017-03-29 12:19 +0200, wm4 wrote: > > On Mon, 27 Mar 2017 14:28:17 +0200 > > wm4 <nfxjfg@googlemail.com> wrote: > > > > > Get rid of the "ret" variable, and always use err. Report the packet as > > > consumed if err is unset. This should be equivalent to the old code, > > > which obviously required err=0 for p->result>=0 (and otherwise, > > > p->result must have had the value err was last set to). The code block > > > added by commit 32a5b631267 is also not needed anymore, because the new > > > code strictly returns err if it's >=0. > > > --- > > > Not totally sure about this, but it seems to work out? I probably missed > > > something obvious. > > > --- > > > libavcodec/pthread_frame.c | 19 +++++-------------- > > > 1 file changed, 5 insertions(+), 14 deletions(-) > > > > > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > > > index b618be0bf5..36e4a8affa 100644 > > > --- a/libavcodec/pthread_frame.c > > > +++ b/libavcodec/pthread_frame.c > > > @@ -468,7 +468,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx, > > > FrameThreadContext *fctx = avctx->internal->thread_ctx; > > > int finished = fctx->next_finished; > > > PerThreadContext *p; > > > - int err, ret = 0; > > > + int err; > > Wouldn't it have been more readable to keep ret instead of err? It actually always used "err" - ret was introduced by a recent commit. err also seemed to be used more often in the function. > Assigning a size to err looks a bit strange. I am not insisting on changing > it now, just wanted to point this out. Actually I think the packet size could be removed, and the utils.c code adjusted.
On 2017-03-30 00:00 +0200, wm4 wrote: > On Wed, 29 Mar 2017 22:25:48 +0200 > Alexander Strasser <eclipse7@gmx.net> wrote: > > > On Mon, 27 Mar 2017 14:28:17 +0200 > > > wm4 <nfxjfg@googlemail.com> wrote: > > > > > > > Get rid of the "ret" variable, and always use err. Report the packet as > > > > consumed if err is unset. This should be equivalent to the old code, > > > > which obviously required err=0 for p->result>=0 (and otherwise, > > > > p->result must have had the value err was last set to). The code block > > > > added by commit 32a5b631267 is also not needed anymore, because the new > > > > code strictly returns err if it's >=0. > > > > --- > > > > Not totally sure about this, but it seems to work out? I probably missed > > > > something obvious. > > > > --- > > > > libavcodec/pthread_frame.c | 19 +++++-------------- > > > > 1 file changed, 5 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c > > > > index b618be0bf5..36e4a8affa 100644 > > > > --- a/libavcodec/pthread_frame.c > > > > +++ b/libavcodec/pthread_frame.c > > > > @@ -468,7 +468,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx, > > > > FrameThreadContext *fctx = avctx->internal->thread_ctx; > > > > int finished = fctx->next_finished; > > > > PerThreadContext *p; > > > > - int err, ret = 0; > > > > + int err; > > > > Wouldn't it have been more readable to keep ret instead of err? > > It actually always used "err" - ret was introduced by a recent commit. > err also seemed to be used more often in the function. Right, I had found that when looking at the history of the file. But "err" was not used to hold a size at that time. > > Assigning a size to err looks a bit strange. I am not insisting on changing > > it now, just wanted to point this out. > > Actually I think the packet size could be removed, and the utils.c code > adjusted. Sorry, I cannot judge that from what I know. Alexander
diff --git a/libavcodec/pthread_frame.c b/libavcodec/pthread_frame.c index b618be0bf5..36e4a8affa 100644 --- a/libavcodec/pthread_frame.c +++ b/libavcodec/pthread_frame.c @@ -468,7 +468,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx, FrameThreadContext *fctx = avctx->internal->thread_ctx; int finished = fctx->next_finished; PerThreadContext *p; - int err, ret = 0; + int err; /* release the async lock, permitting blocked hwaccel threads to * go forward while we are in this function */ @@ -496,7 +496,7 @@ int ff_thread_decode_frame(AVCodecContext *avctx, if (fctx->delaying) { *got_picture_ptr=0; if (avpkt->size) { - ret = avpkt->size; + err = avpkt->size; goto finish; } } @@ -542,21 +542,12 @@ int ff_thread_decode_frame(AVCodecContext *avctx, fctx->next_finished = finished; - /* - * When no frame was found while flushing, but an error occurred in - * any thread, return it instead of 0. - * Otherwise the error can get lost. - */ - if (!avpkt->size && !*got_picture_ptr) - goto finish; - /* return the size of the consumed packet if no error occurred */ - ret = (p->result >= 0) ? avpkt->size : p->result; + if (err >= 0) + err = avpkt->size; finish: async_lock(fctx); - if (err < 0) - return err; - return ret; + return err; } void ff_thread_report_progress(ThreadFrame *f, int n, int field)