| Message ID | 20211219235700.16595-1-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/3] avcodec/ass: Faster ff_ass_add_rect() | expand |
| Context | Check | Description |
|---|---|---|
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | success | Make fate finished |
| andriy/make_ppc | success | Make finished |
| andriy/make_fate_ppc | success | Make fate finished |
On 12/19/2021 8:56 PM, Michael Niedermayer wrote: > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/ass.c | 33 +++++++++++++++++++++++++++------ > libavcodec/ass.h | 7 +++++++ > 2 files changed, 34 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/ass.c b/libavcodec/ass.c > index 725e4d42ba1..06714678722 100644 > --- a/libavcodec/ass.c > +++ b/libavcodec/ass.c > @@ -114,17 +114,31 @@ 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, unsigned *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 (sub->num_rects >= UINT_MAX) > return AVERROR(ENOMEM); > - sub->rects = rects; > + > + if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) { > + new_nb = FFMIN(sub->num_rects + sub->num_rects/16LL + 1, UINT_MAX); Isn't this what av_fast_realloc() is for? > + } else if (!nb_rect_allocated) > + new_nb = sub->num_rects + 1LL; > + > + 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 +151,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..4dffe923d9f 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, unsigned *nb_rect_allocated); > + > /** > * Helper to flush a text subtitles decoder making use of the > * FFASSDecoderContext.
Michael Niedermayer: > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/ass.c | 33 +++++++++++++++++++++++++++------ > libavcodec/ass.h | 7 +++++++ > 2 files changed, 34 insertions(+), 6 deletions(-) > > diff --git a/libavcodec/ass.c b/libavcodec/ass.c > index 725e4d42ba1..06714678722 100644 > --- a/libavcodec/ass.c > +++ b/libavcodec/ass.c > @@ -114,17 +114,31 @@ 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, unsigned *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 (sub->num_rects >= UINT_MAX) > return AVERROR(ENOMEM); > - sub->rects = rects; > + > + if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) { > + new_nb = FFMIN(sub->num_rects + sub->num_rects/16LL + 1, UINT_MAX); This presumes that unsigned is not 64bits itself; I have no problem with this, so LGTM from me. Others may disagree. > + } else if (!nb_rect_allocated) > + new_nb = sub->num_rects + 1LL; +1 is enough (it has been checked that sub->num_rects is < UINT_MAX). > + > + 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 +151,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..4dffe923d9f 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, unsigned *nb_rect_allocated); > + > /** > * Helper to flush a text subtitles decoder making use of the > * FFASSDecoderContext. >
On Sun, Dec 19, 2021 at 09:00:27PM -0300, James Almer wrote: > > > On 12/19/2021 8:56 PM, Michael Niedermayer wrote: > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/ass.c | 33 +++++++++++++++++++++++++++------ > > libavcodec/ass.h | 7 +++++++ > > 2 files changed, 34 insertions(+), 6 deletions(-) > > > > diff --git a/libavcodec/ass.c b/libavcodec/ass.c > > index 725e4d42ba1..06714678722 100644 > > --- a/libavcodec/ass.c > > +++ b/libavcodec/ass.c > > @@ -114,17 +114,31 @@ 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, unsigned *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 (sub->num_rects >= UINT_MAX) > > return AVERROR(ENOMEM); > > - sub->rects = rects; > > + > > + if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) { > > + new_nb = FFMIN(sub->num_rects + sub->num_rects/16LL + 1, UINT_MAX); > > Isn't this what av_fast_realloc() is for? No, its what av_fast_realloc_array() would be for but that is not in git yet I think we should wait for that to become available before changing the used function (1 change less / less work) thx [...]
On Mon, Dec 20, 2021 at 01:39:47AM +0100, Andreas Rheinhardt wrote: > Michael Niedermayer: > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/ass.c | 33 +++++++++++++++++++++++++++------ > > libavcodec/ass.h | 7 +++++++ > > 2 files changed, 34 insertions(+), 6 deletions(-) > > > > diff --git a/libavcodec/ass.c b/libavcodec/ass.c > > index 725e4d42ba1..06714678722 100644 > > --- a/libavcodec/ass.c > > +++ b/libavcodec/ass.c > > @@ -114,17 +114,31 @@ 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, unsigned *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 (sub->num_rects >= UINT_MAX) > > return AVERROR(ENOMEM); > > - sub->rects = rects; > > + > > + if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) { > > + new_nb = FFMIN(sub->num_rects + sub->num_rects/16LL + 1, UINT_MAX); > > This presumes that unsigned is not 64bits itself; I have no problem with > this, so LGTM from me. Others may disagree. if unsigned is 64bit how would you exploit this ? for this to overflow you would first need close to 2^64 successfully allocated rectangles. Thats both alot of space and time. I mean if every human on earth had a 32gb stick then all these together in a single computer would not be enough to allow this to succeed and that has to happen before the function is changed to the correct size_t type as my previous patch did. will post a new patch thx [...]
diff --git a/libavcodec/ass.c b/libavcodec/ass.c index 725e4d42ba1..06714678722 100644 --- a/libavcodec/ass.c +++ b/libavcodec/ass.c @@ -114,17 +114,31 @@ 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, unsigned *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 (sub->num_rects >= UINT_MAX) return AVERROR(ENOMEM); - sub->rects = rects; + + if (nb_rect_allocated && *nb_rect_allocated <= sub->num_rects) { + new_nb = FFMIN(sub->num_rects + sub->num_rects/16LL + 1, UINT_MAX); + } else if (!nb_rect_allocated) + new_nb = sub->num_rects + 1LL; + + 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 +151,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..4dffe923d9f 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, unsigned *nb_rect_allocated); + /** * Helper to flush a text subtitles decoder making use of the * FFASSDecoderContext.
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/ass.c | 33 +++++++++++++++++++++++++++------ libavcodec/ass.h | 7 +++++++ 2 files changed, 34 insertions(+), 6 deletions(-)