[FFmpeg-devel] [PATCH 3/4] avformat/flvenc: Use array instead of linked list for index

[Michael Niedermayer]

On Sat, Oct 26, 2019 at 05:04:20AM +0200, Andreas Rheinhardt wrote:
> On Fri, Oct 25, 2019 at 10:44 PM Michael Niedermayer <michael at niedermayer.cc>
> wrote:
> 
> > On Fri, Oct 25, 2019 at 11:11:46AM +0200, Andreas Rheinhardt wrote:
> > > Using a linked list had very much overhead (the pointer to the next
> > > entry increased the size of the index entry struct from 16 to 24 bytes,
> > > not to mention the overhead of having separate allocations), so it is
> > > better to (re)allocate a continuous array for the index.
> > >
> > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt at gmail.com>
> > > ---
> > >  libavformat/flvenc.c | 58 +++++++++++++++-----------------------------
> > >  1 file changed, 19 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/libavformat/flvenc.c b/libavformat/flvenc.c
> > > index 0e6c66a5ff..a2bd791c59 100644
> > > --- a/libavformat/flvenc.c
> > > +++ b/libavformat/flvenc.c
> > > @@ -74,7 +74,6 @@ typedef enum {
> > >  typedef struct FLVFileposition {
> > >      int64_t keyframe_position;
> > >      double keyframe_timestamp;
> > > -    struct FLVFileposition *next;
> > >  } FLVFileposition;
> > >
> > >  typedef struct FLVContext {
> > > @@ -108,9 +107,9 @@ typedef struct FLVContext {
> > >      int acurframeindex;
> > >      int64_t keyframes_info_offset;
> > >
> > > -    int64_t filepositions_count;
> > >      FLVFileposition *filepositions;
> > > -    FLVFileposition *head_filepositions;
> > > +    size_t filepositions_allocated;
> > > +    int64_t filepositions_count;
> > >
> > >      AVCodecParameters *audio_par;
> > >      AVCodecParameters *video_par;
> > > @@ -549,27 +548,19 @@ static void
> > flv_write_codec_header(AVFormatContext* s, AVCodecParameters* par, i
> > >
> > >  static int flv_append_keyframe_info(AVFormatContext *s, FLVContext
> > *flv, double ts, int64_t pos)
> > >  {
> > > -    FLVFileposition *position = av_malloc(sizeof(FLVFileposition));
> > > -
> > > -    if (!position) {
> > > -        av_log(s, AV_LOG_WARNING, "no mem for add keyframe index!\n");
> > > -        return AVERROR(ENOMEM);
> > > -    }
> > > -
> > > -    position->keyframe_timestamp = ts;
> > > -    position->keyframe_position = pos;
> > > -
> > > -    if (!flv->filepositions_count) {
> > > -        flv->filepositions = position;
> > > -        flv->head_filepositions = flv->filepositions;
> > > -        position->next = NULL;
> > > -    } else {
> > > -        flv->filepositions->next = position;
> > > -        position->next = NULL;
> > > -        flv->filepositions = flv->filepositions->next;
> > > +    if (flv->filepositions_count >= flv->filepositions_allocated) {
> > > +        void *pos = av_realloc_array(flv->filepositions,
> > > +                                     2 * flv->filepositions_allocated +
> > 1,
> > > +                                     sizeof(*flv->filepositions));
> >
> > can the 2* overflow ?
> > av_fast_realloc() would check for that
> > i wonder if a av_fast_realloc_array() would make sense
> >
> >
> av_realloc_array checks that the multiplication doesn't overflow (it
> actually checks that
> the product fits in an int). Given that sizeof(*flv->filepositions) is
> bigger than 2, no overflow
> can happen in 2 * flv->filepositions_allocated + 1.

Iam unsure if that check by the type used in av_realloc_array() is a 
good idea but if you think its ok then please add a comment in the
code explaining this.

Thanks

[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

"Nothing to hide" only works if the folks in power share the values of
you and everyone you know entirely and always will -- Tom Scott

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <https://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20191109/4a6d1eab/attachment.sig>