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

[Andriy Gelman]

On Thu, 15. Oct 23:55, Andreas Rheinhardt wrote:
> 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.

Sounds good. Fixed the typos and pushed.

thanks,
-- 
Andriy