[FFmpeg-devel] avcodec/htmlsubtitles: drop an state

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(-)

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

[...]