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

[Marton Balint]

On Fri, 12 Mar 2021, James Almer wrote:

>>>
>>> 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.
>> 
>> This looks like an unfortunate example, since you are:
>> - adding a reference to the packet, so you don't have to duplicate data
>> - and then want to add zero padding to the partial packet, so you will
>> duplicate data.
>> And I think the padding does not have to be zero for the partial packet.
>
> The padding exists AFAIK because something, like an optimized bitstream 
> reader that tries to process several bytes at the same time, may end up 
> reading or writing pass the reported end of the buffer.
> That means that if reading and it's not all zeroes, it could in theory 
> have unpredictable results. Hence why everything always zeroes the 
> padding, even av_shrink_packet().
>
> And yes, what you describe is what some bitstream filters and the 
> matroska demuxer do. They just create several packet references pointing 
> to the same data buffer, but using different offsets for the data 
> pointer. They all have "padding", but in many cases said padding is the 
> beginning of the payload of another packet, and that's not ideal.

Well, performance reasons come in play and the ability to not copy the 
data. Yeah, it does not play nicely with our historical requirement of 
zero padding.

I did a little experimenting, and except for subtitles (which have crashed 
and burned because of the missing 0 string terminator), most tests handled 
non-zero padding well, a few failed tests, mostly for partial source 
files, for which I guess it is inevitable?

So I guess for now we will stay in the gray area of "if it does not 
crash, then having non-zero padding for some partial packets is 
OKish"...

>> I agree that it is not nice that av_shrink_packet() writes something to 
>> the packet, people may not think about it and misuse it instead of 
>> directly decreaseing pkt->size when they need a partial packet. That is 
>> why I suggested the assert(). One might argue that it kind of breaks 
>> behaviour, but I'd say it does not break it too much, and writing to a 
>> non-writable packet was broken in the first place.
>> 
>> Alternatively we can change av_shrink_packet() to only zero the padding 
>> if the packet is writable, which has the benefit that it will do what 
>> people generally expect, no matter if you throw a writable or a 
>> non-writable packet to it.
>> 
>> A third alternative is to introduce void av_shrink_packet2() in which 
>> you explicitly document that a writable packet is needed and do the 
>> assert there, and deprecate av_shrink_packet().
>
> Not a fan of functions with a 2 suffix. And to be honest, I really don't 
> care about what we do with av_shrink_packet().
> Do you want to keep it around? Ok. Want to deprecate and remove it? 
> Better. Want to add an assert to it? Not a fan and it may result in 
> unexpected crashes for library users, but whatever.
>
> I just want to add av_packet_resize() to have a single resize function 
> that will properly handle AVPackets in their current and any potential 
> future states.

Ok, then I suggest you add av_resize_packet but keep av_shrink_packet() as 
well, and we can add an assert() to it after the release/bump.

Regards,
Marton