[FFmpeg-devel] [PATCH] avcodec/wrapped_avframe: stop using sizeof(AVFrame)

[Marton Balint]

On Sat, 13 Mar 2021, James Almer wrote:

> Signed-off-by: James Almer <jamrial at gmail.com>
> ---
> Setting pkt->size to 0 but leaving pkt->data pointing to the frame ensures that
> the packet isn't mistaken for an empty one, and prevents wrong use of the data
> pointer in case av_packet_make_writable() is called on it (The resulting packet
> will be the same as if you call it on an empty one).
>
> I decided to set the opaque field of the AVBufferRef to the frame pointer so
> we can do something like ensure pkt->size is 0 and pkt->data is equal to it
> before trying to treat pkt->data as an AVFrame, but i'm not sure if that could
> be done now, or only after a major bump (e.g. 4.3 lavf/lavd and 4.4 lavc at
> runtime, where demuxers like the vapoursynth one propagate packets to the
> wrapped_avframe decoder, and if the latter does the above check, it would
> fail).
>
> libavcodec/wrapped_avframe.c | 23 ++++-------------------
> 1 file changed, 4 insertions(+), 19 deletions(-)

Is using sizeof(AVFrame) an issue apart from that it is not part of ABI? 
Is there an actual issue this patch fix?

Isn't it a problem that the packet may not have correct padding now, 
becaue you allocate a 0 sized av_buffer? We had a bug because of this, and 
I fixed it in 436f00b10c062b75c7aab276c4a7d64524bd0444, which was caused 
by av_buffer_realloc(&avpkt->buf, avpkt->size + 
AV_INPUT_BUFFER_PADDING_SIZE) calls in the encode function removed in 
93016f5d1d280f9cb7856883af287fa66affc04c. More info in this thread:
http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-February/207329.html

I guess the fundamental problem of WRAPPED_AVFRAME is that deep copying it 
is not supported, but you don't exactly disallow that by using a size of 
0, because the deep copying (making it writable) will still return 
success, but the optimal thing would be if it would fail or correctly 
clone the AVFrame. Or am I missing something? Maybe we need something 
similar to AVFrame->hw_frames_ctx for AVPacket?

It is also debatable if lowering the packet size is a breaking change or 
not, because previously people could have had an assert in their code 
making sure it is not used with a library built with an AVFrame smaller 
than what they are using...

So unless we try to fix something specific with this patch maybe it is 
better to not touch it unless we have plans how to fix all known issues 
with wrapped avframes?

And also libavdevice/kmsgrab.c need some changes too if a new format is to 
be followed.

Regards,
Marton

>
> diff --git a/libavcodec/wrapped_avframe.c b/libavcodec/wrapped_avframe.c
> index 85ff32d13a..c921990024 100644
> --- a/libavcodec/wrapped_avframe.c
> +++ b/libavcodec/wrapped_avframe.c
> @@ -44,32 +44,20 @@ static int wrapped_avframe_encode(AVCodecContext *avctx, AVPacket *pkt,
>                      const AVFrame *frame, int *got_packet)
> {
>     AVFrame *wrapped = av_frame_clone(frame);
> -    uint8_t *data;
> -    int size = sizeof(*wrapped) + AV_INPUT_BUFFER_PADDING_SIZE;
>
>     if (!wrapped)
>         return AVERROR(ENOMEM);
> 
> -    data = av_mallocz(size);
> -    if (!data) {
> -        av_frame_free(&wrapped);
> -        return AVERROR(ENOMEM);
> -    }
> -
> -    pkt->buf = av_buffer_create(data, size,
> -                                wrapped_avframe_release_buffer, NULL,
> +    pkt->buf = av_buffer_create((uint8_t *)wrapped, 0,
> +                                wrapped_avframe_release_buffer, wrapped,
>                                 AV_BUFFER_FLAG_READONLY);
>     if (!pkt->buf) {
>         av_frame_free(&wrapped);
> -        av_freep(&data);
>         return AVERROR(ENOMEM);
>     }
> 
> -    av_frame_move_ref((AVFrame*)data, wrapped);
> -    av_frame_free(&wrapped);
> -
> -    pkt->data = data;
> -    pkt->size = sizeof(*wrapped);
> +    pkt->data = (uint8_t *)wrapped;
> +    pkt->size = 0;
>
>     pkt->flags |= AV_PKT_FLAG_KEY;
>     *got_packet = 1;
> @@ -87,9 +75,6 @@ static int wrapped_avframe_decode(AVCodecContext *avctx, void *data,
>         return AVERROR(EPERM);
>     }
> 
> -    if (pkt->size < sizeof(AVFrame))
> -        return AVERROR(EINVAL);
> -
>     in  = (AVFrame*)pkt->data;
>     out = data;
> 
> -- 
> 2.30.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".