diff mbox series

[FFmpeg-devel] libavcodec/ccaption_dec.c: Fixed indentation overwriting text in the cea608 caption decoder.

Message ID 20210127233320.364006-1-levi@snapstream.com
State New
Headers show
Series [FFmpeg-devel] libavcodec/ccaption_dec.c: Fixed indentation overwriting text in the cea608 caption decoder.
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

levi@snapstream.com Jan. 27, 2021, 11:33 p.m. UTC
From: Levi Dooley <i.am.stickfigure@gmail.com>

There was an assumption in the existing code that indentation would not occur more than once on the same row.
This was a bad assumption. There are examples of 608 streams which call handle_pac multiple times on the same row with different indentation.
As the code was before this change, the new indentation would overwrite existing text with spaces.
These changes make indentation skip over columns instead. Text gets cleared with spaces on handle_edm.
Instead of relying on the null character, trailing spaces are trimmed off the end of a row.
This is necessary so that a null character doesn't end up between two words.

Signed-off-by: Levi Dooley <i.am.stickfigure@gmail.com>
---
 libavcodec/ccaption_dec.c | 56 ++++++++++++++++++++++++++++++++-------
 tests/ref/fate/sub-scc    |  2 +-
 2 files changed, 47 insertions(+), 11 deletions(-)

Comments

Aman Karmani April 8, 2021, 8:24 p.m. UTC | #1
On Wed, Jan 27, 2021 at 3:40 PM <levi@snapstream.com> wrote:

> From: Levi Dooley <i.am.stickfigure@gmail.com>
>
> There was an assumption in the existing code that indentation would not
> occur more than once on the same row.
> This was a bad assumption. There are examples of 608 streams which call
> handle_pac multiple times on the same row with different indentation.
> As the code was before this change, the new indentation would overwrite
> existing text with spaces.
> These changes make indentation skip over columns instead. Text gets
> cleared with spaces on handle_edm.
> Instead of relying on the null character, trailing spaces are trimmed off
> the end of a row.
> This is necessary so that a null character doesn't end up between two
> words.
>
> Signed-off-by: Levi Dooley <i.am.stickfigure@gmail.com>
>

Tried this out and it looks good to me. I can confirm it fixes subtle
issues where content was previously being overwritten.

Aman


