Message ID | 20220628144236.2155-1-lq@chinaffmpeg.org |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel] avcodec/audiotoolboxenc: return EAGAIN if frame_queue.available is 0 and not at->eof | expand |
Context | Check | Description |
---|---|---|
yinshiyou/commit_msg_loongarch64 | warning | Please wrap lines in the body of the commit message between 60 and 72 characters. |
andriy/commit_msg_x86 | warning | Please wrap lines in the body of the commit message between 60 and 72 characters. |
yinshiyou/make_loongarch64 | success | Make finished |
yinshiyou/make_fate_loongarch64 | success | Make fate finished |
andriy/make_x86 | success | Make finished |
andriy/make_fate_x86 | success | Make fate finished |
andriy/commit_msg_armv7_RPi4 | warning | Please wrap lines in the body of the commit message between 60 and 72 characters. |
andriy/make_armv7_RPi4 | success | Make finished |
andriy/make_fate_armv7_RPi4 | success | Make fate finished |
> On Jun 28, 2022, at 10:42 PM, Steven Liu <lq@chinaffmpeg.org> wrote: > > There will return success and failure after commit 7c05b7951cb47716230c95744240bc60ec5f9433. > But the AudioConverterFillComplexBuffer will return 1 and *got_packet_ptr is 0 > when frame_queue.available == 0 and at->eof == 0. So should return EAGAIN here, > this because the encode function should return either 0 or negative error code. > > Signed-off-by: Steven Liu <lq@chinaffmpeg.org> > --- > libavcodec/audiotoolboxenc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c > index 00293154bf..c23deb06a9 100644 > --- a/libavcodec/audiotoolboxenc.c > +++ b/libavcodec/audiotoolboxenc.c > @@ -558,7 +558,8 @@ static int ffat_encode(AVCodecContext *avctx, AVPacket *avpkt, > } else if (ret && ret != 1) { > av_log(avctx, AV_LOG_ERROR, "Encode error: %i\n", ret); > ret = AVERROR_EXTERNAL; > - } > + } else if (ret == 1) > + ret = AVERROR(EAGAIN); How about keep the old behavior before 7c05b7951, and return early in the error path? diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c index 00293154bf..8bbaabd960 100644 --- a/libavcodec/audiotoolboxenc.c +++ b/libavcodec/audiotoolboxenc.c @@ -554,13 +554,12 @@ static int ffat_encode(AVCodecContext *avctx, AVPacket *avpkt, avctx->frame_size, &avpkt->pts, &avpkt->duration); - ret = 0; } else if (ret && ret != 1) { av_log(avctx, AV_LOG_ERROR, "Encode error: %i\n", ret); - ret = AVERROR_EXTERNAL; + return AVERROR_EXTERNAL; } - return ret; + return 0; } static av_cold void ffat_encode_flush(AVCodecContext *avctx) > > return ret; > } > -- > 2.34.1 > > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
On Tue, Jun 28, 2022 at 5:43 PM Steven Liu <lq@chinaffmpeg.org> wrote: > > There will return success and failure after commit 7c05b7951cb47716230c95744240bc60ec5f9433. > But the AudioConverterFillComplexBuffer will return 1 and *got_packet_ptr is 0 > when frame_queue.available == 0 and at->eof == 0. So should return EAGAIN here, > this because the encode function should return either 0 or negative error code. > This mirrors what I noted to you on IRC. Can you document what ret == 1 actually is? AudioConverterFillComplexBuffer documentation says it returns OSStatus, but with a quick look I can't see any AudioToolbox related errors in f.ex. https://www.osstatus.com/search/results?platform=all&framework=all&search=1 . If it is just equivalent of EAGAIN, then since there is no need to check for got_packet_ptr ? In any case, with errors or special cases early exit should always be preferred. See an example @ https://github.com/jeeb/ffmpeg/commits/audiotoolboxenc_return_codes . (this was originally quickly whipped up before I saw your patch, and thus does not take care of the EAGAIN case). Jan
> 在 2022年6月28日,23:22,Jan Ekström <jeebjp@gmail.com> 写道: > > On Tue, Jun 28, 2022 at 5:43 PM Steven Liu <lq@chinaffmpeg.org> wrote: >> >> There will return success and failure after commit 7c05b7951cb47716230c95744240bc60ec5f9433. >> But the AudioConverterFillComplexBuffer will return 1 and *got_packet_ptr is 0 >> when frame_queue.available == 0 and at->eof == 0. So should return EAGAIN here, >> this because the encode function should return either 0 or negative error code. >> > Hi jeeb, > This mirrors what I noted to you on IRC. > > Can you document what ret == 1 actually is? > AudioConverterFillComplexBuffer documentation says it returns > OSStatus, but with a quick look I can't see any AudioToolbox related > errors in f.ex. > https://www.osstatus.com/search/results?platform=all&framework=all&search=1 > . The return value comes from the callback ffat_encode_callback, > > If it is just equivalent of EAGAIN, then since there is no need to > check for got_packet_ptr ? > > In any case, with errors or special cases early exit should always be > preferred. See an example @ > https://github.com/jeeb/ffmpeg/commits/audiotoolboxenc_return_codes . > (this was originally quickly whipped up before I saw your patch, and > thus does not take care of the EAGAIN case). I think the solution from quink should be ok "just return error code immediately when the failure of old way." What about your opinion to the quinks suggestion? > > Jan > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". > Thanks Steven
diff --git a/libavcodec/audiotoolboxenc.c b/libavcodec/audiotoolboxenc.c index 00293154bf..c23deb06a9 100644 --- a/libavcodec/audiotoolboxenc.c +++ b/libavcodec/audiotoolboxenc.c @@ -558,7 +558,8 @@ static int ffat_encode(AVCodecContext *avctx, AVPacket *avpkt, } else if (ret && ret != 1) { av_log(avctx, AV_LOG_ERROR, "Encode error: %i\n", ret); ret = AVERROR_EXTERNAL; - } + } else if (ret == 1) + ret = AVERROR(EAGAIN); return ret; }
There will return success and failure after commit 7c05b7951cb47716230c95744240bc60ec5f9433. But the AudioConverterFillComplexBuffer will return 1 and *got_packet_ptr is 0 when frame_queue.available == 0 and at->eof == 0. So should return EAGAIN here, this because the encode function should return either 0 or negative error code. Signed-off-by: Steven Liu <lq@chinaffmpeg.org> --- libavcodec/audiotoolboxenc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)