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

[Andreas Rheinhardt]

Michael Niedermayer:
> On Sun, Mar 14, 2021 at 04:51:44PM +0100, Anton Khirnov wrote:
>> Quoting James Almer (2021-03-14 16:35:48)
>>> On 3/14/2021 7:34 AM, Anton Khirnov wrote:
>>>> Quoting James Almer (2021-03-12 18:24:47)
>>>>>
>>>>> 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().
>>>>
>>>> On that topic, I've been wondering about eliminating this requirement.
>>>> Does anyone know which code it is precisely that depends on the padding
>>>> being zeroed? Is this optimization really worth it?
>>>> It seems rather silly to jump through all these hoops for an
>>>> unmeasurable benefit in one decoder.
>>>
>>> Some subtitle demuxers didn't look at pkt->size and depended on the 
>>> padding to work as a 0 string terminator, but Marton fixed that in a 
>>> patchset sent yesterday.
>>>
>>> Beyond that, i think the v210 decoder and encoder read and write past 
>>> the end of the buffer if you use the simd functions.
>>>
>>>>
>>>> It would also give us zero-copy packet splitting.
>>>>
>>>> [...]
>>>>
>>>> I would suggest keeping av_shrink_packet() with a big scary warning that
>>>> it does not check for ownership and it is the caller's responsibility to
>>>> ensure that writing to the packet is safe.
>>>
>>> If we can remove the zero-padding requirement, or the padding 
>>> requirement altogether, then that would no longer be necessary.
>>
>> I don't think we can remove the padding requirement completely, as the
>> bitstream reader reads 4- or 8-byte chunks at once. I imagine changing
>> that into single-byte reads would be very slow.
>> But getting rid of the zeroing requirement should be more feasible.
> 
> mpeg/itu codecs historically always had the all 0 VLC of all relevant tables
> invalid. That was done to ensure a long string of zeros cannot occur. and
> thus a startcode as used in MPEG-PS/TS or in slices (nowadays NALs and such)
> cannot occur in the middle of a valid frame.
> That way simply checking for the vlc to be valid would also always check
> for a startcode or the end if there where 3-4 zero bytes at the end.
> This was probably used in every decoder where it could be used as
> it avoided an extra check per vlc reading loop.
> I have the suspicion it would be faster to check the padding for being 0
> and malloc/memcpy if not in many cases than to add extra checks in every
> affected loop. More so as this only affcts cases where there is no
> "NAL/slice/packet" with a startcode afterwards
> 

So if we mandate that the actually allocated buffers need to be at least
AV_INPUT_BUFFER_PADDING_SIZE bigger than AVPacket.size and that there
has to be at least AV_INPUT_BUFFER_PADDING_SIZE zero bytes before the
end of the allocated area, no crashes can happen. One still needs to
check for overreads, but this needn't be done in the VLC reading loops.
We should probably also mandate that all the data after the end of the
packet and the beginning of the zeroed padding needs to be initialized
(in order not to get Valgrind warnings).
(This is actually what I tell myself in order to justify the behaviour
of the Matroska demuxer when lacing is present.)

- Andreas