[FFmpeg-devel] avcodec/avpacket: ensure the packet is writable in av_shrink_packet()

Submitted by James Almer on March 24, 2018, 9:11 p.m.

Details

Message ID 20180324211153.2440-1-jamrial@gmail.com
State New
Headers show

Commit Message

James Almer March 24, 2018, 9:11 p.m.
Signed-off-by: James Almer <jamrial@gmail.com>
---
This is a good time to deprecate this function and introduce a
replacement using the correct av_packet namespace and this time
returning an int.
What would be better

int av_packet_shrink(AVPacket *pkt, int size);

Or

int av_packet_resize(AVPacket *pkt, int size);

The latter would be a combination of both the current shrink and grow
functions.

 libavcodec/avpacket.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

wm4 March 24, 2018, 9:46 p.m.
On Sat, 24 Mar 2018 18:11:53 -0300
James Almer <jamrial@gmail.com> wrote:

> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> This is a good time to deprecate this function and introduce a
> replacement using the correct av_packet namespace and this time
> returning an int.
> What would be better
> 
> int av_packet_shrink(AVPacket *pkt, int size);
> 
> Or
> 

> int av_packet_resize(AVPacket *pkt, int size);

Seems better.

> 
> The latter would be a combination of both the current shrink and grow
> functions.
> 
>  libavcodec/avpacket.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 0693ca6f62..7faa082395 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -100,9 +100,12 @@ int av_new_packet(AVPacket *pkt, int size)
>  
>  void av_shrink_packet(AVPacket *pkt, int size)
>  {
> +    int packet_is_writable;
>      if (pkt->size <= size)
>          return;
>      pkt->size = size;
> +    packet_is_writable = !av_packet_make_writable(pkt);
> +    av_assert0(packet_is_writable);
>      memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>  }
>  

LGTM
James Almer March 25, 2018, 1:39 a.m.
On 3/24/2018 6:46 PM, wm4 wrote:
> On Sat, 24 Mar 2018 18:11:53 -0300
> James Almer <jamrial@gmail.com> wrote:
> 
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> This is a good time to deprecate this function and introduce a
>> replacement using the correct av_packet namespace and this time
>> returning an int.
>> What would be better
>>
>> int av_packet_shrink(AVPacket *pkt, int size);
>>
>> Or
>>
> 
>> int av_packet_resize(AVPacket *pkt, int size);
> 
> Seems better.
> 
>>
>> The latter would be a combination of both the current shrink and grow
>> functions.
>>
>>  libavcodec/avpacket.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index 0693ca6f62..7faa082395 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -100,9 +100,12 @@ int av_new_packet(AVPacket *pkt, int size)
>>  
>>  void av_shrink_packet(AVPacket *pkt, int size)
>>  {
>> +    int packet_is_writable;
>>      if (pkt->size <= size)
>>          return;
>>      pkt->size = size;
>> +    packet_is_writable = !av_packet_make_writable(pkt);
>> +    av_assert0(packet_is_writable);
>>      memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>  }
>>  
> 
> LGTM

Ok, but i just realized that i can't apply this without first writing an
internal variant specifically for the encoders that makes sure pkt->data
!= avctx->internal->byte_buffer before trying to do anything, otherwise
the supposed benefits of that weird internal buffer code in
ff_alloc_packet2() would be lost.

Will look at that later.
wm4 March 25, 2018, 12:14 p.m.
On Sat, 24 Mar 2018 22:39:45 -0300
James Almer <jamrial@gmail.com> wrote:

