[FFmpeg-devel] [PATCH] avcodec/htmlsubtitles: Factor open brace handling into its own function

[wm4]

On Tue, 13 Jun 2017 00:01:04 +0200
Michael Niedermayer <michael at niedermayer.cc> wrote:

> Suggested-by: wm4
> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> ---
>  libavcodec/htmlsubtitles.c | 44 ++++++++++++++++++++++++++------------------
>  1 file changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/libavcodec/htmlsubtitles.c b/libavcodec/htmlsubtitles.c
> index 70311c66d5..be5c9316ca 100644
> --- a/libavcodec/htmlsubtitles.c
> +++ b/libavcodec/htmlsubtitles.c
> @@ -51,6 +51,30 @@ static void rstrip_spaces_buf(AVBPrint *buf)
>              buf->str[--buf->len] = 0;
>  }
>  
> +/* 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)
> +{
> +    int len = 0;
> +    const char *in = *inp;
> +
> +    *an += sscanf(in, "{\\an%*1u}%n", &len) >= 0 && len > 0;
> +
> +    if (!*closing_brace_missing) {
> +        if (   (*an != 1 && in[1] == '\\')
> +            || (in[1] && strchr("CcFfoPSsYy", in[1]) && in[2] == ':')) {
> +            char *bracep = strchr(in+2, '}');
> +            if (bracep) {
> +                *inp = bracep;
> +                return;
> +            } else
> +                *closing_brace_missing = 1;
> +        }
> +    }
> +
> +    av_bprint_chars(dst, *in, 1);
> +}
> +
>  int ff_htmlmarkup_to_ass(void *log_ctx, AVBPrint *dst, const char *in)
>  {
>      char *param, buffer[128], tmp[128];
> @@ -80,24 +104,8 @@ int ff_htmlmarkup_to_ass(void *log_ctx, AVBPrint *dst, const char *in)
>              if (!line_start)
>                  av_bprint_chars(dst, *in, 1);
>              break;
> -        case '{':    /* skip all {\xxx} substrings except for {\an%d}
> -                        and all microdvd like styles such as {Y:xxx} */
> -            len = 0;
> -            an += sscanf(in, "{\\an%*1u}%n", &len) >= 0 && len > 0;
> -
> -            if (!closing_brace_missing) {
> -                if (   (an != 1 && in[1] == '\\')
> -                    || (in[1] && strchr("CcFfoPSsYy", in[1]) && in[2] == ':')) {
> -                    char *bracep = strchr(in+2, '}');
> -                    if (bracep) {
> -                        in = bracep;
> -                        break;
> -                    } else
> -                        closing_brace_missing = 1;
> -                }
> -            }
> -
> -            av_bprint_chars(dst, *in, 1);
> +        case '{':
> +            handle_open_brace(dst, &in, &an, &closing_brace_missing);
>              break;
>          case '<':
>              tag_close = in[1] == '/';

Looked at the an thing again (at the old code in git master). It makes
no sense to me - skipping should always behave the same.

Maybe it's actually a bug, or it attempted to emit subsequent an tags
by trying to be overly clever.

In theory, only 1 an tag is ok per line, so "{\an1}{\an2}" the second
an tag would either do nothing, or override the first tag. Maybe
someone thought it'd be reasonable to skip the an second tag, but I
don't see how it matters.

It could also be an attempt to get the same behavior between libass and
VSFitler between redundant an tags (maybe VSFilter uses the value of
the first tag, while libass uses the second tag). Then I'd say this
should be fixed in libass instead.

Anyway, I'd remove that extra an handling. It's a good example of
overly complicated code that makes everything less readable for
absolutely no reason. (Unless I'm overlooking something obvious.)

Rest LGTM.