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
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
