| Message ID | 20170415183307.1804-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | c037f2f1ba3a2d3114575323550f456e66695edf |
| Headers | show |
On Sat, Apr 15, 2017 at 08:33:07PM +0200, Marton Balint wrote: > Fixes Coverity CID 1404841. > > Signed-off-by: Marton Balint <cus@passwd.hu> > --- > ffmpeg.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/ffmpeg.c b/ffmpeg.c > index e4b94b2..dd179b0 100644 > --- a/ffmpeg.c > +++ b/ffmpeg.c > @@ -1904,8 +1904,6 @@ static void flush_encoders(void) > if (enc->codec_type != AVMEDIA_TYPE_VIDEO && enc->codec_type != AVMEDIA_TYPE_AUDIO) > continue; > > - avcodec_send_frame(enc, NULL); > - > for (;;) { > const char *desc = NULL; > AVPacket pkt; > @@ -1927,7 +1925,17 @@ static void flush_encoders(void) > pkt.size = 0; > > update_benchmark(NULL); > - ret = avcodec_receive_packet(enc, &pkt); > + > + while ((ret = avcodec_receive_packet(enc, &pkt)) == AVERROR(EAGAIN)) { > + ret = avcodec_send_frame(enc, NULL); > + if (ret < 0) { > + av_log(NULL, AV_LOG_FATAL, "%s encoding failed: %s\n", > + desc, > + av_err2str(ret)); > + exit_program(1); > + } > + } can the code be changed to not require this ? If so this should make its use easier [...]
Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit : > > + while ((ret = avcodec_receive_packet(enc, &pkt)) == AVERROR(EAGAIN)) { > > + ret = avcodec_send_frame(enc, NULL); The doc says: # The functions will not return AVERROR(EAGAIN), unless you forgot to # enter draining mode. > can the code be changed to not require this ? I would say the code does not require this as is. An assert to verify the invariant provided by the doc would probably be a good idea, though. Regards,
On Tue, Apr 18, 2017 at 07:09:30AM +0200, Nicolas George wrote: > Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit : > > > + while ((ret = avcodec_receive_packet(enc, &pkt)) == AVERROR(EAGAIN)) { > > > + ret = avcodec_send_frame(enc, NULL); > > The doc says: > > # The functions will not return AVERROR(EAGAIN), unless you forgot to > # enter draining mode. The full paragraph in the docs which you qoted from says this: * - Call avcodec_receive_frame() (decoding) or avcodec_receive_packet() * (encoding) in a loop until AVERROR_EOF is returned. The functions will * not return AVERROR(EAGAIN), unless you forgot to enter draining mode. the patch adds a check to avcodec_send_frame() > > > can the code be changed to not require this ? > > I would say the code does not require this as is. For decoding theres an explicit "Sending the first flush packet will return success." I cannot find similar for encoding, which is the case the patch changes and what i think should be fixed if possible as it would be simpler, making the patch unneeded. Its quite possible iam missing something that makes it uneeded though [...]
On Tue, 18 Apr 2017, Michael Niedermayer wrote: > On Tue, Apr 18, 2017 at 07:09:30AM +0200, Nicolas George wrote: >> Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit : >>>> + while ((ret = avcodec_receive_packet(enc, &pkt)) == AVERROR(EAGAIN)) { >>>> + ret = avcodec_send_frame(enc, NULL); >> >> The doc says: >> >> # The functions will not return AVERROR(EAGAIN), unless you forgot to >> # enter draining mode. > > The full paragraph in the docs which you qoted from says this: > * - Call avcodec_receive_frame() (decoding) or avcodec_receive_packet() > * (encoding) in a loop until AVERROR_EOF is returned. The functions will > * not return AVERROR(EAGAIN), unless you forgot to enter draining mode. > > the patch adds a check to avcodec_send_frame() > > >> >>> can the code be changed to not require this ? >> >> I would say the code does not require this as is. > > For decoding theres an explicit > "Sending the first flush packet will return success." As far as I see this sentence is only true if there was no decoding error in the legacy API during a flush. So this should probably be changed to something like "The first flush packet will not return AVERROR_EOF, if it returns success then any subsequent flush packets will return AVERROR_EOF." By the way we also guarantee this at libavcodec level, so it is harder to write a codec with the new API which violates this. > I cannot find similar for encoding, which is the case the patch changes > and what i think should be fixed if possible as it would be simpler, > making the patch unneeded. > Its quite possible iam missing something that makes it uneeded though The same is true for send_frame, based on the code involving the legacy API. We can of course decide to change the code involving the legacy API and enforce that flushing always succeed, but I'd rather keep it as is, even if that means a bit more error checking. It would be ugly API-wise that you sometimes have to check the return value of a function, sometimes you don't. Regards, Marton
On Tue, Apr 18, 2017 at 08:46:24PM +0200, Marton Balint wrote: > > > On Tue, 18 Apr 2017, Michael Niedermayer wrote: > > >On Tue, Apr 18, 2017 at 07:09:30AM +0200, Nicolas George wrote: > >>Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit : > >>>>+ while ((ret = avcodec_receive_packet(enc, &pkt)) == AVERROR(EAGAIN)) { > >>>>+ ret = avcodec_send_frame(enc, NULL); > >> > >>The doc says: > >> > >># The functions will not return AVERROR(EAGAIN), unless you forgot to > >># enter draining mode. > > > >The full paragraph in the docs which you qoted from says this: > >* - Call avcodec_receive_frame() (decoding) or avcodec_receive_packet() > >* (encoding) in a loop until AVERROR_EOF is returned. The functions will > >* not return AVERROR(EAGAIN), unless you forgot to enter draining mode. > > > >the patch adds a check to avcodec_send_frame() > > > > > >> > >>>can the code be changed to not require this ? > >> > >>I would say the code does not require this as is. > > > >For decoding theres an explicit > >"Sending the first flush packet will return success." > > As far as I see this sentence is only true if there was no decoding > error in the legacy API during a flush. So this should probably be > changed to something like "The first flush packet will not return > AVERROR_EOF, if it returns success then any subsequent flush packets > will return AVERROR_EOF." By the way we also guarantee this at > libavcodec level, so it is harder to write a codec with the new API > which violates this. > > >I cannot find similar for encoding, which is the case the patch changes > >and what i think should be fixed if possible as it would be simpler, > >making the patch unneeded. > >Its quite possible iam missing something that makes it uneeded though > > The same is true for send_frame, based on the code involving the > legacy API. > > We can of course decide to change the code involving the legacy API > and enforce that flushing always succeed, but I'd rather keep it as > is, even if that means a bit more error checking. It would be ugly > API-wise that you sometimes have to check the return value of a > function, sometimes you don't. iam happy with any solution or the patch as is [...]
On Tue, 18 Apr 2017, Michael Niedermayer wrote: > On Tue, Apr 18, 2017 at 08:46:24PM +0200, Marton Balint wrote: >> >> >> On Tue, 18 Apr 2017, Michael Niedermayer wrote: >> >>> On Tue, Apr 18, 2017 at 07:09:30AM +0200, Nicolas George wrote: >>>> Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit : >>>>>> + while ((ret = avcodec_receive_packet(enc, &pkt)) == AVERROR(EAGAIN)) { >>>>>> + ret = avcodec_send_frame(enc, NULL); >>>> >>>> The doc says: >>>> >>>> # The functions will not return AVERROR(EAGAIN), unless you forgot to >>>> # enter draining mode. >>> >>> The full paragraph in the docs which you qoted from says this: >>> * - Call avcodec_receive_frame() (decoding) or avcodec_receive_packet() >>> * (encoding) in a loop until AVERROR_EOF is returned. The functions will >>> * not return AVERROR(EAGAIN), unless you forgot to enter draining mode. >>> >>> the patch adds a check to avcodec_send_frame() >>> >>> >>>> >>>>> can the code be changed to not require this ? >>>> >>>> I would say the code does not require this as is. >>> >>> For decoding theres an explicit >>> "Sending the first flush packet will return success." >> >> As far as I see this sentence is only true if there was no decoding >> error in the legacy API during a flush. So this should probably be >> changed to something like "The first flush packet will not return >> AVERROR_EOF, if it returns success then any subsequent flush packets >> will return AVERROR_EOF." By the way we also guarantee this at >> libavcodec level, so it is harder to write a codec with the new API >> which violates this. >> >>> I cannot find similar for encoding, which is the case the patch changes >>> and what i think should be fixed if possible as it would be simpler, >>> making the patch unneeded. >>> Its quite possible iam missing something that makes it uneeded though >> >> The same is true for send_frame, based on the code involving the >> legacy API. >> >> We can of course decide to change the code involving the legacy API >> and enforce that flushing always succeed, but I'd rather keep it as >> is, even if that means a bit more error checking. It would be ugly >> API-wise that you sometimes have to check the return value of a >> function, sometimes you don't. > > iam happy with any solution or the patch as is > Unless there are no further comments I will apply this as is and backport it to 3.3 as well. Regards, Marton
On Wed, 19 Apr 2017, Marton Balint wrote: > > > On Tue, 18 Apr 2017, Michael Niedermayer wrote: > >> On Tue, Apr 18, 2017 at 08:46:24PM +0200, Marton Balint wrote: >>> >>> >>> On Tue, 18 Apr 2017, Michael Niedermayer wrote: >>> >>>> On Tue, Apr 18, 2017 at 07:09:30AM +0200, Nicolas George wrote: >>>>> Le nonidi 29 germinal, an CCXXV, Michael Niedermayer a écrit : >>>>>>> + while ((ret = avcodec_receive_packet(enc, &pkt)) == > AVERROR(EAGAIN)) { >>>>>>> + ret = avcodec_send_frame(enc, NULL); >>>>> >>>>> The doc says: >>>>> >>>>> # The functions will not return AVERROR(EAGAIN), unless you forgot to >>>>> # enter draining mode. >>>> >>>> The full paragraph in the docs which you qoted from says this: >>>> * - Call avcodec_receive_frame() (decoding) or avcodec_receive_packet() >>>> * (encoding) in a loop until AVERROR_EOF is returned. The functions > will >>>> * not return AVERROR(EAGAIN), unless you forgot to enter draining mode. >>>> >>>> the patch adds a check to avcodec_send_frame() >>>> >>>> >>>>> >>>>>> can the code be changed to not require this ? >>>>> >>>>> I would say the code does not require this as is. >>>> >>>> For decoding theres an explicit >>>> "Sending the first flush packet will return success." >>> >>> As far as I see this sentence is only true if there was no decoding >>> error in the legacy API during a flush. So this should probably be >>> changed to something like "The first flush packet will not return >>> AVERROR_EOF, if it returns success then any subsequent flush packets >>> will return AVERROR_EOF." By the way we also guarantee this at >>> libavcodec level, so it is harder to write a codec with the new API >>> which violates this. >>> >>>> I cannot find similar for encoding, which is the case the patch changes >>>> and what i think should be fixed if possible as it would be simpler, >>>> making the patch unneeded. >>>> Its quite possible iam missing something that makes it uneeded though >>> >>> The same is true for send_frame, based on the code involving the >>> legacy API. >>> >>> We can of course decide to change the code involving the legacy API >>> and enforce that flushing always succeed, but I'd rather keep it as >>> is, even if that means a bit more error checking. It would be ugly >>> API-wise that you sometimes have to check the return value of a >>> function, sometimes you don't. >> >> iam happy with any solution or the patch as is >> > > Unless there are no further comments I will apply this as is and backport > it to 3.3 as well. Pushed. Regards, Marton
diff --git a/ffmpeg.c b/ffmpeg.c index e4b94b2..dd179b0 100644 --- a/ffmpeg.c +++ b/ffmpeg.c @@ -1904,8 +1904,6 @@ static void flush_encoders(void) if (enc->codec_type != AVMEDIA_TYPE_VIDEO && enc->codec_type != AVMEDIA_TYPE_AUDIO) continue; - avcodec_send_frame(enc, NULL); - for (;;) { const char *desc = NULL; AVPacket pkt; @@ -1927,7 +1925,17 @@ static void flush_encoders(void) pkt.size = 0; update_benchmark(NULL); - ret = avcodec_receive_packet(enc, &pkt); + + while ((ret = avcodec_receive_packet(enc, &pkt)) == AVERROR(EAGAIN)) { + ret = avcodec_send_frame(enc, NULL); + if (ret < 0) { + av_log(NULL, AV_LOG_FATAL, "%s encoding failed: %s\n", + desc, + av_err2str(ret)); + exit_program(1); + } + } + update_benchmark("flush_%s %d.%d", desc, ost->file_index, ost->index); if (ret < 0 && ret != AVERROR_EOF) { av_log(NULL, AV_LOG_FATAL, "%s encoding failed: %s\n",
Fixes Coverity CID 1404841. Signed-off-by: Marton Balint <cus@passwd.hu> --- ffmpeg.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)