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

[Marton Balint]

On Sun, 14 Mar 2021, James Almer wrote:

> On 3/14/2021 3:25 PM, Marton Balint wrote:
>> 
>> 
>> On Sun, 14 Mar 2021, James Almer wrote:
>> 
>>>> 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.

I still think this does not fix the underlying problem. In some ways it 
makes it less fragile, it some ways it makes it more (see the 
av_buffer_realloc() example I pointed earlier). av_packet_make_writable() 
definitely should return an error. Maybe AV_PKT_FLAG_TRUSTED can be 
checked, I have no better idea for a quick fix for that.

One other possibility is to put an AVFrame pointer into the data and not 
an AVFrame struct. That also gets you rid of sizeof(AVFrame) but is
definitely something that is only doable after the bump. (And it still 
won't fix the av_packet_make_writable() issue, but it makes the buffer 
reallocatable at least.)

If you still feel strongly that your method of handling wrapped avframes 
is the best way to go ahead, then feel free to commit, but please consider 
other options.

Thanks,
Marton