Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

frame header bad format #258

Closed
Butterfly-Dragon opened this issue Jan 6, 2020 · 18 comments
Closed

frame header bad format #258

Butterfly-Dragon opened this issue Jan 6, 2020 · 18 comments

Comments

@Butterfly-Dragon
Copy link

Butterfly-Dragon commented Jan 6, 2020

It seems the program produces a bugged ID3v2 library, because, the total frame size is set to 566036 bytes instead of 566012.

So at the end of the ID3 frames, there are 24 bytes more.

10 bytes are padding, and then, the real MP3 frames begin.

Tested with foobar2000, which does frame resynchronisation the first mp3 frame is lost.

This has no incidence because it is a Xing MP3 frame (used for fast forwarding in the file). When you play the files you don't see a problem because resync is part of every player.

But some files do appear to be several hour longer than they are. They end at the correct time, way before the file appears to end, according to the player, and the player moves onto the next file, but the player also allows to move into one of the non-existant zones of the files, after their end, by simply clicking, and when this happens it can create problems as the computer tries to read as part of the MP3 random data on the disk which is not part of the MP3 file.

The 24 bytes MIGHT be the APIC (picture) header (the one with mimetype and image description), that have been counted twice. But i do not know.

@openaudible
Copy link
Owner

We use the latest builds of ffmpeg to convert the file to mp3. Any suggestions on the changing the ffmpeg command we use to create the mp3 would be appreciated.

You can look at the console when we create an mp3.. we add a bunch of meta data and use -id3v2_version because it creates an MP3 with cover art that shows in Windows as the book icon.

I'm guessing someone could also check the ffmpeg bug forums for information about this.. I'd be surprised if this wasn't a known issue.. but if it is unknown, an ffmpeg bug could be submitted.

@Butterfly-Dragon
Copy link
Author

Thanks.

Apparently you are using a nightly build.

I advise against that.

I switched the build with the one that was present in version 1.6.2 since it was the last version i tried before this one.

Problems are currently gone.

Unless a specific property (a bug fix or something) of a nightly build crops up that you are in extreme need of, i advise on using FFMPEG in the last known stable version.

@openaudible
Copy link
Owner

Thanks for the update. Yes, users uncovered a bug #250 in ffmpeg's LAME mp3 encoder that prevented converting some files.

I couldn't find a pre-built ffmpeg binary that included a new version of the LAME source, but would love to find something good and well-tested. I built ffmpeg for windows by scratch, which solved #250, but may have introduced this bug #258.

I think I need an easy way to identify the bug you're seeing. Is there a tagging app that flags it or something like ffprobe?

@Butterfly-Dragon
Copy link
Author

What i posted is a snippet of my dialogue with @gbouthenot regarding this issue:

gbouthenot/mp3splitter-js#2

While i am an electronic engineer i've never actually made looking into an MP3 my primary focus, so you could ask him, i just reported my findings as they happened via tinkering around by substituting various FFMPEG builds from this site: https://ffmpeg.zeranoe.com/builds/

@gbouthenot
Copy link

gbouthenot commented Jan 7, 2020

This seems related to ffmpeg in libavformat/id3v2.c / id3v2enc.c. When using chapter information, the padding is 10 bytes, and framesize is 14 bytes too long (making a total of 24 bytes). Without chapter information, the padding is 10 bytes, and the ID3 tag has the right size.

I am looking into ffmpeg sources changelog to tell when it occured the first time. But it is very time consuming...

@openaudible
Copy link
Owner

Is there an easy way to tell if the tags are correct from the command line?

I see in ffprobe we get messages like:

Skipping 168 bytes of junk at 695461.
or
Skipping 0 bytes of junk at 697175.

Since I am also trying to verify a bug #250 that (apparently?) only affects windows perhaps we could write a windows script to test different ffmpeg builds..

It looks like you wrote your own tag parser in mp3splitter.js... I suppose we could modify that..

I'll download a few versions from https://ffmpeg.zeranoe.com/builds/win64/static/ and see which ones include the LAME fix.

Also, does the release 4.2.1 have this bug?

@gbouthenot
Copy link

I don't know yet which ffmpeg commit introduces this "bug".
I wrote a tool to check if a mp3 file has a correct id3 frame length: https://gist.github.com/gbouthenot/4be2c02cf37f7e34a096f8a92fd8436a

I'm still investigating ffmpeg. It will take some time.

@Butterfly-Dragon
Copy link
Author

oh! Thanks!

@Butterfly-Dragon
Copy link
Author

the version of ffmpeg used in 1.6.2 did not have the bug if that is of any use, i am currently using that specific one.

@openaudible
Copy link
Owner

Ok.. I did some testing. I am running the tag test tool by @gbouthenot . From the zeranoe builds, it looks like it was working from 201902xx including:

ffmpeg-20190607-1e7a8b9-win64-static
and stopped working at:
ffmpeg-20190608-b6ca032-win64-static

I'm still looking for a version that has working AAX conversion and good tag generating...

So far the only one I've found is ffmpeg version N-76456-g6df2c94.. which I was using earlier.. but I need to test it joining mp3 files... So yes, the updated OpenAudible 1.6.4 has a bad tag generator.

@gbouthenot
Copy link

I found the commit that introduced this 'bug': https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/79aa91d562a53187d0b381da7203110f16cc1bb6

