-
-
Notifications
You must be signed in to change notification settings - Fork 65
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
1x1 black picture causes problems in m4b again - ability to disable it (for specific formats)? #305
Comments
If I may, you doesn't seem to be so sure about that, and the log file your user submitted says
Creating an opt-out would certainly "fix" the problem by ignoring it, but I'm not sure I want to do that unless we confirm the feature working as intended is the actual source of the problem. Is there a way to get the file described in your issue or to reproduce the problem? |
You're right. I try to reproduce the problem with |
Ok, here is the code - as always it copies the original file to prevent overwriting the original metadata. I'm specifically interested in why the My Version is atldotnet 6.1.3. var path = "";
var sourceFile = Path.Combine(path, "sample.m4b");
var destinationFile = Path.Combine(path, "sample_copy.m4b");
if (File.Exists(destinationFile))
{
File.Delete(destinationFile);
}
File.Copy(sourceFile, destinationFile);
var track = new Track(destinationFile);
track.Album = "TestAlbum";
track.Chapters = new List<ChapterInfo>
{
new(){StartTime = 0,EndTime = 2000,Title = "1"},
new(){StartTime = 2000,EndTime = 4000,Title = "2"},
new(){StartTime = 4000,EndTime = 60000,Title = "3"},
};
await track.SaveAsync();
|
Thanks a bunch for taking the time to send the details ❤
What happened is the following : when fixing #296, I did prevent ATL from inserting 1x1 px chapter pictures when there are no chapter pictures in the original file, but I completely forgot to turn off the creation of the video track that was aimed at containing these chapter pictures. What mediainfo sees here isn't a cover but that empty video track that has the 1x1 default dimensions.
I eventually rolled my sleeves up and wrote a bona fide unit test to make sure everything works as planned : atldotnet/ATL.unit-test/IO/MetaData/MP4.cs Line 1053 in fc74b20
Here are the rules I put together: // Test behaviour when creating empty chapter pics
// Cases
// A: No chapter track should be created if there's no chapter
// B: No chapter picture (video) track should be created if none of the chapters have an attached picture
// C: No empty chapter picture should be generated if all chapters that contain a picture come one after the other
// D: An empty chapter picture should be generated for any chapter without pictures that's included between chapters that have one Last thing that needs to be discussed is that timestamp gap thingy
I understand what your idea is, but the M4A/MP4 format doesn't even make it possible to have two chapters that are not contiguous, as the notion of "starting time" doesn't exist on Quicktime Chapters - everything is assumed to start at timestamp Could you explain a little more the case you had in mind when you wrote that? BY any chance, are you referring to ID3v2 chapters that do have a starting time? |
Wow, that was quick, thank you very much.
Ah, that makes sense.
Very good. That also will prevent regression issues.
Looks very reasonable, great work.
I think I just mixed up ID3 and MP4 here. Now let's see if I got it right now:
The problem with MP4 is, that the first chapter has a picture, followed by chapter 2 with no picture, the picture of the first chapter will stay visible as long as you don't override it by a 1x1px black image in chapter 2. This problem won't occur in ID3 due to the nature of the standard - which means that my comment
doesn't make any sense other than you can define Chapters in atldotnet like this: var track = new Track("sample.m4b");
track.Album = "TestAlbum";
track.Chapters = new List<ChapterInfo>
{
new(){StartTime = 0,EndTime = 2000,Title = "1"},
new(){StartTime = 2500,EndTime = 4000,Title = "2"}, // 500ms gap
new(){StartTime = 5000,EndTime = 60000,Title = "3"}, // 1s gap
};
await track.SaveAsync(); I don't know what happens in this case internally (MP4 should not allow that) and I don't know what would happen, if you add a picture to chapter 1 and chapter 3 but NO picture to chapter 2. However, you can just ignore the comment about the gap, I don't think it makes much sense. Thank you. |
Correct
Correct
Almost correct - in the case you describe, the picture for chapter 1 will actually disappear when chapter 2 starts. However, if you add a chapter 3 with a picture, that one will immediately be displayed after chapter 1's picture (i.e. during chapter 2).
Thing is What happens under the hood is, when writing chapters to an M4A/MP4 file, ATL sets for each chapter "duration = endTime - startTime", ignoring gaps entirely. I have a question for you, as you've worked on many audiobook files and with many audiobook users : are there actual real-life files that feature actual gaps between chapters? If so, what's the reason for those?
It automatically inserts an empty 1x1 px image on chapter 2 😄 |
Absolutely, this was not meant to question the actual approach, I really worded that in a misleading way... what I really meant was, that the
No, not at all. I never faced any real use case having gaps between chapters. I could think of something like commercials in recorded live streams or author comments that could interrupt the regular chapter stream but usually this is just represented by another chapter with a different name scheme. Since metadata chapters are also available for video and subtitle content, maybe the chapter gap thing could find a use case there - in my opinion it's not required anywhere. Some interesting things I came across in my years of audio book tooling:
I think these kind of problems often cannot be solved very elegant - it's just a limitation of how the spec is designed, and sometimes limitations are good, because they make things less complex. For now I think your approach to fix the |
Fix is available on today's v6.14. Please close the issue if it works on your side~ PS : Have you ever tried working with Matroska chapters? The possibilities are crazy... |
Looks good. I'm going to integrate this in the next tone release and ask for feedback from the original issue reporter. Thank you very much for fixing it that quickly.
No I've never actively worked with MKV / Matroska, but I heard that it is complex. The linked "summary" of chapters sounds very over-engineered, but as in mp4 Matroska also supports Video and in the old DVD Menu days, this might have been a good way to integrate chapters supporting Menu selection. I don't know... at first I thought Menus are a good thing, but later on I mostly hated to wait 1 Minute to start watching the movie :-) I wish I had more time to work on stuff like this, but a few days ago my NVMe drive died and I had to restore my notebook from backups - noticed that I always wanted to try NixOS and down another rabbit hole... learned a lot but switching over to arch now - we'll see. |
You're very welcome. I should've detected it in the first place.
You just made me smile. "Over-engineered" is the exact word that first came to mind when reading the Matroska specs. I'm relieved I'm not the onlyone to perceive that 😄
Good luck with that 😉 |
Recently I got an issue in
m4b-tool
(one of my other projects), which is usingatldotnet
internally to tag files:sandreas/m4b-tool#269
Seems that
tone
is still suffering the1x1
black chapter image issue somehow, although I already updated to the fixedatldotnet
version6.11
(see #296).Would it be possible to remove the 1x1px workaround completely or at least opt out this feature via
Settings
here?Seems that most players have problems with this, although it might technically be correct.
The text was updated successfully, but these errors were encountered: