diff mbox series

[FFmpeg-devel,1/3] avcodec/libwebpenc_animencoder: Don't return pkt without data/side-data

Message ID HE1PR0301MB2154BD7D20080B72EF7FE0218F719@HE1PR0301MB2154.eurprd03.prod.outlook.com
State New
Headers show
Series [FFmpeg-devel,1/3] avcodec/libwebpenc_animencoder: Don't return pkt without data/side-data
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt April 11, 2021, 12:39 a.m. UTC
They are essentially forbidden in our encoding API as they are
considered empty. So just set the data, but leave the size at zero.

(The old encoding API allowed such packets: It used buffer_pkt_valid
to record whether the packet is empty or not. This has been changed
in 827d6fe73d2f5472c1c2128eb14fab6a4db29032 which broke said encoder.
Said regression has been reported in #9179 which this commit partially
fixes.)

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
 libavcodec/libwebpenc_animencoder.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

James Almer April 11, 2021, 1:15 a.m. UTC | #1
On 4/10/2021 9:39 PM, Andreas Rheinhardt wrote:
> They are essentially forbidden in our encoding API as they are
> considered empty. So just set the data, but leave the size at zero.

This doesn't seem like a good solution. You're propagating dummy packets 
when the encoder didn't produce any. It's an ugly hack to workaround a 
misbehaving muxer.
As this is a AV_CODEC_CAP_DELAY encoder, you're supposed to set 
got_packet to 0 in this scenario.

The muxer should then not expect this encoder to return more than one 
packet, which is all it ever truly produces, to assume it effectively 
came from it.

> 
> (The old encoding API allowed such packets: It used buffer_pkt_valid
> to record whether the packet is empty or not. This has been changed
> in 827d6fe73d2f5472c1c2128eb14fab6a4db29032 which broke said encoder.
> Said regression has been reported in #9179 which this commit partially
> fixes.)
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
>   libavcodec/libwebpenc_animencoder.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/libwebpenc_animencoder.c b/libavcodec/libwebpenc_animencoder.c
> index 7f35a0b939..d871e85d43 100644
> --- a/libavcodec/libwebpenc_animencoder.c
> +++ b/libavcodec/libwebpenc_animencoder.c
> @@ -34,6 +34,7 @@ typedef struct LibWebPAnimContext {
>       WebPAnimEncoder *enc;     // the main AnimEncoder object
>       int64_t prev_frame_pts;   // pts of the previously encoded frame.
>       int done;                 // If true, we have assembled the bitstream already
> +    uint8_t padding_buf[AV_INPUT_BUFFER_PADDING_SIZE];
>   } LibWebPAnimContext;
>   
>   static av_cold int libwebp_anim_encode_init(AVCodecContext *avctx)
> @@ -103,6 +104,8 @@ static int libwebp_anim_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>           }
>   
>           pkt->pts = pkt->dts = frame->pts;
> +        // Packets without data and side-data are not supported by the API
> +        pkt->data = s->padding_buf;
>           s->prev_frame_pts = frame->pts;  // Save for next frame.
>           ret = 0;
>           *got_packet = 1;
>
Andreas Rheinhardt April 11, 2021, 1:28 a.m. UTC | #2
James Almer:
> On 4/10/2021 9:39 PM, Andreas Rheinhardt wrote:
>> They are essentially forbidden in our encoding API as they are
>> considered empty. So just set the data, but leave the size at zero.
> 
> This doesn't seem like a good solution. You're propagating dummy packets
> when the encoder didn't produce any. It's an ugly hack to workaround a
> misbehaving muxer.
> As this is a AV_CODEC_CAP_DELAY encoder, you're supposed to set
> got_packet to 0 in this scenario.
> 
> The muxer should then not expect this encoder to return more than one
> packet, which is all it ever truly produces, to assume it effectively
> came from it.
> 

