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 <nfx...@googlemail.com> wrote: > On Tue, 30 Jan 2018 13:43:25 +0100 > wm4 <nfx...@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@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel _______________________________________________ ffmpeg-devel mailing list ffmpeg-devel@ffmpeg.org http://ffmpeg.org/mailman/listinfo/ffmpeg-devel