| Message ID | 20200531163839.561-3-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avutil/buffer: change public function and struct size parameter types to size_t | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
James Almer: > Signed-off-by: James Almer <jamrial@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
On 5/31/2020 2:58 PM, Andreas Rheinhardt wrote: > James Almer: >> Signed-off-by: James Almer <jamrial@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@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
On 5/31/2020 3:35 PM, James Almer wrote: > On 5/31/2020 2:58 PM, Andreas Rheinhardt wrote: >> James Almer: >>> Signed-off-by: James Almer <jamrial@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 That being said, i could also look into just sticking to change AVBufferRef, AVFrame->side_data->size, AVPacket->side_data->size and AVStream->side_data->size, but not bother with AVPacket->size, seeing we don't really need to allow bigger packets. The purpose of this change was after all to remove the size constrains from side data. It should prevent most of the fallout you mentioned, especially avio.
James Almer: > On 5/31/2020 2:58 PM, Andreas Rheinhardt wrote:>> (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. > The only way one can signal flushing to a bsf is by using av_bsf_flush. Empty packets (data == NULL and no side_data) are considered eof. Packets with data != NULL, size = 0 and no side data are currently considered normal packets (a possible use-case would be to send some timestamps to the bsf, although no bsf currently makes use of this; but who knows what needs a future bsf has). Also even rejecting such empty packets in av_bsf_send_packet wouldn't completely solve the problem because a user might send a side-data only packet. So the check would either be switched to i + 1 < in->size (is this really slower than i < in->size - 1?) or a check has to be added before the loop. Also notice that the 1 should actually be 3, see https://ffmpeg.org/pipermail/ffmpeg-devel/2020-May/263632.html. - Andreas
On 5/31/2020 4:11 PM, Andreas Rheinhardt wrote: > James Almer: >> On 5/31/2020 2:58 PM, Andreas Rheinhardt wrote:>> (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. >> > The only way one can signal flushing to a bsf is by using av_bsf_flush. > Empty packets (data == NULL and no side_data) are considered eof. Yes, that's what i meant. The entire codebase uses flush, drain and EOF interchangeably in some cases. > Packets with data != NULL, size = 0 and no side data are currently > considered normal packets (a possible use-case would be to send some > timestamps to the bsf, although no bsf currently makes use of this; but > who knows what needs a future bsf has). data != NULL && size == 0 should IMO not be accepted. If we want to signal that we're feeding the bsf something like timestamps only, we'd have to find a different way to do it than setting pkt->data to some bogus value that's not NULL in order to bypass the IS_EMPTY() check. > > Also even rejecting such empty packets in av_bsf_send_packet wouldn't > completely solve the problem because a user might send a side-data only > packet. So the check would either be switched to i + 1 < in->size (is > this really slower than i < in->size - 1?) or a check has to be added > before the loop. Also notice that the 1 should actually be 3, see > https://ffmpeg.org/pipermail/ffmpeg-devel/2020-May/263632.html. > > - Andreas > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > https://ffmpeg.org/mailman/listinfo/ffmpeg-devel > > To unsubscribe, visit link above, or email > ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe". >
James Almer: > On 5/31/2020 4:11 PM, Andreas Rheinhardt wrote: >> James Almer: >>> On 5/31/2020 2:58 PM, Andreas Rheinhardt wrote:>> (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. >>> >> The only way one can signal flushing to a bsf is by using av_bsf_flush. >> Empty packets (data == NULL and no side_data) are considered eof. > > Yes, that's what i meant. The entire codebase uses flush, drain and EOF > interchangeably in some cases. > >> Packets with data != NULL, size = 0 and no side data are currently >> considered normal packets (a possible use-case would be to send some >> timestamps to the bsf, although no bsf currently makes use of this; but >> who knows what needs a future bsf has). > > data != NULL && size == 0 should IMO not be accepted. If we want to > signal that we're feeding the bsf something like timestamps only, we'd > have to find a different way to do it than setting pkt->data to some > bogus value that's not NULL in order to bypass the IS_EMPTY() check. > Bogus value? You seem to believe that pkt->data just points to somewhere in memory; in a valid packet it points to a buffer of size pkt->size + AV_INPUT_BUFFER_PADDING_SIZE (even when pkt->size == 0). av_packet_make_refcounted creates such packets if the input packet doesn't have data (and the h264_mp4toannexb bsf currently relies on this). - Andreas
On 5/31/2020 4:34 PM, Andreas Rheinhardt wrote: > James Almer: >> On 5/31/2020 4:11 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> On 5/31/2020 2:58 PM, Andreas Rheinhardt wrote:>> (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. >>>> >>> The only way one can signal flushing to a bsf is by using av_bsf_flush. >>> Empty packets (data == NULL and no side_data) are considered eof. >> >> Yes, that's what i meant. The entire codebase uses flush, drain and EOF >> interchangeably in some cases. >> >>> Packets with data != NULL, size = 0 and no side data are currently >>> considered normal packets (a possible use-case would be to send some >>> timestamps to the bsf, although no bsf currently makes use of this; but >>> who knows what needs a future bsf has). >> >> data != NULL && size == 0 should IMO not be accepted. If we want to >> signal that we're feeding the bsf something like timestamps only, we'd >> have to find a different way to do it than setting pkt->data to some >> bogus value that's not NULL in order to bypass the IS_EMPTY() check. >> > Bogus value? You seem to believe that pkt->data just points to somewhere > in memory; in a valid packet it points to a buffer of size pkt->size + > AV_INPUT_BUFFER_PADDING_SIZE (even when pkt->size == 0). > av_packet_make_refcounted creates such packets if the input packet > doesn't have data By bogus i mean a value that has no meaning for the packet you'd be feeding the bsf with seeing it's "empty". It doesn't matter if it points to a real buffer of only input padding bytes size, it's still a pointer that will not be accessed because the packet size is reported as 0. If we want to signal a packet with something like metadata only, then IMO it should be done in a different manner. > (and the h264_mp4toannexb bsf currently relies on this). That bsf expects a packet with size 0? Why? Empty packets with no side data were not meant to be propagated.
James Almer: > On 5/31/2020 4:34 PM, Andreas Rheinhardt wrote: >> James Almer: >>> On 5/31/2020 4:11 PM, Andreas Rheinhardt wrote: >>>> James Almer: >>>>> On 5/31/2020 2:58 PM, Andreas Rheinhardt wrote:>> (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. >>>>> >>>> The only way one can signal flushing to a bsf is by using av_bsf_flush. >>>> Empty packets (data == NULL and no side_data) are considered eof. >>> >>> Yes, that's what i meant. The entire codebase uses flush, drain and EOF >>> interchangeably in some cases. >>> >>>> Packets with data != NULL, size = 0 and no side data are currently >>>> considered normal packets (a possible use-case would be to send some >>>> timestamps to the bsf, although no bsf currently makes use of this; but >>>> who knows what needs a future bsf has). >>> >>> data != NULL && size == 0 should IMO not be accepted. If we want to >>> signal that we're feeding the bsf something like timestamps only, we'd >>> have to find a different way to do it than setting pkt->data to some >>> bogus value that's not NULL in order to bypass the IS_EMPTY() check. >>> >> Bogus value? You seem to believe that pkt->data just points to somewhere >> in memory; in a valid packet it points to a buffer of size pkt->size + >> AV_INPUT_BUFFER_PADDING_SIZE (even when pkt->size == 0). >> av_packet_make_refcounted creates such packets if the input packet >> doesn't have data > > By bogus i mean a value that has no meaning for the packet you'd be > feeding the bsf with seeing it's "empty". It doesn't matter if it points > to a real buffer of only input padding bytes size, it's still a pointer > that will not be accessed because the packet size is reported as 0. > If we want to signal a packet with something like metadata only, then > IMO it should be done in a different manner. > >> (and the h264_mp4toannexb bsf currently relies on this). > > That bsf expects a packet with size 0? Why? Empty packets with no side > data were not meant to be propagated. It relies on pkt->data always pointing to a buffer that has at least four bytes of padding at the end. - Andreas
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 /**
Signed-off-by: James Almer <jamrial@gmail.com> --- doc/APIchanges | 4 ++-- libavcodec/avpacket.c | 49 +++++++++++++++++++++++++++++++++++++++++++ libavcodec/packet.h | 45 +++++++++++++++++++++++++++++++++++++++ libavutil/frame.h | 4 ++++ 4 files changed, 100 insertions(+), 2 deletions(-)