[FFmpeg-devel] [PATCH 1/2] avcodec/movtextenc: fix writing to bytestream on BE arches

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Fri Oct 16 00:55:58 EEST 2020


Andriy Gelman:
> On Thu, 15. Oct 16:06, Andreas Rheinhardt wrote:
>> Andriy Gelman:
>>> From: Andriy Gelman <andriy.gelman at gmail.com>
>>>
>>> Fixes fate-binsub-movtextenc on PPC64
>>>
>>> Currently tags are written in reverse order on BE arches. This is fixed
>>> by using MKBETAG() and AV_RB32() to be arch agnostics.
>>>
>>> Also s->font_count is of type int. On BE arches with 32bit int,
>>> count = AV_RB16(&s->font_count) will read two most significant bytes
>>> instead of the least signifcant bytes. This is fixed by assiging
>>> s->font_count to count first.
>>>
>>> The final change is modyfying the type of len. On BE arches
>>> the most signifanct byte of the int was written instead of the least
>>> significant byte.
>>>
>>> Signed-off-by: Andriy Gelman <andriy.gelman at gmail.com>
>>> ---
>>>  libavcodec/movtextenc.c | 17 +++++++++++------
>>>  1 file changed, 11 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libavcodec/movtextenc.c b/libavcodec/movtextenc.c
>>> index b2368b641b..f38cd9cba2 100644
>>> --- a/libavcodec/movtextenc.c
>>> +++ b/libavcodec/movtextenc.c
>>> @@ -116,6 +116,7 @@ static void encode_styl(MovTextContext *s, uint32_t tsmb_type)
>>>      if ((s->box_flags & STYL_BOX) && s->count) {
>>>          tsmb_size = s->count * STYLE_RECORD_SIZE + SIZE_ADD;
>>>          tsmb_size = AV_RB32(&tsmb_size);
>>> +        tsmb_type = AV_RB32(&tsmb_type);
>>>          style_entries = AV_RB16(&s->count);
>>>          /*The above three attributes are hard coded for now
>>>          but will come from ASS style in the future*/
>>> @@ -149,6 +150,7 @@ static void encode_hlit(MovTextContext *s, uint32_t tsmb_type)
>>>      if (s->box_flags & HLIT_BOX) {
>>>          tsmb_size = 12;
>>>          tsmb_size = AV_RB32(&tsmb_size);
>>> +        tsmb_type = AV_RB32(&tsmb_type);
>>>          start     = AV_RB16(&s->hlit.start);
>>>          end       = AV_RB16(&s->hlit.end);
>>>          av_bprint_append_any(&s->buffer, &tsmb_size, 4);
>>> @@ -164,6 +166,7 @@ static void encode_hclr(MovTextContext *s, uint32_t tsmb_type)
>>>      if (s->box_flags & HCLR_BOX) {
>>>          tsmb_size = 12;
>>>          tsmb_size = AV_RB32(&tsmb_size);
>>> +        tsmb_type = AV_RB32(&tsmb_type);
>>>          color     = AV_RB32(&s->hclr.color);
>>>          av_bprint_append_any(&s->buffer, &tsmb_size, 4);
>>>          av_bprint_append_any(&s->buffer, &tsmb_type, 4);
>>> @@ -172,9 +175,9 @@ static void encode_hclr(MovTextContext *s, uint32_t tsmb_type)
>>>  }
>>>  
>>>  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);
>>> @@ -316,14 +319,16 @@ static int encode_sample_description(AVCodecContext *avctx)
>>>      //     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);
>>> +    tsmb_type = MKBETAG('f','t','a','b');
>>> +    tsmb_type = AV_RB32(&tsmb_type);
>>> +    count = s->font_count;
>>> +    count = AV_RB16(&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);
>>>      //     FontRecord {
>>>      for (i = 0; i < s->font_count; i++) {
>>> -        int len;
>>> +        uint8_t len;
>>>          fontID = i + 1;
>>>          fontID = AV_RB16(&fontID);
>>>          av_bprint_append_any(&s->buffer, &fontID, 2);
>>>
> 
>> I have sent an alternative version that avoids this horrible way of
>> writing output independent of endianness by instead using small buffers
>> and writing the values into it via AV_WBx(). This also allows to combine
>> several av_bprint_append_any() calls into one. Said patch is patch #2 of
>> this patchset:
>> https://ffmpeg.org/pipermail/ffmpeg-devel/2020-October/271057.html (The
>> second email has not yet come through due to the mailing list's random
>> delay.)
> 
> My patch is a smaller diff and is consistent with the style of the code.
>> Up to maintainer but I'd suggest your changes are a separate patch.
> 
This encoder has no maintainer listed, so I suggest you apply your patch
to fix the endianness issue and I rebase my patches on top of yours.
Btw: Your commit message has typos: signifcant, assiging, modyfying,
signifanct.

- Andreas


More information about the ffmpeg-devel mailing list