[FFmpeg-devel] [PATCH 3/3] avcodec/packet: change public function and struct size parameter types to size_t
Andreas Rheinhardt
andreas.rheinhardt at gmail.com
Sun May 31 20:58:52 EEST 2020
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.
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? (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.
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. 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.
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.
- Andreas
More information about the ffmpeg-devel
mailing list