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.
pgpw14crZZhSk.pgp
Description: PGP signature
_______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
