[FFmpeg-devel] [PATCH] id3v2: fix unsynchronization

[wm4]

On Tue, 30 Jan 2018 13:43:25 +0100
wm4 <nfxjfg at googlemail.com> wrote:

> The ID3v2 "unsynchronization scheme" requires replacing any 0xFF 0x00
> sequences with 0xFF. This has to be done on every byte of the source
> data, while the current code skipped a byte after a replacement. This
> meant 0xFF 0x00 0xFF 00 was translated to 0xFF 0xFF 0x00 instead of 0xFF
> 0xFF. It feels a bit messy to do this correctly with the avio use. But
> fortunately, this translation can be done in-place, so we can just do it
> in memory.
> 
> Inspired by what taglib does.
> ---
> Sample (which had corrupted cover art, displays fine with the fix):
> https://0x0.st/sbQ9.bin (unfortunately a bit too large for FATE)
> ---
>  libavformat/id3v2.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/libavformat/id3v2.c b/libavformat/id3v2.c
> index b80178d67a..f7de26a1d8 100644
> --- a/libavformat/id3v2.c
> +++ b/libavformat/id3v2.c
> @@ -976,19 +976,21 @@ static void id3v2_parse(AVIOContext *pb, AVDictionary **metadata,
>                  }
>              }
>              if (unsync || tunsync) {
> -                int64_t end = avio_tell(pb) + tlen;
> -                uint8_t *b;
> -
> -                b = buffer;
> -                while (avio_tell(pb) < end && b - buffer < tlen && !pb->eof_reached) {
> -                    *b++ = avio_r8(pb);
> -                    if (*(b - 1) == 0xff && avio_tell(pb) < end - 1 &&
> -                        b - buffer < tlen &&
> -                        !pb->eof_reached ) {
> -                        uint8_t val = avio_r8(pb);
> -                        *b++ = val ? val : avio_r8(pb);
> -                    }
> +                uint8_t *b = buffer;
> +                uint8_t *t = buffer;
> +                uint8_t *end = t + tlen;
> +
> +                if (avio_read(pb, buffer, tlen) != tlen) {
> +                    av_log(s, AV_LOG_ERROR, "Failed to read tag data\n");
> +                    goto seek;
>                  }
> +
> +                while (t != end) {
> +                    *b++ = *t++;
> +                    if (t != end && t[-1] == 0xff && !t[0])
> +                        t++;
> +                }
> +
>                  ffio_init_context(&pb_local, buffer, b - buffer, 0, NULL, NULL, NULL,
>                                    NULL);
>                  tlen = b - buffer;

Also I just saw that 9ae80e6a9cefcab61e867256ba19ef78a4bfe0cb seems to
claim that the current code is correct (and my commit essentially
reverts it). Taglib decodes the tag correctly, though.