diff mbox

[FFmpeg-devel,1/1] Updated version of patch 1840 (ticket 6021)

Message ID VI1PR01MB132770F2DCD326901409144FC0910@VI1PR01MB1327.eurprd01.prod.exchangelabs.com
State Superseded
Headers show

Commit Message

Erik Bråthen Solem Dec. 19, 2016, 3:05 a.m. UTC 
Between testing and patch generation a character was deleted by mistake, which
broke the patch. This updated version fixes this.

Original patch description:
Character offsets were interpreted as byte offsets, resulting in misplaced
styling tags where multibyte characters were involved. The entire subtitle
stream would even be rendered invalid if such a misplaced tag happened to
split a multibyte character. This patch fixes this for UTF-8; UTF-16 was and
still is broken. These are the only supported encodings according to the spec.
---
 libavcodec/movtextdec.c | 95 +++++++++++++++++++++++++++----------------------
 1 file changed, 53 insertions(+), 42 deletions(-)

Comments

Moritz Barsnick Dec. 21, 2016, 10:41 a.m. UTC | #1 
On Mon, Dec 19, 2016 at 03:05:18 +0000, Erik Bråthen Solem wrote:

> Subject: [FFmpeg-devel] [PATCH 1/1] Updated version of patch 1840 (ticket 6021)

You need to give a proper commit message. Do have a look at other
commits in the repo.

The first line of the message (which is shown in this subject, the way
you are submitting your patch here) would be something like:

lavc/movtextdec: fix incorrect offset calculation for UTF-8 characters

(i.e. describe what it does [to what], not what it is)
and then an empty line and then the full description, which then also
has an extra line at the end such as:

Fixes trac #6021.

> Between testing and patch generation a character was deleted by mistake, which
> broke the patch. This updated version fixes this.

The way you are submitting your patch here, this text becomes part of
the commit message, but it doesn't belong there.

