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

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Mon Mar 15 06:03:26 EET 2021


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

Can we really add an assert to it? I am not so sure about that. The
problem lies in ff_alloc_packet2(): It can return non-refcounted packets
whose data points to avctx->internal->byte_buffer. (Some encoders need
to allocate big buffers for worst-case scenarios and then the data is
initially written to said byte_buffer and copied lateron via
av_packet_make_refcounted() in encode_simple_internal().) This is
basically what Anton said: Not all parts of the code completely abide by
the constraints of the AVBuffer API; and they can still work fine if
they abide by their own notions of ownership.

- Andreas


More information about the ffmpeg-devel mailing list