diff mbox series

[FFmpeg-devel,2/2] avcodec/movtextenc: Unbreak BE arches, simplify writing

Message ID 20201015140052.100725-2-andreas.rheinhardt@gmail.com
State Superseded
Headers show
Series [FFmpeg-devel,1/2] avcodec/movtextenc: Fix potential use of unitialized value
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make warning Make failed

Commit Message

Andreas Rheinhardt Oct. 15, 2020, 2 p.m. UTC
The mov_text encoder uses an AVBPrint to assemble the subtitles;
yet mov_text subtitles are not pure text; they also have a binary
portion that was mostly handled as follows:

    uint32_t size = /* calculation */;
    size = AV_RB32(&size);
    av_bprint_append_data(bprint, (const char*)&size, 4);

Here AV_RB32() is a no-op on big-endian systems and a LE-BE
transformation on little-endian systems; it ensured that the output was
endian-independent if the above pattern has been used. Yet sometimes
this has been forgotten, leading to wrong output on big-endian systems.

This commit fixes this, but not by adding AV_RBx to perform the
endian conversion; instead this (ugly and unclean) way is eschewed
altogether by using a temporary buffer:

    uint8_t buf[4];
    AV_WB32(buf, /* size calculation */);
    av_bprint_append_data(bprint, buf, 4);

In fact bigger buffers holding more than one element are used, saving calls
to av_bprint_append_data() and thereby reducing codesize.

Found-by: Andriy Gelman <andriy.gelman@gmail.com>
Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
Btw: This code seems to be missing a check that the size of the font
actually fits in one byte. And if reallocating the style_attributes
fails, the old style attributes leak (did I already mention that
av_dynarray_add() is horrible?).

One could btw remove the type from box_types and inline it in the
relevant functions.

 libavcodec/movtextenc.c | 155 ++++++++++++++++++----------------------
 1 file changed, 70 insertions(+), 85 deletions(-)
diff mbox series

Patch

diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
index 00ebca2e56..1518fccfe4 100644
--- a/libavcodec/movtextenc.c
+++ b/libavcodec/movtextenc.c
@@ -29,6 +29,7 @@ 
 #include "libavutil/common.h"
 #include "ass_split.h"
 #include "ass.h"
+#include "bytestream.h"
 
 #define STYLE_FLAG_BOLD         (1<<0)
 #define STYLE_FLAG_ITALIC       (1<<1)