> ---
>  libavcodec/ccaption_dec.c | 56 ++++++++++++++++++++++++++++++++-------
>  tests/ref/fate/sub-scc    |  2 +-
>  2 files changed, 47 insertions(+), 11 deletions(-)
>
> diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c
> index a208e19b95..525e010897 100644
> --- a/libavcodec/ccaption_dec.c
> +++ b/libavcodec/ccaption_dec.c
> @@ -352,6 +352,17 @@ static void write_char(CCaptionSubContext *ctx,
> struct Screen *screen, char ch)
>      }
>  }
>
> +/**
> + * Increment the cursor_column by 1, and ensure that there are no null
> characters left behind in the row.
> + */
> +static void skip_char(CCaptionSubContext *ctx, struct Screen *screen)
> +{
> +    if (!screen->characters[ctx->cursor_row][ctx->cursor_column])
> +        write_char(ctx, screen, ' ');
> +    else
> +        ctx->cursor_column++;
> +}
> +
>  /**
>   * This function after validating parity bit, also remove it from data
> pair.
>   * The first byte doesn't pass parity, we replace it with a solid blank
> @@ -459,6 +470,7 @@ static int capture_screen(CCaptionSubContext *ctx)
>          if (CHECK_FLAG(screen->row_used, i)) {
>              const char *row = screen->characters[i];
>              const char *charset = screen->charsets[i];
> +
>              j = 0;
>              while (row[j] == ' ' && charset[j] == CCSET_BASIC_AMERICAN)
>                  j++;
> @@ -476,13 +488,19 @@ static int capture_screen(CCaptionSubContext *ctx)
>              const char *color = screen->colors[i];
>              const char *charset = screen->charsets[i];
>              const char *override;
> -            int x, y, seen_char = 0;
> +            int x, y, row_end, seen_char = 0;
>              j = 0;
>
>              /* skip leading space */
>              while (row[j] == ' ' && charset[j] == CCSET_BASIC_AMERICAN &&
> j < tab)
>                  j++;
>
> +            /* skip trailing space */
> +            row_end = SCREEN_COLUMNS-1;
> +            while (row_end >= 0 && row[row_end] == ' ' &&
> charset[row_end] == CCSET_BASIC_AMERICAN) {
> +                row_end--;
> +            }
> +
>              x = ASS_DEFAULT_PLAYRESX * (0.1 + 0.0250 * j);
>              y = ASS_DEFAULT_PLAYRESY * (0.1 + 0.0533 * i);
>              av_bprintf(&ctx->buffer[bidx], "{\\an7}{\\pos(%d,%d)}", x, y);
> @@ -490,7 +508,7 @@ static int capture_screen(CCaptionSubContext *ctx)
>              for (; j < SCREEN_COLUMNS; j++) {
>                  const char *e_tag = "", *s_tag = "", *c_tag = "", *b_tag
> = "";
>
> -                if (row[j] == 0)
> +                if (j > row_end || row[j] == 0)
>                      break;
>
>                  if (prev_font != font[j]) {
> @@ -624,7 +642,8 @@ static void handle_textattr(CCaptionSubContext *ctx,
> uint8_t hi, uint8_t lo)
>      ctx->cursor_font = pac2_attribs[i][1];
>
>      SET_FLAG(screen->row_used, ctx->cursor_row);
> -    write_char(ctx, screen, ' ');
> +
> +    skip_char(ctx, screen);
>  }
>
>  static void handle_pac(CCaptionSubContext *ctx, uint8_t hi, uint8_t lo)
> @@ -644,13 +663,13 @@ static void handle_pac(CCaptionSubContext *ctx,
> uint8_t hi, uint8_t lo)
>      lo &= 0x1f;
>
>      ctx->cursor_row = row_map[index] - 1;
> -    ctx->cursor_color =  pac2_attribs[lo][0];
> +    ctx->cursor_color = pac2_attribs[lo][0];
>      ctx->cursor_font = pac2_attribs[lo][1];
>      ctx->cursor_charset = CCSET_BASIC_AMERICAN;
>      ctx->cursor_column = 0;
>      indent = pac2_attribs[lo][2];
>      for (i = 0; i < indent; i++) {
> -        write_char(ctx, screen, ' ');
> +        skip_char(ctx, screen);
>      }
>  }
>
> @@ -667,6 +686,14 @@ static int handle_edm(CCaptionSubContext *ctx)
>      screen->row_used = 0;
>      ctx->bg_color = CCCOL_BLACK;
>
> +    for (int i = 0; i < SCREEN_ROWS+1; ++i) {
> +        memset(screen->characters[i], ' ',
> SCREEN_COLUMNS);
> +        memset(screen->colors[i],     CCCOL_WHITE,
> SCREEN_COLUMNS);
> +        memset(screen->bgs[i],        CCCOL_BLACK,
> SCREEN_COLUMNS);
> +        memset(screen->charsets[i],   CCSET_BASIC_AMERICAN,
> SCREEN_COLUMNS);
> +        memset(screen->fonts[i],      CCFONT_REGULAR,
>  SCREEN_COLUMNS);
> +    }
> +
>      // In realtime mode, emit an empty caption so the last one doesn't
>      // stay on the screen.
>      if (ctx->real_time)
> @@ -687,6 +714,7 @@ static int handle_eoc(CCaptionSubContext *ctx)
>          ret = handle_edm(ctx);
>
>      ctx->cursor_column = 0;
> +    ctx->cursor_row = 0;
>
>      // In realtime mode, we display the buffered contents (after
>      // flipping the buffer to active above) as soon as EOC arrives.
> @@ -731,7 +759,6 @@ static void handle_char(CCaptionSubContext *ctx, char
> hi, char lo)
>      if (lo) {
>          write_char(ctx, screen, lo);
>      }
> -    write_char(ctx, screen, 0);
>
>      if (ctx->mode != CCMODE_POPON)
>          ctx->screen_touched = 1;
> @@ -742,6 +769,18 @@ static void handle_char(CCaptionSubContext *ctx, char
> hi, char lo)
>         ff_dlog(ctx, "(%c)\n", hi);
>  }
>
> +static void handle_spacing(CCaptionSubContext *ctx, char lo)
> +{
> +    int i;
> +    struct Screen *screen = get_writing_screen(ctx);
> +
> +    SET_FLAG(screen->row_used, ctx->cursor_row);
> +
> +    for (i = 0; i < lo - 0x20; i++) {
> +        skip_char(ctx, screen);
> +    }
> +}
> +
>  static int process_cc608(CCaptionSubContext *ctx, uint8_t hi, uint8_t lo)
>  {
>      int ret = 0;
> @@ -823,11 +862,8 @@ static int process_cc608(CCaptionSubContext *ctx,
> uint8_t hi, uint8_t lo)
>          handle_char(ctx, hi, lo);
>          ctx->prev_cmd[0] = ctx->prev_cmd[1] = 0;
>      } else if (hi == 0x17 && lo >= 0x21 && lo <= 0x23) {
> -        int i;
>          /* Tab offsets (spacing) */
> -        for (i = 0; i < lo - 0x20; i++) {
> -            handle_char(ctx, ' ', 0);
> -        }
> +        handle_spacing(ctx, lo);
>      } else {
>          /* Ignoring all other non data code */
>          ff_dlog(ctx, "Unknown command 0x%hhx 0x%hhx\n", hi, lo);
> diff --git a/tests/ref/fate/sub-scc b/tests/ref/fate/sub-scc
> index 62cbf6fa4a..e4c1a1d004 100644
> --- a/tests/ref/fate/sub-scc
> +++ b/tests/ref/fate/sub-scc
> @@ -16,7 +16,7 @@ Dialogue:
> 0,0:00:00.69,0:00:03.29,Default,,0,0,0,,{\an7}{\pos(115,228)}[ Crowd ]
>  Dialogue: 0,0:00:03.30,0:00:07.07,Default,,0,0,0,,{\an7}{\pos(38,197)}HOW
> DO YOU KNOW\N{\an7}{\pos(38,213)}SHE IS A WITCH ?\N{\an7}{\pos(153,243)}SHE
> LOOKS LIKE ONE !
>  Dialogue: 0,0:00:07.07,0:00:09.27,Default,,0,0,0,,{\an7}{\pos(192,228)}[
> Shouting\N{\an7}{\pos(192,243)}\h\hAffirmations ]
>  Dialogue:
> 0,0:00:09.26,0:00:11.06,Default,,0,0,0,,{\an7}{\pos(38,243)}BRING HER
> FORWARD.
> -Dialogue:
> 0,0:00:11.07,0:00:14.27,Default,,0,0,0,,{\an7}{\pos(115,228)}I’M NOT A
> WITCH.\N{\an7}{\pos(115,243)}\hI’M{\i1} NOT{\i0} A WITCH.
> +Dialogue:
> 0,0:00:11.07,0:00:14.27,Default,,0,0,0,,{\an7}{\pos(115,228)}I’M NOT A
> WITCH.\N{\an7}{\pos(115,243)}\hI’M {\i1}NOT{\i0} A WITCH.
>  Dialogue: 0,0:00:14.26,0:00:16.03,Default,,0,0,0,,{\an7}{\pos(38,228)}BUT
> YOU ARE DRESSED\N{\an7}{\pos(38,243)}AS ONE.
>  Dialogue:
> 0,0:00:16.03,0:00:19.03,Default,,0,0,0,,{\an7}{\pos(76,197)}THEY DRESSED ME
> UP\N{\an7}{\pos(76,213)}LIKE THIS.\N{\an7}{\pos(76,243)}\h\h\h\h\h\h\h\hNO
> !  WE DIDN’T !
>  Dialogue:
> 0,0:00:19.03,0:00:22.95,Default,,0,0,0,,{\an7}{\pos(115,228)}AND THIS ISN’T
> MY NOSE.\N{\an7}{\pos(115,243)}IT’S A FALSE ONE.
> --
> 2.25.1
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
diff mbox series

Patch

diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c
index a208e19b95..525e010897 100644
--- a/libavcodec/ccaption_dec.c
+++ b/libavcodec/ccaption_dec.c
@@ -352,6 +352,17 @@  static void write_char(CCaptionSubContext *ctx, struct Screen *screen, char ch)
     }
 }
 
+/**
+ * Increment the cursor_column by 1, and ensure that there are no null characters left behind in the row.
+ */
+static void skip_char(CCaptionSubContext *ctx, struct Screen *screen)
+{
+    if (!screen->characters[ctx->cursor_row][ctx->cursor_column])
+        write_char(ctx, screen, ' ');
+    else
+        ctx->cursor_column++;
+}
+
 /**
  * This function after validating parity bit, also remove it from data pair.
  * The first byte doesn't pass parity, we replace it with a solid blank
@@ -459,6 +470,7 @@  static int capture_screen(CCaptionSubContext *ctx)
         if (CHECK_FLAG(screen->row_used, i)) {
             const char *row = screen->characters[i];
             const char *charset = screen->charsets[i];
+
             j = 0;
             while (row[j] == ' ' && charset[j] == CCSET_BASIC_AMERICAN)
                 j++;
@@ -476,13 +488,19 @@  static int capture_screen(CCaptionSubContext *ctx)
             const char *color = screen->colors[i];
             const char *charset = screen->charsets[i];
             const char *override;
-            int x, y, seen_char = 0;
+            int x, y, row_end, seen_char = 0;
             j = 0;
 
             /* skip leading space */
             while (row[j] == ' ' && charset[j] == CCSET_BASIC_AMERICAN && j < tab)
                 j++;
 
+            /* skip trailing space */
+            row_end = SCREEN_COLUMNS-1;
+            while (row_end >= 0 && row[row_end] == ' ' && charset[row_end] == CCSET_BASIC_AMERICAN) {
+                row_end--;
+            }
+
             x = ASS_DEFAULT_PLAYRESX * (0.1 + 0.0250 * j);
             y = ASS_DEFAULT_PLAYRESY * (0.1 + 0.0533 * i);
             av_bprintf(&ctx->buffer[bidx], "{\\an7}{\\pos(%d,%d)}", x, y);
@@ -490,7 +508,7 @@  static int capture_screen(CCaptionSubContext *ctx)
             for (; j < SCREEN_COLUMNS; j++) {
                 const char *e_tag = "", *s_tag = "", *c_tag = "", *b_tag = "";
 
-                if (row[j] == 0)
+                if (j > row_end || row[j] == 0)
                     break;
 
                 if (prev_font != font[j]) {
@@ -624,7 +642,8 @@  static void handle_textattr(CCaptionSubContext *ctx, uint8_t hi, uint8_t lo)
     ctx->cursor_font = pac2_attribs[i][1];
 
     SET_FLAG(screen->row_used, ctx->cursor_row);
-    write_char(ctx, screen, ' ');
+
+    skip_char(ctx, screen);
 }
 
 static void handle_pac(CCaptionSubContext *ctx, uint8_t hi, uint8_t lo)
@@ -644,13 +663,13 @@  static void handle_pac(CCaptionSubContext *ctx, uint8_t hi, uint8_t lo)
     lo &= 0x1f;
 
     ctx->cursor_row = row_map[index] - 1;
-    ctx->cursor_color =  pac2_attribs[lo][0];
+    ctx->cursor_color = pac2_attribs[lo][0];
     ctx->cursor_font = pac2_attribs[lo][1];
     ctx->cursor_charset = CCSET_BASIC_AMERICAN;
     ctx->cursor_column = 0;
     indent = pac2_attribs[lo][2];
     for (i = 0; i < indent; i++) {
-        write_char(ctx, screen, ' ');
+        skip_char(ctx, screen);
     }
 }
 
@@ -667,6 +686,14 @@  static int handle_edm(CCaptionSubContext *ctx)
     screen->row_used = 0;
     ctx->bg_color = CCCOL_BLACK;
 
+    for (int i = 0; i < SCREEN_ROWS+1; ++i) {
+        memset(screen->characters[i], ' ',                  SCREEN_COLUMNS);
+        memset(screen->colors[i],     CCCOL_WHITE,          SCREEN_COLUMNS);
+        memset(screen->bgs[i],        CCCOL_BLACK,          SCREEN_COLUMNS);
+        memset(screen->charsets[i],   CCSET_BASIC_AMERICAN, SCREEN_COLUMNS);
+        memset(screen->fonts[i],      CCFONT_REGULAR,       SCREEN_COLUMNS);
+    }
+
     // In realtime mode, emit an empty caption so the last one doesn't
     // stay on the screen.
     if (ctx->real_time)
@@ -687,6 +714,7 @@  static int handle_eoc(CCaptionSubContext *ctx)
         ret = handle_edm(ctx);
 
     ctx->cursor_column = 0;
