| 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. | expand |
| 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 |
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 --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.