[FFmpeg-devel,1/4] avcodec/ass: Faster ff_ass_add_rect()

Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/ass.c | 32 ++++++++++++++++++++++++++------
 libavcodec/ass.h |  7 +++++++
 2 files changed, 33 insertions(+), 6 deletions(-)

Michael Niedermayer:
> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> ---
>  libavcodec/ass.c | 32 ++++++++++++++++++++++++++------
>  libavcodec/ass.h |  7 +++++++
>  2 files changed, 33 insertions(+), 6 deletions(-)
> 
> diff --git a/libavcodec/ass.c b/libavcodec/ass.c
> index 725e4d42ba1..d0a4d678bb4 100644
> --- a/libavcodec/ass.c
> +++ b/libavcodec/ass.c
> @@ -114,17 +114,30 @@ char *ff_ass_get_dialog(int readorder, int layer, const char *style,
>                         speaker ? speaker : "", text);
>  }
>  
> -int ff_ass_add_rect(AVSubtitle *sub, const char *dialog,
> +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog,
>                      int readorder, int layer, const char *style,
> -                    const char *speaker)
> +                    const char *speaker, size_t *nb_rect_allocated)
>  {
> -    AVSubtitleRect **rects, *rect;
> +    AVSubtitleRect **rects = sub->rects, *rect;
>      char *ass_str;
> +    uint64_t new_nb = 0;
>  
> -    rects = av_realloc_array(sub->rects, sub->num_rects+1, sizeof(*sub->rects));
> -    if (!rects)
> +    if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) {
> +        new_nb = sub->num_rects + sub->num_rects/16LL + 1;
> +    } else if (!nb_rect_allocated)
> +        new_nb = sub->num_rects + 1LL;
> +    if (new_nb > SIZE_MAX)
>          return AVERROR(ENOMEM);

