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

[Michael Niedermayer]

On Tue, Jun 13, 2017 at 01:19:56PM +0200, wm4 wrote:
> 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.)

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
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I do not agree with what you have to say, but I'll defend to the death your
right to say it. -- Voltaire
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20170613/cdcfb96a/attachment.sig>