From patchwork Thu Oct 15 14:00:51 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Andreas Rheinhardt X-Patchwork-Id: 22968 Return-Path: X-Original-To: patchwork@ffaux-bg.ffmpeg.org Delivered-To: patchwork@ffaux-bg.ffmpeg.org Received: from ffbox0-bg.mplayerhq.hu (ffbox0-bg.ffmpeg.org [79.124.17.100]) by ffaux.localdomain (Postfix) with ESMTP id 04789449D6C for ; Thu, 15 Oct 2020 17:29:48 +0300 (EEST) Received: from [127.0.1.1] (localhost [127.0.0.1]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTP id C196268B9C8; Thu, 15 Oct 2020 17:29:47 +0300 (EEST) X-Original-To: ffmpeg-devel@ffmpeg.org Delivered-To: ffmpeg-devel@ffmpeg.org Received: from mail-wr1-f50.google.com (mail-wr1-f50.google.com [209.85.221.50]) by ffbox0-bg.mplayerhq.hu (Postfix) with ESMTPS id 6D78468B85A for ; Thu, 15 Oct 2020 17:29:41 +0300 (EEST) Received: by mail-wr1-f50.google.com with SMTP id h5so3749546wrv.7 for ; Thu, 15 Oct 2020 07:29:41 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=SFDLcoijnpT3JVP3s4Rm01e7MKj7IogPK1havXMn76o=; b=WJmFASCMAYCy03QgSKGgDVJOaISMN9zVDdmaNc51I41kxParQvv7KB2g4UNWp+RVkN QqEC/YZ8DOUfV4x7FWyBprGc8Os5BCOJ2ta16pdjGrjVehjATgORY3xuhBenCXseulwn LX+Sb53Pl7VcBntgzTRGlUkKS/9cK6ALsUFWseo2+9BQ7zkz+4f4J/h6eiXinwm9tYhc CI3raJWzYtVVOyg7HS5CVHLHm7QDM2hG0yMtYt6/aimVIT0ODNEbp8y0zzVcAUb1beLR eVEB6aOhNiwOkCUTLS5eUqP9GBcO+jzsHVb/avRooUlk2Ytx3HdCxWnzaMLzT+00NN8Y oADw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=SFDLcoijnpT3JVP3s4Rm01e7MKj7IogPK1havXMn76o=; b=nIq2x59bLxbzV8tyvcZtE/55VW0OG6/FftvhjPrBb3Lxp4rJLAWDwW0pXw3hHekxob RY1vU5saob1AXdzW++9TZsYcoBmcH+NBAUrQX3WBwwUVbfkJ+afs6YBpy59ONvQOTuom V9KwDV/pFZhm+HAYvWGMxbkN+f03vzncZzbxsToXf9c6QauYhcRtWwfd+lsW6DQkOCJi FS0mLprXX9iBh7hYay3+WH5utl5KE96skIyMRejWKuqc7KhPoNl8MD+omgMbgST6WOpN 1A/Z3IMgWBBHrkIF1BekRym4MQ52wOhJPdYcR21RULc2Jx5pNtp/tvRQZtCCXTrQVmmU ZRkw== X-Gm-Message-State: AOAM530kPmn0Wtmub6iTEaIJLJaKSlp4diYTIs4PluZU786W+NM2ZysF B90MRScc1N/u1sCbRRErrlkBcMG8XZw= X-Google-Smtp-Source: ABdhPJxmphn10ue69MjX3G2nBejX2lKxm6NEtaUBWnkWy8S1PJ8kRrndU3Z6LOa4rbRrGPSrcJvK1A== X-Received: by 2002:a5d:4088:: with SMTP id o8mr4737043wrp.2.1602770481112; Thu, 15 Oct 2020 07:01:21 -0700 (PDT) Received: from sblaptop.fritz.box (ipbcc1aa4b.dynamic.kabel-deutschland.de. [188.193.170.75]) by smtp.gmail.com with ESMTPSA id h16sm5208467wre.87.2020.10.15.07.01.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 15 Oct 2020 07:01:20 -0700 (PDT) From: Andreas Rheinhardt To: ffmpeg-devel@ffmpeg.org Date: Thu, 15 Oct 2020 16:00:51 +0200 Message-Id: <20201015140052.100725-2-andreas.rheinhardt@gmail.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20201015140052.100725-1-andreas.rheinhardt@gmail.com> References: <20201015140052.100725-1-andreas.rheinhardt@gmail.com> MIME-Version: 1.0 Subject: [FFmpeg-devel] [PATCH 2/2] avcodec/movtextenc: Unbreak BE arches, simplify writing X-BeenThere: ffmpeg-devel@ffmpeg.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: FFmpeg development discussions and patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: FFmpeg development discussions and patches Cc: Andriy Gelman , Andreas Rheinhardt Errors-To: ffmpeg-devel-bounces@ffmpeg.org Sender: "ffmpeg-devel" 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 Signed-off-by: Andreas Rheinhardt --- 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 --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); } // };