| Message ID | 20180324211153.2440-1-jamrial@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Sat, 24 Mar 2018 18:11:53 -0300 James Almer <jamrial@gmail.com> wrote: > Signed-off-by: James Almer <jamrial@gmail.com> > --- > This is a good time to deprecate this function and introduce a > replacement using the correct av_packet namespace and this time > returning an int. > What would be better > > int av_packet_shrink(AVPacket *pkt, int size); > > Or > > int av_packet_resize(AVPacket *pkt, int size); Seems better. > > The latter would be a combination of both the current shrink and grow > functions. > > libavcodec/avpacket.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > index 0693ca6f62..7faa082395 100644 > --- a/libavcodec/avpacket.c > +++ b/libavcodec/avpacket.c > @@ -100,9 +100,12 @@ int av_new_packet(AVPacket *pkt, int size) > > void av_shrink_packet(AVPacket *pkt, int size) > { > + int packet_is_writable; > if (pkt->size <= size) > return; > pkt->size = size; > + packet_is_writable = !av_packet_make_writable(pkt); > + av_assert0(packet_is_writable); > memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE); > } > LGTM
On 3/24/2018 6:46 PM, wm4 wrote: > On Sat, 24 Mar 2018 18:11:53 -0300 > James Almer <jamrial@gmail.com> wrote: > >> Signed-off-by: James Almer <jamrial@gmail.com> >> --- >> This is a good time to deprecate this function and introduce a >> replacement using the correct av_packet namespace and this time >> returning an int. >> What would be better >> >> int av_packet_shrink(AVPacket *pkt, int size); >> >> Or >> > >> int av_packet_resize(AVPacket *pkt, int size); > > Seems better. > >> >> The latter would be a combination of both the current shrink and grow >> functions. >> >> libavcodec/avpacket.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >> index 0693ca6f62..7faa082395 100644 >> --- a/libavcodec/avpacket.c >> +++ b/libavcodec/avpacket.c >> @@ -100,9 +100,12 @@ int av_new_packet(AVPacket *pkt, int size) >> >> void av_shrink_packet(AVPacket *pkt, int size) >> { >> + int packet_is_writable; >> if (pkt->size <= size) >> return; >> pkt->size = size; >> + packet_is_writable = !av_packet_make_writable(pkt); >> + av_assert0(packet_is_writable); >> memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE); >> } >> > > LGTM Ok, but i just realized that i can't apply this without first writing an internal variant specifically for the encoders that makes sure pkt->data != avctx->internal->byte_buffer before trying to do anything, otherwise the supposed benefits of that weird internal buffer code in ff_alloc_packet2() would be lost. Will look at that later.
On Sat, 24 Mar 2018 22:39:45 -0300 James Almer <jamrial@gmail.com> wrote: > On 3/24/2018 6:46 PM, wm4 wrote: > > On Sat, 24 Mar 2018 18:11:53 -0300 > > James Almer <jamrial@gmail.com> wrote: > > > >> Signed-off-by: James Almer <jamrial@gmail.com> > >> --- > >> This is a good time to deprecate this function and introduce a > >> replacement using the correct av_packet namespace and this time > >> returning an int. > >> What would be better > >> > >> int av_packet_shrink(AVPacket *pkt, int size); > >> > >> Or > >> > > > >> int av_packet_resize(AVPacket *pkt, int size); > > > > Seems better. > > > >> > >> The latter would be a combination of both the current shrink and grow > >> functions. > >> > >> libavcodec/avpacket.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c > >> index 0693ca6f62..7faa082395 100644 > >> --- a/libavcodec/avpacket.c > >> +++ b/libavcodec/avpacket.c > >> @@ -100,9 +100,12 @@ int av_new_packet(AVPacket *pkt, int size) > >> > >> void av_shrink_packet(AVPacket *pkt, int size) > >> { > >> + int packet_is_writable; > >> if (pkt->size <= size) > >> return; > >> pkt->size = size; > >> + packet_is_writable = !av_packet_make_writable(pkt); > >> + av_assert0(packet_is_writable); > >> memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE); > >> } > >> > > > > LGTM > > Ok, but i just realized that i can't apply this without first writing an > internal variant specifically for the encoders that makes sure pkt->data > != avctx->internal->byte_buffer before trying to do anything, otherwise > the supposed benefits of that weird internal buffer code in > ff_alloc_packet2() would be lost. > > Will look at that later. Didn't look at it again, but this mechanism might be broken anyway since the new encode API must return refcounted packets. Maybe the new dynamic sized buffer pool would help.
On 3/25/2018 9:14 AM, wm4 wrote: > On Sat, 24 Mar 2018 22:39:45 -0300 > James Almer <jamrial@gmail.com> wrote: > >> On 3/24/2018 6:46 PM, wm4 wrote: >>> On Sat, 24 Mar 2018 18:11:53 -0300 >>> James Almer <jamrial@gmail.com> wrote: >>> >>>> Signed-off-by: James Almer <jamrial@gmail.com> >>>> --- >>>> This is a good time to deprecate this function and introduce a >>>> replacement using the correct av_packet namespace and this time >>>> returning an int. >>>> What would be better >>>> >>>> int av_packet_shrink(AVPacket *pkt, int size); >>>> >>>> Or >>>> >>> >>>> int av_packet_resize(AVPacket *pkt, int size); >>> >>> Seems better. >>> >>>> >>>> The latter would be a combination of both the current shrink and grow >>>> functions. >>>> >>>> libavcodec/avpacket.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c >>>> index 0693ca6f62..7faa082395 100644 >>>> --- a/libavcodec/avpacket.c >>>> +++ b/libavcodec/avpacket.c >>>> @@ -100,9 +100,12 @@ int av_new_packet(AVPacket *pkt, int size) >>>> >>>> void av_shrink_packet(AVPacket *pkt, int size) >>>> { >>>> + int packet_is_writable; >>>> if (pkt->size <= size) >>>> return; >>>> pkt->size = size; >>>> + packet_is_writable = !av_packet_make_writable(pkt); >>>> + av_assert0(packet_is_writable); >>>> memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE); >>>> } >>>> >>> >>> LGTM >> >> Ok, but i just realized that i can't apply this without first writing an >> internal variant specifically for the encoders that makes sure pkt->data >> != avctx->internal->byte_buffer before trying to do anything, otherwise >> the supposed benefits of that weird internal buffer code in >> ff_alloc_packet2() would be lost. >> >> Will look at that later. > > Didn't look at it again, but this mechanism might be broken anyway > since the new encode API must return refcounted packets. > > Maybe the new dynamic sized buffer pool would help. Good point. Still waiting for suggestions about how to introduce that in the relevant thread, for that matter :p
diff --git a/libavcodec/avpacket.c b/libavcodec/avpacket.c index 0693ca6f62..7faa082395 100644 --- a/libavcodec/avpacket.c +++ b/libavcodec/avpacket.c @@ -100,9 +100,12 @@ int av_new_packet(AVPacket *pkt, int size) void av_shrink_packet(AVPacket *pkt, int size) { + int packet_is_writable; if (pkt->size <= size) return; pkt->size = size; + packet_is_writable = !av_packet_make_writable(pkt); + av_assert0(packet_is_writable); memset(pkt->data + size, 0, AV_INPUT_BUFFER_PADDING_SIZE); }
Signed-off-by: James Almer <jamrial@gmail.com> --- This is a good time to deprecate this function and introduce a replacement using the correct av_packet namespace and this time returning an int. What would be better int av_packet_shrink(AVPacket *pkt, int size); Or int av_packet_resize(AVPacket *pkt, int size); The latter would be a combination of both the current shrink and grow functions. libavcodec/avpacket.c | 3 +++ 1 file changed, 3 insertions(+)