@@ -111,32 +112,28 @@  static void mov_text_cleanup(MovTextContext *s)
 static void encode_styl(MovTextContext *s, uint32_t tsmb_type)
 {
     int j;
-    uint32_t tsmb_size;
-    uint16_t style_entries;
+
     if ((s->box_flags & STYL_BOX) && s->count) {
-        tsmb_size = s->count * STYLE_RECORD_SIZE + SIZE_ADD;
-        tsmb_size = AV_RB32(&tsmb_size);
-        style_entries = AV_RB16(&s->count);
+        uint8_t buf[12], *p = buf;
+
+        bytestream_put_be32(&p, s->count * STYLE_RECORD_SIZE + SIZE_ADD);
+        bytestream_put_be32(&p, tsmb_type);
+        bytestream_put_be16(&p, s->count);
         /*The above three attributes are hard coded for now
         but will come from ASS style in the future*/
-        av_bprint_append_any(&s->buffer, &tsmb_size, 4);
-        av_bprint_append_any(&s->buffer, &tsmb_type, 4);
-        av_bprint_append_any(&s->buffer, &style_entries, 2);
+        av_bprint_append_any(&s->buffer, buf, 10);
         for (j = 0; j < s->count; j++) {
-            uint16_t style_start, style_end, style_fontID;
-            uint32_t style_color;
-
-            style_start  = AV_RB16(&s->style_attributes[j]->style_start);
-            style_end    = AV_RB16(&s->style_attributes[j]->style_end);
-            style_color  = AV_RB32(&s->style_attributes[j]->style_color);
-            style_fontID = AV_RB16(&s->style_attributes[j]->style_fontID);
-
-            av_bprint_append_any(&s->buffer, &style_start, 2);
-            av_bprint_append_any(&s->buffer, &style_end, 2);
-            av_bprint_append_any(&s->buffer, &style_fontID, 2);
-            av_bprint_append_any(&s->buffer, &s->style_attributes[j]->style_flag, 1);
-            av_bprint_append_any(&s->buffer, &s->style_attributes[j]->style_fontsize, 1);
-            av_bprint_append_any(&s->buffer, &style_color, 4);
+            const StyleBox *style = s->style_attributes[j];
+
+            p = buf;
+            bytestream_put_be16(&p, style->style_start);
+            bytestream_put_be16(&p, style->style_end);
+            bytestream_put_be16(&p, style->style_fontID);
+            bytestream_put_byte(&p, style->style_flag);
+            bytestream_put_byte(&p, style->style_fontsize);
+            bytestream_put_be32(&p, style->style_color);
+
+            av_bprint_append_any(&s->buffer, buf, 12);
         }
     }
     mov_text_cleanup(s);
@@ -144,37 +141,35 @@  static void encode_styl(MovTextContext *s, uint32_t tsmb_type)
 
 static void encode_hlit(MovTextContext *s, uint32_t tsmb_type)
 {
-    uint32_t tsmb_size;
-    uint16_t start, end;
     if (s->box_flags & HLIT_BOX) {
-        tsmb_size = 12;
-        tsmb_size = AV_RB32(&tsmb_size);
-        start     = AV_RB16(&s->hlit.start);
-        end       = AV_RB16(&s->hlit.end);
-        av_bprint_append_any(&s->buffer, &tsmb_size, 4);
-        av_bprint_append_any(&s->buffer, &tsmb_type, 4);
-        av_bprint_append_any(&s->buffer, &start, 2);
-        av_bprint_append_any(&s->buffer, &end, 2);
+        uint8_t buf[12], *p = buf;
+
+        bytestream_put_be32(&p, 12);
+        bytestream_put_be32(&p, tsmb_type);
+        bytestream_put_be16(&p, s->hlit.start);
+        bytestream_put_be16(&p, s->hlit.end);
+
+        av_bprint_append_any(&s->buffer, buf, 12);
     }
 }
 
 static void encode_hclr(MovTextContext *s, uint32_t tsmb_type)
 {
-    uint32_t tsmb_size, color;
     if (s->box_flags & HCLR_BOX) {
-        tsmb_size = 12;
-        tsmb_size = AV_RB32(&tsmb_size);
-        color     = AV_RB32(&s->hclr.color);
-        av_bprint_append_any(&s->buffer, &tsmb_size, 4);
-        av_bprint_append_any(&s->buffer, &tsmb_type, 4);
-        av_bprint_append_any(&s->buffer, &color, 4);
+        uint8_t buf[12], *p = buf;
+
+        bytestream_put_be32(&p, 12);
+        bytestream_put_be32(&p, tsmb_type);
+        bytestream_put_be32(&p, s->hclr.color);
+
+        av_bprint_append_any(&s->buffer, buf, 12);
     }
 }
 
 static const Box box_types[] = {
-    { MKTAG('s','t','y','l'), encode_styl },
-    { MKTAG('h','l','i','t'), encode_hlit },
-    { MKTAG('h','c','l','r'), encode_hclr },
+    { MKBETAG('s','t','y','l'), encode_styl },
+    { MKBETAG('h','l','i','t'), encode_hlit },
+    { MKBETAG('h','c','l','r'), encode_hclr },
 };
 
 const static size_t box_count = FF_ARRAY_ELEMS(box_types);
@@ -202,25 +197,21 @@  static int encode_sample_description(AVCodecContext *avctx)
     ASS * ass;
     ASSStyle * style;
     int i, j;
-    uint32_t tsmb_size, tsmb_type, back_color = 0, style_color;
-    uint16_t style_start, style_end, fontID, count;
+    uint32_t back_color = 0;
     int font_names_total_len = 0;
     MovTextContext *s = avctx->priv_data;
+    uint8_t buf[30], *p = buf;
 
-    static const uint8_t display_and_justification[] = {
-        0x00, 0x00, 0x00, 0x00, // uint32_t displayFlags
-        0x01,                   // int8_t horizontal-justification
-        0xFF,                   // int8_t vertical-justification
-    };
+    //  0x00, 0x00, 0x00, 0x00, // uint32_t displayFlags
+    //  0x01,                   // int8_t horizontal-justification
+    //  0xFF,                   // int8_t vertical-justification
     //  0x00, 0x00, 0x00, 0x00, // uint8_t background-color-rgba[4]
-    static const uint8_t box_record[] = {
     //     BoxRecord {
-        0x00, 0x00,             // int16_t top
-        0x00, 0x00,             // int16_t left
-        0x00, 0x00,             // int16_t bottom
-        0x00, 0x00,             // int16_t right
+    //  0x00, 0x00,             // int16_t top
+    //  0x00, 0x00,             // int16_t left
+    //  0x00, 0x00,             // int16_t bottom
+    //  0x00, 0x00,             // int16_t right
     //     };
-    };
     //     StyleRecord {
     //  0x00, 0x00,             // uint16_t startChar
     //  0x00, 0x00,             // uint16_t endChar
@@ -268,25 +259,19 @@  static int encode_sample_description(AVCodecContext *avctx)
                      (255 - ((uint32_t)style->back_color >> 24));
     }
 
