diff mbox series

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

Message ID 20210314022714.43661-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] avcodec/wrapped_avframe: stop using sizeof(AVFrame)
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

James Almer March 14, 2021, 2:27 a.m. UTC
Signed-off-by: James Almer <jamrial@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(-)

Comments

Marton Balint March 14, 2021, 12:23 p.m. UTC | #1
On Sat, 13 Mar 2021, James Almer wrote:

> Signed-off-by: James Almer <jamrial@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?

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

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?

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

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-request@ffmpeg.org with subject "unsubscribe".
James Almer March 14, 2021, 2:03 p.m. UTC | #2
On 3/14/2021 9:23 AM, Marton Balint wrote:
> 
> 
> On Sat, 13 Mar 2021, James Almer wrote:
> 
>> Signed-off-by: James Almer <jamrial@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-request@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-request@ffmpeg.org with subject "unsubscribe".
Marton Balint March 14, 2021, 6:25 p.m. UTC | #3
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.

[...]

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

Regards,
Marton
James Almer March 14, 2021, 7:21 p.m. UTC | #4
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
Marton Balint March 14, 2021, 9:10 p.m. UTC | #5
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
James Almer March 14, 2021, 9:36 p.m. UTC | #6
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-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

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;