+    ctx->cursor_row = 0;
 
     // In realtime mode, we display the buffered contents (after
     // flipping the buffer to active above) as soon as EOC arrives.
@@ -731,7 +759,6 @@  static void handle_char(CCaptionSubContext *ctx, char hi, char lo)
     if (lo) {
         write_char(ctx, screen, lo);
     }
-    write_char(ctx, screen, 0);
 
     if (ctx->mode != CCMODE_POPON)
         ctx->screen_touched = 1;
@@ -742,6 +769,18 @@  static void handle_char(CCaptionSubContext *ctx, char hi, char lo)
        ff_dlog(ctx, "(%c)\n", hi);
 }
 
+static void handle_spacing(CCaptionSubContext *ctx, char lo)
+{
+    int i;
+    struct Screen *screen = get_writing_screen(ctx);
+
+    SET_FLAG(screen->row_used, ctx->cursor_row);
+
+    for (i = 0; i < lo - 0x20; i++) {
+        skip_char(ctx, screen);
+    }
+}
+
 static int process_cc608(CCaptionSubContext *ctx, uint8_t hi, uint8_t lo)
 {
     int ret = 0;
@@ -823,11 +862,8 @@  static int process_cc608(CCaptionSubContext *ctx, uint8_t hi, uint8_t lo)
         handle_char(ctx, hi, lo);
         ctx->prev_cmd[0] = ctx->prev_cmd[1] = 0;
     } else if (hi == 0x17 && lo >= 0x21 && lo <= 0x23) {
-        int i;
         /* Tab offsets (spacing) */
-        for (i = 0; i < lo - 0x20; i++) {
-            handle_char(ctx, ' ', 0);
-        }
+        handle_spacing(ctx, lo);
     } else {
         /* Ignoring all other non data code */
         ff_dlog(ctx, "Unknown command 0x%hhx 0x%hhx\n", hi, lo);
diff --git a/tests/ref/fate/sub-scc b/tests/ref/fate/sub-scc
index 62cbf6fa4a..e4c1a1d004 100644
--- a/tests/ref/fate/sub-scc
+++ b/tests/ref/fate/sub-scc
@@ -16,7 +16,7 @@  Dialogue: 0,0:00:00.69,0:00:03.29,Default,,0,0,0,,{\an7}{\pos(115,228)}[ Crowd ]
 Dialogue: 0,0:00:03.30,0:00:07.07,Default,,0,0,0,,{\an7}{\pos(38,197)}HOW DO YOU KNOW\N{\an7}{\pos(38,213)}SHE IS A WITCH ?\N{\an7}{\pos(153,243)}SHE LOOKS LIKE ONE !
 Dialogue: 0,0:00:07.07,0:00:09.27,Default,,0,0,0,,{\an7}{\pos(192,228)}[ Shouting\N{\an7}{\pos(192,243)}\h\hAffirmations ]
 Dialogue: 0,0:00:09.26,0:00:11.06,Default,,0,0,0,,{\an7}{\pos(38,243)}BRING HER FORWARD.
-Dialogue: 0,0:00:11.07,0:00:14.27,Default,,0,0,0,,{\an7}{\pos(115,228)}I’M NOT A WITCH.\N{\an7}{\pos(115,243)}\hI’M{\i1} NOT{\i0} A WITCH.
+Dialogue: 0,0:00:11.07,0:00:14.27,Default,,0,0,0,,{\an7}{\pos(115,228)}I’M NOT A WITCH.\N{\an7}{\pos(115,243)}\hI’M {\i1}NOT{\i0} A WITCH.
 Dialogue: 0,0:00:14.26,0:00:16.03,Default,,0,0,0,,{\an7}{\pos(38,228)}BUT YOU ARE DRESSED\N{\an7}{\pos(38,243)}AS ONE.
 Dialogue: 0,0:00:16.03,0:00:19.03,Default,,0,0,0,,{\an7}{\pos(76,197)}THEY DRESSED ME UP\N{\an7}{\pos(76,213)}LIKE THIS.\N{\an7}{\pos(76,243)}\h\h\h\h\h\h\h\hNO !  WE DIDN’T !
 Dialogue: 0,0:00:19.03,0:00:22.95,Default,,0,0,0,,{\an7}{\pos(115,228)}AND THIS ISN’T MY NOSE.\N{\an7}{\pos(115,243)}IT’S A FALSE ONE.