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

Submitted by Michael Niedermayer on June 13, 2017, 1:09 p.m.

Details

Message ID 20170613130954.11348-1-michael@niedermayer.cc
State New
Headers show

Commit Message

Michael Niedermayer June 13, 2017, 1:09 p.m.
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(-)

Comments

wm4 June 13, 2017, 1:21 p.m.
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.
Clément Bœsch June 13, 2017, 2:30 p.m.
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.
wm4 June 13, 2017, 5:11 p.m.
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.
Michael Niedermayer June 18, 2017, 12:51 p.m.
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

[...]

Patch hide | download patch | download mbox

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