diff mbox series

[FFmpeg-devel] avcodec/decode: Return EAGAIN instead of overwriting unused packet

Message ID GV1P250MB0737CBAF57E68F4DD8CE0F9F8F33A@GV1P250MB0737.EURP250.PROD.OUTLOOK.COM
State Accepted
Headers show
Series [FFmpeg-devel] avcodec/decode: Return EAGAIN instead of overwriting unused packet | expand

Checks

Context Check Description
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

Commit Message

Andreas Rheinhardt July 9, 2023, 11:36 p.m. UTC 
Should fix #10457, a regression caused by
69516ab3e917a6e91d26e38d04183c60fd71cbab.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
I am not sure about this one. The problem is that avcodec_send_packet()
and avcodec_receive_frame() must not return EAGAIN at the same time.
If buffer_frame contains a frame when entering avcodec_send_packet(),
the next call to avcodec_receive_frame() will not return EAGAIN.
But is this actually guaranteed if buffer_pkt is not empty?

 libavcodec/decode.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Anton Khirnov July 10, 2023, 12:47 p.m. UTC | #1 
Quoting Andreas Rheinhardt (2023-07-10 01:36:41)
> Should fix #10457, a regression caused by
> 69516ab3e917a6e91d26e38d04183c60fd71cbab.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> I am not sure about this one. The problem is that avcodec_send_packet()
> and avcodec_receive_frame() must not return EAGAIN at the same time.
> If buffer_frame contains a frame when entering avcodec_send_packet(),
> the next call to avcodec_receive_frame() will not return EAGAIN.
> But is this actually guaranteed if buffer_pkt is not empty?

It's not guaranteed, but I believe the correct interpretation of the
"must not return EAGAIN at the same time" line is that
send->receive->send must make progress and not be 3x EAGAIN.


> 
>  libavcodec/decode.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
> index 269633ce10..6595c3ca34 100644
> --- a/libavcodec/decode.c
> +++ b/libavcodec/decode.c
> @@ -666,6 +666,9 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
>      if (avpkt && !avpkt->size && avpkt->data)
>          return AVERROR(EINVAL);
>  
> +    if (!AVPACKET_IS_EMPTY(avci->buffer_pkt))
> +        return AVERROR(EAGAIN);
> +
>      av_packet_unref(avci->buffer_pkt);

This unref here becomes redundant.

Otherwise looks good, thanks.
Andreas Rheinhardt July 10, 2023, 2:25 p.m. UTC | #2 
Anton Khirnov:
> Quoting Andreas Rheinhardt (2023-07-10 01:36:41)
>> Should fix #10457, a regression caused by
>> 69516ab3e917a6e91d26e38d04183c60fd71cbab.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> I am not sure about this one. The problem is that avcodec_send_packet()
>> and avcodec_receive_frame() must not return EAGAIN at the same time.
>> If buffer_frame contains a frame when entering avcodec_send_packet(),
>> the next call to avcodec_receive_frame() will not return EAGAIN.
>> But is this actually guaranteed if buffer_pkt is not empty?
> 
> It's not guaranteed, but I believe the correct interpretation of the
> "must not return EAGAIN at the same time" line is that
> send->receive->send must make progress and not be 3x EAGAIN.
> 
> 
>>
>>  libavcodec/decode.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>> index 269633ce10..6595c3ca34 100644
>> --- a/libavcodec/decode.c
>> +++ b/libavcodec/decode.c
>> @@ -666,6 +666,9 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
>>      if (avpkt && !avpkt->size && avpkt->data)
>>          return AVERROR(EINVAL);
>>  
>> +    if (!AVPACKET_IS_EMPTY(avci->buffer_pkt))
>> +        return AVERROR(EAGAIN);
>> +
>>      av_packet_unref(avci->buffer_pkt);
> 
> This unref here becomes redundant.
> 
> Otherwise looks good, thanks.
> 

Will apply with the unref removed.

- Andreas
Marton Balint July 10, 2023, 7:05 p.m. UTC | #3 
On Mon, 10 Jul 2023, Anton Khirnov wrote:

> Quoting Andreas Rheinhardt (2023-07-10 01:36:41)
>> Should fix #10457, a regression caused by
>> 69516ab3e917a6e91d26e38d04183c60fd71cbab.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> I am not sure about this one. The problem is that avcodec_send_packet()
>> and avcodec_receive_frame() must not return EAGAIN at the same time.
>> If buffer_frame contains a frame when entering avcodec_send_packet(),
>> the next call to avcodec_receive_frame() will not return EAGAIN.
>> But is this actually guaranteed if buffer_pkt is not empty?
>
> It's not guaranteed, but I believe the correct interpretation of the
> "must not return EAGAIN at the same time" line is that
> send->receive->send must make progress and not be 3x EAGAIN.

I don't think that 2x EAGAIN is allowed, the docs says:

... the only guarantee is that an AVERROR(EAGAIN) return value on a 
send/receive call on one end implies that a receive/send call on the other 
end will succeed, or at least will not fail with AVERROR(EAGAIN). In 
general, no codec will permit unlimited buffering of input or output.

Regards,
Marton


>
>
>>
>>  libavcodec/decode.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/libavcodec/decode.c b/libavcodec/decode.c
>> index 269633ce10..6595c3ca34 100644
>> --- a/libavcodec/decode.c
>> +++ b/libavcodec/decode.c
>> @@ -666,6 +666,9 @@ int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
>>      if (avpkt && !avpkt->size && avpkt->data)
>>          return AVERROR(EINVAL);
>>
>> +    if (!AVPACKET_IS_EMPTY(avci->buffer_pkt))
>> +        return AVERROR(EAGAIN);
>> +
>>      av_packet_unref(avci->buffer_pkt);
>
> This unref here becomes redundant.
>
> Otherwise looks good, thanks.
>
> -- 
> Anton Khirnov
> _______________________________________________
> 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".
>
diff mbox series

Patch

diff --git a/libavcodec/decode.c b/libavcodec/decode.c
index 269633ce10..6595c3ca34 100644
--- a/libavcodec/decode.c
+++ b/libavcodec/decode.c
@@ -666,6 +666,9 @@  int attribute_align_arg avcodec_send_packet(AVCodecContext *avctx, const AVPacke
     if (avpkt && !avpkt->size && avpkt->data)
         return AVERROR(EINVAL);
 
+    if (!AVPACKET_IS_EMPTY(avci->buffer_pkt))
+        return AVERROR(EAGAIN);
+
     av_packet_unref(avci->buffer_pkt);
     if (avpkt && (avpkt->data || avpkt->side_data_elems)) {
         ret = av_packet_ref(avci->buffer_pkt, avpkt);