[FFmpeg-devel,1/3] avcodec/avpacket: add av_packet_resize()

Message ID	20210311155011.52961-1-jamrial@gmail.com
State	New
Headers	show Return-Path: <ffmpeg-devel-bounces@ffmpeg.org> From: James Almer <jamrial@gmail.com> To: ffmpeg-devel@ffmpeg.org Date: Thu, 11 Mar 2021 12:50:09 -0300 Message-Id: <20210311155011.52961-1-jamrial@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 1/3] avcodec/avpacket: add av_packet_resize() Precedence: list Reply-To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" <ffmpeg-devel-bounces@ffmpeg.org>
Series	[FFmpeg-devel,1/3] avcodec/avpacket: add av_packet_resize() \| expand [FFmpeg-devel,1/3] avcodec/avpacket: add av_packet_resize() [FFmpeg-devel,2/3] avcodec: replace all uses of av_shrink_packet() with av_packet_resize() [FFmpeg-devel,3/3] avformat: replace all uses of av_shrink_packet() with av_packet_resize()

James Almer March 11, 2021, 3:50 p.m. UTC

This function acts as a replacement for both av_grow_packet() and
av_shrink_packet(), the latter which is now deprecated and will be removed as
it does not correctly handle non-writable packets.

Signed-off-by: James Almer <jamrial@gmail.com>
---
 libavcodec/avpacket.c | 19 +++++++++++++++----
 libavcodec/packet.h   | 16 ++++++++++++++++
 libavcodec/version.h  |  3 +++
 3 files changed, 34 insertions(+), 4 deletions(-)

Marton Balint March 11, 2021, 4:11 p.m. UTC | #1

On Thu, 11 Mar 2021, James Almer wrote:

> This function acts as a replacement for both av_grow_packet() and
> av_shrink_packet(), the latter which is now deprecated and will be removed as
> it does not correctly handle non-writable packets.

I don't think this is a good idea, av_shrink_packet cannot fail, 
av_grow_packet can. By using the same function you are losing the 
information if the end result should be checked or not.

Regards,
Marton

>
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> libavcodec/avpacket.c | 19 +++++++++++++++----
> libavcodec/packet.h   | 16 ++++++++++++++++
> libavcodec/version.h  |  3 +++
> 3 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
> index 32cb71fcf0..7d0dbadbed 100644
> --- a/libavcodec/avpacket.c
> +++ b/libavcodec/avpacket.c
> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>     return 0;
> }
> 
> +#if FF_API_SHRINK_PACKET
> void av_shrink_packet(AVPacket *pkt, int size)
> {
>     if (pkt->size <= size)
> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int size)
>     pkt->size = size;
>     memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
> }
> +#endif
> 
> int av_grow_packet(AVPacket *pkt, int grow_by)
> {
> -    int new_size;
>     av_assert0((unsigned)pkt->size <= INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE);
>     if ((unsigned)grow_by >
>         INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>         return AVERROR(ENOMEM);
> 
> -    new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
> +    return av_packet_resize(pkt, pkt->size + grow_by);
> +}
> +
> +int av_packet_resize(AVPacket *pkt, int size)
> +{
> +    int new_size;
> +
> +    if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
> +        return AVERROR(EINVAL);
> +
> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>     if (pkt->buf) {
>         size_t data_offset;
>         uint8_t *old_data = pkt->data;
> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>         if (!pkt->buf)
>             return AVERROR(ENOMEM);
>         if (pkt->size > 0)
> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size, size));
>         pkt->data = pkt->buf->data;
>     }
> -    pkt->size += grow_by;
> +    pkt->size = size;
>     memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>
>     return 0;
> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
> index 3d9013d783..1720d973f5 100644
> --- a/libavcodec/packet.h
> +++ b/libavcodec/packet.h
> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>  */
> int av_new_packet(AVPacket *pkt, int size);
> 
> +#if FF_API_SHRINK_PACKET
> /**
>  * Reduce packet size, correctly zeroing padding
>  *
>  * @param pkt packet
>  * @param size new size
> + *
> + * @deprecated Use av_packet_resize
>  */
> +attribute_deprecated
> void av_shrink_packet(AVPacket *pkt, int size);
> +#endif
> +
> +/**
> + * Resize the payload of a packet, correctly zeroing padding and avoiding data
> + * copy if possible.
> + *
> + * @param pkt packet
> + * @param size new size
> + *
> + * @return 0 on success, a negative AVERROR on error
> + */
> +int av_packet_resize(AVPacket *pkt, int size);
> 
> /**
>  * Increase packet size, correctly zeroing padding
> diff --git a/libavcodec/version.h b/libavcodec/version.h
> index 3124ec8061..6c362b43e2 100644
> --- a/libavcodec/version.h
> +++ b/libavcodec/version.h
> @@ -162,5 +162,8 @@
> #ifndef FF_API_GET_FRAME_CLASS
> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
> #endif
> +#ifndef FF_API_SHRINK_PACKET
> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR < 60)
> +#endif
> 
> #endif /* AVCODEC_VERSION_H */
> -- 
> 2.30.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@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 March 11, 2021, 4:38 p.m. UTC | #2

On 3/11/2021 1:11 PM, Marton Balint wrote:
> 
> 
> On Thu, 11 Mar 2021, James Almer wrote:
> 
>> This function acts as a replacement for both av_grow_packet() and
>> av_shrink_packet(), the latter which is now deprecated and will be 
>> removed as
>> it does not correctly handle non-writable packets.
> 
> I don't think this is a good idea, av_shrink_packet cannot fail, 
> av_grow_packet can. By using the same function you are losing the 
> information if the end result should be checked or not.

I'm not sure i follow. av_shrink_packet() is not being changed at all, 
just deprecated, scheduled for removal, and its use discouraged.
Maybe i should have split this in two, one to add av_packet_resize() and 
one to deprecate av_shrink_packet(), to avoid confusion.

In any case, the fact av_shrink_packet() cannot fail is the reason I'm 
getting rid of it. It's zeroing the padding without bothering to check 
if the packet is writable at all. And we can't have it attempt to make 
it writable because it can't then report if it failed to reallocate the 
buffer. So this patch here deprecates it for being a function that 
predates reference counted buffers and is not able to properly handle 
them, and adds a replacement for it that also supersedes 
av_grow_packet() while at it.

> 
> Regards,
> Marton
> 
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> libavcodec/avpacket.c | 19 +++++++++++++++----
>> libavcodec/packet.h   | 16 ++++++++++++++++
>> libavcodec/version.h  |  3 +++
>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>> index 32cb71fcf0..7d0dbadbed 100644
>> --- a/libavcodec/avpacket.c
>> +++ b/libavcodec/avpacket.c
>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>     return 0;
>> }
>>
>> +#if FF_API_SHRINK_PACKET
>> void av_shrink_packet(AVPacket *pkt, int size)
>> {
>>     if (pkt->size <= size)
>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int size)
>>     pkt->size = size;
>>     memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>> }
>> +#endif
>>
>> int av_grow_packet(AVPacket *pkt, int grow_by)
>> {
>> -    int new_size;
>>     av_assert0((unsigned)pkt->size <= INT_MAX - 
>> AV_INPUT_BUFFER_PADDING_SIZE);
>>     if ((unsigned)grow_by >
>>         INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>>         return AVERROR(ENOMEM);
>>
>> -    new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>> +}
>> +
>> +int av_packet_resize(AVPacket *pkt, int size)
>> +{
>> +    int new_size;
>> +
>> +    if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>> +        return AVERROR(EINVAL);
>> +
>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>     if (pkt->buf) {
>>         size_t data_offset;
>>         uint8_t *old_data = pkt->data;
>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>>         if (!pkt->buf)
>>             return AVERROR(ENOMEM);
>>         if (pkt->size > 0)
>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size, size));
>>         pkt->data = pkt->buf->data;
>>     }
>> -    pkt->size += grow_by;
>> +    pkt->size = size;
>>     memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>
>>     return 0;
>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>> index 3d9013d783..1720d973f5 100644
>> --- a/libavcodec/packet.h
>> +++ b/libavcodec/packet.h
>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>  */
>> int av_new_packet(AVPacket *pkt, int size);
>>
>> +#if FF_API_SHRINK_PACKET
>> /**
>>  * Reduce packet size, correctly zeroing padding
>>  *
>>  * @param pkt packet
>>  * @param size new size
>> + *
>> + * @deprecated Use av_packet_resize
>>  */
>> +attribute_deprecated
>> void av_shrink_packet(AVPacket *pkt, int size);
>> +#endif
>> +
>> +/**
>> + * Resize the payload of a packet, correctly zeroing padding and 
>> avoiding data
>> + * copy if possible.
>> + *
>> + * @param pkt packet
>> + * @param size new size
>> + *
>> + * @return 0 on success, a negative AVERROR on error
>> + */
>> +int av_packet_resize(AVPacket *pkt, int size);
>>
>> /**
>>  * Increase packet size, correctly zeroing padding
>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>> index 3124ec8061..6c362b43e2 100644
>> --- a/libavcodec/version.h
>> +++ b/libavcodec/version.h
>> @@ -162,5 +162,8 @@
>> #ifndef FF_API_GET_FRAME_CLASS
>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
>> #endif
>> +#ifndef FF_API_SHRINK_PACKET
>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR < 60)
>> +#endif
>>
>> #endif /* AVCODEC_VERSION_H */
>> -- 
>> 2.30.2
>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
> _______________________________________________
> 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 March 11, 2021, 4:56 p.m. UTC | #3

James Almer:
> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>
>>
>> On Thu, 11 Mar 2021, James Almer wrote:
>>
>>> This function acts as a replacement for both av_grow_packet() and
>>> av_shrink_packet(), the latter which is now deprecated and will be
>>> removed as
>>> it does not correctly handle non-writable packets.
>>
>> I don't think this is a good idea, av_shrink_packet cannot fail,
>> av_grow_packet can. By using the same function you are losing the
>> information if the end result should be checked or not.
> 
> I'm not sure i follow. av_shrink_packet() is not being changed at all,
> just deprecated, scheduled for removal, and its use discouraged.

I'd argue that a deprecation is actually a change.

> Maybe i should have split this in two, one to add av_packet_resize() and
> one to deprecate av_shrink_packet(), to avoid confusion.
> 
> In any case, the fact av_shrink_packet() cannot fail is the reason I'm
> getting rid of it. It's zeroing the padding without bothering to check
> if the packet is writable at all. And we can't have it attempt to make
> it writable because it can't then report if it failed to reallocate the
> buffer. So this patch here deprecates it for being a function that
> predates reference counted buffers and is not able to properly handle
> them, and adds a replacement for it that also supersedes
> av_grow_packet() while at it.
> 

Yet you are not documenting that av_packet_resize can't fail if it is
shrinking a packet known to be writable; ergo all unchecked uses of this
function in the second and third patch are API abuse.

