[FFmpeg-devel] [PATCH 1/3] avcodec/avpacket: add av_packet_resize()
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Thu Mar 11 19:27:12 EET 2021
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.
>>
>>>>
>>>> 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".
>>
>
> _______________________________________________
> 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