git show 79aa91d562a53187d0b381da7203110f16cc1bb6
commit 79aa91d562a53187d0b381da7203110f16cc1bb6 (HEAD, refs/bisect/bad)
Author: Paul B Mahol <onemda@gmail.com>
Date:   Tue Jun 4 16:44:19 2019 +0200

    avformat/id3v2enc: write CTOC too

diff --git a/libavformat/id3v2enc.c b/libavformat/id3v2enc.c
index ffe358f019..8d976f2d95 100644
--- a/libavformat/id3v2enc.c
+++ b/libavformat/id3v2enc.c
@@ -255,6 +255,42 @@ static int write_metadata(AVIOContext *pb, AVDictionary **metadata,
     return 0;
 }

+static int write_ctoc(AVFormatContext *s, ID3v2EncContext *id3, int enc)
+{
+    uint8_t *dyn_buf = NULL;
+    AVIOContext *dyn_bc = NULL;
+    char name[123];
+    int len, ret;
+
+    if (s->nb_chapters == 0)
+        return 0;
+
+    if ((ret = avio_open_dyn_buf(&dyn_bc)) < 0)
+        goto fail;
+
+    id3->len += avio_put_str(dyn_bc, "toc");
+    avio_wb16(dyn_bc, 0x03);
+    avio_w8(dyn_bc, s->nb_chapters);
+    for (int i = 0; i < s->nb_chapters; i++) {
+        snprintf(name, 122, "ch%d", i);
+        id3->len += avio_put_str(dyn_bc, name);
+    }
+    len = avio_close_dyn_buf(dyn_bc, &dyn_buf);
+    id3->len += 16 + ID3v2_HEADER_SIZE;
+
+    avio_wb32(s->pb, MKBETAG('C', 'T', 'O', 'C'));
+    avio_wb32(s->pb, len);
+    avio_wb16(s->pb, 0);
+    avio_write(s->pb, dyn_buf, len);
+
+fail:
+    if (dyn_bc && !dyn_buf)
+        avio_close_dyn_buf(dyn_bc, &dyn_buf);
+    av_freep(&dyn_buf);
+
+    return ret;
+}
+
 static int write_chapter(AVFormatContext *s, ID3v2EncContext *id3, int id, int enc)
 {
     const AVRational time_base = {1, 1000};
@@ -306,6 +342,9 @@ int ff_id3v2_write_metadata(AVFormatContext *s, ID3v2EncContext *id3)
     if ((ret = write_metadata(s->pb, &s->metadata, id3, enc)) < 0)
         return ret;

+    if ((ret = write_ctoc(s, id3, enc)) < 0)
+        return ret;
+
     for (i = 0; i < s->nb_chapters; i++) {
         if ((ret = write_chapter(s, id3, i, enc)) < 0)
             return ret;

@gbouthenot
Copy link

gbouthenot commented Jan 8, 2020

I will propose the following patch:

     if ((ret = avio_open_dyn_buf(&dyn_bc)) < 0)
         goto fail;
 
-    id3->len += avio_put_str(dyn_bc, "toc");
+    avio_put_str(dyn_bc, "toc");
     avio_w8(dyn_bc, 0x03);
     avio_w8(dyn_bc, s->nb_chapters);
     for (int i = 0; i < s->nb_chapters; i++) {
         snprintf(name, 122, "ch%d", i);
-        id3->len += avio_put_str(dyn_bc, name);
+        avio_put_str(dyn_bc, name);
     }
     len = avio_close_dyn_buf(dyn_bc, &dyn_buf);
-    id3->len += 16 + ID3v2_HEADER_SIZE;
+    id3->len += len + ID3v2_HEADER_SIZE;
 
     avio_wb32(s->pb, MKBETAG('C', 'T', 'O', 'C'));
     avio_wb32(s->pb, len);

@openaudible
Copy link
Owner

Nice work!

Are you familiar with ffmpeg's libmp3lame code too? Or how the zeranoe builds gets the "latest" LAME source code? I need ffmpeg to use the LAME version that fixes:
https://sourceforge.net/p/lame/bugs/496/

Thanks and Cheers!

@gbouthenot
Copy link

Are you familiar with ffmpeg's libmp3lame code too? Or how the zeranoe builds gets the "latest" LAME source code? I need ffmpeg to use the LAME version that fixes:
https://sourceforge.net/p/lame/bugs/496/

No. I don't know them. You should probably ask.

@gbouthenot
Copy link

Wohoo my patch has been commited to the main ffmpeg tree !
That means, that all new compiled versions won't have a bad ID3v2 header size anymore !
https://git.ffmpeg.org/gitweb/ffmpeg.git/commit/929e5159bc13da374b83f5627879c607acce180b

@openaudible
Copy link
Owner

Nice work!! You C programmers don't like comments... hah.. and congrats.

@ueen
Copy link

ueen commented Jan 19, 2020

Faced the same issue, what I did to fix already downloaded mp3s
get foobar2000
add your audiobooks
select all, right click
Utilities > Fix VBR MP3 headers

@Butterfly-Dragon
Copy link
Author

yeah, i just used mp3diags, it's just a lot of bother for a glitch induced by somebody's assumptions of a string length, instead of calculating it properly 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants