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

[Andreas Rheinhardt]

James Almer:
> On 3/11/2021 2:27 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> 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.
>>>>
>>>> I'd argue that a deprecation is actually a change.
>>>>
>>>>> 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. And we can't have it attempt to make
>>>>> it writable because it can't then report if it failed to reallocate
>>>>> the
>>>>> buffer. So this patch here deprecates it for being a function that
>>>>> predates reference counted buffers and is not able to properly handle
>>>>> them, and adds a replacement for it that also supersedes
>>>>> av_grow_packet() while at it.
>>>>>
>>>>
>>>> Yet you are not documenting that av_packet_resize can't fail if it is
>>>> shrinking a packet known to be writable; ergo all unchecked uses of
>>>> this
>>>> function in the second and third patch are API abuse.
>>>
>>> I can add checks for all of them if you prefer. I didn't because they
>>> are all internal uses known to (in theory) not fail.
>>>
>>
>> No, I don't prefer checks for stuff that can't fail. I'd rather prefer
>> if it were documented that it can't fail in these cases.
> 
> I'm in general against adding that kind of constrain on a function's
> documentation because you never know how it or what it processes could
> be extended in the future.
> Right now it can't fail in that scenario, true, but in the future we
> could add some feature to AVPacket that would need to be handled by this
> function that could start making it fail in that same scenario, and
> suddenly, the doxy is no longer correct, and the function needs to be
> replaced because it became unable to handle the new functionality.
> 
> If a function can fail at all, then the library user should always make
> sure to check the return value, and not be told there's one specific
> scenario where they don't need to.
> 

In this case I am against this patch.

>>
>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Marton
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: James Almer <jamrial at gmail.com>
>>>>>>> ---
>>>>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>>>>> libavcodec/version.h  |  3 +++
>>>>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>>>>       return 0;
>>>>>>> }
>>>>>>>
>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>> {
>>>>>>>       if (pkt->size <= size)
>>>>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>>       pkt->size = size;
>>>>>>>       memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>> }
>>>>>>> +#endif
>>>>>>>
>>>>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>> {
>>>>>>> -    int new_size;
>>>>>>>       av_assert0((unsigned)pkt->size <= INT_MAX -
>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>       if ((unsigned)grow_by >
>>>>>>>           INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>>>>>>>           return AVERROR(ENOMEM);
>>>>>>>
>>>>>>> -    new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>>>>> +}
>>>>>>> +
>>>>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>>>>> +{
>>>>>>> +    int new_size;
>>>>>>> +
>>>>>>> +    if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>>>>>>> +        return AVERROR(EINVAL);
>>>>>>> +
>>>>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>       if (pkt->buf) {
>>>>>>>           size_t data_offset;
>>>>>>>           uint8_t *old_data = pkt->data;
>>>>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>>           if (!pkt->buf)
>>>>>>>               return AVERROR(ENOMEM);
>>>>>>>           if (pkt->size > 0)
>>>>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>>>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size,
>>>>>>> size));
>>>>>>>           pkt->data = pkt->buf->data;
>>>>>>>       }
>>>>>>> -    pkt->size += grow_by;
>>>>>>> +    pkt->size = size;
>>>>>>>       memset(pkt->data + pkt->size, 0,
>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>
>>>>>>>       return 0;
>>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>>>> index 3d9013d783..1720d973f5 100644
>>>>>>> --- a/libavcodec/packet.h
>>>>>>> +++ b/libavcodec/packet.h
>>>>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>>>>    */
>>>>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>>>>
>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>> /**
>>>>>>>    * Reduce packet size, correctly zeroing padding
>>>>>>>    *
>>>>>>>    * @param pkt packet
>>>>>>>    * @param size new size
>>>>>>> + *
>>>>>>> + * @deprecated Use av_packet_resize
>>>>>>>    */
>>>>>>> +attribute_deprecated
>>>>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>>>>> +#endif
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * Resize the payload of a packet, correctly zeroing padding and
>>>>>>> avoiding data
>>>>>>> + * copy if possible.
>>>>>>> + *
>>>>>>> + * @param pkt packet
>>>>>>> + * @param size new size
>>>>>>> + *
>>>>>>> + * @return 0 on success, a negative AVERROR on error
>>>>>>> + */
>>>>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>>>>
>>>>>>> /**
>>>>>>>    * Increase packet size, correctly zeroing padding
>>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>>> index 3124ec8061..6c362b43e2 100644
>>>>>>> --- a/libavcodec/version.h
>>>>>>> +++ b/libavcodec/version.h
>>>>>>> @@ -162,5 +162,8 @@
>>>>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>>>> #endif
>>>>>>> +#ifndef FF_API_SHRINK_PACKET
>>>>>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>>>> +#endif
>>>>>>>
>>>>>>> #endif /* AVCODEC_VERSION_H */
>>>>>>> -- 
>>>>>>> 2.30.2
>>>>>>>