[FFmpeg-devel] avcodec/audiotoolboxenc: return EAGAIN if frame_queue.available is 0 and not at->eof

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(-)

> 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