[FFmpeg-devel] [PATCH 3/3] avcodec/packet: change public function and struct size parameter types to size_t
James Almer
jamrial at gmail.com
Sun May 31 21:35:25 EEST 2020
On 5/31/2020 2:58 PM, Andreas Rheinhardt wrote:
> James Almer:
>> Signed-off-by: James Almer <jamrial at gmail.com>
>> ---
>> doc/APIchanges | 4 ++--
>> libavcodec/avpacket.c | 49 +++++++++++++++++++++++++++++++++++++++++++
>> libavcodec/packet.h | 45 +++++++++++++++++++++++++++++++++++++++
>> libavutil/frame.h | 4 ++++
>> 4 files changed, 100 insertions(+), 2 deletions(-)
>>
>> diff --git a/doc/APIchanges b/doc/APIchanges
>> index 8d353fdcef..7f15090031 100644
>> --- a/doc/APIchanges
>> +++ b/doc/APIchanges
>> @@ -16,8 +16,8 @@ libavutil: 2017-10-21
>> API changes, most recent first:
>>
>> 2020-06-xx - xxxxxxxxxx
>> - Change AVBufferRef and relevant AVFrame function and struct size
>> - parameter and fields type to size_t at next major bump.
>> + Change AVBufferRef and relevant AVFrame and AVPacket function and
>> + struct size parameter and fields type to size_t at next major bump.
>>
>> 2020-xx-xx - xxxxxxxxxx - lavc 58.88.100 - avcodec.h codec.h
>> Move AVCodec-related public API to new header codec.h.
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index 033f2d8f26..e43c584576 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -69,7 +69,11 @@ void av_packet_free(AVPacket **pkt)
>> av_freep(pkt);
>> }
>>
>> +#if FF_API_BUFFER_SIZE_T
>> static int packet_alloc(AVBufferRef **buf, int size)
>> +#else
>> +static int packet_alloc(AVBufferRef **buf, size_t size)
>> +#endif
>> {
>> int ret;
>> if (size < 0 || size >= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)> @@ -84,7 +88,11 @@ static int packet_alloc(AVBufferRef **buf, int size)
>> return 0;
>> }
>>
>> +#if FF_API_BUFFER_SIZE_T
>> int av_new_packet(AVPacket *pkt, int size)
>> +#else
>> +int av_new_packet(AVPacket *pkt, size_t size)
>> +#endif
>> {
>> AVBufferRef *buf = NULL;
>> int ret = packet_alloc(&buf, size);
>> @@ -99,7 +107,11 @@ int av_new_packet(AVPacket *pkt, int size)
>> return 0;
>> }
>>
>> +#if FF_API_BUFFER_SIZE_T
>> void av_shrink_packet(AVPacket *pkt, int size)
>> +#else
>> +void av_shrink_packet(AVPacket *pkt, size_t size)
>> +#endif
>> {
>> if (pkt->size <= size)
>> return;
>> @@ -107,12 +119,21 @@ void av_shrink_packet(AVPacket *pkt, int size)
>> memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>> }
>>
>> +#if FF_API_BUFFER_SIZE_T
>> 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))
>> +#else
>> +int av_grow_packet(AVPacket *pkt, size_t grow_by)
>> +{
>> + size_t new_size;
>> + av_assert0(pkt->size <= SIZE_MAX - AV_INPUT_BUFFER_PADDING_SIZE);
>> + if (grow_by >
>> + SIZE_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>> +#endif
>> return AVERROR(ENOMEM);
>>
>> new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
>> @@ -124,7 +145,11 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>> pkt->data = pkt->buf->data;
>> } else {
>> data_offset = pkt->data - pkt->buf->data;
>> +#if FF_API_BUFFER_SIZE_T
>> if (data_offset > INT_MAX - new_size)
>> +#else
>> + if (data_offset > SIZE_MAX - new_size)
>> +#endif
>> return AVERROR(ENOMEM);
>> }
>>
>> @@ -151,7 +176,11 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>> return 0;
>> }
>>
>> +#if FF_API_BUFFER_SIZE_T
>> int av_packet_from_data(AVPacket *pkt, uint8_t *data, int size)
>> +#else
>> +int av_packet_from_data(AVPacket *pkt, uint8_t *data, size_t size)
>> +#endif
>> {
>> if (size >= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>> return AVERROR(EINVAL);
>> @@ -329,7 +358,11 @@ int av_packet_add_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>>
>>
>> uint8_t *av_packet_new_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>> +#if FF_API_BUFFER_SIZE_T
>> int size)
>> +#else
>> + size_t size)
>> +#endif
>> {
>> int ret;
>> uint8_t *data;
>> @@ -350,7 +383,11 @@ uint8_t *av_packet_new_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>> }
>>
>> uint8_t *av_packet_get_side_data(const AVPacket *pkt, enum AVPacketSideDataType type,
>> +#if FF_API_BUFFER_SIZE_T
>> int *size)
>> +#else
>> + size_t *size)
>> +#endif
>> {
>> int i;
>>
>> @@ -490,7 +527,11 @@ int av_packet_split_side_data(AVPacket *pkt){
>> }
>> #endif
>>
>> +#if FF_API_BUFFER_SIZE_T
>> uint8_t *av_packet_pack_dictionary(AVDictionary *dict, int *size)
>> +#else
>> +uint8_t *av_packet_pack_dictionary(AVDictionary *dict, size_t *size)
>> +#endif
>> {
>> AVDictionaryEntry *t = NULL;
>> uint8_t *data = NULL;
>> @@ -525,7 +566,11 @@ fail:
>> return NULL;
>> }
>>
>> +#if FF_API_BUFFER_SIZE_T
>> int av_packet_unpack_dictionary(const uint8_t *data, int size, AVDictionary **dict)
>> +#else
>> +int av_packet_unpack_dictionary(const uint8_t *data, size_t size, AVDictionary **dict)
>> +#endif
>> {
>> const uint8_t *end;
>> int ret;
>> @@ -552,7 +597,11 @@ int av_packet_unpack_dictionary(const uint8_t *data, int size, AVDictionary **di
>> }
>>
>> int av_packet_shrink_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>> +#if FF_API_BUFFER_SIZE_T
>> int size)
>> +#else
>> + size_t size)
>> +#endif
>> {
>> int i;
>>
>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>> index 41485f4527..0fd7b3486a 100644
>> --- a/libavcodec/packet.h
>> +++ b/libavcodec/packet.h
>> @@ -297,7 +297,11 @@ enum AVPacketSideDataType {
>>
>> typedef struct AVPacketSideData {
>> uint8_t *data;
>> +#if FF_API_BUFFER_SIZE_T
>> int size;
>> +#else
>> + size_t size;
>> +#endif
>> enum AVPacketSideDataType type;
>> } AVPacketSideData;
>>
>> @@ -353,7 +357,11 @@ typedef struct AVPacket {
>> */
>> int64_t dts;
>> uint8_t *data;
>> +#if FF_API_BUFFER_SIZE_T
>> int size;
>> +#else
>> + size_t size;
>> +#endif
>> int stream_index;
>> /**
>> * A combination of AV_PKT_FLAG values
>> @@ -465,7 +473,11 @@ void av_init_packet(AVPacket *pkt);
>> * @param size wanted payload size
>> * @return 0 if OK, AVERROR_xxx otherwise
>> */
>> +#if FF_API_BUFFER_SIZE_T
>> int av_new_packet(AVPacket *pkt, int size);
>> +#else
>> +int av_new_packet(AVPacket *pkt, size_t size);
>> +#endif
>>
>> /**
>> * Reduce packet size, correctly zeroing padding
>> @@ -473,7 +485,11 @@ int av_new_packet(AVPacket *pkt, int size);
>> * @param pkt packet
>> * @param size new size
>> */
>> +#if FF_API_BUFFER_SIZE_T
>> void av_shrink_packet(AVPacket *pkt, int size);
>> +#else
>> +void av_shrink_packet(AVPacket *pkt, size_t size);
>> +#endif
>>
>> /**
>> * Increase packet size, correctly zeroing padding
>> @@ -481,7 +497,11 @@ void av_shrink_packet(AVPacket *pkt, int size);
>> * @param pkt packet
>> * @param grow_by number of bytes by which to increase the size of the packet
>> */
>> +#if FF_API_BUFFER_SIZE_T
>> int av_grow_packet(AVPacket *pkt, int grow_by);
>> +#else
>> +int av_grow_packet(AVPacket *pkt, size_t grow_by);
>> +#endif
>>
>> /**
>> * Initialize a reference-counted packet from av_malloc()ed data.
>> @@ -496,7 +516,11 @@ int av_grow_packet(AVPacket *pkt, int grow_by);
>> *
>> * @return 0 on success, a negative AVERROR on error
>> */
>> +#if FF_API_BUFFER_SIZE_T
>> int av_packet_from_data(AVPacket *pkt, uint8_t *data, int size);
>> +#else
>> +int av_packet_from_data(AVPacket *pkt, uint8_t *data, size_t size);
>> +#endif
>>
>> #if FF_API_AVPACKET_OLD_API
>> /**
>> @@ -546,7 +570,11 @@ void av_free_packet(AVPacket *pkt);
>> * @return pointer to fresh allocated data or NULL otherwise
>> */
>> uint8_t* av_packet_new_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>> +#if FF_API_BUFFER_SIZE_T
>> int size);
>> +#else
>> + size_t size);
>> +#endif
>>
>> /**
>> * Wrap an existing array as a packet side data.
>> @@ -573,7 +601,11 @@ int av_packet_add_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>> * @return 0 on success, < 0 on failure
>> */
>> int av_packet_shrink_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>> +#if FF_API_BUFFER_SIZE_T
>> int size);
>> +#else
>> + size_t size);
>> +#endif
>>
>> /**
>> * Get side information from packet.
>> @@ -584,7 +616,11 @@ int av_packet_shrink_side_data(AVPacket *pkt, enum AVPacketSideDataType type,
>> * @return pointer to data if present or NULL otherwise
>> */
>> uint8_t* av_packet_get_side_data(const AVPacket *pkt, enum AVPacketSideDataType type,
>> +#if FF_API_BUFFER_SIZE_T
>> int *size);
>> +#else
>> + size_t *size);
>> +#endif
>>
>> #if FF_API_MERGE_SD_API
>> attribute_deprecated
>> @@ -603,7 +639,12 @@ const char *av_packet_side_data_name(enum AVPacketSideDataType type);
>> * @param size pointer to store the size of the returned data
>> * @return pointer to data if successful, NULL otherwise
>> */
>> +#if FF_API_BUFFER_SIZE_T
>> uint8_t *av_packet_pack_dictionary(AVDictionary *dict, int *size);
>> +#else
>> +uint8_t *av_packet_pack_dictionary(AVDictionary *dict, size_t *size);
>> +#endif
>> +
>> /**
>> * Unpack a dictionary from side_data.
>> *
>> @@ -612,7 +653,11 @@ uint8_t *av_packet_pack_dictionary(AVDictionary *dict, int *size);
>> * @param dict the metadata storage dictionary
>> * @return 0 on success, < 0 on failure
>> */
>> +#if FF_API_BUFFER_SIZE_T
>> int av_packet_unpack_dictionary(const uint8_t *data, int size, AVDictionary **dict);
>> +#else
>> +int av_packet_unpack_dictionary(const uint8_t *data, size_t size, AVDictionary **dict);
>> +#endif
>>
>>
>> /**
>> diff --git a/libavutil/frame.h b/libavutil/frame.h
>> index fa4931edb8..20b093ec9d 100644
>> --- a/libavutil/frame.h
>> +++ b/libavutil/frame.h
>> @@ -616,7 +616,11 @@ typedef struct AVFrame {
>> * - encoding: unused
>> * - decoding: set by libavcodec, read by user.
>> */
>> +#if FF_API_BUFFER_SIZE_T
>> int pkt_size;
>> +#else
>> + size_t pkt_size;
>> +#endif
>>
>> #if FF_API_FRAME_QP
>> /**
>>
> 1. Your patch leads to lots of ifdefs. How about adding a typedef
> AV_BUFFER_SIZE or so that is currently an int and will become a size_t
> in the future. This would of course have to be accompanied by an
> AV_BUFFER_SIZE_MAX. Then one would not have two versions of
> av_grow_packet at all. Said typedef should be public so that our users
> can already adapt their code; it would need to be kept for some time
> (until the next major version bump after the switch) after the switch.
I don't want new API for a field type change. I agree however we could
use one internally.
This approach has already been used in other places for the same
purpose, even right now in lavu, but I'll nonetheless go with whatever
the majority prefers.
> 2. packet_alloc still errors out if it is supposed to allocate more than
> INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE. This is an unintentional
> oversight, isn't it?
Yeah, missed it. Thanks for pointing it out.
> (Anyway, when this function is switched to size_t,
> the correct error would be AVERROR(ERANGE). It is actually already the
> correct error in case size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE.)
> 3. That's unfortunately only a tiny fraction of the work to do before
> this switch can happen:
> a) libavcodec/mjpega_dump_header_bsf.c contains the following line:
> for (i = 0; i < in->size - 1; i++) {
> If in->size == 0, then this will not have the desired effect, because
> the subtraction now wraps around. There are probably more places like
> this (and overflow checks that don't work as expected any more). We
> would need to find them all and port them.
Empty packets are considered flush packets, right? Guess we should be
rejecting packets where pkt->size == 0 && pkt->data != NULL in
av_bsf_send_packet(), same as we do in avcodec_send_packet(). I'll send
a patch for that later.
And sure, I can check for other cases of code that might need extra
precautions.
> b) Lots of other code needs to be ported, too: The AVStream side data
> API comes to mind for reasons of compatibility to the packet side data
> API.
Yes. Will port it as well.
> Furthermore the avio API (otherwise e.g. ff_raw_write_packet might
> no longer work as expected) and the bytestream2-API. And other helper
> functions, too. And it is not always simply switching types. E.g.
> ff_avc_parse_nal_units now needs to handle the case in which the size of
> a NAL unit exceeds the 32bit size field.
Will look into these.
> c) And the same goes for all our users. In other words: This is a
> serious API break. I don't think that this will be possible without the
> typical two year period.
I don't share the sentiment, but if that's the general consensus, then
I'll not be against it.
>
> - Andreas
> _______________________________________________
> 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