diff mbox series

[FFmpeg-devel,3/3] avcodec/packet: change public function and struct size parameter types to size_t

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
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

James Almer May 31, 2020, 4:38 p.m. UTC
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(-)

Comments

Andreas Rheinhardt May 31, 2020, 5:58 p.m. UTC | #1
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
James Almer May 31, 2020, 6:35 p.m. UTC | #2
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".
>
James Almer May 31, 2020, 6:50 p.m. UTC | #3
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.
Andreas Rheinhardt May 31, 2020, 7:11 p.m. UTC | #4
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
James Almer May 31, 2020, 7:27 p.m. UTC | #5
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".
>
Andreas Rheinhardt May 31, 2020, 7:34 p.m. UTC | #6
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
James Almer May 31, 2020, 7:45 p.m. UTC | #7
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.
Andreas Rheinhardt May 31, 2020, 7:50 p.m. UTC | #8
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 mbox series

Patch

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
     /**