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

[Andreas Rheinhardt]

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.

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