[FFmpeg-devel] [PATCH 2/4] avcodec/samidec: use ff_htmlmarkup_to_ass()

[Clément Bœsch]

On Fri, Aug 07, 2015 at 11:03:29PM -0700, Yayoi wrote:
> ---
>  libavcodec/samidec.c    | 59 +++++++++++++++++++++++++++++--------------------
>  tests/ref/fate/sub-sami | 18 +++++++--------
>  2 files changed, 44 insertions(+), 33 deletions(-)
> 
> diff --git a/libavcodec/samidec.c b/libavcodec/samidec.c
> index 47850e2..41826a7 100644
> --- a/libavcodec/samidec.c
> +++ b/libavcodec/samidec.c
> @@ -27,6 +27,7 @@
>  #include "ass.h"
>  #include "libavutil/avstring.h"
>  #include "libavutil/bprint.h"
> +#include "htmlsubtitles.h"
>  
>  typedef struct {
>      AVBPrint source;
> @@ -41,11 +42,12 @@ static int sami_paragraph_to_ass(AVCodecContext *avctx, const char *src)
>      char *tag = NULL;
>      char *dupsrc = av_strdup(src);
>      char *p = dupsrc;
> +    char *pcopy = NULL;
> +    int index = 0;
> +    int second_paragraph = 0;
>  
> -    av_bprint_clear(&sami->content);
>      for (;;) {
>          char *saveptr = NULL;
> -        int prev_chr_is_space = 0;
>          AVBPrint *dst = &sami->content;
>  
>          /* parse & extract paragraph tag */
> @@ -77,37 +79,46 @@ static int sami_paragraph_to_ass(AVCodecContext *avctx, const char *src)
>              goto end;
>          }
>  

> -        /* extract the text, stripping most of the tags */
> +        /* check for the second paragrph */

Why change the comment? What does "check" mean here? What is the "second
paragraph"?

> +        pcopy = av_strdup(p);
>          while (*p) {
>              if (*p == '<') {
> -                if (!av_strncasecmp(p, "<P", 2) && (p[2] == '>' || av_isspace(p[2])))
> +                if (!av_strncasecmp(p, "<p", 2) && (p[2] == '>' || av_isspace(p[2]))) {
> +                    second_paragraph = 1;
>                      break;
> -                if (!av_strncasecmp(p, "<BR", 3))
> -                    av_bprintf(dst, "\\N");
> -                p++;
> -                while (*p && *p != '>')
> -                    p++;
> -                if (!*p)
> -                    break;
> -                if (*p == '>')
> -                    p++;
> -                continue;
> +                }
>              }
> -            if (!av_isspace(*p))
> -                av_bprint_chars(dst, *p, 1);
> -            else if (!prev_chr_is_space)
> -                av_bprint_chars(dst, ' ', 1);
> -            prev_chr_is_space = av_isspace(*p);
>              p++;
> +            index++;
> +        }
> +        p = p - index;
> +        if (second_paragraph) {
> +            p[index] = 0;
>          }
> -    }
>  
> -    av_bprint_clear(&sami->full);
> -    if (sami->source.len)
> -        av_bprintf(&sami->full, "{\\i1}%s{\\i0}\\N", sami->source.str);
> -    av_bprintf(&sami->full, "%s", sami->content.str);
> +        ff_htmlmarkup_to_ass(avctx, dst, p);
> +
> +        /* add the source if there are any. */
> +        av_bprint_clear(&sami->full);
> +        if (sami->source.len) {
> +            av_bprintf(&sami->full, "{\\i1}%s{\\i0}\\N", sami->source.str);
> +            av_bprintf(&sami->full, "%s", sami->content.str);
> +            if (second_paragraph) {
> +                second_paragraph = 0;
> +                p = pcopy;
> +                p += index;
> +                index = 0;
> +                continue;
> +            }
> +        } else {
> +            av_bprintf(&sami->full, "%s", sami->content.str);
> +        }
> +        av_bprint_clear(&sami->content);
> +
> +    }
>  

This looks clumsy at best: you are finalizing the subtitle event inside
the paragraph loop when it should be outside. It also seems there is a
duplicating "second paragraph" logic even though the loop is supposed to
handle one paragraph at a time.

If you are uncomfortable with the current logic or believe it's badly
designed for the logic you're trying to mangle, feel free to rewrite it;
it's better than trying to inhibit the old behaviour with hacks.

[...]

-- 
Clément B.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20150808/6ffa4e9b/attachment.sig>