>>
>> Regards,
>> Marton
>>
>>>
>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>> ---
>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>> libavcodec/version.h  |  3 +++
>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>> index 32cb71fcf0..7d0dbadbed 100644
>>> --- a/libavcodec/avpacket.c
>>> +++ b/libavcodec/avpacket.c
>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>     return 0;
>>> }
>>>
>>> +#if FF_API_SHRINK_PACKET
>>> void av_shrink_packet(AVPacket *pkt, int size)
>>> {
>>>     if (pkt->size <= size)
>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int size)
>>>     pkt->size = size;
>>>     memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>> }
>>> +#endif
>>>
>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>> {
>>> -    int new_size;
>>>     av_assert0((unsigned)pkt->size <= INT_MAX -
>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>     if ((unsigned)grow_by >
>>>         INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>>>         return AVERROR(ENOMEM);
>>>
>>> -    new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>> +}
>>> +
>>> +int av_packet_resize(AVPacket *pkt, int size)
>>> +{
>>> +    int new_size;
>>> +
>>> +    if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>>> +        return AVERROR(EINVAL);
>>> +
>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>     if (pkt->buf) {
>>>         size_t data_offset;
>>>         uint8_t *old_data = pkt->data;
>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>>>         if (!pkt->buf)
>>>             return AVERROR(ENOMEM);
>>>         if (pkt->size > 0)
>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size, size));
>>>         pkt->data = pkt->buf->data;
>>>     }
>>> -    pkt->size += grow_by;
>>> +    pkt->size = size;
>>>     memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>
>>>     return 0;
>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>> index 3d9013d783..1720d973f5 100644
>>> --- a/libavcodec/packet.h
>>> +++ b/libavcodec/packet.h
>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>  */
>>> int av_new_packet(AVPacket *pkt, int size);
>>>
>>> +#if FF_API_SHRINK_PACKET
>>> /**
>>>  * Reduce packet size, correctly zeroing padding
>>>  *
>>>  * @param pkt packet
>>>  * @param size new size
>>> + *
>>> + * @deprecated Use av_packet_resize
>>>  */
>>> +attribute_deprecated
>>> void av_shrink_packet(AVPacket *pkt, int size);
>>> +#endif
>>> +
>>> +/**
>>> + * Resize the payload of a packet, correctly zeroing padding and
>>> avoiding data
>>> + * copy if possible.
>>> + *
>>> + * @param pkt packet
>>> + * @param size new size
>>> + *
>>> + * @return 0 on success, a negative AVERROR on error
>>> + */
>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>
>>> /**
>>>  * Increase packet size, correctly zeroing padding
>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>> index 3124ec8061..6c362b43e2 100644
>>> --- a/libavcodec/version.h
>>> +++ b/libavcodec/version.h
>>> @@ -162,5 +162,8 @@
>>> #ifndef FF_API_GET_FRAME_CLASS
>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
>>> #endif
>>> +#ifndef FF_API_SHRINK_PACKET
>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR < 60)
>>> +#endif
>>>
>>> #endif /* AVCODEC_VERSION_H */
>>> -- 
>>> 2.30.2
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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 March 11, 2021, 5 p.m. UTC | #4

On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>
>>>
>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>
>>>> This function acts as a replacement for both av_grow_packet() and
>>>> av_shrink_packet(), the latter which is now deprecated and will be
>>>> removed as
>>>> it does not correctly handle non-writable packets.
>>>
>>> I don't think this is a good idea, av_shrink_packet cannot fail,
>>> av_grow_packet can. By using the same function you are losing the
>>> information if the end result should be checked or not.
>>
>> I'm not sure i follow. av_shrink_packet() is not being changed at all,
>> just deprecated, scheduled for removal, and its use discouraged.
> 
> I'd argue that a deprecation is actually a change.
> 
>> Maybe i should have split this in two, one to add av_packet_resize() and
>> one to deprecate av_shrink_packet(), to avoid confusion.
>>
>> In any case, the fact av_shrink_packet() cannot fail is the reason I'm
>> getting rid of it. It's zeroing the padding without bothering to check
>> if the packet is writable at all. And we can't have it attempt to make
>> it writable because it can't then report if it failed to reallocate the
>> buffer. So this patch here deprecates it for being a function that
>> predates reference counted buffers and is not able to properly handle
>> them, and adds a replacement for it that also supersedes
>> av_grow_packet() while at it.
>>
> 
> Yet you are not documenting that av_packet_resize can't fail if it is
> shrinking a packet known to be writable; ergo all unchecked uses of this
> function in the second and third patch are API abuse.

I can add checks for all of them if you prefer. I didn't because they 
are all internal uses known to (in theory) not fail.

> 
>>>
>>> Regards,
>>> Marton
>>>
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>> libavcodec/version.h  |  3 +++
>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>> --- a/libavcodec/avpacket.c
>>>> +++ b/libavcodec/avpacket.c
>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>      return 0;
>>>> }
>>>>
>>>> +#if FF_API_SHRINK_PACKET
>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>> {
>>>>      if (pkt->size <= size)
>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int size)
>>>>      pkt->size = size;
>>>>      memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>> }
>>>> +#endif
>>>>
>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>> {
>>>> -    int new_size;
>>>>      av_assert0((unsigned)pkt->size <= INT_MAX -
>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>      if ((unsigned)grow_by >
>>>>          INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>>>>          return AVERROR(ENOMEM);
>>>>
>>>> -    new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>> +}
>>>> +
>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>> +{
>>>> +    int new_size;
>>>> +
>>>> +    if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>>>> +        return AVERROR(EINVAL);
>>>> +
>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>      if (pkt->buf) {
>>>>          size_t data_offset;
>>>>          uint8_t *old_data = pkt->data;
>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>          if (!pkt->buf)
>>>>              return AVERROR(ENOMEM);
>>>>          if (pkt->size > 0)
>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size, size));
>>>>          pkt->data = pkt->buf->data;
>>>>      }
>>>> -    pkt->size += grow_by;
>>>> +    pkt->size = size;
>>>>      memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>
>>>>      return 0;
>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>> index 3d9013d783..1720d973f5 100644
>>>> --- a/libavcodec/packet.h
>>>> +++ b/libavcodec/packet.h
>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>   */
>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>
>>>> +#if FF_API_SHRINK_PACKET
>>>> /**
>>>>   * Reduce packet size, correctly zeroing padding
>>>>   *
>>>>   * @param pkt packet
>>>>   * @param size new size
>>>> + *
>>>> + * @deprecated Use av_packet_resize
>>>>   */
>>>> +attribute_deprecated
>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>> +#endif
>>>> +
>>>> +/**
>>>> + * Resize the payload of a packet, correctly zeroing padding and
>>>> avoiding data
>>>> + * copy if possible.
>>>> + *
>>>> + * @param pkt packet
>>>> + * @param size new size
>>>> + *
>>>> + * @return 0 on success, a negative AVERROR on error
>>>> + */
>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>
>>>> /**
>>>>   * Increase packet size, correctly zeroing padding
>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>> index 3124ec8061..6c362b43e2 100644
>>>> --- a/libavcodec/version.h
>>>> +++ b/libavcodec/version.h
>>>> @@ -162,5 +162,8 @@
>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
>>>> #endif
>>>> +#ifndef FF_API_SHRINK_PACKET
>>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR < 60)
>>>> +#endif
>>>>
>>>> #endif /* AVCODEC_VERSION_H */
>>>> -- 
>>>> 2.30.2
>>>>
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>> To unsubscribe, visit link above, or email
>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>> _______________________________________________
>>> 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".
>>
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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 March 11, 2021, 5:27 p.m. UTC | #5

James Almer:
> On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>
>>>>
>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>
>>>>> This function acts as a replacement for both av_grow_packet() and
>>>>> av_shrink_packet(), the latter which is now deprecated and will be
>>>>> removed as
>>>>> it does not correctly handle non-writable packets.
>>>>
>>>> I don't think this is a good idea, av_shrink_packet cannot fail,
>>>> av_grow_packet can. By using the same function you are losing the
>>>> information if the end result should be checked or not.
>>>
>>> I'm not sure i follow. av_shrink_packet() is not being changed at all,
>>> just deprecated, scheduled for removal, and its use discouraged.
>>
>> I'd argue that a deprecation is actually a change.
>>
>>> Maybe i should have split this in two, one to add av_packet_resize() and
>>> one to deprecate av_shrink_packet(), to avoid confusion.
>>>
>>> In any case, the fact av_shrink_packet() cannot fail is the reason I'm
>>> getting rid of it. It's zeroing the padding without bothering to check
>>> if the packet is writable at all. And we can't have it attempt to make
>>> it writable because it can't then report if it failed to reallocate the
>>> buffer. So this patch here deprecates it for being a function that
>>> predates reference counted buffers and is not able to properly handle
>>> them, and adds a replacement for it that also supersedes
>>> av_grow_packet() while at it.
>>>
>>
>> Yet you are not documenting that av_packet_resize can't fail if it is
>> shrinking a packet known to be writable; ergo all unchecked uses of this
>> function in the second and third patch are API abuse.
> 
> I can add checks for all of them if you prefer. I didn't because they
> are all internal uses known to (in theory) not fail.
> 

No, I don't prefer checks for stuff that can't fail. I'd rather prefer
if it were documented that it can't fail in these cases.

>>
>>>>
>>>> Regards,
>>>> Marton
>>>>
>>>>>
>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>> ---
>>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>>> libavcodec/version.h  |  3 +++
>>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>>> --- a/libavcodec/avpacket.c
>>>>> +++ b/libavcodec/avpacket.c
>>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>>      return 0;
>>>>> }
>>>>>
>>>>> +#if FF_API_SHRINK_PACKET
>>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>>> {
>>>>>      if (pkt->size <= size)
>>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int size)
>>>>>      pkt->size = size;
>>>>>      memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>> }
>>>>> +#endif
>>>>>
>>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>> {
>>>>> -    int new_size;
>>>>>      av_assert0((unsigned)pkt->size <= INT_MAX -
>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>      if ((unsigned)grow_by >
>>>>>          INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>>>>>          return AVERROR(ENOMEM);
>>>>>
>>>>> -    new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>>> +}
>>>>> +
>>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>>> +{
>>>>> +    int new_size;
>>>>> +
>>>>> +    if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>>>>> +        return AVERROR(EINVAL);
>>>>> +
>>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>      if (pkt->buf) {
>>>>>          size_t data_offset;
>>>>>          uint8_t *old_data = pkt->data;
>>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>          if (!pkt->buf)
>>>>>              return AVERROR(ENOMEM);
>>>>>          if (pkt->size > 0)
>>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size,
>>>>> size));
>>>>>          pkt->data = pkt->buf->data;
>>>>>      }
>>>>> -    pkt->size += grow_by;
>>>>> +    pkt->size = size;
>>>>>      memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>
>>>>>      return 0;
>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>> index 3d9013d783..1720d973f5 100644
>>>>> --- a/libavcodec/packet.h
>>>>> +++ b/libavcodec/packet.h
>>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>>   */
>>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>>
>>>>> +#if FF_API_SHRINK_PACKET
>>>>> /**
>>>>>   * Reduce packet size, correctly zeroing padding
>>>>>   *
>>>>>   * @param pkt packet
>>>>>   * @param size new size
>>>>> + *
>>>>> + * @deprecated Use av_packet_resize
>>>>>   */
>>>>> +attribute_deprecated
>>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>>> +#endif
>>>>> +
>>>>> +/**
>>>>> + * Resize the payload of a packet, correctly zeroing padding and
>>>>> avoiding data
>>>>> + * copy if possible.
>>>>> + *
>>>>> + * @param pkt packet
>>>>> + * @param size new size
>>>>> + *
>>>>> + * @return 0 on success, a negative AVERROR on error
>>>>> + */
>>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>>
>>>>> /**
>>>>>   * Increase packet size, correctly zeroing padding
>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>> index 3124ec8061..6c362b43e2 100644
>>>>> --- a/libavcodec/version.h
>>>>> +++ b/libavcodec/version.h
>>>>> @@ -162,5 +162,8 @@
>>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>> #endif
>>>>> +#ifndef FF_API_SHRINK_PACKET
>>>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>> +#endif
>>>>>
>>>>> #endif /* AVCODEC_VERSION_H */
>>>>> -- 
>>>>> 2.30.2
>>>>>
>>>>> _______________________________________________
>>>>> ffmpeg-devel mailing list
>>>>> ffmpeg-devel@ffmpeg.org
>>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>
>>>>> To unsubscribe, visit link above, or email
>>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>> _______________________________________________
>>>> 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".
>>>
>>> _______________________________________________
>>> 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".
>>
>> _______________________________________________
>> 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".
>>
> 
> _______________________________________________
> 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 March 11, 2021, 5:44 p.m. UTC | #6

On 3/11/2021 2:27 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>>
>>>>>
>>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>>
>>>>>> This function acts as a replacement for both av_grow_packet() and
>>>>>> av_shrink_packet(), the latter which is now deprecated and will be
>>>>>> removed as
>>>>>> it does not correctly handle non-writable packets.
>>>>>
>>>>> I don't think this is a good idea, av_shrink_packet cannot fail,
>>>>> av_grow_packet can. By using the same function you are losing the
>>>>> information if the end result should be checked or not.
>>>>
>>>> I'm not sure i follow. av_shrink_packet() is not being changed at all,
>>>> just deprecated, scheduled for removal, and its use discouraged.
>>>
>>> I'd argue that a deprecation is actually a change.
>>>
>>>> Maybe i should have split this in two, one to add av_packet_resize() and
>>>> one to deprecate av_shrink_packet(), to avoid confusion.
>>>>
>>>> In any case, the fact av_shrink_packet() cannot fail is the reason I'm
>>>> getting rid of it. It's zeroing the padding without bothering to check
>>>> if the packet is writable at all. And we can't have it attempt to make
>>>> it writable because it can't then report if it failed to reallocate the
>>>> buffer. So this patch here deprecates it for being a function that
>>>> predates reference counted buffers and is not able to properly handle
>>>> them, and adds a replacement for it that also supersedes
>>>> av_grow_packet() while at it.
>>>>
>>>
>>> Yet you are not documenting that av_packet_resize can't fail if it is
>>> shrinking a packet known to be writable; ergo all unchecked uses of this
>>> function in the second and third patch are API abuse.
>>
>> I can add checks for all of them if you prefer. I didn't because they
>> are all internal uses known to (in theory) not fail.
>>
> 
> No, I don't prefer checks for stuff that can't fail. I'd rather prefer
> if it were documented that it can't fail in these cases.

I'm in general against adding that kind of constrain on a function's 
documentation because you never know how it or what it processes could 
be extended in the future.
Right now it can't fail in that scenario, true, but in the future we 
could add some feature to AVPacket that would need to be handled by this 
function that could start making it fail in that same scenario, and 
suddenly, the doxy is no longer correct, and the function needs to be 
replaced because it became unable to handle the new functionality.

If a function can fail at all, then the library user should always make 
sure to check the return value, and not be told there's one specific 
scenario where they don't need to.

> 
>>>
>>>>>
>>>>> Regards,
>>>>> Marton
>>>>>
>>>>>>
>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>> ---
>>>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>>>> libavcodec/version.h  |  3 +++
>>>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>>>> --- a/libavcodec/avpacket.c
>>>>>> +++ b/libavcodec/avpacket.c
>>>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>>>       return 0;
>>>>>> }
>>>>>>
>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>>>> {
>>>>>>       if (pkt->size <= size)
>>>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>       pkt->size = size;
>>>>>>       memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>> }
>>>>>> +#endif
>>>>>>
>>>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>> {
>>>>>> -    int new_size;
>>>>>>       av_assert0((unsigned)pkt->size <= INT_MAX -
>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>       if ((unsigned)grow_by >
>>>>>>           INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>>>>>>           return AVERROR(ENOMEM);
>>>>>>
>>>>>> -    new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>>>> +}
>>>>>> +
>>>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>>>> +{
>>>>>> +    int new_size;
>>>>>> +
>>>>>> +    if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>>>>>> +        return AVERROR(EINVAL);
>>>>>> +
>>>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>       if (pkt->buf) {
>>>>>>           size_t data_offset;
>>>>>>           uint8_t *old_data = pkt->data;
>>>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>           if (!pkt->buf)
>>>>>>               return AVERROR(ENOMEM);
>>>>>>           if (pkt->size > 0)
>>>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size,
>>>>>> size));
>>>>>>           pkt->data = pkt->buf->data;
>>>>>>       }
>>>>>> -    pkt->size += grow_by;
>>>>>> +    pkt->size = size;
>>>>>>       memset(pkt->data + pkt->size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>
>>>>>>       return 0;
>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>>> index 3d9013d783..1720d973f5 100644
>>>>>> --- a/libavcodec/packet.h
>>>>>> +++ b/libavcodec/packet.h
>>>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>>>    */
>>>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>>>
>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>> /**
>>>>>>    * Reduce packet size, correctly zeroing padding
>>>>>>    *
>>>>>>    * @param pkt packet
>>>>>>    * @param size new size
>>>>>> + *
>>>>>> + * @deprecated Use av_packet_resize
>>>>>>    */
>>>>>> +attribute_deprecated
>>>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>>>> +#endif
>>>>>> +
>>>>>> +/**
>>>>>> + * Resize the payload of a packet, correctly zeroing padding and
>>>>>> avoiding data
>>>>>> + * copy if possible.
>>>>>> + *
>>>>>> + * @param pkt packet
>>>>>> + * @param size new size
>>>>>> + *
>>>>>> + * @return 0 on success, a negative AVERROR on error
>>>>>> + */
>>>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>>>
>>>>>> /**
>>>>>>    * Increase packet size, correctly zeroing padding
>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>> index 3124ec8061..6c362b43e2 100644
>>>>>> --- a/libavcodec/version.h
>>>>>> +++ b/libavcodec/version.h
>>>>>> @@ -162,5 +162,8 @@
>>>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>>> #endif
>>>>>> +#ifndef FF_API_SHRINK_PACKET
>>>>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>>> +#endif
>>>>>>
>>>>>> #endif /* AVCODEC_VERSION_H */
>>>>>> -- 
>>>>>> 2.30.2
>>>>>>
>>>>>> _______________________________________________
>>>>>> ffmpeg-devel mailing list
>>>>>> ffmpeg-devel@ffmpeg.org
>>>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>>>
>>>>>> To unsubscribe, visit link above, or email
>>>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>>> _______________________________________________
>>>>> 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".
>>>>
>>>> _______________________________________________
>>>> 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".
>>>
>>> _______________________________________________
>>> 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".
>>>
>>
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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 March 11, 2021, 5:48 p.m. UTC | #7

James Almer:
> On 3/11/2021 2:27 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>>>
>>>>>>
>>>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>>>
>>>>>>> This function acts as a replacement for both av_grow_packet() and
>>>>>>> av_shrink_packet(), the latter which is now deprecated and will be
>>>>>>> removed as
>>>>>>> it does not correctly handle non-writable packets.
>>>>>>
>>>>>> I don't think this is a good idea, av_shrink_packet cannot fail,
>>>>>> av_grow_packet can. By using the same function you are losing the
>>>>>> information if the end result should be checked or not.
>>>>>
>>>>> I'm not sure i follow. av_shrink_packet() is not being changed at all,
>>>>> just deprecated, scheduled for removal, and its use discouraged.
>>>>
>>>> I'd argue that a deprecation is actually a change.
>>>>
>>>>> Maybe i should have split this in two, one to add
>>>>> av_packet_resize() and
>>>>> one to deprecate av_shrink_packet(), to avoid confusion.
>>>>>
>>>>> In any case, the fact av_shrink_packet() cannot fail is the reason I'm
>>>>> getting rid of it. It's zeroing the padding without bothering to check
>>>>> if the packet is writable at all. And we can't have it attempt to make
>>>>> it writable because it can't then report if it failed to reallocate
>>>>> the
>>>>> buffer. So this patch here deprecates it for being a function that
>>>>> predates reference counted buffers and is not able to properly handle
>>>>> them, and adds a replacement for it that also supersedes
>>>>> av_grow_packet() while at it.
>>>>>
>>>>
>>>> Yet you are not documenting that av_packet_resize can't fail if it is
>>>> shrinking a packet known to be writable; ergo all unchecked uses of
>>>> this
>>>> function in the second and third patch are API abuse.
>>>
>>> I can add checks for all of them if you prefer. I didn't because they
>>> are all internal uses known to (in theory) not fail.
>>>
>>
>> No, I don't prefer checks for stuff that can't fail. I'd rather prefer
>> if it were documented that it can't fail in these cases.
> 
> I'm in general against adding that kind of constrain on a function's
> documentation because you never know how it or what it processes could
> be extended in the future.
> Right now it can't fail in that scenario, true, but in the future we
> could add some feature to AVPacket that would need to be handled by this
> function that could start making it fail in that same scenario, and
> suddenly, the doxy is no longer correct, and the function needs to be
> replaced because it became unable to handle the new functionality.
> 
> If a function can fail at all, then the library user should always make
> sure to check the return value, and not be told there's one specific
> scenario where they don't need to.
> 

In this case I am against this patch.

>>
>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Marton
>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>> ---
>>>>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>>>>> libavcodec/version.h  |  3 +++
>>>>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>>>>       return 0;
>>>>>>> }
>>>>>>>
>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>> {
>>>>>>>       if (pkt->size <= size)
>>>>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>>       pkt->size = size;
>>>>>>>       memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>> }
>>>>>>> +#endif
>>>>>>>
>>>>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>> {
>>>>>>> -    int new_size;
>>>>>>>       av_assert0((unsigned)pkt->size <= INT_MAX -
>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>       if ((unsigned)grow_by >
>>>>>>>           INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>>>>>>>           return AVERROR(ENOMEM);
>>>>>>>
>>>>>>> -    new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>>>>> +}
>>>>>>> +
>>>>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>>>>> +{
>>>>>>> +    int new_size;
>>>>>>> +
>>>>>>> +    if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>>>>>>> +        return AVERROR(EINVAL);
>>>>>>> +
>>>>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>       if (pkt->buf) {
>>>>>>>           size_t data_offset;
>>>>>>>           uint8_t *old_data = pkt->data;
>>>>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>>           if (!pkt->buf)
>>>>>>>               return AVERROR(ENOMEM);
>>>>>>>           if (pkt->size > 0)
>>>>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>>>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size,
>>>>>>> size));
>>>>>>>           pkt->data = pkt->buf->data;
>>>>>>>       }
>>>>>>> -    pkt->size += grow_by;
>>>>>>> +    pkt->size = size;
>>>>>>>       memset(pkt->data + pkt->size, 0,
>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>
>>>>>>>       return 0;
>>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>>>> index 3d9013d783..1720d973f5 100644
>>>>>>> --- a/libavcodec/packet.h
>>>>>>> +++ b/libavcodec/packet.h
>>>>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>>>>    */
>>>>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>>>>
>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>> /**
>>>>>>>    * Reduce packet size, correctly zeroing padding
>>>>>>>    *
>>>>>>>    * @param pkt packet
>>>>>>>    * @param size new size
>>>>>>> + *
>>>>>>> + * @deprecated Use av_packet_resize
>>>>>>>    */
>>>>>>> +attribute_deprecated
>>>>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>>>>> +#endif
>>>>>>> +
>>>>>>> +/**
>>>>>>> + * Resize the payload of a packet, correctly zeroing padding and
>>>>>>> avoiding data
>>>>>>> + * copy if possible.
>>>>>>> + *
>>>>>>> + * @param pkt packet
>>>>>>> + * @param size new size
>>>>>>> + *
>>>>>>> + * @return 0 on success, a negative AVERROR on error
>>>>>>> + */
>>>>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>>>>
>>>>>>> /**
>>>>>>>    * Increase packet size, correctly zeroing padding
>>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>>> index 3124ec8061..6c362b43e2 100644
>>>>>>> --- a/libavcodec/version.h
>>>>>>> +++ b/libavcodec/version.h
>>>>>>> @@ -162,5 +162,8 @@
>>>>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>>>> #endif
>>>>>>> +#ifndef FF_API_SHRINK_PACKET
>>>>>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>>>> +#endif
>>>>>>>
>>>>>>> #endif /* AVCODEC_VERSION_H */
>>>>>>> -- 
>>>>>>> 2.30.2
>>>>>>>

James Almer March 11, 2021, 6:05 p.m. UTC | #8

On 3/11/2021 2:48 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 3/11/2021 2:27 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>>>>
>>>>>>>
>>>>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>>>>
>>>>>>>> This function acts as a replacement for both av_grow_packet() and
>>>>>>>> av_shrink_packet(), the latter which is now deprecated and will be
>>>>>>>> removed as
>>>>>>>> it does not correctly handle non-writable packets.
>>>>>>>
>>>>>>> I don't think this is a good idea, av_shrink_packet cannot fail,
>>>>>>> av_grow_packet can. By using the same function you are losing the
>>>>>>> information if the end result should be checked or not.
>>>>>>
>>>>>> I'm not sure i follow. av_shrink_packet() is not being changed at all,
>>>>>> just deprecated, scheduled for removal, and its use discouraged.
>>>>>
>>>>> I'd argue that a deprecation is actually a change.
>>>>>
>>>>>> Maybe i should have split this in two, one to add
>>>>>> av_packet_resize() and
>>>>>> one to deprecate av_shrink_packet(), to avoid confusion.
>>>>>>
>>>>>> In any case, the fact av_shrink_packet() cannot fail is the reason I'm
>>>>>> getting rid of it. It's zeroing the padding without bothering to check
>>>>>> if the packet is writable at all. And we can't have it attempt to make
>>>>>> it writable because it can't then report if it failed to reallocate
>>>>>> the
>>>>>> buffer. So this patch here deprecates it for being a function that
>>>>>> predates reference counted buffers and is not able to properly handle
>>>>>> them, and adds a replacement for it that also supersedes
>>>>>> av_grow_packet() while at it.
>>>>>>
>>>>>
>>>>> Yet you are not documenting that av_packet_resize can't fail if it is
>>>>> shrinking a packet known to be writable; ergo all unchecked uses of
>>>>> this
>>>>> function in the second and third patch are API abuse.
>>>>
>>>> I can add checks for all of them if you prefer. I didn't because they
>>>> are all internal uses known to (in theory) not fail.
>>>>
>>>
>>> No, I don't prefer checks for stuff that can't fail. I'd rather prefer
>>> if it were documented that it can't fail in these cases.
>>
>> I'm in general against adding that kind of constrain on a function's
>> documentation because you never know how it or what it processes could
>> be extended in the future.
>> Right now it can't fail in that scenario, true, but in the future we
>> could add some feature to AVPacket that would need to be handled by this
>> function that could start making it fail in that same scenario, and
>> suddenly, the doxy is no longer correct, and the function needs to be
>> replaced because it became unable to handle the new functionality.
>>
>> If a function can fail at all, then the library user should always make
>> sure to check the return value, and not be told there's one specific
>> scenario where they don't need to.
>>
> 
> In this case I am against this patch.

To add to what explained said above, it's the entire reason making 
av_shrink_packet() return void was a mistake. It worked great before 
reference counted buffers, but it was short sighted and the function is 
now technically unusable after they were introduced, and why we need to 
replace it now.
This patchset wouldn't exist had it been designed to return an int.

What you're asking for is to make the exact same mistake again, and it 
could very well come to bite us in the future, again. It's definitely 
not a good reason to try and block this patch at all.

> 
>>>
>>>>>
>>>>>>>
>>>>>>> Regards,
>>>>>>> Marton
>>>>>>>
>>>>>>>>
>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>> ---
>>>>>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>>>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>>>>>> libavcodec/version.h  |  3 +++
>>>>>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>>>>>        return 0;
>>>>>>>> }
>>>>>>>>
>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>>> {
>>>>>>>>        if (pkt->size <= size)
>>>>>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>>>        pkt->size = size;
>>>>>>>>        memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>> }
>>>>>>>> +#endif
>>>>>>>>
>>>>>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>>> {
>>>>>>>> -    int new_size;
>>>>>>>>        av_assert0((unsigned)pkt->size <= INT_MAX -
>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>        if ((unsigned)grow_by >
>>>>>>>>            INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>>>>>>>>            return AVERROR(ENOMEM);
>>>>>>>>
>>>>>>>> -    new_size = pkt->size + grow_by + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>>>>>> +{
>>>>>>>> +    int new_size;
>>>>>>>> +
>>>>>>>> +    if (size < 0 || size > INT_MAX - AV_INPUT_BUFFER_PADDING_SIZE)
>>>>>>>> +        return AVERROR(EINVAL);
>>>>>>>> +
>>>>>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>        if (pkt->buf) {
>>>>>>>>            size_t data_offset;
>>>>>>>>            uint8_t *old_data = pkt->data;
>>>>>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>>>            if (!pkt->buf)
>>>>>>>>                return AVERROR(ENOMEM);
>>>>>>>>            if (pkt->size > 0)
>>>>>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>>>>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size,
>>>>>>>> size));
>>>>>>>>            pkt->data = pkt->buf->data;
>>>>>>>>        }
>>>>>>>> -    pkt->size += grow_by;
>>>>>>>> +    pkt->size = size;
>>>>>>>>        memset(pkt->data + pkt->size, 0,
>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>
>>>>>>>>        return 0;
>>>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>>>>> index 3d9013d783..1720d973f5 100644
>>>>>>>> --- a/libavcodec/packet.h
>>>>>>>> +++ b/libavcodec/packet.h
>>>>>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>>>>>     */
>>>>>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>>>>>
>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>> /**
>>>>>>>>     * Reduce packet size, correctly zeroing padding
>>>>>>>>     *
>>>>>>>>     * @param pkt packet
>>>>>>>>     * @param size new size
>>>>>>>> + *
>>>>>>>> + * @deprecated Use av_packet_resize
>>>>>>>>     */
>>>>>>>> +attribute_deprecated
>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>>>>>> +#endif
>>>>>>>> +
>>>>>>>> +/**
>>>>>>>> + * Resize the payload of a packet, correctly zeroing padding and
>>>>>>>> avoiding data
>>>>>>>> + * copy if possible.
>>>>>>>> + *
>>>>>>>> + * @param pkt packet
>>>>>>>> + * @param size new size
>>>>>>>> + *
>>>>>>>> + * @return 0 on success, a negative AVERROR on error
>>>>>>>> + */
>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>>>>>
>>>>>>>> /**
>>>>>>>>     * Increase packet size, correctly zeroing padding
>>>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>>>> index 3124ec8061..6c362b43e2 100644
>>>>>>>> --- a/libavcodec/version.h
>>>>>>>> +++ b/libavcodec/version.h
>>>>>>>> @@ -162,5 +162,8 @@
>>>>>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>>>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>>>>> #endif
>>>>>>>> +#ifndef FF_API_SHRINK_PACKET
>>>>>>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>>>>> +#endif
>>>>>>>>
>>>>>>>> #endif /* AVCODEC_VERSION_H */
>>>>>>>> -- 
>>>>>>>> 2.30.2
>>>>>>>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@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 March 11, 2021, 6:26 p.m. UTC | #9

James Almer:
> On 3/11/2021 2:48 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 3/11/2021 2:27 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
>>>>>> James Almer:
>>>>>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>>>>>
>>>>>>>>> This function acts as a replacement for both av_grow_packet() and
>>>>>>>>> av_shrink_packet(), the latter which is now deprecated and will be
>>>>>>>>> removed as
>>>>>>>>> it does not correctly handle non-writable packets.
>>>>>>>>
>>>>>>>> I don't think this is a good idea, av_shrink_packet cannot fail,
>>>>>>>> av_grow_packet can. By using the same function you are losing the
>>>>>>>> information if the end result should be checked or not.
>>>>>>>
>>>>>>> I'm not sure i follow. av_shrink_packet() is not being changed at
>>>>>>> all,
>>>>>>> just deprecated, scheduled for removal, and its use discouraged.
>>>>>>
>>>>>> I'd argue that a deprecation is actually a change.
>>>>>>
>>>>>>> Maybe i should have split this in two, one to add
>>>>>>> av_packet_resize() and
>>>>>>> one to deprecate av_shrink_packet(), to avoid confusion.
>>>>>>>
>>>>>>> In any case, the fact av_shrink_packet() cannot fail is the
>>>>>>> reason I'm
>>>>>>> getting rid of it. It's zeroing the padding without bothering to
>>>>>>> check
>>>>>>> if the packet is writable at all. And we can't have it attempt to
>>>>>>> make
>>>>>>> it writable because it can't then report if it failed to reallocate
>>>>>>> the
>>>>>>> buffer. So this patch here deprecates it for being a function that
>>>>>>> predates reference counted buffers and is not able to properly
>>>>>>> handle
>>>>>>> them, and adds a replacement for it that also supersedes
>>>>>>> av_grow_packet() while at it.
>>>>>>>
>>>>>>
>>>>>> Yet you are not documenting that av_packet_resize can't fail if it is
>>>>>> shrinking a packet known to be writable; ergo all unchecked uses of
>>>>>> this
>>>>>> function in the second and third patch are API abuse.
>>>>>
>>>>> I can add checks for all of them if you prefer. I didn't because they
>>>>> are all internal uses known to (in theory) not fail.
>>>>>
>>>>
>>>> No, I don't prefer checks for stuff that can't fail. I'd rather prefer
>>>> if it were documented that it can't fail in these cases.
>>>
>>> I'm in general against adding that kind of constrain on a function's
>>> documentation because you never know how it or what it processes could
>>> be extended in the future.
>>> Right now it can't fail in that scenario, true, but in the future we
>>> could add some feature to AVPacket that would need to be handled by this
>>> function that could start making it fail in that same scenario, and
>>> suddenly, the doxy is no longer correct, and the function needs to be
>>> replaced because it became unable to handle the new functionality.
>>>
>>> If a function can fail at all, then the library user should always make
>>> sure to check the return value, and not be told there's one specific
>>> scenario where they don't need to.
>>>
>>
>> In this case I am against this patch.
> 
> To add to what explained said above, it's the entire reason making
> av_shrink_packet() return void was a mistake. It worked great before
> reference counted buffers, but it was short sighted and the function is
> now technically unusable after they were introduced, and why we need to
> replace it now.
> This patchset wouldn't exist had it been designed to return an int.
> 

The function is very well useable, but only for its use-case of already
writable packets. (Not documenting this when the refcounted API was
introduced was an error; or not deprecating it and replacing it with a
function that explicitly says so.)

> What you're asking for is to make the exact same mistake again, and it
> could very well come to bite us in the future, again. It's definitely
> not a good reason to try and block this patch at all.
> av_shrink_packet has four lines, two of which do the actual work. It is
extremely simple and could nearly be inlined. If any future extension
requires adding something that can fail to this, then said future
extension sounds like a huge step backwards.

>>
>>>>
>>>>>>
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>> Marton
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>>> ---
>>>>>>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>>>>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>>>>>>> libavcodec/version.h  |  3 +++
>>>>>>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>>>>>>        return 0;
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>>>> {
>>>>>>>>>        if (pkt->size <= size)
>>>>>>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int
>>>>>>>>> size)
>>>>>>>>>        pkt->size = size;
>>>>>>>>>        memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>> }
>>>>>>>>> +#endif
>>>>>>>>>
>>>>>>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>>>> {
>>>>>>>>> -    int new_size;
>>>>>>>>>        av_assert0((unsigned)pkt->size <= INT_MAX -
>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>        if ((unsigned)grow_by >
>>>>>>>>>            INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>>>>>>>>>            return AVERROR(ENOMEM);
>>>>>>>>>
>>>>>>>>> -    new_size = pkt->size + grow_by +
>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>>>>>>> +}
>>>>>>>>> +
>>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>>>>>>> +{
>>>>>>>>> +    int new_size;
>>>>>>>>> +
>>>>>>>>> +    if (size < 0 || size > INT_MAX -
>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE)
>>>>>>>>> +        return AVERROR(EINVAL);
>>>>>>>>> +
>>>>>>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>>        if (pkt->buf) {
>>>>>>>>>            size_t data_offset;
>>>>>>>>>            uint8_t *old_data = pkt->data;
>>>>>>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int
>>>>>>>>> grow_by)
>>>>>>>>>            if (!pkt->buf)
>>>>>>>>>                return AVERROR(ENOMEM);
>>>>>>>>>            if (pkt->size > 0)
>>>>>>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>>>>>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size,
>>>>>>>>> size));
>>>>>>>>>            pkt->data = pkt->buf->data;
>>>>>>>>>        }
>>>>>>>>> -    pkt->size += grow_by;
>>>>>>>>> +    pkt->size = size;
>>>>>>>>>        memset(pkt->data + pkt->size, 0,
>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>
>>>>>>>>>        return 0;
>>>>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>>>>>> index 3d9013d783..1720d973f5 100644
>>>>>>>>> --- a/libavcodec/packet.h
>>>>>>>>> +++ b/libavcodec/packet.h
>>>>>>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>>>>>>     */
>>>>>>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>>>>>>
>>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>>> /**
>>>>>>>>>     * Reduce packet size, correctly zeroing padding
>>>>>>>>>     *
>>>>>>>>>     * @param pkt packet
>>>>>>>>>     * @param size new size
>>>>>>>>> + *
>>>>>>>>> + * @deprecated Use av_packet_resize
>>>>>>>>>     */
>>>>>>>>> +attribute_deprecated
>>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>>>>>>> +#endif
>>>>>>>>> +
>>>>>>>>> +/**
>>>>>>>>> + * Resize the payload of a packet, correctly zeroing padding and
>>>>>>>>> avoiding data
>>>>>>>>> + * copy if possible.
>>>>>>>>> + *
>>>>>>>>> + * @param pkt packet
>>>>>>>>> + * @param size new size
>>>>>>>>> + *
>>>>>>>>> + * @return 0 on success, a negative AVERROR on error
>>>>>>>>> + */
>>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>>>>>>
>>>>>>>>> /**
>>>>>>>>>     * Increase packet size, correctly zeroing padding
>>>>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>>>>> index 3124ec8061..6c362b43e2 100644
>>>>>>>>> --- a/libavcodec/version.h
>>>>>>>>> +++ b/libavcodec/version.h
>>>>>>>>> @@ -162,5 +162,8 @@
>>>>>>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>>>>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>>>>>> #endif
>>>>>>>>> +#ifndef FF_API_SHRINK_PACKET
>>>>>>>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR <
>>>>>>>>> 60)
>>>>>>>>> +#endif
>>>>>>>>>
>>>>>>>>> #endif /* AVCODEC_VERSION_H */
>>>>>>>>> -- 
>>>>>>>>> 2.30.2
>>>>>>>>>

James Almer March 11, 2021, 6:40 p.m. UTC | #10

On 3/11/2021 3:26 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 3/11/2021 2:48 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 3/11/2021 2:27 PM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
>>>>>>> James Almer:
>>>>>>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>>>>>>
>>>>>>>>>> This function acts as a replacement for both av_grow_packet() and
>>>>>>>>>> av_shrink_packet(), the latter which is now deprecated and will be
>>>>>>>>>> removed as
>>>>>>>>>> it does not correctly handle non-writable packets.
>>>>>>>>>
>>>>>>>>> I don't think this is a good idea, av_shrink_packet cannot fail,
>>>>>>>>> av_grow_packet can. By using the same function you are losing the
>>>>>>>>> information if the end result should be checked or not.
>>>>>>>>
>>>>>>>> I'm not sure i follow. av_shrink_packet() is not being changed at
>>>>>>>> all,
>>>>>>>> just deprecated, scheduled for removal, and its use discouraged.
>>>>>>>
>>>>>>> I'd argue that a deprecation is actually a change.
>>>>>>>
>>>>>>>> Maybe i should have split this in two, one to add
>>>>>>>> av_packet_resize() and
>>>>>>>> one to deprecate av_shrink_packet(), to avoid confusion.
>>>>>>>>
>>>>>>>> In any case, the fact av_shrink_packet() cannot fail is the
>>>>>>>> reason I'm
>>>>>>>> getting rid of it. It's zeroing the padding without bothering to
>>>>>>>> check
>>>>>>>> if the packet is writable at all. And we can't have it attempt to
>>>>>>>> make
>>>>>>>> it writable because it can't then report if it failed to reallocate
>>>>>>>> the
>>>>>>>> buffer. So this patch here deprecates it for being a function that
>>>>>>>> predates reference counted buffers and is not able to properly
>>>>>>>> handle
>>>>>>>> them, and adds a replacement for it that also supersedes
>>>>>>>> av_grow_packet() while at it.
>>>>>>>>
>>>>>>>
>>>>>>> Yet you are not documenting that av_packet_resize can't fail if it is
>>>>>>> shrinking a packet known to be writable; ergo all unchecked uses of
>>>>>>> this
>>>>>>> function in the second and third patch are API abuse.
>>>>>>
>>>>>> I can add checks for all of them if you prefer. I didn't because they
>>>>>> are all internal uses known to (in theory) not fail.
>>>>>>
>>>>>
>>>>> No, I don't prefer checks for stuff that can't fail. I'd rather prefer
>>>>> if it were documented that it can't fail in these cases.
>>>>
>>>> I'm in general against adding that kind of constrain on a function's
>>>> documentation because you never know how it or what it processes could
>>>> be extended in the future.
>>>> Right now it can't fail in that scenario, true, but in the future we
>>>> could add some feature to AVPacket that would need to be handled by this
>>>> function that could start making it fail in that same scenario, and
>>>> suddenly, the doxy is no longer correct, and the function needs to be
>>>> replaced because it became unable to handle the new functionality.
>>>>
>>>> If a function can fail at all, then the library user should always make
>>>> sure to check the return value, and not be told there's one specific
>>>> scenario where they don't need to.
>>>>
>>>
>>> In this case I am against this patch.
>>
>> To add to what explained said above, it's the entire reason making
>> av_shrink_packet() return void was a mistake. It worked great before
>> reference counted buffers, but it was short sighted and the function is
>> now technically unusable after they were introduced, and why we need to
>> replace it now.
>> This patchset wouldn't exist had it been designed to return an int.
>>
> 
> The function is very well useable, but only for its use-case of already
> writable packets. (Not documenting this when the refcounted API was
> introduced was an error; or not deprecating it and replacing it with a
> function that explicitly says so.)

It should have been deprecated and replaced back then, but wasn't. And 
so I'm righting that wrong now.

> 
>> What you're asking for is to make the exact same mistake again, and it
>> could very well come to bite us in the future, again. It's definitely
>> not a good reason to try and block this patch at all.
>> av_shrink_packet has four lines, two of which do the actual work. It is
> extremely simple and could nearly be inlined. If any future extension
> requires adding something that can fail to this, then said future
> extension sounds like a huge step backwards.

Well, then you're calling reference counted buffers a step backwards, 
because their addition required making a compliant shrink + zero padding 
function be able to fail, as already explained.
Unnecessary constrains like the one you suggest are proven to be short 
sighted and not future proof, and all just to let the caller not check a 
return value in one very specific scenario.

> 
>>>
>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>> Regards,
>>>>>>>>> Marton
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>>>> ---
>>>>>>>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>>>>>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>>>>>>>> libavcodec/version.h  |  3 +++
>>>>>>>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>>>>>>>         return 0;
>>>>>>>>>> }
>>>>>>>>>>
>>>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>>>>> {
>>>>>>>>>>         if (pkt->size <= size)
>>>>>>>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int
>>>>>>>>>> size)
>>>>>>>>>>         pkt->size = size;
>>>>>>>>>>         memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>> }
>>>>>>>>>> +#endif
>>>>>>>>>>
>>>>>>>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>>>>> {
>>>>>>>>>> -    int new_size;
>>>>>>>>>>         av_assert0((unsigned)pkt->size <= INT_MAX -
>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>         if ((unsigned)grow_by >
>>>>>>>>>>             INT_MAX - (pkt->size + AV_INPUT_BUFFER_PADDING_SIZE))
>>>>>>>>>>             return AVERROR(ENOMEM);
>>>>>>>>>>
>>>>>>>>>> -    new_size = pkt->size + grow_by +
>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>>>>>>>> +{
>>>>>>>>>> +    int new_size;
>>>>>>>>>> +
>>>>>>>>>> +    if (size < 0 || size > INT_MAX -
>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE)
>>>>>>>>>> +        return AVERROR(EINVAL);
>>>>>>>>>> +
>>>>>>>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>>>         if (pkt->buf) {
>>>>>>>>>>             size_t data_offset;
>>>>>>>>>>             uint8_t *old_data = pkt->data;
>>>>>>>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int
>>>>>>>>>> grow_by)
>>>>>>>>>>             if (!pkt->buf)
>>>>>>>>>>                 return AVERROR(ENOMEM);
>>>>>>>>>>             if (pkt->size > 0)
>>>>>>>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>>>>>>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size,
>>>>>>>>>> size));
>>>>>>>>>>             pkt->data = pkt->buf->data;
>>>>>>>>>>         }
>>>>>>>>>> -    pkt->size += grow_by;
>>>>>>>>>> +    pkt->size = size;
>>>>>>>>>>         memset(pkt->data + pkt->size, 0,
>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>
>>>>>>>>>>         return 0;
>>>>>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>>>>>>> index 3d9013d783..1720d973f5 100644
>>>>>>>>>> --- a/libavcodec/packet.h
>>>>>>>>>> +++ b/libavcodec/packet.h
>>>>>>>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>>>>>>>      */
>>>>>>>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>>>>>>>
>>>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>>>> /**
>>>>>>>>>>      * Reduce packet size, correctly zeroing padding
>>>>>>>>>>      *
>>>>>>>>>>      * @param pkt packet
>>>>>>>>>>      * @param size new size
>>>>>>>>>> + *
>>>>>>>>>> + * @deprecated Use av_packet_resize
>>>>>>>>>>      */
>>>>>>>>>> +attribute_deprecated
>>>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>>>>>>>> +#endif
>>>>>>>>>> +
>>>>>>>>>> +/**
>>>>>>>>>> + * Resize the payload of a packet, correctly zeroing padding and
>>>>>>>>>> avoiding data
>>>>>>>>>> + * copy if possible.
>>>>>>>>>> + *
>>>>>>>>>> + * @param pkt packet
>>>>>>>>>> + * @param size new size
>>>>>>>>>> + *
>>>>>>>>>> + * @return 0 on success, a negative AVERROR on error
>>>>>>>>>> + */
>>>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>>>>>>>
>>>>>>>>>> /**
>>>>>>>>>>      * Increase packet size, correctly zeroing padding
>>>>>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>>>>>> index 3124ec8061..6c362b43e2 100644
>>>>>>>>>> --- a/libavcodec/version.h
>>>>>>>>>> +++ b/libavcodec/version.h
>>>>>>>>>> @@ -162,5 +162,8 @@
>>>>>>>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>>>>>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR < 60)
>>>>>>>>>> #endif
>>>>>>>>>> +#ifndef FF_API_SHRINK_PACKET
>>>>>>>>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR <
>>>>>>>>>> 60)
>>>>>>>>>> +#endif
>>>>>>>>>>
>>>>>>>>>> #endif /* AVCODEC_VERSION_H */
>>>>>>>>>> -- 
>>>>>>>>>> 2.30.2
>>>>>>>>>>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@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 March 11, 2021, 8:08 p.m. UTC | #11

James Almer:
> On 3/11/2021 3:26 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 3/11/2021 2:48 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 3/11/2021 2:27 PM, Andreas Rheinhardt wrote:
>>>>>> James Almer:
>>>>>>> On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
>>>>>>>> James Almer:
>>>>>>>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>>>>>>>
>>>>>>>>>>> This function acts as a replacement for both av_grow_packet()
>>>>>>>>>>> and
>>>>>>>>>>> av_shrink_packet(), the latter which is now deprecated and
>>>>>>>>>>> will be
>>>>>>>>>>> removed as
>>>>>>>>>>> it does not correctly handle non-writable packets.
>>>>>>>>>>
>>>>>>>>>> I don't think this is a good idea, av_shrink_packet cannot fail,
>>>>>>>>>> av_grow_packet can. By using the same function you are losing the
>>>>>>>>>> information if the end result should be checked or not.
>>>>>>>>>
>>>>>>>>> I'm not sure i follow. av_shrink_packet() is not being changed at
>>>>>>>>> all,
>>>>>>>>> just deprecated, scheduled for removal, and its use discouraged.
>>>>>>>>
>>>>>>>> I'd argue that a deprecation is actually a change.
>>>>>>>>
>>>>>>>>> Maybe i should have split this in two, one to add
>>>>>>>>> av_packet_resize() and
>>>>>>>>> one to deprecate av_shrink_packet(), to avoid confusion.
>>>>>>>>>
>>>>>>>>> In any case, the fact av_shrink_packet() cannot fail is the
>>>>>>>>> reason I'm
>>>>>>>>> getting rid of it. It's zeroing the padding without bothering to
>>>>>>>>> check
>>>>>>>>> if the packet is writable at all. And we can't have it attempt to
>>>>>>>>> make
>>>>>>>>> it writable because it can't then report if it failed to
>>>>>>>>> reallocate
>>>>>>>>> the
>>>>>>>>> buffer. So this patch here deprecates it for being a function that
>>>>>>>>> predates reference counted buffers and is not able to properly
>>>>>>>>> handle
>>>>>>>>> them, and adds a replacement for it that also supersedes
>>>>>>>>> av_grow_packet() while at it.
>>>>>>>>>
>>>>>>>>
>>>>>>>> Yet you are not documenting that av_packet_resize can't fail if
>>>>>>>> it is
>>>>>>>> shrinking a packet known to be writable; ergo all unchecked uses of
>>>>>>>> this
>>>>>>>> function in the second and third patch are API abuse.
>>>>>>>
>>>>>>> I can add checks for all of them if you prefer. I didn't because
>>>>>>> they
>>>>>>> are all internal uses known to (in theory) not fail.
>>>>>>>
>>>>>>
>>>>>> No, I don't prefer checks for stuff that can't fail. I'd rather
>>>>>> prefer
>>>>>> if it were documented that it can't fail in these cases.
>>>>>
>>>>> I'm in general against adding that kind of constrain on a function's
>>>>> documentation because you never know how it or what it processes could
>>>>> be extended in the future.
>>>>> Right now it can't fail in that scenario, true, but in the future we
>>>>> could add some feature to AVPacket that would need to be handled by
>>>>> this
>>>>> function that could start making it fail in that same scenario, and
>>>>> suddenly, the doxy is no longer correct, and the function needs to be
>>>>> replaced because it became unable to handle the new functionality.
>>>>>
>>>>> If a function can fail at all, then the library user should always
>>>>> make
>>>>> sure to check the return value, and not be told there's one specific
>>>>> scenario where they don't need to.
>>>>>
>>>>
>>>> In this case I am against this patch.
>>>
>>> To add to what explained said above, it's the entire reason making
>>> av_shrink_packet() return void was a mistake. It worked great before
>>> reference counted buffers, but it was short sighted and the function is
>>> now technically unusable after they were introduced, and why we need to
>>> replace it now.
>>> This patchset wouldn't exist had it been designed to return an int.
>>>
>>
>> The function is very well useable, but only for its use-case of already
>> writable packets. (Not documenting this when the refcounted API was
>> introduced was an error; or not deprecating it and replacing it with a
>> function that explicitly says so.)
> 
> It should have been deprecated and replaced back then, but wasn't. And
> so I'm righting that wrong now.
> 
>>
>>> What you're asking for is to make the exact same mistake again, and it
>>> could very well come to bite us in the future, again. It's definitely
>>> not a good reason to try and block this patch at all.
>>> av_shrink_packet has four lines, two of which do the actual work. It is
>> extremely simple and could nearly be inlined. If any future extension
>> requires adding something that can fail to this, then said future
>> extension sounds like a huge step backwards.
> 
> Well, then you're calling reference counted buffers a step backwards,
> because their addition required making a compliant shrink + zero padding
> function be able to fail, as already explained.

Before the introduction of refcounted packets avformat.h contained:
"If AVPacket.destruct is set on the returned packet, then the packet is
allocated dynamically and the user may keep it indefinitely. Otherwise,
if AVPacket.destruct is NULL, the packet data is backed by a static
storage somewhere inside the demuxer and the packet is only valid until
the next av_read_frame() call or closing the file."
So there were non-ownership packets even before refcounting was a thing;
this means that av_shrink_packet was wrong even then. But for the
equivalent of writable packets (namely those with destruct set) it was
correct and this remained true even after the introduction of refcounted
packets.

> Unnecessary constrains like the one you suggest are proven to be short
> sighted and not future proof, and all just to let the caller not check a
> return value in one very specific scenario.
> 

This is not an unnecessary constraint. The owner of a writable packet is
allowed to write to it by definition. One needn't perform another check
to see whether one gets permission to write to it, one already has it
(as long as one respects the packet's size, which is automatically true
when shrinking a packet). So demanding that such an operation doesn't
fail is entirely reasonable and natural (as natural as expecting that
memset can't fail).

>>
>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Regards,
>>>>>>>>>> Marton
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>>>>> ---
>>>>>>>>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>>>>>>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>>>>>>>>> libavcodec/version.h  |  3 +++
>>>>>>>>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>>>>>>>>         return 0;
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>>>>>> {
>>>>>>>>>>>         if (pkt->size <= size)
>>>>>>>>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int
>>>>>>>>>>> size)
>>>>>>>>>>>         pkt->size = size;
>>>>>>>>>>>         memset(pkt->data + size, 0,
>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>> }
>>>>>>>>>>> +#endif
>>>>>>>>>>>
>>>>>>>>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>>>>>> {
>>>>>>>>>>> -    int new_size;
>>>>>>>>>>>         av_assert0((unsigned)pkt->size <= INT_MAX -
>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>>         if ((unsigned)grow_by >
>>>>>>>>>>>             INT_MAX - (pkt->size +
>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE))
>>>>>>>>>>>             return AVERROR(ENOMEM);
>>>>>>>>>>>
>>>>>>>>>>> -    new_size = pkt->size + grow_by +
>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>>>>>>>>> +}
>>>>>>>>>>> +
>>>>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>>>>>>>>> +{
>>>>>>>>>>> +    int new_size;
>>>>>>>>>>> +
>>>>>>>>>>> +    if (size < 0 || size > INT_MAX -
>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE)
>>>>>>>>>>> +        return AVERROR(EINVAL);
>>>>>>>>>>> +
>>>>>>>>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>>>>         if (pkt->buf) {
>>>>>>>>>>>             size_t data_offset;
>>>>>>>>>>>             uint8_t *old_data = pkt->data;
>>>>>>>>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int
>>>>>>>>>>> grow_by)
>>>>>>>>>>>             if (!pkt->buf)
>>>>>>>>>>>                 return AVERROR(ENOMEM);
>>>>>>>>>>>             if (pkt->size > 0)
>>>>>>>>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>>>>>>>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size,
>>>>>>>>>>> size));
>>>>>>>>>>>             pkt->data = pkt->buf->data;
>>>>>>>>>>>         }
>>>>>>>>>>> -    pkt->size += grow_by;
>>>>>>>>>>> +    pkt->size = size;
>>>>>>>>>>>         memset(pkt->data + pkt->size, 0,
>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>>
>>>>>>>>>>>         return 0;
>>>>>>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>>>>>>>> index 3d9013d783..1720d973f5 100644
>>>>>>>>>>> --- a/libavcodec/packet.h
>>>>>>>>>>> +++ b/libavcodec/packet.h
>>>>>>>>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>>>>>>>>      */
>>>>>>>>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>>>>>>>>
>>>>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>>>>> /**
>>>>>>>>>>>      * Reduce packet size, correctly zeroing padding
>>>>>>>>>>>      *
>>>>>>>>>>>      * @param pkt packet
>>>>>>>>>>>      * @param size new size
>>>>>>>>>>> + *
>>>>>>>>>>> + * @deprecated Use av_packet_resize
>>>>>>>>>>>      */
>>>>>>>>>>> +attribute_deprecated
>>>>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>>>>>>>>> +#endif
>>>>>>>>>>> +
>>>>>>>>>>> +/**
>>>>>>>>>>> + * Resize the payload of a packet, correctly zeroing padding
>>>>>>>>>>> and
>>>>>>>>>>> avoiding data
>>>>>>>>>>> + * copy if possible.
>>>>>>>>>>> + *
>>>>>>>>>>> + * @param pkt packet
>>>>>>>>>>> + * @param size new size
>>>>>>>>>>> + *
>>>>>>>>>>> + * @return 0 on success, a negative AVERROR on error
>>>>>>>>>>> + */
>>>>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>>>>>>>>
>>>>>>>>>>> /**
>>>>>>>>>>>      * Increase packet size, correctly zeroing padding
>>>>>>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>>>>>>> index 3124ec8061..6c362b43e2 100644
>>>>>>>>>>> --- a/libavcodec/version.h
>>>>>>>>>>> +++ b/libavcodec/version.h
>>>>>>>>>>> @@ -162,5 +162,8 @@
>>>>>>>>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>>>>>>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR
>>>>>>>>>>> < 60)
>>>>>>>>>>> #endif
>>>>>>>>>>> +#ifndef FF_API_SHRINK_PACKET
>>>>>>>>>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR <
>>>>>>>>>>> 60)
>>>>>>>>>>> +#endif
>>>>>>>>>>>
>>>>>>>>>>> #endif /* AVCODEC_VERSION_H */
>>>>>>>>>>> -- 
>>>>>>>>>>> 2.30.2
>>>>>>>>>>>
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel@ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>
> 
> _______________________________________________
> 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 March 11, 2021, 8:38 p.m. UTC | #12

On 3/11/2021 5:08 PM, Andreas Rheinhardt wrote:
> James Almer:
>> On 3/11/2021 3:26 PM, Andreas Rheinhardt wrote:
>>> James Almer:
>>>> On 3/11/2021 2:48 PM, Andreas Rheinhardt wrote:
>>>>> James Almer:
>>>>>> On 3/11/2021 2:27 PM, Andreas Rheinhardt wrote:
>>>>>>> James Almer:
>>>>>>>> On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
>>>>>>>>> James Almer:
>>>>>>>>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>>>>>>>>
>>>>>>>>>>>> This function acts as a replacement for both av_grow_packet()
>>>>>>>>>>>> and
>>>>>>>>>>>> av_shrink_packet(), the latter which is now deprecated and
>>>>>>>>>>>> will be
>>>>>>>>>>>> removed as
>>>>>>>>>>>> it does not correctly handle non-writable packets.
>>>>>>>>>>>
>>>>>>>>>>> I don't think this is a good idea, av_shrink_packet cannot fail,
>>>>>>>>>>> av_grow_packet can. By using the same function you are losing the
>>>>>>>>>>> information if the end result should be checked or not.
>>>>>>>>>>
>>>>>>>>>> I'm not sure i follow. av_shrink_packet() is not being changed at
>>>>>>>>>> all,
>>>>>>>>>> just deprecated, scheduled for removal, and its use discouraged.
>>>>>>>>>
>>>>>>>>> I'd argue that a deprecation is actually a change.
>>>>>>>>>
>>>>>>>>>> Maybe i should have split this in two, one to add
>>>>>>>>>> av_packet_resize() and
>>>>>>>>>> one to deprecate av_shrink_packet(), to avoid confusion.
>>>>>>>>>>
>>>>>>>>>> In any case, the fact av_shrink_packet() cannot fail is the
>>>>>>>>>> reason I'm
>>>>>>>>>> getting rid of it. It's zeroing the padding without bothering to
>>>>>>>>>> check
>>>>>>>>>> if the packet is writable at all. And we can't have it attempt to
>>>>>>>>>> make
>>>>>>>>>> it writable because it can't then report if it failed to
>>>>>>>>>> reallocate
>>>>>>>>>> the
>>>>>>>>>> buffer. So this patch here deprecates it for being a function that
>>>>>>>>>> predates reference counted buffers and is not able to properly
>>>>>>>>>> handle
>>>>>>>>>> them, and adds a replacement for it that also supersedes
>>>>>>>>>> av_grow_packet() while at it.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Yet you are not documenting that av_packet_resize can't fail if
>>>>>>>>> it is
>>>>>>>>> shrinking a packet known to be writable; ergo all unchecked uses of
>>>>>>>>> this
>>>>>>>>> function in the second and third patch are API abuse.
>>>>>>>>
>>>>>>>> I can add checks for all of them if you prefer. I didn't because
>>>>>>>> they
>>>>>>>> are all internal uses known to (in theory) not fail.
>>>>>>>>
>>>>>>>
>>>>>>> No, I don't prefer checks for stuff that can't fail. I'd rather
>>>>>>> prefer
>>>>>>> if it were documented that it can't fail in these cases.
>>>>>>
>>>>>> I'm in general against adding that kind of constrain on a function's
>>>>>> documentation because you never know how it or what it processes could
>>>>>> be extended in the future.
>>>>>> Right now it can't fail in that scenario, true, but in the future we
>>>>>> could add some feature to AVPacket that would need to be handled by
>>>>>> this
>>>>>> function that could start making it fail in that same scenario, and
>>>>>> suddenly, the doxy is no longer correct, and the function needs to be
>>>>>> replaced because it became unable to handle the new functionality.
>>>>>>
>>>>>> If a function can fail at all, then the library user should always
>>>>>> make
>>>>>> sure to check the return value, and not be told there's one specific
>>>>>> scenario where they don't need to.
>>>>>>
>>>>>
>>>>> In this case I am against this patch.
>>>>
>>>> To add to what explained said above, it's the entire reason making
>>>> av_shrink_packet() return void was a mistake. It worked great before
>>>> reference counted buffers, but it was short sighted and the function is
>>>> now technically unusable after they were introduced, and why we need to
>>>> replace it now.
>>>> This patchset wouldn't exist had it been designed to return an int.
>>>>
>>>
>>> The function is very well useable, but only for its use-case of already
>>> writable packets. (Not documenting this when the refcounted API was
>>> introduced was an error; or not deprecating it and replacing it with a
>>> function that explicitly says so.)
>>
>> It should have been deprecated and replaced back then, but wasn't. And
>> so I'm righting that wrong now.
>>
>>>
>>>> What you're asking for is to make the exact same mistake again, and it
>>>> could very well come to bite us in the future, again. It's definitely
>>>> not a good reason to try and block this patch at all.
>>>> av_shrink_packet has four lines, two of which do the actual work. It is
>>> extremely simple and could nearly be inlined. If any future extension
>>> requires adding something that can fail to this, then said future
>>> extension sounds like a huge step backwards.
>>
>> Well, then you're calling reference counted buffers a step backwards,
>> because their addition required making a compliant shrink + zero padding
>> function be able to fail, as already explained.
> 
> Before the introduction of refcounted packets avformat.h contained:
> "If AVPacket.destruct is set on the returned packet, then the packet is
> allocated dynamically and the user may keep it indefinitely. Otherwise,
> if AVPacket.destruct is NULL, the packet data is backed by a static
> storage somewhere inside the demuxer and the packet is only valid until
> the next av_read_frame() call or closing the file."
> So there were non-ownership packets even before refcounting was a thing;
> this means that av_shrink_packet was wrong even then. But for the
> equivalent of writable packets (namely those with destruct set) it was
> correct and this remained true even after the introduction of refcounted
> packets.
> 
>> Unnecessary constrains like the one you suggest are proven to be short
>> sighted and not future proof, and all just to let the caller not check a
>> return value in one very specific scenario.
>>
> 
> This is not an unnecessary constraint. The owner of a writable packet is
> allowed to write to it by definition.

Why do you think this is, or will always be, about "Writing"? This is 
about "Shrinking" and "Resizing". Right now, using knowledge about 
current internal workings and definitions of AVPacket, that means that 
if writable, it can't fail. But tomorrow?

> One needn't perform another check
> to see whether one gets permission to write to it, one already has it
> (as long as one respects the packet's size, which is automatically true
> when shrinking a packet). So demanding that such an operation doesn't
> fail is entirely reasonable and natural (as natural as expecting that
> memset can't fail).

Tell that to whoever in five years comes up with new functionality that 
can generate a failure in an AVPacket payload shrinking scenario 
regardless of writability or ownership that will need to replace this 
function because we arrogantly thought we knew better.

It's a constrain we gain nothing from by defining, while putting the 
usability of this function in the long term at risk. It's literally one 
very specific scenario you want users to be allowed to not check one 
miserable return value, and you've spent an entire evening arguing about 
it. I don't know about you but i think there are better things to spend 
said time on.

As i mentioned before, there's precedent for this kind of assumption 
ultimately making an API non future proof. There's no reason whatsoever 
to spend so much time demanding we make the same mistake again. None.

> 
>>>
>>>>>
>>>>>>>
>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Regards,
>>>>>>>>>>> Marton
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>>>>>>>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>>>>>>>>>> libavcodec/version.h  |  3 +++
>>>>>>>>>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>>>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>>>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>>>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>>>>>>>>>          return 0;
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>>>>>>> {
>>>>>>>>>>>>          if (pkt->size <= size)
>>>>>>>>>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int
>>>>>>>>>>>> size)
>>>>>>>>>>>>          pkt->size = size;
>>>>>>>>>>>>          memset(pkt->data + size, 0,
>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>>> }
>>>>>>>>>>>> +#endif
>>>>>>>>>>>>
>>>>>>>>>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>>>>>>> {
>>>>>>>>>>>> -    int new_size;
>>>>>>>>>>>>          av_assert0((unsigned)pkt->size <= INT_MAX -
>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>>>          if ((unsigned)grow_by >
>>>>>>>>>>>>              INT_MAX - (pkt->size +
>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE))
>>>>>>>>>>>>              return AVERROR(ENOMEM);
>>>>>>>>>>>>
>>>>>>>>>>>> -    new_size = pkt->size + grow_by +
>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>>>>>>>>>> +}
>>>>>>>>>>>> +
>>>>>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>>>>>>>>>> +{
>>>>>>>>>>>> +    int new_size;
>>>>>>>>>>>> +
>>>>>>>>>>>> +    if (size < 0 || size > INT_MAX -
>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE)
>>>>>>>>>>>> +        return AVERROR(EINVAL);
>>>>>>>>>>>> +
>>>>>>>>>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>>>>>          if (pkt->buf) {
>>>>>>>>>>>>              size_t data_offset;
>>>>>>>>>>>>              uint8_t *old_data = pkt->data;
>>>>>>>>>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int
>>>>>>>>>>>> grow_by)
>>>>>>>>>>>>              if (!pkt->buf)
>>>>>>>>>>>>                  return AVERROR(ENOMEM);
>>>>>>>>>>>>              if (pkt->size > 0)
>>>>>>>>>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>>>>>>>>>> +            memcpy(pkt->buf->data, pkt->data, FFMIN(pkt->size,
>>>>>>>>>>>> size));
>>>>>>>>>>>>              pkt->data = pkt->buf->data;
>>>>>>>>>>>>          }
>>>>>>>>>>>> -    pkt->size += grow_by;
>>>>>>>>>>>> +    pkt->size = size;
>>>>>>>>>>>>          memset(pkt->data + pkt->size, 0,
>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>>>
>>>>>>>>>>>>          return 0;
>>>>>>>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>>>>>>>>> index 3d9013d783..1720d973f5 100644
>>>>>>>>>>>> --- a/libavcodec/packet.h
>>>>>>>>>>>> +++ b/libavcodec/packet.h
>>>>>>>>>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>>>>>>>>>       */
>>>>>>>>>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>>>>>>>>>
>>>>>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>>>>>> /**
>>>>>>>>>>>>       * Reduce packet size, correctly zeroing padding
>>>>>>>>>>>>       *
>>>>>>>>>>>>       * @param pkt packet
>>>>>>>>>>>>       * @param size new size
>>>>>>>>>>>> + *
>>>>>>>>>>>> + * @deprecated Use av_packet_resize
>>>>>>>>>>>>       */
>>>>>>>>>>>> +attribute_deprecated
>>>>>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>>>>>>>>>> +#endif
>>>>>>>>>>>> +
>>>>>>>>>>>> +/**
>>>>>>>>>>>> + * Resize the payload of a packet, correctly zeroing padding
>>>>>>>>>>>> and
>>>>>>>>>>>> avoiding data
>>>>>>>>>>>> + * copy if possible.
>>>>>>>>>>>> + *
>>>>>>>>>>>> + * @param pkt packet
>>>>>>>>>>>> + * @param size new size
>>>>>>>>>>>> + *
>>>>>>>>>>>> + * @return 0 on success, a negative AVERROR on error
>>>>>>>>>>>> + */
>>>>>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>>>>>>>>>
>>>>>>>>>>>> /**
>>>>>>>>>>>>       * Increase packet size, correctly zeroing padding
>>>>>>>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>>>>>>>> index 3124ec8061..6c362b43e2 100644
>>>>>>>>>>>> --- a/libavcodec/version.h
>>>>>>>>>>>> +++ b/libavcodec/version.h
>>>>>>>>>>>> @@ -162,5 +162,8 @@
>>>>>>>>>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>>>>>>>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR
>>>>>>>>>>>> < 60)
>>>>>>>>>>>> #endif
>>>>>>>>>>>> +#ifndef FF_API_SHRINK_PACKET
>>>>>>>>>>>> +#define FF_API_SHRINK_PACKET       (LIBAVCODEC_VERSION_MAJOR <
>>>>>>>>>>>> 60)
>>>>>>>>>>>> +#endif
>>>>>>>>>>>>
>>>>>>>>>>>> #endif /* AVCODEC_VERSION_H */
>>>>>>>>>>>> -- 
>>>>>>>>>>>> 2.30.2
>>>>>>>>>>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel@ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>
>>
>> _______________________________________________
>> 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".
> 
> _______________________________________________
> 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".
>

Marton Balint March 11, 2021, 10:40 p.m. UTC | #13

On Thu, 11 Mar 2021, James Almer wrote:

> On 3/11/2021 1:11 PM, Marton Balint wrote:
>> 
>> 
>> On Thu, 11 Mar 2021, James Almer wrote:
>> 
>>> This function acts as a replacement for both av_grow_packet() and
>>> av_shrink_packet(), the latter which is now deprecated and will be 
>>> removed as
>>> it does not correctly handle non-writable packets.
>> 
>> I don't think this is a good idea, av_shrink_packet cannot fail, 
>> av_grow_packet can. By using the same function you are losing the 
>> information if the end result should be checked or not.
>
> I'm not sure i follow. av_shrink_packet() is not being changed at all, 
> just deprecated, scheduled for removal, and its use discouraged.

But why are you complicating code with mandatory return value checking? 
Because as soon as a function returns an error, you have to check it, and 
forward it upwards.

> Maybe i should have split this in two, one to add av_packet_resize() and 
> one to deprecate av_shrink_packet(), to avoid confusion.
>
> In any case, the fact av_shrink_packet() cannot fail is the reason I'm 
> getting rid of it. It's zeroing the padding without bothering to check 
> if the packet is writable at all.

Add an assert to it then. Shrinking a non-writable packet seems bad API 
usage anyways.

If you want to shrink a writable packet, then maybe you don't even need 
zero padding, because the buffer possibly already contains some defined 
value, and the main reason of zero padding is avoiding reading 
uninitialized data...

Regards,
Marton

James Almer March 11, 2021, 11:24 p.m. UTC | #14

On 3/11/2021 7:40 PM, Marton Balint wrote:
> 
> 
> On Thu, 11 Mar 2021, James Almer wrote:
> 
>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>
>>>
>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>
>>>> This function acts as a replacement for both av_grow_packet() and
>>>> av_shrink_packet(), the latter which is now deprecated and will be 
>>>> removed as
>>>> it does not correctly handle non-writable packets.
>>>
>>> I don't think this is a good idea, av_shrink_packet cannot fail, 
>>> av_grow_packet can. By using the same function you are losing the 
>>> information if the end result should be checked or not.
>>
>> I'm not sure i follow. av_shrink_packet() is not being changed at all, 
>> just deprecated, scheduled for removal, and its use discouraged.
> 
> But why are you complicating code with mandatory return value checking?

Because unlike av_shrink_packet(), av_packet_resize() is more thorough 
when handling AVPackets and allows new usage scenarios that were not 
allowed with the former, thus it can fail.

> Because as soon as a function returns an error, you have to check it, 
> and forward it upwards.

Is error checking that much of a problem? I don't understand why it 
bothers people so much.

> 
>> Maybe i should have split this in two, one to add av_packet_resize() 
>> and one to deprecate av_shrink_packet(), to avoid confusion.
>>
>> In any case, the fact av_shrink_packet() cannot fail is the reason I'm 
>> getting rid of it. It's zeroing the padding without bothering to check 
>> if the packet is writable at all.
> 
> Add an assert to it then.

We only assert on internal bugs, not invalid arguments passed by callers 
to a public function. The asserts would need to be added above each 
av_shrink_packet() call. And that's only for our internal uses of the 
function. It does nothing to the issue of it being public API that can't 
properly handle AVPackets in their current form.

 > Shrinking a non-writable packet seems bad API usage anyways.

I get a packet from a demuxer. It contains two independent portions 
(NALUs, OBUs, etc) i want to separate in order to feed them to something 
like a multi threaded decoder. And so I create a new reference to it, 
resulting in two packets pointing to the same data. I shrink one to only 
contain the first portion, and i add the required byte offset to the 
data pointer and subtract it to the size field on the second packet so 
it contains only the second portion.
The result if i use av_packet_resize() will be two valid, properly 
padded packets containing their respective expected data, but if i use 
av_shrink_packet(), the result will be the packet with the second 
portion featuring padding bytes worth of data zeroed at the start of its 
payload.

I'm sure there are other similar valid scenarios where attempting to 
shrink a non writable packet is not "bad API usage".

> 
> If you want to shrink a writable packet, then maybe you don't even need 
> zero padding, because the buffer possibly already contains some defined 
> value, and the main reason of zero padding is avoiding reading 
> uninitialized data...

If i allocate a payload of size 1024, ultimately fill only 512 bytes, 
then resize it to that value (typical scenario in lavf demuxers), if i 
don't zero the bytes after the 512 offset then they will remain 
uninitialized.

> 
> Regards,
> Marton
> _______________________________________________
> 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".

Marton Balint March 12, 2021, 5:09 p.m. UTC | #15

On Thu, 11 Mar 2021, James Almer wrote:

> On 3/11/2021 7:40 PM, Marton Balint wrote:
>> 
>> 
>> On Thu, 11 Mar 2021, James Almer wrote:
>> 
>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>
>>>>
>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>
>>>>> This function acts as a replacement for both av_grow_packet() and
>>>>> av_shrink_packet(), the latter which is now deprecated and will be 
>>>>> removed as
>>>>> it does not correctly handle non-writable packets.
>>>>
>>>> I don't think this is a good idea, av_shrink_packet cannot fail, 
>>>> av_grow_packet can. By using the same function you are losing the 
>>>> information if the end result should be checked or not.
>>>
>>> I'm not sure i follow. av_shrink_packet() is not being changed at all, 
>>> just deprecated, scheduled for removal, and its use discouraged.
>> 
>> But why are you complicating code with mandatory return value checking?
>
> Because unlike av_shrink_packet(), av_packet_resize() is more thorough 
> when handling AVPackets and allows new usage scenarios that were not 
> allowed with the former, thus it can fail.
>
>> Because as soon as a function returns an error, you have to check it, 
>> and forward it upwards.
>
> Is error checking that much of a problem? I don't understand why it 
> bothers people so much.
>
>> 
>>> Maybe i should have split this in two, one to add av_packet_resize() 
>>> and one to deprecate av_shrink_packet(), to avoid confusion.
>>>
>>> In any case, the fact av_shrink_packet() cannot fail is the reason I'm 
>>> getting rid of it. It's zeroing the padding without bothering to check 
>>> if the packet is writable at all.
>> 
>> Add an assert to it then.
>
> We only assert on internal bugs, not invalid arguments passed by callers 
> to a public function. The asserts would need to be added above each 
> av_shrink_packet() call.

There are a lot of cases when we assert for bad API usage. See 
av_frame_ref() or av_frame_move_ref(). We assert if the dst is not empty.

> And that's only for our internal uses of the 
> function. It does nothing to the issue of it being public API that can't 
> properly handle AVPackets in their current form.
>
> > Shrinking a non-writable packet seems bad API usage anyways.
>
> I get a packet from a demuxer. It contains two independent portions 
> (NALUs, OBUs, etc) i want to separate in order to feed them to something 
> like a multi threaded decoder. And so I create a new reference to it, 
> resulting in two packets pointing to the same data. I shrink one to only 
> contain the first portion, and i add the required byte offset to the 
> data pointer and subtract it to the size field on the second packet so 
> it contains only the second portion.
> The result if i use av_packet_resize() will be two valid, properly 
> padded packets containing their respective expected data, but if i use 
> av_shrink_packet(), the result will be the packet with the second 
> portion featuring padding bytes worth of data zeroed at the start of its 
> payload.

This looks like an unfortunate example, since you are:
- adding a reference to the packet, so you don't have to duplicate data
- and then want to add zero padding to the partial packet, so you will
duplicate data.
And I think the padding does not have to be zero for the partial packet.

>
> I'm sure there are other similar valid scenarios where attempting to 
> shrink a non writable packet is not "bad API usage".
>
>> 
>> If you want to shrink a writable packet, then maybe you don't even need 
>> zero padding, because the buffer possibly already contains some defined 
>> value, and the main reason of zero padding is avoiding reading 
>> uninitialized data...
>
> If i allocate a payload of size 1024, ultimately fill only 512 bytes, 
> then resize it to that value (typical scenario in lavf demuxers), if i 
> don't zero the bytes after the 512 offset then they will remain 
> uninitialized.

I am not sure I see your point here, if the data after the padding is 
uninitalized, that is not a problem. I made a typo above, and meant 
non-writable packet in my comment. And my reasoning is that if a packet is 
non-writable, it already contains initialized data with a zero padding. If 
you shrink that by reducing pkt->size only, you will not have 
uninitialized data, only the padding will not be zeroed. And that is 
preferable to copying the data only for zeroing the padding, because - as 
far as I know - the padding does not have to be zeroed, it only have to be 
initialized.

I agree that it is not nice that av_shrink_packet() writes something to 
the packet, people may not think about it and misuse it instead of 
directly decreaseing pkt->size when they need a partial packet. That is 
why I suggested the assert(). One might argue that it kind of breaks 
behaviour, but I'd say it does not break it too much, and writing to a 
non-writable packet was broken in the first place.

Alternatively we can change av_shrink_packet() to only zero the padding if 
the packet is writable, which has the benefit that it will do what people 
generally expect, no matter if you throw a writable or a non-writable 
packet to it.

A third alternative is to introduce void av_shrink_packet2() in which you 
explicitly document that a writable packet is needed and do the assert 
there, and deprecate av_shrink_packet().

Regards,
Marton

James Almer March 12, 2021, 5:24 p.m. UTC | #16

On 3/12/2021 2:09 PM, Marton Balint wrote:
> 
> 
> On Thu, 11 Mar 2021, James Almer wrote:
> 
>> On 3/11/2021 7:40 PM, Marton Balint wrote:
>>>
>>>
>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>
>>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>>
>>>>>
>>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>>
>>>>>> This function acts as a replacement for both av_grow_packet() and
>>>>>> av_shrink_packet(), the latter which is now deprecated and will be 
>>>>>> removed as
>>>>>> it does not correctly handle non-writable packets.
>>>>>
>>>>> I don't think this is a good idea, av_shrink_packet cannot fail, 
>>>>> av_grow_packet can. By using the same function you are losing the 
>>>>> information if the end result should be checked or not.
>>>>
>>>> I'm not sure i follow. av_shrink_packet() is not being changed at 
>>>> all, just deprecated, scheduled for removal, and its use discouraged.
>>>
>>> But why are you complicating code with mandatory return value checking?
>>
>> Because unlike av_shrink_packet(), av_packet_resize() is more thorough 
>> when handling AVPackets and allows new usage scenarios that were not 
>> allowed with the former, thus it can fail.
>>
>>> Because as soon as a function returns an error, you have to check it, 
>>> and forward it upwards.
>>
>> Is error checking that much of a problem? I don't understand why it 
>> bothers people so much.
>>
>>>
>>>> Maybe i should have split this in two, one to add av_packet_resize() 
>>>> and one to deprecate av_shrink_packet(), to avoid confusion.
>>>>
>>>> In any case, the fact av_shrink_packet() cannot fail is the reason 
>>>> I'm getting rid of it. It's zeroing the padding without bothering to 
>>>> check if the packet is writable at all.
>>>
>>> Add an assert to it then.
>>
>> We only assert on internal bugs, not invalid arguments passed by 
>> callers to a public function. The asserts would need to be added above 
>> each av_shrink_packet() call.
> 
> There are a lot of cases when we assert for bad API usage. See 
> av_frame_ref() or av_frame_move_ref(). We assert if the dst is not empty.
> 
>> And that's only for our internal uses of the function. It does nothing 
>> to the issue of it being public API that can't properly handle 
>> AVPackets in their current form.
>>
>> > Shrinking a non-writable packet seems bad API usage anyways.
>>
>> I get a packet from a demuxer. It contains two independent portions 
>> (NALUs, OBUs, etc) i want to separate in order to feed them to 
>> something like a multi threaded decoder. And so I create a new 
>> reference to it, resulting in two packets pointing to the same data. I 
>> shrink one to only contain the first portion, and i add the required 
>> byte offset to the data pointer and subtract it to the size field on 
>> the second packet so it contains only the second portion.
>> The result if i use av_packet_resize() will be two valid, properly 
>> padded packets containing their respective expected data, but if i use 
>> av_shrink_packet(), the result will be the packet with the second 
>> portion featuring padding bytes worth of data zeroed at the start of 
>> its payload.
> 
> This looks like an unfortunate example, since you are:
> - adding a reference to the packet, so you don't have to duplicate data
> - and then want to add zero padding to the partial packet, so you will
> duplicate data.
> And I think the padding does not have to be zero for the partial packet.

The padding exists AFAIK because something, like an optimized bitstream 
reader that tries to process several bytes at the same time, may end up 
reading or writing pass the reported end of the buffer.
That means that if reading and it's not all zeroes, it could in theory 
have unpredictable results. Hence why everything always zeroes the 
padding, even av_shrink_packet().

And yes, what you describe is what some bitstream filters and the 
matroska demuxer do. They just create several packet references pointing 
to the same data buffer, but using different offsets for the data 
pointer. They all have "padding", but in many cases said padding is the 
beginning of the payload of another packet, and that's not ideal.

> 
>>
>> I'm sure there are other similar valid scenarios where attempting to 
>> shrink a non writable packet is not "bad API usage".
>>
>>>
>>> If you want to shrink a writable packet, then maybe you don't even 
>>> need zero padding, because the buffer possibly already contains some 
>>> defined value, and the main reason of zero padding is avoiding 
>>> reading uninitialized data...
>>
>> If i allocate a payload of size 1024, ultimately fill only 512 bytes, 
>> then resize it to that value (typical scenario in lavf demuxers), if i 
>> don't zero the bytes after the 512 offset then they will remain 
>> uninitialized.
> 
> I am not sure I see your point here, if the data after the padding is 
> uninitalized, that is not a problem. I made a typo above, and meant 
> non-writable packet in my comment. And my reasoning is that if a packet 
> is non-writable, it already contains initialized data with a zero 
> padding. If you shrink that by reducing pkt->size only, you will not 
> have uninitialized data, only the padding will not be zeroed. And that 
> is preferable to copying the data only for zeroing the padding, because 
> - as far as I know - the padding does not have to be zeroed, it only 
> have to be initialized.
> 
> I agree that it is not nice that av_shrink_packet() writes something to 
> the packet, people may not think about it and misuse it instead of 
> directly decreaseing pkt->size when they need a partial packet. That is 
> why I suggested the assert(). One might argue that it kind of breaks 
> behaviour, but I'd say it does not break it too much, and writing to a 
> non-writable packet was broken in the first place.
> 
> Alternatively we can change av_shrink_packet() to only zero the padding 
> if the packet is writable, which has the benefit that it will do what 
> people generally expect, no matter if you throw a writable or a 
> non-writable packet to it.
> 
> A third alternative is to introduce void av_shrink_packet2() in which 
> you explicitly document that a writable packet is needed and do the 
> assert there, and deprecate av_shrink_packet().

Not a fan of functions with a 2 suffix. And to be honest, I really don't 
care about what we do with av_shrink_packet().
Do you want to keep it around? Ok. Want to deprecate and remove it? 
Better. Want to add an assert to it? Not a fan and it may result in 
unexpected crashes for library users, but whatever.

I just want to add av_packet_resize() to have a single resize function 
that will properly handle AVPackets in their current and any potential 
future states.

Marton Balint March 13, 2021, 7:30 p.m. UTC | #17

On Fri, 12 Mar 2021, James Almer wrote:

>>>
>>> I get a packet from a demuxer. It contains two independent portions 
>>> (NALUs, OBUs, etc) i want to separate in order to feed them to 
>>> something like a multi threaded decoder. And so I create a new 
>>> reference to it, resulting in two packets pointing to the same data. I 
>>> shrink one to only contain the first portion, and i add the required 
>>> byte offset to the data pointer and subtract it to the size field on 
>>> the second packet so it contains only the second portion.
>>> The result if i use av_packet_resize() will be two valid, properly 
>>> padded packets containing their respective expected data, but if i use 
>>> av_shrink_packet(), the result will be the packet with the second 
>>> portion featuring padding bytes worth of data zeroed at the start of 
>>> its payload.
>> 
>> This looks like an unfortunate example, since you are:
>> - adding a reference to the packet, so you don't have to duplicate data
>> - and then want to add zero padding to the partial packet, so you will
>> duplicate data.
>> And I think the padding does not have to be zero for the partial packet.
>
> The padding exists AFAIK because something, like an optimized bitstream 
> reader that tries to process several bytes at the same time, may end up 
> reading or writing pass the reported end of the buffer.
> That means that if reading and it's not all zeroes, it could in theory 
> have unpredictable results. Hence why everything always zeroes the 
> padding, even av_shrink_packet().
>
> And yes, what you describe is what some bitstream filters and the 
> matroska demuxer do. They just create several packet references pointing 
> to the same data buffer, but using different offsets for the data 
> pointer. They all have "padding", but in many cases said padding is the 
> beginning of the payload of another packet, and that's not ideal.

Well, performance reasons come in play and the ability to not copy the 
data. Yeah, it does not play nicely with our historical requirement of 
zero padding.

I did a little experimenting, and except for subtitles (which have crashed 
and burned because of the missing 0 string terminator), most tests handled 
non-zero padding well, a few failed tests, mostly for partial source 
files, for which I guess it is inevitable?

So I guess for now we will stay in the gray area of "if it does not 
crash, then having non-zero padding for some partial packets is 
OKish"...

>> I agree that it is not nice that av_shrink_packet() writes something to 
>> the packet, people may not think about it and misuse it instead of 
>> directly decreaseing pkt->size when they need a partial packet. That is 
>> why I suggested the assert(). One might argue that it kind of breaks 
>> behaviour, but I'd say it does not break it too much, and writing to a 
>> non-writable packet was broken in the first place.
>> 
>> Alternatively we can change av_shrink_packet() to only zero the padding 
>> if the packet is writable, which has the benefit that it will do what 
>> people generally expect, no matter if you throw a writable or a 
>> non-writable packet to it.
>> 
>> A third alternative is to introduce void av_shrink_packet2() in which 
>> you explicitly document that a writable packet is needed and do the 
>> assert there, and deprecate av_shrink_packet().
>
> Not a fan of functions with a 2 suffix. And to be honest, I really don't 
> care about what we do with av_shrink_packet().
> Do you want to keep it around? Ok. Want to deprecate and remove it? 
> Better. Want to add an assert to it? Not a fan and it may result in 
> unexpected crashes for library users, but whatever.
>
> I just want to add av_packet_resize() to have a single resize function 
> that will properly handle AVPackets in their current and any potential 
> future states.

Ok, then I suggest you add av_resize_packet but keep av_shrink_packet() as 
well, and we can add an assert() to it after the release/bump.

Regards,
Marton

Anton Khirnov March 14, 2021, 10:34 a.m. UTC | #18

Quoting James Almer (2021-03-12 18:24:47)
> 
> The padding exists AFAIK because something, like an optimized bitstream 
> reader that tries to process several bytes at the same time, may end up 
> reading or writing pass the reported end of the buffer.
> That means that if reading and it's not all zeroes, it could in theory 
> have unpredictable results. Hence why everything always zeroes the 
> padding, even av_shrink_packet().

On that topic, I've been wondering about eliminating this requirement.
Does anyone know which code it is precisely that depends on the padding
being zeroed? Is this optimization really worth it?
It seems rather silly to jump through all these hoops for an
unmeasurable benefit in one decoder.

It would also give us zero-copy packet splitting.

> 
> And yes, what you describe is what some bitstream filters and the 
> matroska demuxer do. They just create several packet references pointing 
> to the same data buffer, but using different offsets for the data 
> pointer. They all have "padding", but in many cases said padding is the 
> beginning of the payload of another packet, and that's not ideal.
> 
> > 
> >>
> >> I'm sure there are other similar valid scenarios where attempting to 
> >> shrink a non writable packet is not "bad API usage".
> >>
> >>>
> >>> If you want to shrink a writable packet, then maybe you don't even 
> >>> need zero padding, because the buffer possibly already contains some 
> >>> defined value, and the main reason of zero padding is avoiding 
> >>> reading uninitialized data...
> >>
> >> If i allocate a payload of size 1024, ultimately fill only 512 bytes, 
> >> then resize it to that value (typical scenario in lavf demuxers), if i 
> >> don't zero the bytes after the 512 offset then they will remain 
> >> uninitialized.
> > 
> > I am not sure I see your point here, if the data after the padding is 
> > uninitalized, that is not a problem. I made a typo above, and meant 
> > non-writable packet in my comment. And my reasoning is that if a packet 
> > is non-writable, it already contains initialized data with a zero 
> > padding. If you shrink that by reducing pkt->size only, you will not 
> > have uninitialized data, only the padding will not be zeroed. And that 
> > is preferable to copying the data only for zeroing the padding, because 
> > - as far as I know - the padding does not have to be zeroed, it only 
> > have to be initialized.
> > 
> > I agree that it is not nice that av_shrink_packet() writes something to 
> > the packet, people may not think about it and misuse it instead of 
> > directly decreaseing pkt->size when they need a partial packet. That is 
> > why I suggested the assert(). One might argue that it kind of breaks 
> > behaviour, but I'd say it does not break it too much, and writing to a 
> > non-writable packet was broken in the first place.
> > 
> > Alternatively we can change av_shrink_packet() to only zero the padding 
> > if the packet is writable, which has the benefit that it will do what 
> > people generally expect, no matter if you throw a writable or a 
> > non-writable packet to it.
> > 
> > A third alternative is to introduce void av_shrink_packet2() in which 
> > you explicitly document that a writable packet is needed and do the 
> > assert there, and deprecate av_shrink_packet().
> 
> Not a fan of functions with a 2 suffix. And to be honest, I really don't 
> care about what we do with av_shrink_packet().
> Do you want to keep it around? Ok. Want to deprecate and remove it? 
> Better. Want to add an assert to it? Not a fan and it may result in 
> unexpected crashes for library users, but whatever.

I would suggest keeping av_shrink_packet() with a big scary warning that
it does not check for ownership and it is the caller's responsibility to
ensure that writing to the packet is safe.

Also, I'd like to emphasise that av_*_is_writable() is not gospel. It is
merely a convention that is used "by default", when you don't have more
accurate information.
Some bits of code may use other conventions and consider a buffer
writable even if av_buffer_is_writable() returns 0. For example this is
at the core of frame threading, where a reference to a frame currently
being decoded is propagated to other threads before decoding finishes.
The owner thread then writes to the frame because frame-mt conventions
allow it to, even though there are multiple references to the frame.

James Almer March 14, 2021, 3:35 p.m. UTC | #19

On 3/14/2021 7:34 AM, Anton Khirnov wrote:
> Quoting James Almer (2021-03-12 18:24:47)
>>
>> The padding exists AFAIK because something, like an optimized bitstream
>> reader that tries to process several bytes at the same time, may end up
>> reading or writing pass the reported end of the buffer.
>> That means that if reading and it's not all zeroes, it could in theory
>> have unpredictable results. Hence why everything always zeroes the
>> padding, even av_shrink_packet().
> 
> On that topic, I've been wondering about eliminating this requirement.
> Does anyone know which code it is precisely that depends on the padding
> being zeroed? Is this optimization really worth it?
> It seems rather silly to jump through all these hoops for an
> unmeasurable benefit in one decoder.

Some subtitle demuxers didn't look at pkt->size and depended on the 
padding to work as a 0 string terminator, but Marton fixed that in a 
patchset sent yesterday.

Beyond that, i think the v210 decoder and encoder read and write past 
the end of the buffer if you use the simd functions.

> 
> It would also give us zero-copy packet splitting.
> 
>>
>> And yes, what you describe is what some bitstream filters and the
>> matroska demuxer do. They just create several packet references pointing
>> to the same data buffer, but using different offsets for the data
>> pointer. They all have "padding", but in many cases said padding is the
>> beginning of the payload of another packet, and that's not ideal.
>>
>>>
>>>>
>>>> I'm sure there are other similar valid scenarios where attempting to
>>>> shrink a non writable packet is not "bad API usage".
>>>>
>>>>>
>>>>> If you want to shrink a writable packet, then maybe you don't even
>>>>> need zero padding, because the buffer possibly already contains some
>>>>> defined value, and the main reason of zero padding is avoiding
>>>>> reading uninitialized data...
>>>>
>>>> If i allocate a payload of size 1024, ultimately fill only 512 bytes,
>>>> then resize it to that value (typical scenario in lavf demuxers), if i
>>>> don't zero the bytes after the 512 offset then they will remain
>>>> uninitialized.
>>>
>>> I am not sure I see your point here, if the data after the padding is
>>> uninitalized, that is not a problem. I made a typo above, and meant
>>> non-writable packet in my comment. And my reasoning is that if a packet
>>> is non-writable, it already contains initialized data with a zero
>>> padding. If you shrink that by reducing pkt->size only, you will not
>>> have uninitialized data, only the padding will not be zeroed. And that
>>> is preferable to copying the data only for zeroing the padding, because
>>> - as far as I know - the padding does not have to be zeroed, it only
>>> have to be initialized.
>>>
>>> I agree that it is not nice that av_shrink_packet() writes something to
>>> the packet, people may not think about it and misuse it instead of
>>> directly decreaseing pkt->size when they need a partial packet. That is
>>> why I suggested the assert(). One might argue that it kind of breaks
>>> behaviour, but I'd say it does not break it too much, and writing to a
>>> non-writable packet was broken in the first place.
>>>
>>> Alternatively we can change av_shrink_packet() to only zero the padding
>>> if the packet is writable, which has the benefit that it will do what
>>> people generally expect, no matter if you throw a writable or a
>>> non-writable packet to it.
>>>
>>> A third alternative is to introduce void av_shrink_packet2() in which
>>> you explicitly document that a writable packet is needed and do the
>>> assert there, and deprecate av_shrink_packet().
>>
>> Not a fan of functions with a 2 suffix. And to be honest, I really don't
>> care about what we do with av_shrink_packet().
>> Do you want to keep it around? Ok. Want to deprecate and remove it?
>> Better. Want to add an assert to it? Not a fan and it may result in
>> unexpected crashes for library users, but whatever.
> 
> I would suggest keeping av_shrink_packet() with a big scary warning that
> it does not check for ownership and it is the caller's responsibility to
> ensure that writing to the packet is safe.

If we can remove the zero-padding requirement, or the padding 
requirement altogether, then that would no longer be necessary.

> 
> Also, I'd like to emphasise that av_*_is_writable() is not gospel. It is
> merely a convention that is used "by default", when you don't have more
> accurate information.
> Some bits of code may use other conventions and consider a buffer
> writable even if av_buffer_is_writable() returns 0. For example this is
> at the core of frame threading, where a reference to a frame currently
> being decoded is propagated to other threads before decoding finishes.
> The owner thread then writes to the frame because frame-mt conventions
> allow it to, even though there are multiple references to the frame.
>

Anton Khirnov March 14, 2021, 3:51 p.m. UTC | #20

Quoting James Almer (2021-03-14 16:35:48)
> On 3/14/2021 7:34 AM, Anton Khirnov wrote:
> > Quoting James Almer (2021-03-12 18:24:47)
> >>
> >> The padding exists AFAIK because something, like an optimized bitstream
> >> reader that tries to process several bytes at the same time, may end up
> >> reading or writing pass the reported end of the buffer.
> >> That means that if reading and it's not all zeroes, it could in theory
> >> have unpredictable results. Hence why everything always zeroes the
> >> padding, even av_shrink_packet().
> > 
> > On that topic, I've been wondering about eliminating this requirement.
> > Does anyone know which code it is precisely that depends on the padding
> > being zeroed? Is this optimization really worth it?
> > It seems rather silly to jump through all these hoops for an
> > unmeasurable benefit in one decoder.
> 
> Some subtitle demuxers didn't look at pkt->size and depended on the 
> padding to work as a 0 string terminator, but Marton fixed that in a 
> patchset sent yesterday.
> 
> Beyond that, i think the v210 decoder and encoder read and write past 
> the end of the buffer if you use the simd functions.
> 
> > 
> > It would also give us zero-copy packet splitting.
> > 
> > [...]
> > 
> > I would suggest keeping av_shrink_packet() with a big scary warning that
> > it does not check for ownership and it is the caller's responsibility to
> > ensure that writing to the packet is safe.
> 
> If we can remove the zero-padding requirement, or the padding 
> requirement altogether, then that would no longer be necessary.

I don't think we can remove the padding requirement completely, as the
bitstream reader reads 4- or 8-byte chunks at once. I imagine changing
that into single-byte reads would be very slow.
But getting rid of the zeroing requirement should be more feasible.

Michael Niedermayer March 14, 2021, 7:45 p.m. UTC | #21

On Sun, Mar 14, 2021 at 04:51:44PM +0100, Anton Khirnov wrote:
> Quoting James Almer (2021-03-14 16:35:48)
> > On 3/14/2021 7:34 AM, Anton Khirnov wrote:
> > > Quoting James Almer (2021-03-12 18:24:47)
> > >>
> > >> The padding exists AFAIK because something, like an optimized bitstream
> > >> reader that tries to process several bytes at the same time, may end up
> > >> reading or writing pass the reported end of the buffer.
> > >> That means that if reading and it's not all zeroes, it could in theory
> > >> have unpredictable results. Hence why everything always zeroes the
> > >> padding, even av_shrink_packet().
> > > 
> > > On that topic, I've been wondering about eliminating this requirement.
> > > Does anyone know which code it is precisely that depends on the padding
> > > being zeroed? Is this optimization really worth it?
> > > It seems rather silly to jump through all these hoops for an
> > > unmeasurable benefit in one decoder.
> > 
> > Some subtitle demuxers didn't look at pkt->size and depended on the 
> > padding to work as a 0 string terminator, but Marton fixed that in a 
> > patchset sent yesterday.
> > 
> > Beyond that, i think the v210 decoder and encoder read and write past 
> > the end of the buffer if you use the simd functions.
> > 
> > > 
> > > It would also give us zero-copy packet splitting.
> > > 
> > > [...]
> > > 
> > > I would suggest keeping av_shrink_packet() with a big scary warning that
> > > it does not check for ownership and it is the caller's responsibility to
> > > ensure that writing to the packet is safe.
> > 
> > If we can remove the zero-padding requirement, or the padding 
> > requirement altogether, then that would no longer be necessary.
> 
> I don't think we can remove the padding requirement completely, as the
> bitstream reader reads 4- or 8-byte chunks at once. I imagine changing
> that into single-byte reads would be very slow.
> But getting rid of the zeroing requirement should be more feasible.

mpeg/itu codecs historically always had the all 0 VLC of all relevant tables
invalid. That was done to ensure a long string of zeros cannot occur. and
thus a startcode as used in MPEG-PS/TS or in slices (nowadays NALs and such)
cannot occur in the middle of a valid frame.
That way simply checking for the vlc to be valid would also always check
for a startcode or the end if there where 3-4 zero bytes at the end.
This was probably used in every decoder where it could be used as
it avoided an extra check per vlc reading loop.
I have the suspicion it would be faster to check the padding for being 0
and malloc/memcpy if not in many cases than to add extra checks in every
affected loop. More so as this only affcts cases where there is no
"NAL/slice/packet" with a startcode afterwards

Thanks

[...]

Andreas Rheinhardt March 15, 2021, 4:03 a.m. UTC | #22

Marton Balint:
> 
> 
> On Fri, 12 Mar 2021, James Almer wrote:
> 
>>>>
>>>> I get a packet from a demuxer. It contains two independent portions
>>>> (NALUs, OBUs, etc) i want to separate in order to feed them to
>>>> something like a multi threaded decoder. And so I create a new
>>>> reference to it, resulting in two packets pointing to the same data.
>>>> I shrink one to only contain the first portion, and i add the
>>>> required byte offset to the data pointer and subtract it to the size
>>>> field on the second packet so it contains only the second portion.
>>>> The result if i use av_packet_resize() will be two valid, properly
>>>> padded packets containing their respective expected data, but if i
>>>> use av_shrink_packet(), the result will be the packet with the
>>>> second portion featuring padding bytes worth of data zeroed at the
>>>> start of its payload.
>>>
>>> This looks like an unfortunate example, since you are:
>>> - adding a reference to the packet, so you don't have to duplicate data
>>> - and then want to add zero padding to the partial packet, so you will
>>> duplicate data.
>>> And I think the padding does not have to be zero for the partial packet.
>>
>> The padding exists AFAIK because something, like an optimized
>> bitstream reader that tries to process several bytes at the same time,
>> may end up reading or writing pass the reported end of the buffer.
>> That means that if reading and it's not all zeroes, it could in theory
>> have unpredictable results. Hence why everything always zeroes the
>> padding, even av_shrink_packet().
>>
>> And yes, what you describe is what some bitstream filters and the
>> matroska demuxer do. They just create several packet references
>> pointing to the same data buffer, but using different offsets for the
>> data pointer. They all have "padding", but in many cases said padding
>> is the beginning of the payload of another packet, and that's not ideal.
> 
> Well, performance reasons come in play and the ability to not copy the
> data. Yeah, it does not play nicely with our historical requirement of
> zero padding.
> 
> I did a little experimenting, and except for subtitles (which have
> crashed and burned because of the missing 0 string terminator), most
> tests handled non-zero padding well, a few failed tests, mostly for
> partial source files, for which I guess it is inevitable?
> 
> So I guess for now we will stay in the gray area of "if it does not
> crash, then having non-zero padding for some partial packets is OKish"...
> 
>>> I agree that it is not nice that av_shrink_packet() writes something
>>> to the packet, people may not think about it and misuse it instead of
>>> directly decreaseing pkt->size when they need a partial packet. That
>>> is why I suggested the assert(). One might argue that it kind of
>>> breaks behaviour, but I'd say it does not break it too much, and
>>> writing to a non-writable packet was broken in the first place.
>>>
>>> Alternatively we can change av_shrink_packet() to only zero the
>>> padding if the packet is writable, which has the benefit that it will
>>> do what people generally expect, no matter if you throw a writable or
>>> a non-writable packet to it.
>>>
>>> A third alternative is to introduce void av_shrink_packet2() in which
>>> you explicitly document that a writable packet is needed and do the
>>> assert there, and deprecate av_shrink_packet().
>>
>> Not a fan of functions with a 2 suffix. And to be honest, I really
>> don't care about what we do with av_shrink_packet().
>> Do you want to keep it around? Ok. Want to deprecate and remove it?
>> Better. Want to add an assert to it? Not a fan and it may result in
>> unexpected crashes for library users, but whatever.
>>
>> I just want to add av_packet_resize() to have a single resize function
>> that will properly handle AVPackets in their current and any potential
>> future states.
> 
> Ok, then I suggest you add av_resize_packet but keep av_shrink_packet()
> as well, and we can add an assert() to it after the release/bump.
> 

Can we really add an assert to it? I am not so sure about that. The
problem lies in ff_alloc_packet2(): It can return non-refcounted packets
whose data points to avctx->internal->byte_buffer. (Some encoders need
to allocate big buffers for worst-case scenarios and then the data is
initially written to said byte_buffer and copied lateron via
av_packet_make_refcounted() in encode_simple_internal().) This is
basically what Anton said: Not all parts of the code completely abide by
the constraints of the AVBuffer API; and they can still work fine if
they abide by their own notions of ownership.

- Andreas

Andreas Rheinhardt March 15, 2021, 4:10 a.m. UTC | #23

Michael Niedermayer:
> On Sun, Mar 14, 2021 at 04:51:44PM +0100, Anton Khirnov wrote:
>> Quoting James Almer (2021-03-14 16:35:48)
>>> On 3/14/2021 7:34 AM, Anton Khirnov wrote:
>>>> Quoting James Almer (2021-03-12 18:24:47)
>>>>>
>>>>> The padding exists AFAIK because something, like an optimized bitstream
>>>>> reader that tries to process several bytes at the same time, may end up
>>>>> reading or writing pass the reported end of the buffer.
>>>>> That means that if reading and it's not all zeroes, it could in theory
>>>>> have unpredictable results. Hence why everything always zeroes the
>>>>> padding, even av_shrink_packet().
>>>>
>>>> On that topic, I've been wondering about eliminating this requirement.
>>>> Does anyone know which code it is precisely that depends on the padding
>>>> being zeroed? Is this optimization really worth it?
>>>> It seems rather silly to jump through all these hoops for an
>>>> unmeasurable benefit in one decoder.
>>>
>>> Some subtitle demuxers didn't look at pkt->size and depended on the 
>>> padding to work as a 0 string terminator, but Marton fixed that in a 
>>> patchset sent yesterday.
>>>
>>> Beyond that, i think the v210 decoder and encoder read and write past 
>>> the end of the buffer if you use the simd functions.
>>>
>>>>
>>>> It would also give us zero-copy packet splitting.
>>>>
>>>> [...]
>>>>
>>>> I would suggest keeping av_shrink_packet() with a big scary warning that
>>>> it does not check for ownership and it is the caller's responsibility to
>>>> ensure that writing to the packet is safe.
>>>
>>> If we can remove the zero-padding requirement, or the padding 
>>> requirement altogether, then that would no longer be necessary.
>>
>> I don't think we can remove the padding requirement completely, as the
>> bitstream reader reads 4- or 8-byte chunks at once. I imagine changing
>> that into single-byte reads would be very slow.
>> But getting rid of the zeroing requirement should be more feasible.
> 
> mpeg/itu codecs historically always had the all 0 VLC of all relevant tables
> invalid. That was done to ensure a long string of zeros cannot occur. and
> thus a startcode as used in MPEG-PS/TS or in slices (nowadays NALs and such)
> cannot occur in the middle of a valid frame.
> That way simply checking for the vlc to be valid would also always check
> for a startcode or the end if there where 3-4 zero bytes at the end.
> This was probably used in every decoder where it could be used as
> it avoided an extra check per vlc reading loop.
> I have the suspicion it would be faster to check the padding for being 0
> and malloc/memcpy if not in many cases than to add extra checks in every
> affected loop. More so as this only affcts cases where there is no
> "NAL/slice/packet" with a startcode afterwards
> 

So if we mandate that the actually allocated buffers need to be at least
AV_INPUT_BUFFER_PADDING_SIZE bigger than AVPacket.size and that there
has to be at least AV_INPUT_BUFFER_PADDING_SIZE zero bytes before the
end of the allocated area, no crashes can happen. One still needs to
check for overreads, but this needn't be done in the VLC reading loops.
We should probably also mandate that all the data after the end of the
packet and the beginning of the zeroed padding needs to be initialized
(in order not to get Valgrind warnings).
(This is actually what I tell myself in order to justify the behaviour
of the Matroska demuxer when lacing is present.)

- Andreas

Andreas Rheinhardt March 15, 2021, 4:15 a.m. UTC | #24

James Almer:
> On 3/14/2021 7:34 AM, Anton Khirnov wrote:
>> Quoting James Almer (2021-03-12 18:24:47)
>>>
>>> The padding exists AFAIK because something, like an optimized bitstream
>>> reader that tries to process several bytes at the same time, may end up
>>> reading or writing pass the reported end of the buffer.
>>> That means that if reading and it's not all zeroes, it could in theory
>>> have unpredictable results. Hence why everything always zeroes the
>>> padding, even av_shrink_packet().
>>
>> On that topic, I've been wondering about eliminating this requirement.
>> Does anyone know which code it is precisely that depends on the padding
>> being zeroed? Is this optimization really worth it?
>> It seems rather silly to jump through all these hoops for an
>> unmeasurable benefit in one decoder.
> 
> Some subtitle demuxers didn't look at pkt->size and depended on the
> padding to work as a 0 string terminator, but Marton fixed that in a
> patchset sent yesterday.

You mean subtitle decoders; some subtitle demuxers have a different bug:
They only zero-terminate their extradata with a single '\0', not with
AV_INPUT_BUFFER_PADDING_SIZE. See ff_bprint_to_codecpar_extradata().

And ff_startcode_find_candidate_c() also requires that data is
zero-terminated immediately after the buffer. It is trivial to fix. (My
old patchset would actually avoid overreading altogether.)

> 
> Beyond that, i think the v210 decoder and encoder read and write past
> the end of the buffer if you use the simd functions.
> 
>>
>> It would also give us zero-copy packet splitting.
>>
>>>
>>> And yes, what you describe is what some bitstream filters and the
>>> matroska demuxer do. They just create several packet references pointing
>>> to the same data buffer, but using different offsets for the data
>>> pointer. They all have "padding", but in many cases said padding is the
>>> beginning of the payload of another packet, and that's not ideal.
>>>
>>>>
>>>>>
>>>>> I'm sure there are other similar valid scenarios where attempting to
>>>>> shrink a non writable packet is not "bad API usage".
>>>>>
>>>>>>
>>>>>> If you want to shrink a writable packet, then maybe you don't even
>>>>>> need zero padding, because the buffer possibly already contains some
>>>>>> defined value, and the main reason of zero padding is avoiding
>>>>>> reading uninitialized data...
>>>>>
>>>>> If i allocate a payload of size 1024, ultimately fill only 512 bytes,
>>>>> then resize it to that value (typical scenario in lavf demuxers), if i
>>>>> don't zero the bytes after the 512 offset then they will remain
>>>>> uninitialized.
>>>>
>>>> I am not sure I see your point here, if the data after the padding is
>>>> uninitalized, that is not a problem. I made a typo above, and meant
>>>> non-writable packet in my comment. And my reasoning is that if a packet
>>>> is non-writable, it already contains initialized data with a zero
>>>> padding. If you shrink that by reducing pkt->size only, you will not
>>>> have uninitialized data, only the padding will not be zeroed. And that
>>>> is preferable to copying the data only for zeroing the padding, because
>>>> - as far as I know - the padding does not have to be zeroed, it only
>>>> have to be initialized.
>>>>
>>>> I agree that it is not nice that av_shrink_packet() writes something to
>>>> the packet, people may not think about it and misuse it instead of
>>>> directly decreaseing pkt->size when they need a partial packet. That is
>>>> why I suggested the assert(). One might argue that it kind of breaks
>>>> behaviour, but I'd say it does not break it too much, and writing to a
>>>> non-writable packet was broken in the first place.
>>>>
>>>> Alternatively we can change av_shrink_packet() to only zero the padding
>>>> if the packet is writable, which has the benefit that it will do what
>>>> people generally expect, no matter if you throw a writable or a
>>>> non-writable packet to it.
>>>>
>>>> A third alternative is to introduce void av_shrink_packet2() in which
>>>> you explicitly document that a writable packet is needed and do the
>>>> assert there, and deprecate av_shrink_packet().
>>>
>>> Not a fan of functions with a 2 suffix. And to be honest, I really don't
>>> care about what we do with av_shrink_packet().
>>> Do you want to keep it around? Ok. Want to deprecate and remove it?
>>> Better. Want to add an assert to it? Not a fan and it may result in
>>> unexpected crashes for library users, but whatever.
>>
>> I would suggest keeping av_shrink_packet() with a big scary warning that
>> it does not check for ownership and it is the caller's responsibility to
>> ensure that writing to the packet is safe.
> 
> If we can remove the zero-padding requirement, or the padding
> requirement altogether, then that would no longer be necessary.
> 
>>
>> Also, I'd like to emphasise that av_*_is_writable() is not gospel. It is
>> merely a convention that is used "by default", when you don't have more
>> accurate information.
>> Some bits of code may use other conventions and consider a buffer
>> writable even if av_buffer_is_writable() returns 0. For example this is
>> at the core of frame threading, where a reference to a frame currently
>> being decoded is propagated to other threads before decoding finishes.
>> The owner thread then writes to the frame because frame-mt conventions
>> allow it to, even though there are multiple references to the frame.
>>
> 
> _______________________________________________
> 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 March 15, 2021, 5:25 a.m. UTC | #25

James Almer:
> On 3/11/2021 5:08 PM, Andreas Rheinhardt wrote:
>> James Almer:
>>> On 3/11/2021 3:26 PM, Andreas Rheinhardt wrote:
>>>> James Almer:
>>>>> On 3/11/2021 2:48 PM, Andreas Rheinhardt wrote:
>>>>>> James Almer:
>>>>>>> On 3/11/2021 2:27 PM, Andreas Rheinhardt wrote:
>>>>>>>> James Almer:
>>>>>>>>> On 3/11/2021 1:56 PM, Andreas Rheinhardt wrote:
>>>>>>>>>> James Almer:
>>>>>>>>>>> On 3/11/2021 1:11 PM, Marton Balint wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, 11 Mar 2021, James Almer wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>> This function acts as a replacement for both av_grow_packet()
>>>>>>>>>>>>> and
>>>>>>>>>>>>> av_shrink_packet(), the latter which is now deprecated and
>>>>>>>>>>>>> will be
>>>>>>>>>>>>> removed as
>>>>>>>>>>>>> it does not correctly handle non-writable packets.
>>>>>>>>>>>>
>>>>>>>>>>>> I don't think this is a good idea, av_shrink_packet cannot
>>>>>>>>>>>> fail,
>>>>>>>>>>>> av_grow_packet can. By using the same function you are
>>>>>>>>>>>> losing the
>>>>>>>>>>>> information if the end result should be checked or not.
>>>>>>>>>>>
>>>>>>>>>>> I'm not sure i follow. av_shrink_packet() is not being
>>>>>>>>>>> changed at
>>>>>>>>>>> all,
>>>>>>>>>>> just deprecated, scheduled for removal, and its use discouraged.
>>>>>>>>>>
>>>>>>>>>> I'd argue that a deprecation is actually a change.
>>>>>>>>>>
>>>>>>>>>>> Maybe i should have split this in two, one to add
>>>>>>>>>>> av_packet_resize() and
>>>>>>>>>>> one to deprecate av_shrink_packet(), to avoid confusion.
>>>>>>>>>>>
>>>>>>>>>>> In any case, the fact av_shrink_packet() cannot fail is the
>>>>>>>>>>> reason I'm
>>>>>>>>>>> getting rid of it. It's zeroing the padding without bothering to
>>>>>>>>>>> check
>>>>>>>>>>> if the packet is writable at all. And we can't have it
>>>>>>>>>>> attempt to
>>>>>>>>>>> make
>>>>>>>>>>> it writable because it can't then report if it failed to
>>>>>>>>>>> reallocate
>>>>>>>>>>> the
>>>>>>>>>>> buffer. So this patch here deprecates it for being a function
>>>>>>>>>>> that
>>>>>>>>>>> predates reference counted buffers and is not able to properly
>>>>>>>>>>> handle
>>>>>>>>>>> them, and adds a replacement for it that also supersedes
>>>>>>>>>>> av_grow_packet() while at it.
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Yet you are not documenting that av_packet_resize can't fail if
>>>>>>>>>> it is
>>>>>>>>>> shrinking a packet known to be writable; ergo all unchecked
>>>>>>>>>> uses of
>>>>>>>>>> this
>>>>>>>>>> function in the second and third patch are API abuse.
>>>>>>>>>
>>>>>>>>> I can add checks for all of them if you prefer. I didn't because
>>>>>>>>> they
>>>>>>>>> are all internal uses known to (in theory) not fail.
>>>>>>>>>
>>>>>>>>
>>>>>>>> No, I don't prefer checks for stuff that can't fail. I'd rather
>>>>>>>> prefer
>>>>>>>> if it were documented that it can't fail in these cases.
>>>>>>>
>>>>>>> I'm in general against adding that kind of constrain on a function's
>>>>>>> documentation because you never know how it or what it processes
>>>>>>> could
>>>>>>> be extended in the future.
>>>>>>> Right now it can't fail in that scenario, true, but in the future we
>>>>>>> could add some feature to AVPacket that would need to be handled by
>>>>>>> this
>>>>>>> function that could start making it fail in that same scenario, and
>>>>>>> suddenly, the doxy is no longer correct, and the function needs
>>>>>>> to be
>>>>>>> replaced because it became unable to handle the new functionality.
>>>>>>>
>>>>>>> If a function can fail at all, then the library user should always
>>>>>>> make
>>>>>>> sure to check the return value, and not be told there's one specific
>>>>>>> scenario where they don't need to.
>>>>>>>
>>>>>>
>>>>>> In this case I am against this patch.
>>>>>
>>>>> To add to what explained said above, it's the entire reason making
>>>>> av_shrink_packet() return void was a mistake. It worked great before
>>>>> reference counted buffers, but it was short sighted and the
>>>>> function is
>>>>> now technically unusable after they were introduced, and why we
>>>>> need to
>>>>> replace it now.
>>>>> This patchset wouldn't exist had it been designed to return an int.
>>>>>
>>>>
>>>> The function is very well useable, but only for its use-case of already
>>>> writable packets. (Not documenting this when the refcounted API was
>>>> introduced was an error; or not deprecating it and replacing it with a
>>>> function that explicitly says so.)
>>>
>>> It should have been deprecated and replaced back then, but wasn't. And
>>> so I'm righting that wrong now.
>>>
>>>>
>>>>> What you're asking for is to make the exact same mistake again, and it
>>>>> could very well come to bite us in the future, again. It's definitely
>>>>> not a good reason to try and block this patch at all.
>>>>> av_shrink_packet has four lines, two of which do the actual work.
>>>>> It is
>>>> extremely simple and could nearly be inlined. If any future extension
>>>> requires adding something that can fail to this, then said future
>>>> extension sounds like a huge step backwards.
>>>
>>> Well, then you're calling reference counted buffers a step backwards,
>>> because their addition required making a compliant shrink + zero padding
>>> function be able to fail, as already explained.
>>
>> Before the introduction of refcounted packets avformat.h contained:
>> "If AVPacket.destruct is set on the returned packet, then the packet is
>> allocated dynamically and the user may keep it indefinitely. Otherwise,
>> if AVPacket.destruct is NULL, the packet data is backed by a static
>> storage somewhere inside the demuxer and the packet is only valid until
>> the next av_read_frame() call or closing the file."
>> So there were non-ownership packets even before refcounting was a thing;
>> this means that av_shrink_packet was wrong even then. But for the
>> equivalent of writable packets (namely those with destruct set) it was
>> correct and this remained true even after the introduction of refcounted
>> packets.
>>
>>> Unnecessary constrains like the one you suggest are proven to be short
>>> sighted and not future proof, and all just to let the caller not check a
>>> return value in one very specific scenario.
>>>
>>
>> This is not an unnecessary constraint. The owner of a writable packet is
>> allowed to write to it by definition.
> 
> Why do you think this is, or will always be, about "Writing"? This is
> about "Shrinking" and "Resizing". Right now, using knowledge about
> current internal workings and definitions of AVPacket, that means that
> if writable, it can't fail. But tomorrow?
> 
>> One needn't perform another check
>> to see whether one gets permission to write to it, one already has it
>> (as long as one respects the packet's size, which is automatically true
>> when shrinking a packet). So demanding that such an operation doesn't
>> fail is entirely reasonable and natural (as natural as expecting that
>> memset can't fail).
> 
> Tell that to whoever in five years comes up with new functionality that
> can generate a failure in an AVPacket payload shrinking scenario
> regardless of writability or ownership that will need to replace this
> function because we arrogantly thought we knew better.
> 
> It's a constrain we gain nothing from by defining, while putting the
> usability of this function in the long term at risk. It's literally one
> very specific scenario you want users to be allowed to not check one
> miserable return value, and you've spent an entire evening arguing about
> it. I don't know about you but i think there are better things to spend
> said time on.
> 
> As i mentioned before, there's precedent for this kind of assumption
> ultimately making an API non future proof. There's no reason whatsoever
> to spend so much time demanding we make the same mistake again. None.
> 

I pondered this and I think that the gains in API flexibility that you
are expecting are illusionary: We allow our users to modify the packets
themselves. In particular, we allow them to do on their own what
av_shrink_packet() does now. If we modify AVPacket in such a way that
av_packet_resize() can fail when shrinking a writable packet, then said
modification also breaks the users' ability to shrink packets by this
simple procedure. They will have to adapt and so said modification will
have to be subject to the typical API changes procedure (i.e. it will
have to wait).
This would of course be different if it were disallowed to shrink a
packet manually; one would likely have to forbid other things as well
and provide getters/setters for users. But this is not what your patch
is about. And I don't think I need to remind you that the current
getters/setters are deprecated and about to be removed.

Finally, let me add how glad I am at the arrogant and short-sighted
design of functions like memset and memcpy which can't return an
ordinary error. Imagine how our code would look if the designers of C
had chosen differently to allow for implementations that act like modern
sanitizers do and check whether e.g. dst and src point to accessible
buffers of sufficient size.

>>
>>>>
>>>>>>
>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Regards,
>>>>>>>>>>>> Marton
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>> libavcodec/avpacket.c | 19 +++++++++++++++----
>>>>>>>>>>>>> libavcodec/packet.h   | 16 ++++++++++++++++
>>>>>>>>>>>>> libavcodec/version.h  |  3 +++
>>>>>>>>>>>>> 3 files changed, 34 insertions(+), 4 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c
>>>>>>>>>>>>> index 32cb71fcf0..7d0dbadbed 100644
>>>>>>>>>>>>> --- a/libavcodec/avpacket.c
>>>>>>>>>>>>> +++ b/libavcodec/avpacket.c
>>>>>>>>>>>>> @@ -100,6 +100,7 @@ int av_new_packet(AVPacket *pkt, int size)
>>>>>>>>>>>>>          return 0;
>>>>>>>>>>>>> }
>>>>>>>>>>>>>
>>>>>>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size)
>>>>>>>>>>>>> {
>>>>>>>>>>>>>          if (pkt->size <= size)
>>>>>>>>>>>>> @@ -107,16 +108,26 @@ void av_shrink_packet(AVPacket *pkt, int
>>>>>>>>>>>>> size)
>>>>>>>>>>>>>          pkt->size = size;
>>>>>>>>>>>>>          memset(pkt->data + size, 0,
>>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>>>> }
>>>>>>>>>>>>> +#endif
>>>>>>>>>>>>>
>>>>>>>>>>>>> int av_grow_packet(AVPacket *pkt, int grow_by)
>>>>>>>>>>>>> {
>>>>>>>>>>>>> -    int new_size;
>>>>>>>>>>>>>          av_assert0((unsigned)pkt->size <= INT_MAX -
>>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>>>>          if ((unsigned)grow_by >
>>>>>>>>>>>>>              INT_MAX - (pkt->size +
>>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE))
>>>>>>>>>>>>>              return AVERROR(ENOMEM);
>>>>>>>>>>>>>
>>>>>>>>>>>>> -    new_size = pkt->size + grow_by +
>>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>>>>>> +    return av_packet_resize(pkt, pkt->size + grow_by);
>>>>>>>>>>>>> +}
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size)
>>>>>>>>>>>>> +{
>>>>>>>>>>>>> +    int new_size;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +    if (size < 0 || size > INT_MAX -
>>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE)
>>>>>>>>>>>>> +        return AVERROR(EINVAL);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +    new_size = size + AV_INPUT_BUFFER_PADDING_SIZE;
>>>>>>>>>>>>>          if (pkt->buf) {
>>>>>>>>>>>>>              size_t data_offset;
>>>>>>>>>>>>>              uint8_t *old_data = pkt->data;
>>>>>>>>>>>>> @@ -143,10 +154,10 @@ int av_grow_packet(AVPacket *pkt, int
>>>>>>>>>>>>> grow_by)
>>>>>>>>>>>>>              if (!pkt->buf)
>>>>>>>>>>>>>                  return AVERROR(ENOMEM);
>>>>>>>>>>>>>              if (pkt->size > 0)
>>>>>>>>>>>>> -            memcpy(pkt->buf->data, pkt->data, pkt->size);
>>>>>>>>>>>>> +            memcpy(pkt->buf->data, pkt->data,
>>>>>>>>>>>>> FFMIN(pkt->size,
>>>>>>>>>>>>> size));
>>>>>>>>>>>>>              pkt->data = pkt->buf->data;
>>>>>>>>>>>>>          }
>>>>>>>>>>>>> -    pkt->size += grow_by;
>>>>>>>>>>>>> +    pkt->size = size;
>>>>>>>>>>>>>          memset(pkt->data + pkt->size, 0,
>>>>>>>>>>>>> AV_INPUT_BUFFER_PADDING_SIZE);
>>>>>>>>>>>>>
>>>>>>>>>>>>>          return 0;
>>>>>>>>>>>>> diff --git a/libavcodec/packet.h b/libavcodec/packet.h
>>>>>>>>>>>>> index 3d9013d783..1720d973f5 100644
>>>>>>>>>>>>> --- a/libavcodec/packet.h
>>>>>>>>>>>>> +++ b/libavcodec/packet.h
>>>>>>>>>>>>> @@ -484,13 +484,29 @@ void av_init_packet(AVPacket *pkt);
>>>>>>>>>>>>>       */
>>>>>>>>>>>>> int av_new_packet(AVPacket *pkt, int size);
>>>>>>>>>>>>>
>>>>>>>>>>>>> +#if FF_API_SHRINK_PACKET
>>>>>>>>>>>>> /**
>>>>>>>>>>>>>       * Reduce packet size, correctly zeroing padding
>>>>>>>>>>>>>       *
>>>>>>>>>>>>>       * @param pkt packet
>>>>>>>>>>>>>       * @param size new size
>>>>>>>>>>>>> + *
>>>>>>>>>>>>> + * @deprecated Use av_packet_resize
>>>>>>>>>>>>>       */
>>>>>>>>>>>>> +attribute_deprecated
>>>>>>>>>>>>> void av_shrink_packet(AVPacket *pkt, int size);
>>>>>>>>>>>>> +#endif
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +/**
>>>>>>>>>>>>> + * Resize the payload of a packet, correctly zeroing padding
>>>>>>>>>>>>> and
>>>>>>>>>>>>> avoiding data
>>>>>>>>>>>>> + * copy if possible.
>>>>>>>>>>>>> + *
>>>>>>>>>>>>> + * @param pkt packet
>>>>>>>>>>>>> + * @param size new size
>>>>>>>>>>>>> + *
>>>>>>>>>>>>> + * @return 0 on success, a negative AVERROR on error
>>>>>>>>>>>>> + */
>>>>>>>>>>>>> +int av_packet_resize(AVPacket *pkt, int size);
>>>>>>>>>>>>>
>>>>>>>>>>>>> /**
>>>>>>>>>>>>>       * Increase packet size, correctly zeroing padding
>>>>>>>>>>>>> diff --git a/libavcodec/version.h b/libavcodec/version.h
>>>>>>>>>>>>> index 3124ec8061..6c362b43e2 100644
>>>>>>>>>>>>> --- a/libavcodec/version.h
>>>>>>>>>>>>> +++ b/libavcodec/version.h
>>>>>>>>>>>>> @@ -162,5 +162,8 @@
>>>>>>>>>>>>> #ifndef FF_API_GET_FRAME_CLASS
>>>>>>>>>>>>> #define FF_API_GET_FRAME_CLASS     (LIBAVCODEC_VERSION_MAJOR
>>>>>>>>>>>>> < 60)
>>>>>>>>>>>>> #endif
>>>>>>>>>>>>> +#ifndef FF_API_SHRINK_PACKET
>>>>>>>>>>>>> +#define FF_API_SHRINK_PACKET      
>>>>>>>>>>>>> (LIBAVCODEC_VERSION_MAJOR <
>>>>>>>>>>>>> 60)
>>>>>>>>>>>>> +#endif
>>>>>>>>>>>>>
>>>>>>>>>>>>> #endif /* AVCODEC_VERSION_H */
>>>>>>>>>>>>> -- 
>>>>>>>>>>>>> 2.30.2
>>>>>>>>>>>>>
>>>> _______________________________________________
>>>> ffmpeg-devel mailing list
>>>> ffmpeg-devel@ffmpeg.org
>>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>>
>>>> To unsubscribe, visit link above, or email
>>>> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
>>>>
>>>
>>> _______________________________________________
>>> 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".
>>
>> _______________________________________________
>> 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".
>>
> 
> _______________________________________________
> 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".

[FFmpeg-devel,1/3] avcodec/avpacket: add av_packet_resize()

Commit Message

Comments

Patch