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