[FFmpeg-devel] [PATCH 1/3] avcodec/avpacket: add av_packet_resize()
James Almer
jamrial at gmail.com
Thu Mar 11 19:00:07 EET 2021
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.
>
>>>
>>> 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".
>
> _______________________________________________
> 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".
>
More information about the ffmpeg-devel
mailing list