| Message ID | 20240311004047.140612-1-marth64@proxyid.net |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/ccaption_dec: don't print multiple \pos tags per cue | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | fail | Make fate failed |
| andriy/make_x86 | success | Make finished |
| andriy/make_fate_x86 | fail | Make fate failed |
On date Sunday 2024-03-10 19:40:47 -0500, Marth64 wrote: > Currently, Closed Captions decoder prints multiple \pos ASS tags per line, > per cue. This is invalid behavior, because only the first \pos tag in a cue > is honored by ASS anyway. Don't write multiple \pos tags. > > Signed-off-by: Marth64 <marth64@proxyid.net> > --- > libavcodec/ccaption_dec.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c > index faf058ce97..9d4a93647c 100644 > --- a/libavcodec/ccaption_dec.c > +++ b/libavcodec/ccaption_dec.c > @@ -456,7 +456,7 @@ static void roll_up(CCaptionSubContext *ctx) > > static int capture_screen(CCaptionSubContext *ctx) > { > - int i, j, tab = 0; > + int i, j, tab = 0, seen_row = 0; > struct Screen *screen = ctx->screen + ctx->active_screen; > enum cc_font prev_font = CCFONT_REGULAR; > enum cc_color_code prev_color = CCCOL_WHITE; > @@ -496,7 +496,11 @@ static int capture_screen(CCaptionSubContext *ctx) > > 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); > + > + if (!seen_row) { > + av_bprintf(&ctx->buffer[bidx], "{\\an7}{\\pos(%d,%d)}", x, y); > + seen_row = 1; > + } > > for (; j < SCREEN_COLUMNS; j++) { > const char *e_tag = "", *s_tag = "", *c_tag = "", *b_tag = ""; LGTM, thanks.
On Sun, Mar 10, 2024 at 07:40:47PM -0500, Marth64 wrote: > Currently, Closed Captions decoder prints multiple \pos ASS tags per line, > per cue. This is invalid behavior, because only the first \pos tag in a cue > is honored by ASS anyway. Don't write multiple \pos tags. > > Signed-off-by: Marth64 <marth64@proxyid.net> > --- > libavcodec/ccaption_dec.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) This breaks fate --- ./tests/ref/fate/sub-cc-scte20 2024-03-05 02:37:36.223299993 +0100 +++ tests/data/fate/sub-cc-scte20 2024-03-11 22:13:08.091910294 +0100 @@ -13,5 +13,5 @@ [Events] Format: Layer, Start, End, Style, Name, MarginL, MarginR, MarginV, Effect, Text Dialogue: 0,0:00:00.00,0:00:01.44,Default,,0,0,0,,{\an7}{\pos(48,182)}BESIDES THE -Dialogue: 0,0:00:01.43,0:00:03.93,Default,,0,0,0,,{\an7}{\pos(38,166)}\hBESIDES THE \N{\an7}{\pos(38,197)}SPENDING AND THIS, IS THAT CAR -Dialogue: 0,0:00:03.94,0:00:06.31,Default,,0,0,0,,{\an7}{\pos(38,182)}SPENDING AND THIS, IS THAT CAR \N{\an7}{\pos(38,197)}MANUFACTURERS ARE ABOUT AS +Dialogue: 0,0:00:01.43,0:00:03.93,Default,,0,0,0,,{\an7}{\pos(38,166)}\hBESIDES THE \NSPENDING AND THIS, IS THAT CAR +Dialogue: 0,0:00:03.94,0:00:06.31,Default,,0,0,0,,{\an7}{\pos(38,182)}SPENDING AND THIS, IS THAT CAR \NMANUFACTURERS ARE ABOUT AS Test sub-cc-scte20 failed. Look at tests/data/fate/sub-cc-scte20.err for details. [...]
> This breaks fate
Looking into it - thanks.
PS, sorry for previous top-posting in replies. Will be more cautious of
that.
On 3/11/24 17:48, Marth64 wrote: >> This breaks fate > Looking into it - thanks. > It is possible that the change is correct, and that the fate test needs to be changed to reflect the correct behavior. Not necessarily, but possibly. - Leo Izen (Traneptora)
Thanks, will follow up with a v3 set (consolidating/resolving feedback on the other CC fixes as well).
diff --git a/libavcodec/ccaption_dec.c b/libavcodec/ccaption_dec.c index faf058ce97..9d4a93647c 100644 --- a/libavcodec/ccaption_dec.c +++ b/libavcodec/ccaption_dec.c @@ -456,7 +456,7 @@ static void roll_up(CCaptionSubContext *ctx) static int capture_screen(CCaptionSubContext *ctx) { - int i, j, tab = 0; + int i, j, tab = 0, seen_row = 0; struct Screen *screen = ctx->screen + ctx->active_screen; enum cc_font prev_font = CCFONT_REGULAR; enum cc_color_code prev_color = CCCOL_WHITE; @@ -496,7 +496,11 @@ static int capture_screen(CCaptionSubContext *ctx) 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); + + if (!seen_row) { + av_bprintf(&ctx->buffer[bidx], "{\\an7}{\\pos(%d,%d)}", x, y); + seen_row = 1; + } for (; j < SCREEN_COLUMNS; j++) { const char *e_tag = "", *s_tag = "", *c_tag = "", *b_tag = "";
Currently, Closed Captions decoder prints multiple \pos ASS tags per line, per cue. This is invalid behavior, because only the first \pos tag in a cue is honored by ASS anyway. Don't write multiple \pos tags. Signed-off-by: Marth64 <marth64@proxyid.net> --- libavcodec/ccaption_dec.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)