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

[James Almer]

On 3/14/2021 3:25 PM, Marton Balint wrote:
> 
> 
> On Sun, 14 Mar 2021, James Almer wrote:
> 
>>> Is using sizeof(AVFrame) an issue apart from that it is not part of 
>>> ABI? Is there an actual issue this patch fix?
>>
>> It doesn't fix a current issue, it ensures an issue doesn't arise in 
>> the future when a new field is added to AVFrame and it's lost when 
>> propagated with wrapped_avframe in an scenario where you use new 
>> libraries at runtime with software that linked to old libraries (See, 
>> every distro updating ffmpeg within a given soname).
> 
> Is this also an issue if libav* libraries are upgraded at the same time? 
> Because I thought that in that case we are good.

Now that you mention it, I think it would only be an issue if you 
upgrade lavu only, or lavc+lavu only. If you upgrade all libraries, it 
would depend on if the user also screwed up and used sizeof(AVFrame) on 
their application as a bad method to ensure the packet really contains 
the wrapped frame, which you suggested was an unfortunate possibility.

> 
> [...]
> 
>>> 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?
>>
>> If you do av_packet_make_writable(), there will be no attempt at 
>> copying data because size is 0. The resulting packet, like i 
>> mentioned, will be the same as calling that function on a freshly 
>> allocated/unref'd packet.
> 
> But why is that an improvement? The packet made writable will still not 
> be usable as a WRAPPED_AVFRAME packet, because that data pointer will 
> point to a newly allocated AV_INPUT_BUFFER_PADDING_SIZE-d memory area, 
> instead of an AVFrame. So it will just going to crash differently.

Well, you're not meant to ever make it writable, before or after this 
patch. But if you ultimately do it, after this patch and following my 
suggestion to check that pkt->data == av_buffer_get_opaque(pkt->buf), it 
will not be mistaken as a valid wrapped_avframe. Before this patch, 
pkt->size will be sizeof(AVFrame) and pkt->data point to an AVFrame 
structure, but all the references will be invalid, and there's no way to 
know that's the case.

Either way, you're focusing on the wrong things. Even with "proper" 
usage, we're violating the API/ABI of AVFrame and potentially 
constraining library backwards compat if we start adding fields to 
AVFrame. That's the main issue.
In any other case, without this patch we're also risking propagating 
dangling pointers, so fixing that is a plus.

> 
> Regards,
> Marton