-    av_bprint_append_any(&s->buffer, display_and_justification,
-                                     sizeof(display_and_justification));
-    back_color = AV_RB32(&back_color);
-    av_bprint_append_any(&s->buffer, &back_color, 4);
-    //     BoxRecord {
-    av_bprint_append_any(&s->buffer, box_record, sizeof(box_record));
-    //     };
+    bytestream_put_be32(&p, 0); // displayFlags
+    bytestream_put_be16(&p, 0x01FF); // horizontal/vertical justification (2x int8_t)
+    bytestream_put_be32(&p, back_color);
+    bytestream_put_be64(&p, 0); // BoxRecord - 4xint16_t: top, left, bottom, right
     //     StyleRecord {
-    style_start  = AV_RB16(&s->d.style_start);
-    style_end    = AV_RB16(&s->d.style_end);
-    fontID = AV_RB16(&s->d.style_fontID);
-    style_color  = AV_RB32(&s->d.style_color);
-    av_bprint_append_any(&s->buffer, &style_start, 2);
-    av_bprint_append_any(&s->buffer, &style_end, 2);
-    av_bprint_append_any(&s->buffer, &fontID, 2);
-    av_bprint_append_any(&s->buffer, &s->d.style_flag, 1);
-    av_bprint_append_any(&s->buffer, &s->d.style_fontsize, 1);
-    av_bprint_append_any(&s->buffer, &style_color, 4);
+    bytestream_put_be16(&p, s->d.style_start);
+    bytestream_put_be16(&p, s->d.style_end);
+    bytestream_put_be16(&p, s->d.style_fontID);
+    bytestream_put_byte(&p, s->d.style_flag);
+    bytestream_put_byte(&p, s->d.style_fontsize);
+    bytestream_put_be32(&p, s->d.style_color);
     //     };
+    av_bprint_append_any(&s->buffer, buf, 30);
 
     // Build font table
     // We can't build a complete font table since that would require
@@ -314,21 +299,21 @@  static int encode_sample_description(AVCodecContext *avctx)
         av_dynarray_add(&s->fonts, &s->font_count, (char*)"Serif");
 
     //     FontTableBox {
-    tsmb_size = SIZE_ADD + 3 * s->font_count + font_names_total_len;
-    tsmb_size = AV_RB32(&tsmb_size);
-    tsmb_type = MKTAG('f','t','a','b');
-    count = AV_RB16(&s->font_count);
-    av_bprint_append_any(&s->buffer, &tsmb_size, 4);
-    av_bprint_append_any(&s->buffer, &tsmb_type, 4);
-    av_bprint_append_any(&s->buffer, &count, 2);
+    p = buf;
+    bytestream_put_be32(&p, SIZE_ADD + 3 * s->font_count + font_names_total_len); // Size
+    bytestream_put_be32(&p, MKBETAG('f','t','a','b'));
+    bytestream_put_be16(&p, s->font_count);
+
+    av_bprint_append_any(&s->buffer, buf, 10);
     //     FontRecord {
     for (i = 0; i < s->font_count; i++) {
-        int len;
-        fontID = i + 1;
-        fontID = AV_RB16(&fontID);
-        av_bprint_append_any(&s->buffer, &fontID, 2);
-        len = strlen(s->fonts[i]);
-        av_bprint_append_any(&s->buffer, &len, 1);
+        size_t len = strlen(s->fonts[i]);
+
+        p = buf;
+        bytestream_put_be16(&p, i + 1); //fontID
+        bytestream_put_byte(&p, len);
+
+        av_bprint_append_any(&s->buffer, buf, 3);
         av_bprint_append_any(&s->buffer, s->fonts[i], len);
     }
     //     };