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

[James Almer]

On 3/14/2021 6:10 PM, Marton Balint wrote:
> 
> 
> 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). 

Doing av_buffer_realloc(&pkt->buf, pkt->size + 
AV_INPUT_BUFFER_PADDING_SIZE) on a wrapped_frame packet after this patch 
would create a new buffer of AV_INPUT_BUFFER_PADDING_SIZE bytes, no data 
would be copied to it, and the original buffer would be unreffed (Which 
means the wrapped AVFrame is freed).
Doing it without this patch generates what i described above and what 
you mentioned in the email you linked: A copy of the AVFrame struct with 
a lot of invalid pointers.

> 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.)

We shouldn't try to make reallocate/make_writable a usable or expected 
scenario for wrapped_avframe, only ensure that if it happens, the result 
isn't a problem (Namely, it can't be mistaken for a valid wrapped_frame 
packet if you do the proper check).

> 
> 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.

No, i will not push this without a consensus. And my intention of making 
pkt->data == av_buffer_get_opaque(pkt->buf) the widespread and actually 
robust "Is this a valid wrapped_frame packet?" check can't be done until 
after a major bump anyway.

> 
> Thanks,
> Marton
> _______________________________________________
> 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".