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

Suggested-by: wm4
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/htmlsubtitles.c | 44 ++++++++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 18 deletions(-)

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

> Suggested-by: wm4
> Signed-off-by: Michael Niedermayer <michael@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.

On Tue, Jun 13, 2017 at 01:19:56PM +0200, wm4 wrote:
> On Tue, 13 Jun 2017 00:01:04 +0200
> Michael Niedermayer <michael@niedermayer.cc> wrote:
> 
> > Suggested-by: wm4
> > Signed-off-by: Michael Niedermayer <michael@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.)

ok, ive sent a patch that does that


> 
> Rest LGTM.

will apply the 3 patches in a day or 2 unless there are more comments

thx


[...9