> On 3/24/2018 6:46 PM, wm4 wrote:
> > On Sat, 24 Mar 2018 18:11:53 -0300
> > James Almer <jamrial@gmail.com> wrote:
> >   
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >> This is a good time to deprecate this function and introduce a
> >> replacement using the correct av_packet namespace and this time
> >> returning an int.
> >> What would be better
> >>
> >> int av_packet_shrink(AVPacket *pkt, int size);
> >>
> >> Or
> >>  
> >   
> >> int av_packet_resize(AVPacket *pkt, int size);  
> > 
> > Seems better.
> >   
> >>
> >> The latter would be a combination of both the current shrink and grow
> >> functions.
> >>
> >>  libavcodec/avpacket.c | 3 +++
> >>  1 file changed, 3 insertions(+)
> >>
> >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> >> index 0693ca6f62..7faa082395 100644
> >> --- a/libavcodec/avpacket.c
> >> +++ b/libavcodec/avpacket.c
> >> @@ -100,9 +100,12 @@ int av_new_packet(AVPacket *pkt, int size)
> >>  
> >>  void av_shrink_packet(AVPacket *pkt, int size)
> >>  {
> >> +    int packet_is_writable;
> >>      if (pkt->size <= size)
> >>          return;
> >>      pkt->size = size;
> >> +    packet_is_writable = !av_packet_make_writable(pkt);
> >> +    av_assert0(packet_is_writable);
> >>      memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> >>  }
> >>    
> > 
> > LGTM  
> 
> Ok, but i just realized that i can't apply this without first writing an
> internal variant specifically for the encoders that makes sure pkt->data
> != avctx->internal->byte_buffer before trying to do anything, otherwise
> the supposed benefits of that weird internal buffer code in
> ff_alloc_packet2() would be lost.
> 
> Will look at that later.

Didn't look at it again, but this mechanism might be broken anyway
since the new encode API must return refcounted packets.

Maybe the new dynamic sized buffer pool would help.
James Almer March 25, 2018, 2:23 p.m.
On 3/25/2018 9:14 AM, wm4 wrote:
> On Sat, 24 Mar 2018 22:39:45 -0300
> James Almer <jamrial@gmail.com> wrote:
> 
>> On 3/24/2018 6:46 PM, wm4 wrote:
>>> On Sat, 24 Mar 2018 18:11:53 -0300
>>> James Almer <jamrial@gmail.com> wrote:
>>>   
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> This is a good time to deprecate this function and introduce a
>>>> replacement using the correct av_packet namespace and this time
>>>> returning an int.
>>>> What would be better
>>>>
>>>> int av_packet_shrink(AVPacket *pkt, int size);
>>>>
>>>> Or
>>>>  
>>>   
>>>> int av_packet_resize(AVPacket *pkt, int size);  
>>>
>>> Seems better.
>>>   
>>>>
>>>> The latter would be a combination of both the current shrink and grow
>>>> functions.
>>>>
>>>>  libavcodec/avpacket.c | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>> index 0693ca6f62..7faa082395 100644
>>>> --- a/libavcodec/avpacket.c
>>>> +++ b/libavcodec/avpacket.c
>>>> @@ -100,9 +100,12 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>  
>>>>  void av_shrink_packet(AVPacket *pkt, int size)
>>>>  {
>>>> +    int packet_is_writable;
>>>>      if (pkt->size <= size)
>>>>          return;
>>>>      pkt->size = size;
>>>> +    packet_is_writable = !av_packet_make_writable(pkt);
>>>> +    av_assert0(packet_is_writable);
>>>>      memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>  }
>>>>    
>>>
>>> LGTM  
>>
>> Ok, but i just realized that i can't apply this without first writing an
>> internal variant specifically for the encoders that makes sure pkt->data
>> != avctx->internal->byte_buffer before trying to do anything, otherwise
>> the supposed benefits of that weird internal buffer code in
>> ff_alloc_packet2() would be lost.
>>
>> Will look at that later.
> 
> Didn't look at it again, but this mechanism might be broken anyway
> since the new encode API must return refcounted packets.
> 
> Maybe the new dynamic sized buffer pool would help.

Good point. Still waiting for suggestions about how to introduce that in
the relevant thread, for that matter :p

Patch hide | download patch | download mbox

diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
index 0693ca6f62..7faa082395 100644
--- a/libavcodec/avpacket.c
+++ b/libavcodec/avpacket.c
@@ -100,9 +100,12 @@  int av_new_packet(AVPacket *pkt, int size)
 
 void av_shrink_packet(AVPacket *pkt, int size)
 {
+    int packet_is_writable;
     if (pkt->size <= size)
         return;
     pkt->size = size;
+    packet_is_writable = !av_packet_make_writable(pkt);
+    av_assert0(packet_is_writable);
     memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
 }