Then feel free to fix this yourself. All I know is that the muxer
explicitly checks for the number of frames to be > 1. Not that I know
why. (And I also presume the "Note: without the following, avio only
writes 8 bytes to the file" to be outdated.)

>>
>> (The old encoding API allowed such packets: It used buffer_pkt_valid
>> to record whether the packet is empty or not. This has been changed
>> in 827d6fe73d2f5472c1c2128eb14fab6a4db29032 which broke said encoder.
>> Said regression has been reported in #9179 which this commit partially
>> fixes.)
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>>   libavcodec/libwebpenc_animencoder.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/libavcodec/libwebpenc_animencoder.c
>> b/libavcodec/libwebpenc_animencoder.c
>> index 7f35a0b939..d871e85d43 100644
>> --- a/libavcodec/libwebpenc_animencoder.c
>> +++ b/libavcodec/libwebpenc_animencoder.c
>> @@ -34,6 +34,7 @@ typedef struct LibWebPAnimContext {
>>       WebPAnimEncoder *enc;     // the main AnimEncoder object
>>       int64_t prev_frame_pts;   // pts of the previously encoded frame.
>>       int done;                 // If true, we have assembled the
>> bitstream already
>> +    uint8_t padding_buf[AV_INPUT_BUFFER_PADDING_SIZE];
>>   } LibWebPAnimContext;
>>     static av_cold int libwebp_anim_encode_init(AVCodecContext *avctx)
>> @@ -103,6 +104,8 @@ static int
>> libwebp_anim_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>>           }
>>             pkt->pts = pkt->dts = frame->pts;
>> +        // Packets without data and side-data are not supported by
>> the API
>> +        pkt->data = s->padding_buf;
>>           s->prev_frame_pts = frame->pts;  // Save for next frame.
>>           ret = 0;
>>           *got_packet = 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".
James Almer April 11, 2021, 1:47 a.m. UTC | #3
On 4/10/2021 10:28 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 4/10/2021 9:39 PM, Andreas Rheinhardt wrote:
>>> They are essentially forbidden in our encoding API as they are
>>> considered empty. So just set the data, but leave the size at zero.
>>
>> This doesn't seem like a good solution. You're propagating dummy packets
>> when the encoder didn't produce any. It's an ugly hack to workaround a
>> misbehaving muxer.
>> As this is a AV_CODEC_CAP_DELAY encoder, you're supposed to set
>> got_packet to 0 in this scenario.
>>
>> The muxer should then not expect this encoder to return more than one
>> packet, which is all it ever truly produces, to assume it effectively
>> came from it.
>>
> 
> Then feel free to fix this yourself. All I know is that the muxer
> explicitly checks for the number of frames to be > 1. Not that I know
> why. (And I also presume the "Note: without the following, avio only
> writes 8 bytes to the file" to be outdated.)

I assume the muxer expected more than one packet because that's what the 
encoder produced, even if only the last one was legit. It nonetheless is 
a weird check, since for the purpose of writing the loop value it makes 
no difference whatsoever if the muxer got one or several packets, only 
that the is_animated_webp_packet() check succeeded, and that won't 
happen if a non animated webp image was fed.

> 
>>>
>>> (The old encoding API allowed such packets: It used buffer_pkt_valid
>>> to record whether the packet is empty or not. This has been changed
>>> in 827d6fe73d2f5472c1c2128eb14fab6a4db29032 which broke said encoder.
>>> Said regression has been reported in #9179 which this commit partially
>>> fixes.)
>>>
>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>> ---
>>>    libavcodec/libwebpenc_animencoder.c | 3 +++
>>>    1 file changed, 3 insertions(+)
>>>
>>> diff --git a/libavcodec/libwebpenc_animencoder.c
>>> b/libavcodec/libwebpenc_animencoder.c
>>> index 7f35a0b939..d871e85d43 100644
>>> --- a/libavcodec/libwebpenc_animencoder.c
>>> +++ b/libavcodec/libwebpenc_animencoder.c
>>> @@ -34,6 +34,7 @@ typedef struct LibWebPAnimContext {
>>>        WebPAnimEncoder *enc;     // the main AnimEncoder object
>>>        int64_t prev_frame_pts;   // pts of the previously encoded frame.
>>>        int done;                 // If true, we have assembled the
>>> bitstream already
>>> +    uint8_t padding_buf[AV_INPUT_BUFFER_PADDING_SIZE];
>>>    } LibWebPAnimContext;
>>>      static av_cold int libwebp_anim_encode_init(AVCodecContext *avctx)
>>> @@ -103,6 +104,8 @@ static int
>>> libwebp_anim_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
>>>            }
>>>              pkt->pts = pkt->dts = frame->pts;
>>> +        // Packets without data and side-data are not supported by
>>> the API
>>> +        pkt->data = s->padding_buf;
>>>            s->prev_frame_pts = frame->pts;  // Save for next frame.
>>>            ret = 0;
>>>            *got_packet = 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".
> 
> _______________________________________________
> 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/libwebpenc_animencoder.c b/libavcodec/libwebpenc_animencoder.c
index 7f35a0b939..d871e85d43 100644
--- a/libavcodec/libwebpenc_animencoder.c
+++ b/libavcodec/libwebpenc_animencoder.c
@@ -34,6 +34,7 @@  typedef struct LibWebPAnimContext {
     WebPAnimEncoder *enc;     // the main AnimEncoder object
     int64_t prev_frame_pts;   // pts of the previously encoded frame.
     int done;                 // If true, we have assembled the bitstream already
+    uint8_t padding_buf[AV_INPUT_BUFFER_PADDING_SIZE];
 } LibWebPAnimContext;
 
 static av_cold int libwebp_anim_encode_init(AVCodecContext *avctx)
@@ -103,6 +104,8 @@  static int libwebp_anim_encode_frame(AVCodecContext *avctx, AVPacket *pkt,
         }
 
         pkt->pts = pkt->dts = frame->pts;
+        // Packets without data and side-data are not supported by the API
+        pkt->data = s->padding_buf;
         s->prev_frame_pts = frame->pts;  // Save for next frame.
         ret = 0;
         *got_packet = 1;