diff mbox series

[FFmpeg-devel] avformat/movenc: Reduce size of the allocated MOVIentry array

Message ID 20200325175644.1458-1-jamrial@gmail.com
State New
Headers show
Series [FFmpeg-devel] avformat/movenc: Reduce size of the allocated MOVIentry array
Related show

Checks

Context Check Description
andriy/ffmpeg-patchwork pending
andriy/ffmpeg-patchwork success Applied patch
andriy/ffmpeg-patchwork success Configure finished
andriy/ffmpeg-patchwork success Make finished
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

James Almer March 25, 2020, 5:56 p.m. UTC
Increase it in a linear way instead.
Helps reduce memory usage, especially on long, non fragmented output.

Signed-off-by: James Almer <jamrial@gmail.com>
---
An alternative is using av_fast_realloc(), in a similar fashion as
ff_add_index_entry(), which will keep the memory usage more closely tied to the
amount of entries needed, but the amount of reallocations will be much higher,
so i don't know if the tradeof is beneficial.

 libavformat/movenc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Anton Khirnov March 26, 2020, 2:06 p.m. UTC | #1
Quoting James Almer (2020-03-25 18:56:44)
> Increase it in a linear way instead.
> Helps reduce memory usage, especially on long, non fragmented output.
> 
> Signed-off-by: James Almer <jamrial@gmail.com>
> ---
> An alternative is using av_fast_realloc(), in a similar fashion as
> ff_add_index_entry(), which will keep the memory usage more closely tied to the
> amount of entries needed, but the amount of reallocations will be much higher,
> so i don't know if the tradeof is beneficial.

Wait, wouldn't fast_realloc() do pretty much the same as your patch? Why
should the number of reallocations be higher?
James Almer March 26, 2020, 2:18 p.m. UTC | #2
On 3/26/2020 11:06 AM, Anton Khirnov wrote:
> Quoting James Almer (2020-03-25 18:56:44)
>> Increase it in a linear way instead.
>> Helps reduce memory usage, especially on long, non fragmented output.
>>
>> Signed-off-by: James Almer <jamrial@gmail.com>
>> ---
>> An alternative is using av_fast_realloc(), in a similar fashion as
>> ff_add_index_entry(), which will keep the memory usage more closely tied to the
>> amount of entries needed, but the amount of reallocations will be much higher,
>> so i don't know if the tradeof is beneficial.
> 
> Wait, wouldn't fast_realloc() do pretty much the same as your patch? Why
> should the number of reallocations be higher?

I meant mimicking the behavior of ff_add_index_entry(), doing something like

cluster = av_fast_realloc(trk->cluster,
                          &trk->cluster_capacity,
                          (trk->entry + 1) * sizeof(*trk->cluster));
trk->cluster = cluster;

Which will get rid of the fixed MOV_INDEX_CLUSTER_SIZE increase per
realloc, and resulting in more, smaller size increment reallocs (but of
course not after every single new entry, which is the entire point of
av_fast_realloc()).
Anton Khirnov March 26, 2020, 2:22 p.m. UTC | #3
Quoting James Almer (2020-03-26 15:18:53)
> On 3/26/2020 11:06 AM, Anton Khirnov wrote:
> > Quoting James Almer (2020-03-25 18:56:44)
> >> Increase it in a linear way instead.
> >> Helps reduce memory usage, especially on long, non fragmented output.
> >>
> >> Signed-off-by: James Almer <jamrial@gmail.com>
> >> ---
> >> An alternative is using av_fast_realloc(), in a similar fashion as
> >> ff_add_index_entry(), which will keep the memory usage more closely tied to the
> >> amount of entries needed, but the amount of reallocations will be much higher,
> >> so i don't know if the tradeof is beneficial.
> > 
> > Wait, wouldn't fast_realloc() do pretty much the same as your patch? Why
> > should the number of reallocations be higher?
> 
> I meant mimicking the behavior of ff_add_index_entry(), doing something like
> 
> cluster = av_fast_realloc(trk->cluster,
>                           &trk->cluster_capacity,
>                           (trk->entry + 1) * sizeof(*trk->cluster));
> trk->cluster = cluster;
> 
> Which will get rid of the fixed MOV_INDEX_CLUSTER_SIZE increase per
> realloc, and resulting in more, smaller size increment reallocs (but of
> course not after every single new entry, which is the entire point of
> av_fast_realloc()).

Ah, got it.

I'd say your patch is better, but it probably doesn't matter much.
James Almer March 26, 2020, 2:48 p.m. UTC | #4
On 3/26/2020 11:22 AM, Anton Khirnov wrote:
> Quoting James Almer (2020-03-26 15:18:53)
>> On 3/26/2020 11:06 AM, Anton Khirnov wrote:
>>> Quoting James Almer (2020-03-25 18:56:44)
>>>> Increase it in a linear way instead.
>>>> Helps reduce memory usage, especially on long, non fragmented output.
>>>>
>>>> Signed-off-by: James Almer <jamrial@gmail.com>
>>>> ---
>>>> An alternative is using av_fast_realloc(), in a similar fashion as
>>>> ff_add_index_entry(), which will keep the memory usage more closely tied to the
>>>> amount of entries needed, but the amount of reallocations will be much higher,
>>>> so i don't know if the tradeof is beneficial.
>>>
>>> Wait, wouldn't fast_realloc() do pretty much the same as your patch? Why
>>> should the number of reallocations be higher?
>>
>> I meant mimicking the behavior of ff_add_index_entry(), doing something like
>>
>> cluster = av_fast_realloc(trk->cluster,
>>                           &trk->cluster_capacity,
>>                           (trk->entry + 1) * sizeof(*trk->cluster));
>> trk->cluster = cluster;
>>
>> Which will get rid of the fixed MOV_INDEX_CLUSTER_SIZE increase per
>> realloc, and resulting in more, smaller size increment reallocs (but of
>> course not after every single new entry, which is the entire point of
>> av_fast_realloc()).
> 
> Ah, got it.
> 
> I'd say your patch is better, but it probably doesn't matter much.

Applied then, thanks.
diff mbox series

Patch

diff --git a/libavformat/movenc.c b/libavformat/movenc.c
index 85df5d1374..ce82acf914 100644
--- a/libavformat/movenc.c
+++ b/libavformat/movenc.c
@@ -5559,7 +5559,7 @@  int ff_mov_write_packet(AVFormatContext *s, AVPacket *pkt)
     }
 
     if (trk->entry >= trk->cluster_capacity) {
-        unsigned new_capacity = 2 * (trk->entry + MOV_INDEX_CLUSTER_SIZE);
+        unsigned new_capacity = trk->entry + MOV_INDEX_CLUSTER_SIZE;
         if (av_reallocp_array(&trk->cluster, new_capacity,
                               sizeof(*trk->cluster))) {
             ret = AVERROR(ENOMEM);