| Message ID | HE1PR0301MB2154359908F97DA8FD7A57858F7E9@HE1PR0301MB2154.eurprd03.prod.outlook.com |
|---|---|
| State | Accepted |
| Commit | 5d4234b3ea376c59a53a9ab6fb69aae8c5ee8fac |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/ac3enc: Use actual size of buffer in init_put_bits() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
| andriy/PPC64_make | success | Make finished |
| andriy/PPC64_make_fate | success | Make fate finished |
On 3/29/2021 2:12 PM, Andreas Rheinhardt wrote: > AC-3 and EAC-3 are codecs whose packet sizes are known in advance, > so one can use the min_size parameter of ff_alloc_packet2() to > allocate exactly this amount. This avoids a memcpy later in > av_packet_make_refcounted() in encode_simple_internal(). > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> > --- > Is there actually a reason not to use av_new_packet() (or > ff_get_encode_buffer()) directly? I'm waiting for the bump when avcodec_encode_*() are removed before making most encoders DR1, and thus use ff_get_encode_buffer(). And that includes this one. This patch LGTM, but it may not be worth applying if it's going to be changed again very soon. > > libavcodec/ac3enc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c > index 4cfd0afe12..fc4d45651d 100644 > --- a/libavcodec/ac3enc.c > +++ b/libavcodec/ac3enc.c > @@ -1759,7 +1759,8 @@ int ff_ac3_encode_frame_common_end(AVCodecContext *avctx, AVPacket *avpkt, > > ac3_quantize_mantissas(s); > > - if ((ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, 0)) < 0) > + ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, s->frame_size); > + if (ret < 0) > return ret; > ac3_output_frame(s, avpkt->data); > >
James Almer: > On 3/29/2021 2:12 PM, Andreas Rheinhardt wrote: >> AC-3 and EAC-3 are codecs whose packet sizes are known in advance, >> so one can use the min_size parameter of ff_alloc_packet2() to >> allocate exactly this amount. This avoids a memcpy later in >> av_packet_make_refcounted() in encode_simple_internal(). >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >> --- >> Is there actually a reason not to use av_new_packet() (or >> ff_get_encode_buffer()) directly? > > I'm waiting for the bump when avcodec_encode_*() are removed before > making most encoders DR1, and thus use ff_get_encode_buffer(). And that > includes this one. > > This patch LGTM, but it may not be worth applying if it's going to be > changed again very soon. > What are your plans for encoders that don't know the final size in advance and overallocate? >> >> libavcodec/ac3enc.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c >> index 4cfd0afe12..fc4d45651d 100644 >> --- a/libavcodec/ac3enc.c >> +++ b/libavcodec/ac3enc.c >> @@ -1759,7 +1759,8 @@ int >> ff_ac3_encode_frame_common_end(AVCodecContext *avctx, AVPacket *avpkt, >> ac3_quantize_mantissas(s); >> - if ((ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, 0)) < 0) >> + ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, s->frame_size); >> + if (ret < 0) >> return ret; >> ac3_output_frame(s, avpkt->data); >> > > _______________________________________________ > 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/29/2021 2:20 PM, Andreas Rheinhardt wrote: > James Almer: >> On 3/29/2021 2:12 PM, Andreas Rheinhardt wrote: >>> AC-3 and EAC-3 are codecs whose packet sizes are known in advance, >>> so one can use the min_size parameter of ff_alloc_packet2() to >>> allocate exactly this amount. This avoids a memcpy later in >>> av_packet_make_refcounted() in encode_simple_internal(). >>> >>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>> --- >>> Is there actually a reason not to use av_new_packet() (or >>> ff_get_encode_buffer()) directly? >> >> I'm waiting for the bump when avcodec_encode_*() are removed before >> making most encoders DR1, and thus use ff_get_encode_buffer(). And that >> includes this one. >> >> This patch LGTM, but it may not be worth applying if it's going to be >> changed again very soon. >> > > What are your plans for encoders that don't know the final size in > advance and overallocate? Keep using av_packet_shrink() like now. > >>> >>> libavcodec/ac3enc.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c >>> index 4cfd0afe12..fc4d45651d 100644 >>> --- a/libavcodec/ac3enc.c >>> +++ b/libavcodec/ac3enc.c >>> @@ -1759,7 +1759,8 @@ int >>> ff_ac3_encode_frame_common_end(AVCodecContext *avctx, AVPacket *avpkt, >>> ac3_quantize_mantissas(s); >>> - if ((ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, 0)) < 0) >>> + ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, s->frame_size); >>> + if (ret < 0) >>> return ret; >>> ac3_output_frame(s, avpkt->data); >>> >> >> _______________________________________________ >> 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/29/2021 2:20 PM, Andreas Rheinhardt wrote: >> James Almer: >>> On 3/29/2021 2:12 PM, Andreas Rheinhardt wrote: >>>> AC-3 and EAC-3 are codecs whose packet sizes are known in advance, >>>> so one can use the min_size parameter of ff_alloc_packet2() to >>>> allocate exactly this amount. This avoids a memcpy later in >>>> av_packet_make_refcounted() in encode_simple_internal(). >>>> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>> --- >>>> Is there actually a reason not to use av_new_packet() (or >>>> ff_get_encode_buffer()) directly? >>> >>> I'm waiting for the bump when avcodec_encode_*() are removed before >>> making most encoders DR1, and thus use ff_get_encode_buffer(). And that >>> includes this one. >>> >>> This patch LGTM, but it may not be worth applying if it's going to be >>> changed again very soon. >>> >> >> What are your plans for encoders that don't know the final size in >> advance and overallocate? > > Keep using av_packet_shrink() like now. > But only a few encoders only use av_shrink_packet(); most just use ff_alloc_packet2() with min_size == 0 (in which case the returned packet isn't refcounted) and just set the packet's size. The av_packet_make_refcounted() in encode_simple_internal() then allocates the final buffer (and zeroes the padding). So do you intend to return these oversized buffers to the user or do you intend to use ff_get_encode_buffer() for allocating the packet's buffer when it isn't refcounted? >> >>>> >>>> libavcodec/ac3enc.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c >>>> index 4cfd0afe12..fc4d45651d 100644 >>>> --- a/libavcodec/ac3enc.c >>>> +++ b/libavcodec/ac3enc.c >>>> @@ -1759,7 +1759,8 @@ int >>>> ff_ac3_encode_frame_common_end(AVCodecContext *avctx, AVPacket *avpkt, >>>> ac3_quantize_mantissas(s); >>>> - if ((ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, 0)) >>>> < 0) >>>> + ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, >>>> s->frame_size); >>>> + if (ret < 0) >>>> return ret; >>>> ac3_output_frame(s, avpkt->data); >>>> >>> >>> _______________________________________________ >>> 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/29/2021 2:35 PM, Andreas Rheinhardt wrote: > James Almer: >> On 3/29/2021 2:20 PM, Andreas Rheinhardt wrote: >>> James Almer: >>>> On 3/29/2021 2:12 PM, Andreas Rheinhardt wrote: >>>>> AC-3 and EAC-3 are codecs whose packet sizes are known in advance, >>>>> so one can use the min_size parameter of ff_alloc_packet2() to >>>>> allocate exactly this amount. This avoids a memcpy later in >>>>> av_packet_make_refcounted() in encode_simple_internal(). >>>>> >>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> >>>>> --- >>>>> Is there actually a reason not to use av_new_packet() (or >>>>> ff_get_encode_buffer()) directly? >>>> >>>> I'm waiting for the bump when avcodec_encode_*() are removed before >>>> making most encoders DR1, and thus use ff_get_encode_buffer(). And that >>>> includes this one. >>>> >>>> This patch LGTM, but it may not be worth applying if it's going to be >>>> changed again very soon. >>>> >>> >>> What are your plans for encoders that don't know the final size in >>> advance and overallocate? >> >> Keep using av_packet_shrink() like now. >> > > But only a few encoders only use av_shrink_packet(); most just use > ff_alloc_packet2() with min_size == 0 (in which case the returned packet > isn't refcounted) and just set the packet's size. The > av_packet_make_refcounted() in encode_simple_internal() then allocates > the final buffer (and zeroes the padding). > So do you intend to return these oversized buffers to the user or do you > intend to use ff_get_encode_buffer() for allocating the packet's buffer > when it isn't refcounted? If we want to ensure that buffers allocated with ff_get_encode_buffer() will be the exact required size on encoders that currently overallocate with ff_alloc_packet2() and min_size == 0, then yes, we can keep calling ff_alloc_packet2() to get the non refcounted packet with pkt->data pointing to the internal buffer, then effectively allocate the final buffer with ff_get_encode_buffer() once we know the actual packet size, and memcpy the data. But we could consider to instead propagate those oversized packets and save ourselves the memcpy, unless the overallocation is a really bad worst case scenario, like in cfhdenc. > >>> >>>>> >>>>> libavcodec/ac3enc.c | 3 ++- >>>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/libavcodec/ac3enc.c b/libavcodec/ac3enc.c >>>>> index 4cfd0afe12..fc4d45651d 100644 >>>>> --- a/libavcodec/ac3enc.c >>>>> +++ b/libavcodec/ac3enc.c >>>>> @@ -1759,7 +1759,8 @@ int >>>>> ff_ac3_encode_frame_common_end(AVCodecContext *avctx, AVPacket *avpkt, >>>>> ac3_quantize_mantissas(s); >>>>> - if ((ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, 0)) >>>>> < 0) >>>>> + ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, >>>>> s->frame_size); >>>>> + if (ret < 0) >>>>> return ret; >>>>> ac3_output_frame(s, avpkt->data); >>>>> >>>> >>>> _______________________________________________ >>>> 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/ac3enc.c b/libavcodec/ac3enc.c index 4cfd0afe12..fc4d45651d 100644 --- a/libavcodec/ac3enc.c +++ b/libavcodec/ac3enc.c @@ -1759,7 +1759,8 @@ int ff_ac3_encode_frame_common_end(AVCodecContext *avctx, AVPacket *avpkt, ac3_quantize_mantissas(s); - if ((ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, 0)) < 0) + ret = ff_alloc_packet2(avctx, avpkt, s->frame_size, s->frame_size); + if (ret < 0) return ret; ac3_output_frame(s, avpkt->data);
AC-3 and EAC-3 are codecs whose packet sizes are known in advance, so one can use the min_size parameter of ff_alloc_packet2() to allocate exactly this amount. This avoids a memcpy later in av_packet_make_refcounted() in encode_simple_internal(). Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com> --- Is there actually a reason not to use av_new_packet() (or ff_get_encode_buffer()) directly? libavcodec/ac3enc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)