| Message ID | 20201014214908.279899-1-andriy.gelman@gmail.com |
|---|---|
| State | Accepted |
| Commit | d4c46dc32856bd9c7c7ab29ee727676c7855fa1c |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/movtextenc: fix writing to bytestream on BE arches | expand |
| Context | Check | Description |
|---|---|---|
| andriy/x86_make | success | Make finished |
| andriy/x86_make_fate | success | Make fate finished |
Andriy Gelman: > From: Andriy Gelman <andriy.gelman@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@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.) - Andreas
On Thu, 15. Oct 16:06, Andreas Rheinhardt wrote: > Andriy Gelman: > > From: Andriy Gelman <andriy.gelman@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@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. -- Andriy
Andriy Gelman: > On Thu, 15. Oct 16:06, Andreas Rheinhardt wrote: >> Andriy Gelman: >>> From: Andriy Gelman <andriy.gelman@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@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
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@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@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,
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);