On Fri, Apr 24, 2020 at 7:29 AM Andreas Rheinhardt
<andreas.rheinha...@gmail.com> wrote:
>
> Mattias Wadman:
> > lavf flacenc could previously write truncated metadata picture size if
> > the picture data did not fit in 24 bits. Detect this by truncting the
> > size found inside the picture block and if it matches the block size
> > use it and read rest of picture data.
> >
> > Also only enable this workaround flac files and not ogg files with flac
> > METADATA_BLOCK_PICTURE comment.
> >
> > flacenc was fixed in e447a4d112bcfee10126c54eb4481fa8712957c8
> > before the fix a broken flac for reproduction could be generated with:
> > ffmpeg -f lavfi -i sine -f lavfi -i color=red:size=2400x2400 -map 0:0 -map
> > 1:0 -c:v:0 bmp -disposition:1 attached_pic -t 1 test.flac
> >
> > Fixes ticket 6333
> > ---
> > libavformat/flac_picture.c | 27 ++++++++++++++++++++++++---
> > libavformat/flac_picture.h | 2 +-
> > libavformat/flacdec.c | 2 +-
> > libavformat/oggparsevorbis.c | 2 +-
> > 4 files changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/libavformat/flac_picture.c b/libavformat/flac_picture.c
> > index 81ddf80465..f59ec8843f 100644
> > --- a/libavformat/flac_picture.c
> > +++ b/libavformat/flac_picture.c
> > @@ -27,16 +27,17 @@
> > #include "id3v2.h"
> > #include "internal.h"
> >
> > -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size)
> > +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> > int truncate_workaround)
> > {
> > const CodecMime *mime = ff_id3v2_mime_tags;
> > enum AVCodecID id = AV_CODEC_ID_NONE;
> > AVBufferRef *data = NULL;
> > - uint8_t mimetype[64], *desc = NULL;
> > + uint8_t mimetype[64], *desc = NULL, *picturebuf = NULL;
> > GetByteContext g;
> > AVStream *st;
> > int width, height, ret = 0;
> > - unsigned int len, type;
> > + unsigned int type;
> > + uint32_t len, left;
> >
> > if (buf_size < 34) {
> > av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > @@ -114,6 +115,25 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> >
> > /* picture data */
> > len = bytestream2_get_be32u(&g);
> > +
> > + // Workaround lavf flacenc bug that allowed writing truncated metadata
> > picture block size if
> > + // picture size did not fit in 24 bits
> > + left = bytestream2_get_bytes_left(&g);
> > + if (truncate_workaround && len > left && (len & 0xffffff) == left) {
> > + uint32_t lendiff;
> > +
> > + av_log(s, AV_LOG_INFO, "Correcting truncated metadata picture size
> > from %d to %d\n", left, len);
> > +
> > + picturebuf = av_malloc(len);
> > + if (!picturebuf)
> > + RETURN_ERROR(AVERROR(ENOMEM));
> > + lendiff = len - left;
> > + bytestream2_get_buffer(&g, picturebuf, left);
> > + if (avio_read(s->pb, picturebuf+left, lendiff) < lendiff)
> > + RETURN_ERROR(AVERROR_INVALIDDATA);
> > + bytestream2_init(&g, picturebuf, len);
> > + }
> > +
> > if (len <= 0 || len > bytestream2_get_bytes_left(&g)) {
> > av_log(s, AV_LOG_ERROR, "Attached picture metadata block too
> > short\n");
> > if (s->error_recognition & AV_EF_EXPLODE)
> > @@ -155,6 +175,7 @@ int ff_flac_parse_picture(AVFormatContext *s, uint8_t
> > *buf, int buf_size)
> > fail:
> > av_buffer_unref(&data);
> > av_freep(&desc);
> > + av_freep(&picturebuf);
>
> I don't like that you are allocating a very big buffer and read data
> into this buffer just to copy it into another buffer (that is not given
> in advance, but allocated by this function, too).
Ok thanks, yeap not so nice. Guess there are some alternatives i come up with:
1) Probe for the issue in flac_read_header and if detected realloc and
read more then call unmodified ff_flac_parse_picture
2) Make ff_flac_parse_picture able to say it needs more data
3) Make ff_flac_parse_picture able to realloc the passed buf, so
change buf arg to uint8_t **buf
-Mattias
>
> - Andreas
>
> >
> > return ret;
> > }
> > diff --git a/libavformat/flac_picture.h b/libavformat/flac_picture.h
> > index 4374b6f4f6..61fd0c8806 100644
> > --- a/libavformat/flac_picture.h
> > +++ b/libavformat/flac_picture.h
> > @@ -26,6 +26,6 @@
> >
> > #define RETURN_ERROR(code) do { ret = (code); goto fail; } while (0)
> >
> > -int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size);
> > +int ff_flac_parse_picture(AVFormatContext *s, uint8_t *buf, int buf_size,
> > int truncate_workaround);
> >
> > #endif /* AVFORMAT_FLAC_PICTURE_H */
> > diff --git a/libavformat/flacdec.c b/libavformat/flacdec.c
> > index cb516fb1f3..79c05f14bf 100644
> > --- a/libavformat/flacdec.c
> > +++ b/libavformat/flacdec.c
> > @@ -146,7 +146,7 @@ static int flac_read_header(AVFormatContext *s)
> > }
> > av_freep(&buffer);
> > } else if (metadata_type == FLAC_METADATA_TYPE_PICTURE) {
> > - ret = ff_flac_parse_picture(s, buffer, metadata_size);
> > + ret = ff_flac_parse_picture(s, buffer, metadata_size, 1);
> > av_freep(&buffer);
> > if (ret < 0) {
> > av_log(s, AV_LOG_ERROR, "Error parsing attached
> > picture.\n");
> > diff --git a/libavformat/oggparsevorbis.c b/libavformat/oggparsevorbis.c
> > index 27d2c686b6..6f15204ada 100644
> > --- a/libavformat/oggparsevorbis.c
> > +++ b/libavformat/oggparsevorbis.c
> > @@ -165,7 +165,7 @@ int ff_vorbis_comment(AVFormatContext *as, AVDictionary
> > **m,
> > av_freep(&tt);
> > av_freep(&ct);
> > if (ret > 0)
> > - ret = ff_flac_parse_picture(as, pict, ret);
> > + ret = ff_flac_parse_picture(as, pict, ret, 0);
> > av_freep(&pict);
> > if (ret < 0) {
> > av_log(as, AV_LOG_WARNING, "Failed to parse cover art
> > block.\n");
> >
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
To unsubscribe, visit link above, or email
ffmpeg-devel-requ...@ffmpeg.org with subject "unsubscribe".
