[FFmpeg-devel] [PATCH 1/3] avcodec/avpacket: add av_packet_resize()

[James Almer]

On 3/11/2021 7:40 PM, Marton Balint wrote:
> 
> 
> On Thu, 11 Mar 2021, James Almer wrote:
> 
>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>
>>>
>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>
>>>> This function acts as a replacement for both av_grow_packet() and
>>>> av_shrink_packet(), the latter which is now deprecated and will be 
>>>> removed as
>>>> it does not correctly handle non-writable packets.
>>>
>>> I don't think this is a good idea, av_shrink_packet cannot fail, 
>>> av_grow_packet can. By using the same function you are losing the 
>>> information if the end result should be checked or not.
>>
>> I'm not sure i follow. av_shrink_packet() is not being changed at all, 
>> just deprecated, scheduled for removal, and its use discouraged.
> 
> But why are you complicating code with mandatory return value checking?

Because unlike av_shrink_packet(), av_packet_resize() is more thorough 
when handling AVPackets and allows new usage scenarios that were not 
allowed with the former, thus it can fail.

> Because as soon as a function returns an error, you have to check it, 
> and forward it upwards.

Is error checking that much of a problem? I don't understand why it 
bothers people so much.

> 
>> Maybe i should have split this in two, one to add av_packet_resize() 
>> and one to deprecate av_shrink_packet(), to avoid confusion.
>>
>> In any case, the fact av_shrink_packet() cannot fail is the reason I'm 
>> getting rid of it. It's zeroing the padding without bothering to check 
>> if the packet is writable at all.
> 
> Add an assert to it then.

We only assert on internal bugs, not invalid arguments passed by callers 
to a public function. The asserts would need to be added above each 
av_shrink_packet() call. And that's only for our internal uses of the 
function. It does nothing to the issue of it being public API that can't 
properly handle AVPackets in their current form.

 > Shrinking a non-writable packet seems bad API usage anyways.

I get a packet from a demuxer. It contains two independent portions 
(NALUs, OBUs, etc) i want to separate in order to feed them to something 
like a multi threaded decoder. And so I create a new reference to it, 
resulting in two packets pointing to the same data. I shrink one to only 
contain the first portion, and i add the required byte offset to the 
data pointer and subtract it to the size field on the second packet so 
it contains only the second portion.
The result if i use av_packet_resize() will be two valid, properly 
padded packets containing their respective expected data, but if i use 
av_shrink_packet(), the result will be the packet with the second 
portion featuring padding bytes worth of data zeroed at the start of its 
payload.

I'm sure there are other similar valid scenarios where attempting to 
shrink a non writable packet is not "bad API usage".

> 
> If you want to shrink a writable packet, then maybe you don't even need 
> zero padding, because the buffer possibly already contains some defined 
> value, and the main reason of zero padding is avoiding reading 
> uninitialized data...

If i allocate a payload of size 1024, ultimately fill only 512 bytes, 
then resize it to that value (typical scenario in lavf demuxers), if i 
don't zero the bytes after the 512 offset then they will remain 
uninitialized.

> 
> Regards,
> Marton
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".