Message ID | 20210311155011.52961-1-jamrial@gmail.com |
---|---|
State | New |
Headers | show |
Series | [FFmpeg-devel,1/3] avcodec/avpacket: add av_packet_resize() | expand |
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".
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".
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".
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". >
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".
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". >
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 >>>>>>>
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". >
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 >>>>>>>>>
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". >
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".
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". >
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
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".
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
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.
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
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.
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. >
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.
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 [...]
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
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
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".
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".
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 */
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(-)