On 3/14/2021 9:23 AM, Marton Balint wrote:
On Sat, 13 Mar 2021, James Almer wrote:
Signed-off-by: James Almer <jamr...@gmail.com>
---
Setting pkt->size to 0 but leaving pkt->data pointing to the frame
ensures that
the packet isn't mistaken for an empty one, and prevents wrong use of
the data
pointer in case av_packet_make_writable() is called on it (The
resulting packet
will be the same as if you call it on an empty one).
I decided to set the opaque field of the AVBufferRef to the frame
pointer so
we can do something like ensure pkt->size is 0 and pkt->data is equal
to it
before trying to treat pkt->data as an AVFrame, but i'm not sure if
that could
be done now, or only after a major bump (e.g. 4.3 lavf/lavd and 4.4
lavc at
runtime, where demuxers like the vapoursynth one propagate packets to the
wrapped_avframe decoder, and if the latter does the above check, it would
fail).
libavcodec/wrapped_avframe.c | 23 ++++-------------------
1 file changed, 4 insertions(+), 19 deletions(-)
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).
Isn't it a problem that the packet may not have correct padding now,
becaue you allocate a 0 sized av_buffer? We had a bug because of this,
and I fixed it in 436f00b10c062b75c7aab276c4a7d64524bd0444, which was
caused by av_buffer_realloc(&avpkt->buf, avpkt->size +
AV_INPUT_BUFFER_PADDING_SIZE) calls in the encode function removed in
93016f5d1d280f9cb7856883af287fa66affc04c. More info in this thread:
http://lists.ffmpeg.org/pipermail/ffmpeg-devel/2017-February/207329.html
We'd be propagating a packet reported as size 0 but with data pointing
at something. This has the benefit that is not treated as an empty
packet, and whatever is pointed to by data is never accessed by anything
(other than users that know what to do with wrapped_frame packets), so
padding isn't needed.
Same with the underlying AVBufferRef, size 0 ensures data isn't read
during a make_writable() attempt, nor written to.
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.
The wrapped frame will still be freed once av_buffer_unref(&pkt->buf) is
called as part of that process, same as now. The difference is that
currently we're making a copy of the AVFrame struct contents (because
pkt->size is sizeof(AVFrame)), while also freeing the actual frame, so
everything referenced by the zombie AVFrame-as-AVPacket-payload will be
invalid.
It is also debatable if lowering the packet size is a breaking change or
not, because previously people could have had an assert in their code
making sure it is not used with a library built with an AVFrame smaller
than what they are using...
So you mean even library users may have been using sizeof(AVFrame)
because this pseudo-codec made it seem like it was fine? Nothing in the
codec_id documentation says that pkt->size on a packet returned by a
demuxer or encoder reporting codec_id as wrapped_avframe should have any
specific size. It's internal knowledge being (ab)used. This whole thing
is just nasty. Even more so when a packet flag had to be added to mark a
packet as "trusted" because of the fact the payload may be pointing at
other stuff.
Maybe wrapped_avframe should just be removed and replaced with something
properly designed.
So unless we try to fix something specific with this patch maybe it is
better to not touch it unless we have plans how to fix all known issues
with wrapped avframes?
I don't know of issues with wrapped_frame other than what i detailed
above. It violates the API/ABI rules by looking at sizeof(AVFrame), and
an attempt at making the packets writable will result in dangling
pointers. This patch solves both.
And also libavdevice/kmsgrab.c need some changes too if a new format is
to be followed.
Regards,
Marton
diff --git a/libavcodec/wrapped_avframe.c b/libavcodec/wrapped_avframe.c
index 85ff32d13a..c921990024 100644
--- a/libavcodec/wrapped_avframe.c
+++ b/libavcodec/wrapped_avframe.c
@@ -44,32 +44,20 @@ static int wrapped_avframe_encode(AVCodecContext
*avctx, AVPacket *pkt,
const AVFrame *frame, int *got_packet)
{
AVFrame *wrapped = av_frame_clone(frame);
- uint8_t *data;
- int size = sizeof(*wrapped) + AV_INPUT_BUFFER_PADDING_SIZE;
if (!wrapped)
return AVERROR(ENOMEM);
- data = av_mallocz(size);
- if (!data) {
- av_frame_free(&wrapped);
- return AVERROR(ENOMEM);
- }
-
- pkt->buf = av_buffer_create(data, size,
- wrapped_avframe_release_buffer, NULL,
+ pkt->buf = av_buffer_create((uint8_t *)wrapped, 0,
+ wrapped_avframe_release_buffer, wrapped,
AV_BUFFER_FLAG_READONLY);
if (!pkt->buf) {
av_frame_free(&wrapped);
- av_freep(&data);
return AVERROR(ENOMEM);
}
- av_frame_move_ref((AVFrame*)data, wrapped);
- av_frame_free(&wrapped);
-
- pkt->data = data;
- pkt->size = sizeof(*wrapped);
+ pkt->data = (uint8_t *)wrapped;
+ pkt->size = 0;
pkt->flags |= AV_PKT_FLAG_KEY;
*got_packet = 1;
@@ -87,9 +75,6 @@ static int wrapped_avframe_decode(AVCodecContext
*avctx, void *data,
return AVERROR(EPERM);
}
- if (pkt->size < sizeof(AVFrame))
- return AVERROR(EINVAL);
-
in = (AVFrame*)pkt->data;
out = data;
--
2.30.2
_______________________________________________
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".
_______________________________________________
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".