>      while (text < text_end) {
> -        if (m->box_flags & STYL_BOX) {
> -            for (i = 0; i < m->style_entries; i++) {
> -                if (m->s[i]->style_flag && text_pos == m->s[i]->style_end) {
> -                    av_bprintf(buf, "{\\r}");
> +        if ((*text & 0xC0) != 0x80) { // Boxes never split multibyte characters
> +            if (m->box_flags & STYL_BOX) {
> +                for (i = 0; i < m->style_entries; i++) {
> +                    if (m->s[i]->style_flag &&
> +                        text_pos_chars == m->s[i]->style_end)
> +                    {
> +                        av_bprintf(buf, "{\\r}");
> +                    }
>                  }

You are doing two to three things at the same time in this part of the
patch:
- wrapping existing code inside an if() clause or shifting its
  layering: fine
- re-indenting the code that has now moved into a new block:
  this is hard to review (i.e. to see that the block has remained
  unchanged) and it is usually requested that you do the re-indentation
  in a follow-up patch.
- re-formatting the code: even if useful, falls into the previous
  category
- re-formatting to non-ffmpeg style: that's a no-go.

(Let me guess that your editor is doing the reformatting for you.
Usually, that's nifty. ;-))

I believe the patch would be *much* easier to read and much shorter if
you only did the very first step in the initial patch.

> -        return ff_ass_subtitle_header(avctx, m->d.font, m->d.fontsize, m->d.color,
> -                                m->d.back_color, m->d.bold, m->d.italic,
> -                                m->d.underline, ASS_DEFAULT_BORDERSTYLE,
> -                                m->d.alignment);
> +        return ff_ass_subtitle_header(avctx, m->d.font, m->d.fontsize,
> +                                m->d.color, m->d.back_color, m->d.bold,
> +                                m->d.italic, m->d.underline,
> +                                ASS_DEFAULT_BORDERSTYLE, m->d.alignment);

And as far as I can tell, this is *only* reformatting. This also
belongs into a separate patch (if at all useful).

> -                    if (m->tracksize + m->size_var + box_types[i].base_size > avpkt->size)
> +                    if (m->tracksize + m->size_var +
> +                        box_types[i].base_size > avpkt->size)

Same here.

Cheers,
Moritz
diff mbox

Patch

diff --git a/libavcodec/movtextdec.c b/libavcodec/movtextdec.c
index 7b5b161..4cffacf 100644
--- a/libavcodec/movtextdec.c
+++ b/libavcodec/movtextdec.c
@@ -328,6 +328,7 @@  static int text_to_ass(AVBPrint *buf, const char *text, const char *text_end,
     int i = 0;
     int j = 0;
     int text_pos = 0;
+    int text_pos_chars = 0;
 
     if (text < text_end && m->box_flags & TWRP_BOX) {
         if (m->w.wrap_flag == 1) {
@@ -338,50 +339,59 @@  static int text_to_ass(AVBPrint *buf, const char *text, const char *text_end,
     }
 
     while (text < text_end) {
-        if (m->box_flags & STYL_BOX) {
-            for (i = 0; i < m->style_entries; i++) {
-                if (m->s[i]->style_flag && text_pos == m->s[i]->style_end) {
-                    av_bprintf(buf, "{\\r}");
+        if ((*text & 0xC0) != 0x80) { // Boxes never split multibyte characters
+            if (m->box_flags & STYL_BOX) {
+                for (i = 0; i < m->style_entries; i++) {
+                    if (m->s[i]->style_flag &&
+                        text_pos_chars == m->s[i]->style_end)
+                    {
+                        av_bprintf(buf, "{\\r}");
+                    }
                 }
-            }
-            for (i = 0; i < m->style_entries; i++) {
-                if (m->s[i]->style_flag && text_pos == m->s[i]->style_start) {
-                    if (m->s[i]->style_flag & STYLE_FLAG_BOLD)
-                        av_bprintf(buf, "{\\b1}");
-                    if (m->s[i]->style_flag & STYLE_FLAG_ITALIC)
-                        av_bprintf(buf, "{\\i1}");
-                    if (m->s[i]->style_flag & STYLE_FLAG_UNDERLINE)
-                        av_bprintf(buf, "{\\u1}");
-                    av_bprintf(buf, "{\\fs%d}", m->s[i]->fontsize);
-                    for (j = 0; j < m->ftab_entries; j++) {
-                        if (m->s[i]->style_fontID == m->ftab[j]->fontID)
-                            av_bprintf(buf, "{\\fn%s}", m->ftab[j]->font);
+                for (i = 0; i < m->style_entries; i++) {
+                    if (m->s[i]->style_flag
+                        && text_pos_chars == m->s[i]->style_start)
+                    {
+                        if (m->s[i]->style_flag & STYLE_FLAG_BOLD)
+                            av_bprintf(buf, "{\\b1}");
+                        if (m->s[i]->style_flag & STYLE_FLAG_ITALIC)
+                            av_bprintf(buf, "{\\i1}");
+                        if (m->s[i]->style_flag & STYLE_FLAG_UNDERLINE)
+                            av_bprintf(buf, "{\\u1}");
+                        /* (No need to print font style if equal to default?) */
+                        av_bprintf(buf, "{\\fs%d}", m->s[i]->fontsize);
+                        for (j = 0; j < m->ftab_entries; j++) {
+                            if (m->s[i]->style_fontID == m->ftab[j]->fontID)
+                                av_bprintf(buf, "{\\fn%s}", m->ftab[j]->font);
+                        }
                     }
                 }
             }
-        }
-        if (m->box_flags & HLIT_BOX) {
-            if (text_pos == m->h.hlit_start) {
-                /* If hclr box is present, set the secondary color to the color
-                 * specified. Otherwise, set primary color to white and secondary
-                 * color to black. These colors will come from TextSampleModifier
-                 * boxes in future and inverse video technique for highlight will
-                 * be implemented.
-                 */
-                if (m->box_flags & HCLR_BOX) {
-                    av_bprintf(buf, "{\\2c&H%02x%02x%02x&}", m->c.hlit_color[2],
-                                m->c.hlit_color[1], m->c.hlit_color[0]);
-                } else {
-                    av_bprintf(buf, "{\\1c&H000000&}{\\2c&HFFFFFF&}");
+            if (m->box_flags & HLIT_BOX) {
+                if (text_pos_chars == m->h.hlit_start) {
+                    /* If hclr box is present, set the secondary color to the
+                     * color specified. Otherwise, set primary color to white
+                     * and secondary color to black. These colors will come from
+                     * TextSampleModifier boxes in future and inverse video
+                     * technique for highlight will be implemented.
+                     */
+                    if (m->box_flags & HCLR_BOX) {
+                        av_bprintf(buf, "{\\2c&H%02x%02x%02x&}",
+                                    m->c.hlit_color[2], m->c.hlit_color[1],
+                                    m->c.hlit_color[0]);
+                    } else {
+                        av_bprintf(buf, "{\\1c&H000000&}{\\2c&HFFFFFF&}");
+                    }
                 }
-            }
-            if (text_pos == m->h.hlit_end) {
-                if (m->box_flags & HCLR_BOX) {
-                    av_bprintf(buf, "{\\2c&H000000&}");
-                } else {
-                    av_bprintf(buf, "{\\1c&HFFFFFF&}{\\2c&H000000&}");
+                if (text_pos_chars == m->h.hlit_end) {
+                    if (m->box_flags & HCLR_BOX) {
+                        av_bprintf(buf, "{\\2c&H000000&}");
+                    } else {
+                        av_bprintf(buf, "{\\1c&HFFFFFF&}{\\2c&H000000&}");
+                    }
                 }
             }
+        text_pos_chars++;
         }
 
         switch (*text) {
@@ -412,10 +422,10 @@  static int mov_text_init(AVCodecContext *avctx) {
     MovTextContext *m = avctx->priv_data;
     ret = mov_text_tx3g(avctx, m);
     if (ret == 0) {
-        return ff_ass_subtitle_header(avctx, m->d.font, m->d.fontsize, m->d.color,
-                                m->d.back_color, m->d.bold, m->d.italic,
-                                m->d.underline, ASS_DEFAULT_BORDERSTYLE,
-                                m->d.alignment);
+        return ff_ass_subtitle_header(avctx, m->d.font, m->d.fontsize,
+                                m->d.color, m->d.back_color, m->d.bold,
+                                m->d.italic, m->d.underline,
+                                ASS_DEFAULT_BORDERSTYLE, m->d.alignment);
     } else
         return ff_ass_subtitle_header_default(avctx);
 }
@@ -491,7 +501,8 @@  static int mov_text_decode_frame(AVCodecContext *avctx,
 
             for (size_t i = 0; i < box_count; i++) {
                 if (tsmb_type == box_types[i].type) {
-                    if (m->tracksize + m->size_var + box_types[i].base_size > avpkt->size)
+                    if (m->tracksize + m->size_var +
+                        box_types[i].base_size > avpkt->size)
                         break;
                     ret_tsmb = box_types[i].decode(tsmb, m, avpkt);
                     if (ret_tsmb == -1)