diff mbox series

[FFmpeg-devel,02/23] lavc/movtextdec: simplify style record walk

Message ID 20200406175218.1299994-3-jstebbins@jetheaddev.com
State Accepted
Commit 2949f17e992be600c379112d4e84e8455bae180a
Headers show
Series [FFmpeg-devel,01/23] lavc/movtextdec: fix ass header colors | expand

Checks

Context Check Description
andriy/ffmpeg-patchwork success Make fate finished

Commit Message

John Stebbins April 6, 2020, 5:51 p.m. UTC 
It's not necessary to walk the style record list twice per subtitle
character.  style records are in order and do not overlap.
---
 libavcodec/movtextdec.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

Comments

Philip Langdale April 6, 2020, 9:46 p.m. UTC | #1 
On Mon, 6 Apr 2020 11:51:57 -0600
John Stebbins <jstebbins@jetheaddev.com> wrote:

> It's not necessary to walk the style record list twice per subtitle
> character.  style records are in order and do not overlap.
> ---
>  libavcodec/movtextdec.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c
> index 05becaf64d..47a8401119 100644
> --- a/libavcodec/movtextdec.c
> +++ b/libavcodec/movtextdec.c
> @@ -355,8 +355,9 @@ static int text_to_ass(AVBPrint *buf, const char
> *text, const char *text_end, {
>      MovTextContext *m = avctx->priv_data;
>      int i = 0;
> -    int j = 0;
>      int text_pos = 0;
> +    int style_active = 0;
> +    int entry = 0;
>  
>      if (text < text_end && m->box_flags & TWRP_BOX) {
>          if (m->w.wrap_flag == 1) {
> @@ -369,26 +370,27 @@ static int text_to_ass(AVBPrint *buf, const
> char *text, const char *text_end, while (text < text_end) {
>          int len;
>  
> -        if (m->box_flags & STYL_BOX) {
> -            for (i = 0; i < m->style_entries; i++) {
> -                if (m->s[i]->style_flag && text_pos ==
> m->s[i]->style_end) {
> -                    av_bprintf(buf, "{\\r}");
> +        if ((m->box_flags & STYL_BOX) && entry < m->style_entries) {
> +            if (text_pos == m->s[entry]->style_start) {
> +                style_active = 1;
> +                if (m->s[entry]->style_flag & STYLE_FLAG_BOLD)
> +                    av_bprintf(buf, "{\\b1}");
> +                if (m->s[entry]->style_flag & STYLE_FLAG_ITALIC)
> +                    av_bprintf(buf, "{\\i1}");
> +                if (m->s[entry]->style_flag & STYLE_FLAG_UNDERLINE)
> +                    av_bprintf(buf, "{\\u1}");
> +                av_bprintf(buf, "{\\fs%d}", m->s[entry]->fontsize);
> +                for (i = 0; i < m->ftab_entries; i++) {
> +                    if (m->s[entry]->style_fontID ==
> m->ftab[i]->fontID)
> +                        av_bprintf(buf, "{\\fn%s}",
> m->ftab[i]->font); }
>              }
> -            for (i = 0; i < m->style_entries; i++) {
> -                if (m->s[i]->style_flag && text_pos ==
> m->s[i]->style_start) {
> -                    if (m->s[i]->style_flag & STYLE_FLAG_BOLD)
> -                        av_bprintf(buf, "{\\b1}");
> -                    if (m->s[i]->style_flag & STYLE_FLAG_ITALIC)
> -                        av_bprintf(buf, "{\\i1}");
> -                    if (m->s[i]->style_flag & STYLE_FLAG_UNDERLINE)
> -                        av_bprintf(buf, "{\\u1}");
> -                    av_bprintf(buf, "{\\fs%d}", m->s[i]->fontsize);
> -                    for (j = 0; j < m->ftab_entries; j++) {
> -                        if (m->s[i]->style_fontID ==
> m->ftab[j]->fontID)
> -                            av_bprintf(buf, "{\\fn%s}",
> m->ftab[j]->font);
> -                    }
> +            if (text_pos == m->s[entry]->style_end) {
> +                if (style_active) {
> +                    av_bprintf(buf, "{\\r}");
> +                    style_active = 0;
>                  }
> +                entry++;
>              }
>          }
>          if (m->box_flags & HLIT_BOX) {

OK. I could not convince myself originally that styles could not
overlap, but I could easily have missed that in the spec.

--phil
John Stebbins April 6, 2020, 10:09 p.m. UTC | #2 
On Mon, 2020-04-06 at 14:46 -0700, Philip Langdale wrote:
> On Mon, 6 Apr 2020 11:51:57 -0600
> John Stebbins <jstebbins@jetheaddev.com> wrote:
> 
> > It's not necessary to walk the style record list twice per subtitle
> > character.  style records are in order and do not overlap.
> > ---
> >  libavcodec/movtextdec.c | 38 ++++++++++++++++++++-----------------
> > -
> >  1 file changed, 20 insertions(+), 18 deletions(-)
> > 
> > diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c
> > index 05becaf64d..47a8401119 100644
> > --- a/libavcodec/movtextdec.c
> > +++ b/libavcodec/movtextdec.c
> > @@ -355,8 +355,9 @@ static int text_to_ass(AVBPrint *buf, const
> > char
> > *text, const char *text_end, {
> >      MovTextContext *m = avctx->priv_data;
> >      int i = 0;
> > -    int j = 0;
> >      int text_pos = 0;
> > +    int style_active = 0;
> > +    int entry = 0;
> >  
> >      if (text < text_end && m->box_flags & TWRP_BOX) {
> >          if (m->w.wrap_flag == 1) {
> > @@ -369,26 +370,27 @@ static int text_to_ass(AVBPrint *buf, const
> > char *text, const char *text_end, while (text < text_end) {
> >          int len;
> >  
> > -        if (m->box_flags & STYL_BOX) {
> > -            for (i = 0; i < m->style_entries; i++) {
> > -                if (m->s[i]->style_flag && text_pos ==
> > m->s[i]->style_end) {
> > -                    av_bprintf(buf, "{\\r}");
> > +        if ((m->box_flags & STYL_BOX) && entry < m->style_entries) 
> > {
> > +            if (text_pos == m->s[entry]->style_start) {
> > +                style_active = 1;
> > +                if (m->s[entry]->style_flag & STYLE_FLAG_BOLD)
> > +                    av_bprintf(buf, "{\\b1}");
> > +                if (m->s[entry]->style_flag & STYLE_FLAG_ITALIC)
> > +                    av_bprintf(buf, "{\\i1}");
> > +                if (m->s[entry]->style_flag &
> > STYLE_FLAG_UNDERLINE)
> > +                    av_bprintf(buf, "{\\u1}");
> > +                av_bprintf(buf, "{\\fs%d}", m->s[entry]-
> > >fontsize);
> > +                for (i = 0; i < m->ftab_entries; i++) {
> > +                    if (m->s[entry]->style_fontID ==
> > m->ftab[i]->fontID)
> > +                        av_bprintf(buf, "{\\fn%s}",
> > m->ftab[i]->font); }
> >              }
> > -            for (i = 0; i < m->style_entries; i++) {
> > -                if (m->s[i]->style_flag && text_pos ==
> > m->s[i]->style_start) {
> > -                    if (m->s[i]->style_flag & STYLE_FLAG_BOLD)
> > -                        av_bprintf(buf, "{\\b1}");
> > -                    if (m->s[i]->style_flag & STYLE_FLAG_ITALIC)
> > -                        av_bprintf(buf, "{\\i1}");
> > -                    if (m->s[i]->style_flag &
> > STYLE_FLAG_UNDERLINE)
> > -                        av_bprintf(buf, "{\\u1}");
> > -                    av_bprintf(buf, "{\\fs%d}", m->s[i]-
> > >fontsize);
> > -                    for (j = 0; j < m->ftab_entries; j++) {
> > -                        if (m->s[i]->style_fontID ==
> > m->ftab[j]->fontID)
> > -                            av_bprintf(buf, "{\\fn%s}",
> > m->ftab[j]->font);
> > -                    }
> > +            if (text_pos == m->s[entry]->style_end) {
> > +                if (style_active) {
> > +                    av_bprintf(buf, "{\\r}");
> > +                    style_active = 0;
> >                  }
> > +                entry++;
> >              }
> >          }
> >          if (m->box_flags & HLIT_BOX) {
> 
> OK. I could not convince myself originally that styles could not
> overlap, but I could easily have missed that in the spec.
> 

I went back and double checked this in the spec before changing the
code.

"The styles shall be ordered by starting character offset, and the
starting offset of one style record shall be greater than or equal to
the ending character offset of the preceding record; styles records
shall not overlap their character ranges."
Philip Langdale April 6, 2020, 11:48 p.m. UTC | #3 
On Mon, 6 Apr 2020 22:09:22 +0000
John Stebbins <jstebbins@jetheaddev.com> wrote:

> 
> I went back and double checked this in the spec before changing the
> code.
> 
> "The styles shall be ordered by starting character offset, and the
> starting offset of one style record shall be greater than or equal to
> the ending character offset of the preceding record; styles records
> shall not overlap their character ranges."

Alright. Great! LGTM then.

--phil
diff mbox series

Patch

diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c
index 05becaf64d..47a8401119 100644
--- a/libavcodec/movtextdec.c
+++ b/libavcodec/movtextdec.c
@@ -355,8 +355,9 @@  static int text_to_ass(AVBPrint *buf, const char *text, const char *text_end,
 {
     MovTextContext *m = avctx->priv_data;
     int i = 0;
-    int j = 0;
     int text_pos = 0;
+    int style_active = 0;
+    int entry = 0;
 
     if (text < text_end && m->box_flags & TWRP_BOX) {
         if (m->w.wrap_flag == 1) {
@@ -369,26 +370,27 @@  static int text_to_ass(AVBPrint *buf, const char *text, const char *text_end,
     while (text < text_end) {
         int len;
 
-        if (m->box_flags & STYL_BOX) {
-            for (i = 0; i < m->style_entries; i++) {
-                if (m->s[i]->style_flag && text_pos == m->s[i]->style_end) {
-                    av_bprintf(buf, "{\\r}");
+        if ((m->box_flags & STYL_BOX) && entry < m->style_entries) {
+            if (text_pos == m->s[entry]->style_start) {
+                style_active = 1;
+                if (m->s[entry]->style_flag & STYLE_FLAG_BOLD)
+                    av_bprintf(buf, "{\\b1}");
+                if (m->s[entry]->style_flag & STYLE_FLAG_ITALIC)
+                    av_bprintf(buf, "{\\i1}");
+                if (m->s[entry]->style_flag & STYLE_FLAG_UNDERLINE)
+                    av_bprintf(buf, "{\\u1}");
+                av_bprintf(buf, "{\\fs%d}", m->s[entry]->fontsize);
+                for (i = 0; i < m->ftab_entries; i++) {
+                    if (m->s[entry]->style_fontID == m->ftab[i]->fontID)
+                        av_bprintf(buf, "{\\fn%s}", m->ftab[i]->font);
                 }
             }
-            for (i = 0; i < m->style_entries; i++) {
-                if (m->s[i]->style_flag && text_pos == m->s[i]->style_start) {
-                    if (m->s[i]->style_flag & STYLE_FLAG_BOLD)
-                        av_bprintf(buf, "{\\b1}");
-                    if (m->s[i]->style_flag & STYLE_FLAG_ITALIC)
-                        av_bprintf(buf, "{\\i1}");
-                    if (m->s[i]->style_flag & STYLE_FLAG_UNDERLINE)
-                        av_bprintf(buf, "{\\u1}");
-                    av_bprintf(buf, "{\\fs%d}", m->s[i]->fontsize);
-                    for (j = 0; j < m->ftab_entries; j++) {
-                        if (m->s[i]->style_fontID == m->ftab[j]->fontID)
-                            av_bprintf(buf, "{\\fn%s}", m->ftab[j]->font);
-                    }
+            if (text_pos == m->s[entry]->style_end) {
+                if (style_active) {
+                    av_bprintf(buf, "{\\r}");
+                    style_active = 0;
                 }
+                entry++;
             }
         }
         if (m->box_flags & HLIT_BOX) {