AVSubtitle.num_rects is unsigned, so this number should always be
bounded by UINT_MAX (and nb_rect_allocated can be a pointer to unsigned,
too).
(Feels like I should resurrect my old av_fast_realloc_array:
https://patchwork.ffmpeg.org/project/ffmpeg/patch/20200101132758.4452-1-andreas.rheinhardt@gmail.com/)

> -    sub->rects = rects;
> +
> +    if (new_nb) {
> +        rects = av_realloc_array(rects, new_nb, sizeof(*sub->rects));
> +        if (!rects)
> +            return AVERROR(ENOMEM);
> +        if (nb_rect_allocated)
> +            *nb_rect_allocated = new_nb;
> +        sub->rects = rects;
> +    }
> +
>      rect       = av_mallocz(sizeof(*rect));
>      if (!rect)
>          return AVERROR(ENOMEM);
> @@ -137,6 +150,13 @@ int ff_ass_add_rect(AVSubtitle *sub, const char *dialog,
>      return 0;
>  }
>  
> +int ff_ass_add_rect(AVSubtitle *sub, const char *dialog,
> +                    int readorder, int layer, const char *style,
> +                    const char *speaker)
> +{
> +    return ff_ass_add_rect2(sub, dialog, readorder, layer, style, speaker, NULL);
> +}
> +
>  void ff_ass_decoder_flush(AVCodecContext *avctx)
>  {
>      FFASSDecoderContext *s = avctx->priv_data;
> diff --git a/libavcodec/ass.h b/libavcodec/ass.h
> index 2c260e4e785..784f46c42c5 100644
> --- a/libavcodec/ass.h
> +++ b/libavcodec/ass.h
> @@ -118,6 +118,13 @@ int ff_ass_add_rect(AVSubtitle *sub, const char *dialog,
>                      int readorder, int layer, const char *style,
>                      const char *speaker);
>  
> +/**
> + * Add an ASS dialog to a subtitle.
> + */
> +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog,
> +                     int readorder, int layer, const char *style,
> +                     const char *speaker, size_t *nb_rect_allocated);
> +
>  /**
>   * Helper to flush a text subtitles decoder making use of the
>   * FFASSDecoderContext.
>

On Fri, Dec 17, 2021 at 11:15:06PM +0100, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
> > ---
> >  libavcodec/ass.c | 32 ++++++++++++++++++++++++++------
> >  libavcodec/ass.h |  7 +++++++
> >  2 files changed, 33 insertions(+), 6 deletions(-)
> > 
> > diff --git a/libavcodec/ass.c b/libavcodec/ass.c
> > index 725e4d42ba1..d0a4d678bb4 100644
> > --- a/libavcodec/ass.c
> > +++ b/libavcodec/ass.c
> > @@ -114,17 +114,30 @@ char *ff_ass_get_dialog(int readorder, int layer, const char *style,
> >                         speaker ? speaker : "", text);
> >  }
> >  
> > -int ff_ass_add_rect(AVSubtitle *sub, const char *dialog,
> > +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog,
> >                      int readorder, int layer, const char *style,
> > -                    const char *speaker)
> > +                    const char *speaker, size_t *nb_rect_allocated)
> >  {
> > -    AVSubtitleRect **rects, *rect;
> > +    AVSubtitleRect **rects = sub->rects, *rect;
> >      char *ass_str;
> > +    uint64_t new_nb = 0;
> >  
> > -    rects = av_realloc_array(sub->rects, sub->num_rects+1, sizeof(*sub->rects));
> > -    if (!rects)
> > +    if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) {
> > +        new_nb = sub->num_rects + sub->num_rects/16LL + 1;
> > +    } else if (!nb_rect_allocated)
> > +        new_nb = sub->num_rects + 1LL;
> > +    if (new_nb > SIZE_MAX)
> >          return AVERROR(ENOMEM);
> 
> AVSubtitle.num_rects is unsigned, so this number should always be
> bounded by UINT_MAX (and nb_rect_allocated can be a pointer to unsigned,
> too).

i had that initially but then wanted to move to better types but missed
this, changed it back locally

thx

[...]

Michael Niedermayer:
> On Fri, Dec 17, 2021 at 11:15:06PM +0100, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/ass.c | 32 ++++++++++++++++++++++++++------
>>>  libavcodec/ass.h |  7 +++++++
>>>  2 files changed, 33 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavcodec/ass.c b/libavcodec/ass.c
>>> index 725e4d42ba1..d0a4d678bb4 100644
>>> --- a/libavcodec/ass.c
>>> +++ b/libavcodec/ass.c
>>> @@ -114,17 +114,30 @@ char *ff_ass_get_dialog(int readorder, int layer, const char *style,
>>>                         speaker ? speaker : "", text);
>>>  }
>>>  
>>> -int ff_ass_add_rect(AVSubtitle *sub, const char *dialog,
>>> +int ff_ass_add_rect2(AVSubtitle *sub, const char *dialog,
>>>                      int readorder, int layer, const char *style,
>>> -                    const char *speaker)
>>> +                    const char *speaker, size_t *nb_rect_allocated)
>>>  {
>>> -    AVSubtitleRect **rects, *rect;
>>> +    AVSubtitleRect **rects = sub->rects, *rect;
>>>      char *ass_str;
>>> +    uint64_t new_nb = 0;
>>>  
>>> -    rects = av_realloc_array(sub->rects, sub->num_rects+1, sizeof(*sub->rects));
>>> -    if (!rects)
>>> +    if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) {
>>> +        new_nb = sub->num_rects + sub->num_rects/16LL + 1;
>>> +    } else if (!nb_rect_allocated)
>>> +        new_nb = sub->num_rects + 1LL;
>>> +    if (new_nb > SIZE_MAX)
>>>          return AVERROR(ENOMEM);
>>
>> AVSubtitle.num_rects is unsigned, so this number should always be
>> bounded by UINT_MAX (and nb_rect_allocated can be a pointer to unsigned,
>> too).
> 
> i had that initially but then wanted to move to better types but missed
> this, changed it back locally
> 
Simply changing it back is wrong, because it might happen that the old
num_rects is < UINT_MAX with new_nb > UINT_MAX. In this case one should
clip new_nb to UINT_MAX.
(This case can't happen currently: The loop in ccaption_dec can lead to
at most INT_MAX/3 rects; the other two ff_ass_add_rect() can bring this
to at most INT_MAX/3 + 2.)

- Andreas