diff mbox series

[FFmpeg-devel,2/2] avcodec/ac3enc: Avoid needlessly copying encoded packets around

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

Checks

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

Commit Message

Andreas Rheinhardt March 29, 2021, 5:12 p.m. UTC
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(-)

Comments

James Almer March 29, 2021, 5:16 p.m. UTC | #1
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);
>   
>
Andreas Rheinhardt March 29, 2021, 5:20 p.m. UTC | #2
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".
James Almer March 29, 2021, 5:29 p.m. UTC | #3
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".
>
Andreas Rheinhardt March 29, 2021, 5:35 p.m. UTC | #4
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".
James Almer March 29, 2021, 6:34 p.m. UTC | #5
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 mbox series

Patch

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);