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

[Richard Shaffer]

LGTM, for whatever my opinion is worth.

Also easier to follow than the old code.

-Richard

On Tue, Jan 30, 2018 at 4:47 AM, wm4 <nfxjfg at googlemail.com> wrote:
> 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.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> http://ffmpeg.org/mailman/listinfo/ffmpeg-devel