| Message ID | 20200325175644.1458-1-jamrial@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] avformat/movenc: Reduce size of the allocated MOVIentry array | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
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?
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()).
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.
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 --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);
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(-)