Message ID | 20170613130954.11348-1-michael@niedermayer.cc |
---|---|
State | New |
Headers | show |
On Tue, 13 Jun 2017 15:09:54 +0200 Michael Niedermayer <michael@niedermayer.cc> wrote: > Simplifies code but changes handling of multiple an tags > > Suggested-by: wm4 > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/htmlsubtitles.c | 11 +++++------ > tests/ref/fate/sub-srt | 2 +- > 2 files changed, 6 insertions(+), 7 deletions(-) > > diff --git a/libavcodec/htmlsubtitles.c b/libavcodec/htmlsubtitles.c > index be5c9316ca..8bea867051 100644 > --- a/libavcodec/htmlsubtitles.c > +++ b/libavcodec/htmlsubtitles.c > @@ -53,15 +53,14 @@ static void rstrip_spaces_buf(AVBPrint *buf) > > /* skip all {\xxx} substrings except for {\an%d} > and all microdvd like styles such as {Y:xxx} */ > -static void handle_open_brace(AVBPrint *dst, const char **inp, int *an, int *closing_brace_missing) > +static void handle_open_brace(AVBPrint *dst, const char **inp, int *closing_brace_missing) > { > int len = 0; > const char *in = *inp; > - > - *an += sscanf(in, "{\\an%*1u}%n", &len) >= 0 && len > 0; > + int an = sscanf(in, "{\\an%*1u}%n", &len) >= 0 && len > 0; > > if (!*closing_brace_missing) { > - if ( (*an != 1 && in[1] == '\\') > + if ( (!an && in[1] == '\\') > || (in[1] && strchr("CcFfoPSsYy", in[1]) && in[2] == ':')) { > char *bracep = strchr(in+2, '}'); > if (bracep) { > @@ -78,7 +77,7 @@ static void handle_open_brace(AVBPrint *dst, const char **inp, int *an, int *clo > int ff_htmlmarkup_to_ass(void *log_ctx, AVBPrint *dst, const char *in) > { > char *param, buffer[128], tmp[128]; > - int len, tag_close, sptr = 1, line_start = 1, an = 0, end = 0; > + int len, tag_close, sptr = 1, line_start = 1, end = 0; > SrtStack stack[16]; > int closing_brace_missing = 0; > > @@ -105,7 +104,7 @@ int ff_htmlmarkup_to_ass(void *log_ctx, AVBPrint *dst, const char *in) > av_bprint_chars(dst, *in, 1); > break; > case '{': > - handle_open_brace(dst, &in, &an, &closing_brace_missing); > + handle_open_brace(dst, &in, &closing_brace_missing); > break; > case '<': > tag_close = in[1] == '/'; > diff --git a/tests/ref/fate/sub-srt b/tests/ref/fate/sub-srt > index 40b20cde90..c64f46864c 100644 > --- a/tests/ref/fate/sub-srt > +++ b/tests/ref/fate/sub-srt > @@ -24,7 +24,7 @@ Dialogue: 0,0:00:22.50,0:00:24.50,Default,,0,0,0,,Implementation is the same of > Dialogue: 0,0:00:22.50,0:00:24.50,Default,,0,0,0,,{\an5}This text should be at the\Nmiddle and horizontally centered > Dialogue: 0,0:00:22.50,0:00:24.50,Default,,0,0,0,,{\an2}This text should be at the\Nbottom and horizontally centered > Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,This text should be at the\Ntop and horizontally at the left{\an7} > -Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an4}This text should be at the\Nmiddle and horizontally at the left\N(The second position must be ignored) > +Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an4}This text should be at the\Nmiddle and horiz{\an6}ontally at the left\N(The second position must be ignored) > Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an1}This text should be at the\Nbottom and horizontally at the left > Dialogue: 0,0:00:26.50,0:00:28.50,Default,,0,0,0,,{\an9}This text should be at the\Ntop and horizontally at the right > Dialogue: 0,0:00:26.50,0:00:28.50,Default,,0,0,0,,{\an6}This text should be at the\Nmiddle and horizontally at the right Fine with me.
On Tue, Jun 13, 2017 at 03:09:54PM +0200, Michael Niedermayer wrote: [...] > -Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an4}This text should be at the\Nmiddle and horizontally at the left\N(The second position must be ignored) > +Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an4}This text should be at the\Nmiddle and horiz{\an6}ontally at the left\N(The second position must be ignored) "The second position must be ignored" I guess that was the use case.
On Tue, 13 Jun 2017 16:30:42 +0200 Clément Bœsch <u@pkh.me> wrote: > On Tue, Jun 13, 2017 at 03:09:54PM +0200, Michael Niedermayer wrote: > [...] > > -Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an4}This text should be at the\Nmiddle and horizontally at the left\N(The second position must be ignored) > > +Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an4}This text should be at the\Nmiddle and horiz{\an6}ontally at the left\N(The second position must be ignored) > > "The second position must be ignored" > > I guess that was the use case. > Who says that? VSFilter? Some other software? What does libass do? It's a VSFilter extension in SRT subtitles. FFmpeg just passes these along to libass. If libass has different behavior, it should be fixed, as it mostly strives to be VSFilter compatible. It makes not the slightest sense to "fix" this here.
On Tue, Jun 13, 2017 at 04:30:42PM +0200, Clément Bœsch wrote: > On Tue, Jun 13, 2017 at 03:09:54PM +0200, Michael Niedermayer wrote: > [...] > > -Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an4}This text should be at the\Nmiddle and horizontally at the left\N(The second position must be ignored) > > +Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an4}This text should be at the\Nmiddle and horiz{\an6}ontally at the left\N(The second position must be ignored) > > "The second position must be ignored" > > I guess that was the use case. Ill apply patches 1 and 2 and leave this one until theres a clear consensus thx [...]
diff --git a/libavcodec/htmlsubtitles.c b/libavcodec/htmlsubtitles.c index be5c9316ca..8bea867051 100644 --- a/libavcodec/htmlsubtitles.c +++ b/libavcodec/htmlsubtitles.c @@ -53,15 +53,14 @@ static void rstrip_spaces_buf(AVBPrint *buf) /* skip all {\xxx} substrings except for {\an%d} and all microdvd like styles such as {Y:xxx} */ -static void handle_open_brace(AVBPrint *dst, const char **inp, int *an, int *closing_brace_missing) +static void handle_open_brace(AVBPrint *dst, const char **inp, int *closing_brace_missing) { int len = 0; const char *in = *inp; - - *an += sscanf(in, "{\\an%*1u}%n", &len) >= 0 && len > 0; + int an = sscanf(in, "{\\an%*1u}%n", &len) >= 0 && len > 0; if (!*closing_brace_missing) { - if ( (*an != 1 && in[1] == '\\') + if ( (!an && in[1] == '\\') || (in[1] && strchr("CcFfoPSsYy", in[1]) && in[2] == ':')) { char *bracep = strchr(in+2, '}'); if (bracep) { @@ -78,7 +77,7 @@ static void handle_open_brace(AVBPrint *dst, const char **inp, int *an, int *clo int ff_htmlmarkup_to_ass(void *log_ctx, AVBPrint *dst, const char *in) { char *param, buffer[128], tmp[128]; - int len, tag_close, sptr = 1, line_start = 1, an = 0, end = 0; + int len, tag_close, sptr = 1, line_start = 1, end = 0; SrtStack stack[16]; int closing_brace_missing = 0; @@ -105,7 +104,7 @@ int ff_htmlmarkup_to_ass(void *log_ctx, AVBPrint *dst, const char *in) av_bprint_chars(dst, *in, 1); break; case '{': - handle_open_brace(dst, &in, &an, &closing_brace_missing); + handle_open_brace(dst, &in, &closing_brace_missing); break; case '<': tag_close = in[1] == '/'; diff --git a/tests/ref/fate/sub-srt b/tests/ref/fate/sub-srt index 40b20cde90..c64f46864c 100644 --- a/tests/ref/fate/sub-srt +++ b/tests/ref/fate/sub-srt @@ -24,7 +24,7 @@ Dialogue: 0,0:00:22.50,0:00:24.50,Default,,0,0,0,,Implementation is the same of Dialogue: 0,0:00:22.50,0:00:24.50,Default,,0,0,0,,{\an5}This text should be at the\Nmiddle and horizontally centered Dialogue: 0,0:00:22.50,0:00:24.50,Default,,0,0,0,,{\an2}This text should be at the\Nbottom and horizontally centered Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,This text should be at the\Ntop and horizontally at the left{\an7} -Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an4}This text should be at the\Nmiddle and horizontally at the left\N(The second position must be ignored) +Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an4}This text should be at the\Nmiddle and horiz{\an6}ontally at the left\N(The second position must be ignored) Dialogue: 0,0:00:24.50,0:00:26.50,Default,,0,0,0,,{\an1}This text should be at the\Nbottom and horizontally at the left Dialogue: 0,0:00:26.50,0:00:28.50,Default,,0,0,0,,{\an9}This text should be at the\Ntop and horizontally at the right Dialogue: 0,0:00:26.50,0:00:28.50,Default,,0,0,0,,{\an6}This text should be at the\Nmiddle and horizontally at the right
Simplifies code but changes handling of multiple an tags Suggested-by: wm4 Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/htmlsubtitles.c | 11 +++++------ tests/ref/fate/sub-srt | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-)