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