M4B Clearing Tag fails to load Audio Headers or more likely miss aligned.

CooPzZ commented 2 years ago

The problem

I was wanting to see what the Images in Chapters would look like in VLC using a M4B file and I keep coming up against an Issue/Bug I cannot seem to work around that the Audio Headers disappear in ATL and in VLC may or may not play it after. (MP3 Chapters and Images work fine however I can't see them in VLC - I read somewhere that they were now supported but not from what I can see using ATL)

I ran into the issue as from playing with ID3 tags in the past it was easier to remove the tag and add again to get rid of the curruption.

Steps:

Converting some MP3's into one M4B using either Audiobook Converter (ABC) latest version or InAudible (1.97)
On Viewing the file with ATL I can see the Meta and Chapters and all looks good.
Changing the meta simply and saving also worked fine.
Adding an Image to 1 or more chapters, the file saved however the images failed to save in the chapters that I can see (And it's really slow when it's going through the resize [LengthenStream] (on a bigger file, even worse over a network share) - a progress output in here would help also)
On Removing the Meta tag (using Track.Remove(ANY)) and saving again the meta appears to save correctly however the Audio Headers are no longer loaded. I can only assume this is what it was for and hope I'm not using it incorrectly. I did try on removing only the Native as well with the same result.

Environment

ATL version (or git revision) that exhibits the issue: 4.9
Windows 10

Details

I've done this in an app I was playing with written in vb.net 2021 and repurposing a code snippet in the ATL lib which I'll use as an example. Your Example in the UnitTests works perfectly so I can only assume it's something not handled from the Conversion App files output - yes probably not your issue but the Clearing causing the Bad Header which I thought was weird and worth knowing about. I don't know enough about the format to diagnose further.

Focus on the Remove Tag: [TestMethod, TestCategory("snippets")] public void CS_WriteChaptersMyTest() { //string fileToTestOn = audioFilePath; string fileToTestOn = "M:\Temp\Audio\TestFromABC.m4b"; System.IO.File.Delete(fileToTestOn); System.IO.File.Copy("M:\Temp\Audio\TestFromABC-Orig.m4b", fileToTestOn); Track theFile = new Track(fileToTestOn); double tDuration = theFile.DurationMs; System.Console.WriteLine(theFile.DurationMs.ToString()); theFile.Remove(ATL.AudioData.MetaDataIOFactory.TagType.ANY); //theFile.Save(); //Dont need. theFile = new Track(fileToTestOn); System.Console.WriteLine(theFile.DurationMs.ToString()); //Gone!? Assert.AreEqual(tDuration, theFile.DurationMs, "Duration has dissapeared or wrong."); }

Thankyou for your time - It's a very interesting lib that I'm sure to use going forward and appreciate the amount of work that looks to have already gone into it.

Zeugma440 commented 2 years ago

Thanks for the detailed report.

To facilitate reproductibility, could you please send one of the files that you created at step 1 using a 3rd party file sharing platform ?

Thanks in advance~

CooPzZ commented 2 years ago

Email sent - another piece of info I've noticed in summary: On adding a character to the first chapter that causes a file expansion - the AudioDataOffset didn't move but the audiodatasize increased by 1. Interestingly when I removed it again it went back to normal.

Zeugma440 commented 2 years ago

Mail received, thanks a bunch.

I'm gonna be on vacations for one week without access to Visual Studio. You'll probably get a partial answer from me, but the code itself will come a bit later 😉

Zeugma440 commented 2 years ago

There are many things in your post and the e-mail.

What's certain is Remove does have issues on MP4/M4A. You seem to be the first one to notice it. I'll work on that and keep you informed.

As for the other issues

Adding a "1" at the end of the title of chapter 1 : I haven't noticed any problem
CopySameStream has been designed to avoid creating a 2nd file on disk or, even worse, instanciating it entirely into memory. As for performance issues, you may find tips in the FAQ : https://github.com/Zeugma440/atldotnet/wiki/4.-FAQ-(Frequently-Asked-Questions)#FAQM.5

Zeugma440 commented 2 years ago

Removing metadata from MP4/M4A should be fixed now.

The problem was mainly due to bad handing of files with multiple tracks, plus the fact that removal hadn't been extensively tested.

Please grab the latest commit from main and let me know if that works for you. Your sample file does come out unscathed now 😁

CooPzZ commented 2 years ago

The clear appears to work ok now. Editing the chapter - same file without a clear does not - makes the file un-playable! although it looks good from duration point of view. As mentioned before I note the Offset didn't change but the audio datasize did which is odd - I would think the offset would change and the audio data to be the same.

Doing a test on removing the tag first helped and the file was still playable and it looks like the audio datasize stayed the same this time with the offset moving - although in playing in vlc the chapter image would not change from the first one.. I also did another test based on this one where I changed the chapter 1 like in the first test and it worked fine also. But I know in playing around - especially with big files I broke it again with something.

CooPzZ commented 2 years ago

Possible speed improvement and status with the remove tags (ANY or NATIVE) - and you notice this much more on big files - it goes though the CopySameStream 4+ times. Presumingly this is because it is re-sizing the file for each tag it is removing. Again on a big file when using a buffer of 256M this could be 3-10secs each time when running locally - on the network also it's extends it even more.
I'm assuming you might be able to resize once after all the changes are made to make it do it once.

Also without a status it does look like it's hanging because of the resizing of the file. As its the chapters I'm editing mostly it happens everytime I save it, hence why I wanted to show a status on it.

PS: I'm not telling you what to do here, and only letting you know with my use of the library what I'm seeing. This can go into another Issue if you'd like me to log it.

CooPzZ commented 2 years ago

Heres the code for those 2 tests

        [TestMethod, TestCategory("snippets")]
        public void CS_UpdateChapter1SmallBookTest()
        {
            //string fileToTestOn = audioFilePath;
            string fileToTestOn = "M:\\Temp\\Audio\\TestFromABC.m4b";
            System.IO.File.Delete(fileToTestOn);
            System.IO.File.Copy("M:\\Temp\\Audio\\TestFromABC-Orig.m4b", fileToTestOn);
            Track theFile = new Track(fileToTestOn);
            double tDuration = theFile.DurationMs;
            long lDataOffset = theFile.TechnicalInformation.AudioDataOffset;
            long lDataAudioSize = theFile.TechnicalInformation.AudioDataSize;
            theFile.Chapters[0].Title += 'x';
            theFile.Save();
            theFile = new Track(fileToTestOn);
            Assert.AreEqual(tDuration, theFile.DurationMs, "Duration has dissapeared or wrong.");
            Assert.AreEqual(lDataAudioSize, theFile.TechnicalInformation.AudioDataSize, "Audio Datasize has changed.");
            Assert.AreNotEqual(lDataOffset, theFile.TechnicalInformation.AudioDataOffset, "DataOffset has not moved.");
            //File is not longer playable in vlc or naudio
        }

        public void CS_UpdateClearAndUpdateSmallBookTest()
        {
            //string fileToTestOn = audioFilePath;
            string fileToTestOn = "M:\\Temp\\Audio\\TestFromABC.m4b";
            System.IO.File.Delete(fileToTestOn);
            System.IO.File.Copy("M:\\Temp\\Audio\\TestFromABC-Orig.m4b", fileToTestOn);
            Track theFile = new Track(fileToTestOn);
            double tDuration = theFile.DurationMs;
            long lDataOffset = theFile.TechnicalInformation.AudioDataOffset;
            long lDataAudioSize = theFile.TechnicalInformation.AudioDataSize;
            theFile.Remove(ATL.AudioData.MetaDataIOFactory.TagType.ANY); //clear the tags

            //re-add some meta
            theFile.Chapters = new System.Collections.Generic.List<ChapterInfo>();
            ChapterInfo ch = new ChapterInfo();
            ch.StartTime = 123;
            ch.EndTime = 30229;
            ch.UniqueID = "";
            ch.Title = "aaa";
            ch.Subtitle = "bbb";
            ch.Url = new ChapterInfo.UrlInfo("ccc", "ddd");
            theFile.Chapters.Add(ch);
            ch = new ChapterInfo();
            ch.StartTime = 30230;
            ch.EndTime = 120890;
            ch.UniqueID = "002";
            ch.Title = "aaa0";
            ch.Subtitle = "bbb0";
            ch.Url = new ChapterInfo.UrlInfo("ccc", "ddd0");
            // Add a picture to the 2nd chapter
            ch.Picture = PictureInfo.fromBinaryData(System.IO.File.ReadAllBytes(imagePath));
            theFile.Chapters.Add(ch);
            theFile.Save();
            ch = new ChapterInfo();
            ch.StartTime = 120891;
            ch.EndTime = 220890;
            ch.UniqueID = "003";
            ch.Title = "aaa1";
            ch.Subtitle = "bbb1";
            // Add a picture to the 3rd chapter
            ch.Picture = PictureInfo.fromBinaryData(System.IO.File.ReadAllBytes("M:\\Temp\\Audio\\Born Dead.jpg"));
            theFile.Chapters.Add(ch);
            theFile.Save();

            //Change a chapter
            theFile.Chapters[0].Title += 'x';
            theFile.Save();
            theFile = new Track(fileToTestOn);
            Assert.AreEqual(tDuration, theFile.DurationMs, "Duration has dissapeared or wrong.");
            Assert.AreEqual(lDataAudioSize, theFile.TechnicalInformation.AudioDataSize, "Audio Datasize has changed.");
            Assert.AreNotEqual(lDataOffset, theFile.TechnicalInformation.AudioDataOffset, "DataOffset has not moved.");
            //All looks good and plays in vlc - however the image doesnt change from the KFC one.
        }

Zeugma440 commented 2 years ago

First things first, I'm glad to see we're going in the right direction !

Thanks a bunch for the test code you just posted, I'll see if I can repro these two issues using the file you sent.

As mentioned before I note the Offset didn't change but the audio datasize did which is odd - I would think the offset would change and the audio data to be the same.

That's specific to MP4/M4A chapters. On other formats, tags are clearly separated from audio data; they both live in their own "zones" of the file and your expectations about audio data size and offset are correct.

For MP4/M4A chapters, as described here, chapter data, including title and pictures, are interleaved with audio data inside the mdat atom. What you're observing is explained by ATL's implementation :

AudioDataOffset = <offset of the MDAT atom>
AudioDataSize = <size of the MDAT atom>

I'm assuming you might be able to resize once after all the changes are made to make it do it once.

By design, ATL always operates on the file itself to avoid losing time copying bytes to memory or to a second file.

To avoid resizing back and forth with every micro change, ATL leverages an internal buffer and the file's own padding atoms, but those can't solve every single case, especially on MP4/M4A files which :

Tend to be large if you're into podcasts or audiobooks
Have a complex structure that requires many adjustments in various parts of the file during editing

Last but not least, the impact on performance is magnified when you operate on files located on a slow storage (e.g. NAS or SD card). If that's the case, I recommend copying the file locally for editing.

Also without a status it does look like it's hanging because of the resizing of the file. As its the chapters I'm editing mostly it happens everytime I save it, hence why I wanted to show a status on it. This can go into another Issue if you'd like me to log it.

I see you've found #46, which is the place that discussion should go to.

CooPzZ commented 2 years ago

Thanks for the clarification > The technique sounds kool, and it's new to me, as are all these file formats - hence I don't have the time to learn them all and went looking for a lib like this and was on the verge of giving up. I totally understand the effort you've gone to and thank you again for your time and others to simplify doing this work.

Zeugma440 commented 2 years ago

Alright, here's what I found :

1st issue (unplayable file) was a regression and has been fixed
2nd issue (broken chapter pictures on VLC) was caused by various factors I just discovered :
- Some obscure duration rounding issue that has been fixed
- The fact that VLC seems to hate when the first chapter doesn't start at 0:00
- The fact that VLC seems to hate when you add pictures to certain chapters and not to others

I know I'm somehow to blame for these last two as the code snippet I published does everything wrong 😆

I just updated the snippet and logged a warning on the ATL logger whenever one of these two issues happen.

Could you tell me if everything works on your side now ?

CooPzZ commented 2 years ago

Sorry Zeugma,

I believe is fixed
Chapter images - looking the result of the test files in \tmp using m4a file.

Correct chapters - TagIO_RW_MP4_Chapters_QT_Pic_Create()

the first chapter image didn't appear, well it look like it tried but empty pic, when I opened it in vlc the 2nd did (with the wood) I can also see the chapter images when opened using Track

Incorrect chapters - TagIO_RW_MP4_Chapters_QT_Pic_QA()

opening with the Track object didn't load/show the picture in chapter 2, however vlc showed the image all the way through.

I'm not sure if it matters but in trying to do m4b's and anytime I save the images in the chapters using Track it does not make them available after a save, nor do I know if they've saved them. Looking through the AudioDataManager they don't exist either - but vlc seems to find something sometimes.

Another Test using those files: I tried using your pictures and my own pictures.

        [TestMethod, TestCategory("snippets")]
        public void CS_UpdateChapter1AndPicSmallBookTest()
        {
            //string fileToTestOn = audioFilePath;
            string fileToTestOn = "M:\\Temp\\Audio\\TestFromABC.m4b";
            System.IO.File.Delete(fileToTestOn);
            System.IO.File.Copy("M:\\Temp\\Audio\\TestFromABC-Orig.m4b", fileToTestOn);
            Track theFile = new Track(fileToTestOn);
            double tDuration = theFile.DurationMs;
            long lDataOffset = theFile.TechnicalInformation.AudioDataOffset;
            long lDataAudioSize = theFile.TechnicalInformation.AudioDataSize;
            theFile.Chapters[0].Title += 'x';
            theFile.Chapters[0].Picture = PictureInfo.fromBinaryData(System.IO.File.ReadAllBytes(imagePath1));
            //theFile.Chapters[0].Picture = PictureInfo.fromBinaryData(System.IO.File.ReadAllBytes("M:\\Temp\\Audio\\avatarOfSorts.png"));
            theFile.Chapters[0].Picture.ComputePicHash();
            theFile.Chapters[1].Title += 'x';
            theFile.Chapters[1].Picture = PictureInfo.fromBinaryData(System.IO.File.ReadAllBytes(imagePath2));
            //theFile.Chapters[1].Picture = PictureInfo.fromBinaryData(System.IO.File.ReadAllBytes("M:\\Temp\\Audio\\themeOfTheTrack.jpg"));
            theFile.Chapters[1].Picture.ComputePicHash();

            theFile.Save();
            theFile = new Track(fileToTestOn);

            Assert.IsNotNull(theFile.Chapters[0].Picture, "1st picture doesn't exist");
            Assert.IsNotNull(theFile.Chapters[1].Picture,"2nd picture doesn't exist");
        }

Zeugma440 commented 2 years ago

Thanks for your feedback.

The point of TagIO_RW_MP4_Chapters_QT_Pic_QA is to make sure warnings appear when setting up chapters that VLC doesn't like. It's natural that the resulting file has issues, as the test creates them on purpose.

I'm not sure if it matters but in trying to do m4b's and anytime I save the images in the chapters using Track it does not make them available after a save, nor do I know if they've saved them. Looking through the AudioDataManager they don't exist either - but vlc seems to find something sometimes.

I'm not sure what to make of that statement. How can I repro that issue ?

As for the replacement of existing pictures, I managed to reproduce the problem with the code you provided. I'll keep you informed.

CooPzZ commented 2 years ago

I'm not sure what to make of that statement. How can I repro that issue ?

It was a comment for the unittest added below it - so it looks like you already have found there is an issue 👍. I'm just not sure if m4b and m4a are the same as you don't seem to test m4b's - naudio can't play the m4a's so I wasn't sure if there is another set of unit tests you need for m4b's

The point of TagIO_RW_MP4_Chapters_QT_Pic_QA is to make sure warnings appear when setting up chapters that VLC doesn't like. It's natural that the resulting file has issues, as the test creates them on purpose.

I understand that it's not going to work in vlc, however on loading the file again the pic on chapter 2 has disappeared which I assume you wouldn't want.

Zeugma440 commented 2 years ago

Looks good now ! 😄 (for both issues)

I'm just not sure if m4b and m4a are the same as you don't seem to test m4b's - naudio can't play the m4a's so I wasn't sure if there is another set of unit tests you need for m4b's

MP4, AAX, M4A and M4B's share the exact same structure. That's how versatile that format actually is 😉

Afaik, the extension is there to tell the OS not to read these files with the same app, as some of them have sound only, some others have video, some are meant to contain chapters...

CooPzZ commented 2 years ago

Sorry, the 2nd Assert you added of mine was wrong and when corrected its not working. And therefore both are still failing with the missing Chapter pics. Interestingly on saving the tag they are no longer available either.

Zeugma440 commented 2 years ago

Dang... sorry about that 🙏 I'll get back to you soon

Zeugma440 commented 2 years ago

CS_UpdateChapter1AndPicSmallBookTest now works as expected ✅

TL;DR : I realized the Assertion you put on TagIO_RW_MP4_Chapters_QT_Pic_QA can't work at all, and decided to remove it.

On that unit test, we're saving :

No picture inside the first chapter that starts at 0:01
The 2nd wood picture inside the 2nd chapter that starts at 0:10

To store that inside an ID3v2 structure, we'd describe two chapters, respectively starting at 0:01 and 0:10, one without picture and the other with a picture, and that would fit without issues.

This is very different with an MP4 structure, as :

Chapter titles are described in a separate structure as chapter pictures
Neither of them are associated with a starting time. They are associated with a duration, assuming everything starts at 0:00

As a consequence, a timespan when there's no displayed picture cannot be described in an MP4 structure. Reading the file generated in the unit test results in finding the wood picture associated with the first chapter (as both appear at index 0 and are assumed starting at 0:00), and no picture associated with the second chapter.

The only solution I can imagine to make that work would be for ATL to auto-generate a blank picture and insert it on chapters with no declared picture to maintain titles and pictures ordered the very same way... but creating media out of thin air sounds real messy. I'd rather stick to creating warnings in the log imho.

CooPzZ commented 2 years ago

I think there is a problem with storage otherwise again it is improved. I'll send a video with what I see.

Zeugma440 commented 2 years ago

The storage problem is fixed. File size now decreases as expected when removing chapter pictures - see cc2bb1310855e5c0ce4e712a927faa5d3c891cbb. Picture data from removed chapters is now removed from the file.

Regarding VLC behaviour, I don't know either how I should follow-up to what you observed.

To be honest, I'm not sure whether VLC has a shabby implementation or if ATL does things the way it isn't supposed to. It's difficult to know for sure as I haven't found any "best practice" or "guidelines" on how to implement chapter pictures. I just implemented a spec-compliant solution that is simple to manage from the library's point of view.

If that's still a concern for you, I advise you open a new "VLC investigations" Github issue, as it's an entirely different problem.

CooPzZ commented 2 years ago

Excellent work - I think were almost there. The chapter images work pretty darn good now - I'm not necessarily going to use this feature but thought to help you through the issues with m4b files. I agree with what you have stated - if there isn't a best practice you do as good as you can -> however, your implementation may become it. 😉

So there is one last issue I can see in that not all the pictures are re-loaded after the 'not all chaps have images' warning, even though they are still in the file and VLC shows them (better now), I'll do another video to quantify.

Zeugma440 commented 2 years ago

1.LastChapterIssue.mp4

What you see at 1:27 is the result of VLC caching cover pictures (see https://forum.videolan.org/viewtopic.php?p=426999). Judging by the file size, it's impossible the file contains anything else than the original cover pic. => Not an ATL bug
At 2:16, I see the problem but I can't manage to reproduce it on my side. Could you send me the modified file with the issue ?

CooPzZ commented 2 years ago

No problem – added the output with only the first 2 chapters have images.

TestFromABC-Orig - with1st2chapimages.m4b

Cheers CooPzZ

From: Zeugma440 @.> Sent: Tuesday, 13 September 2022 5:13 AM To: Zeugma440/atldotnet @.> Cc: CooPzZ @.>; Author @.> Subject: Re: [Zeugma440/atldotnet] M4B Clearing Tag fails to load Audio Headers or more likely miss aligned. (Issue #160)

1.LastChapterIssue.mp4

What you see at 1:27 is the result of VLC caching cover pictures (see https://forum.videolan.org/viewtopic.php?p=426999). Judging by the file size, it's impossible the file contains anything else than the original cover pic. => Not an ATL bug
At 2:16, I see the problem but I can't manage to reproduce it on my side. Could you send me the modified file with the issue ?

Zeugma440 commented 2 years ago

I can't see it. Are you certain your put it in the correct place ?

CooPzZ commented 2 years ago

Sorry, I had closed the app that syncs - to save CPU cycles yesterday - it is there now.

Zeugma440 commented 2 years ago

It works on my side...

Track theFile = new Track(filePath);

theFile.Chapters[0].Picture.ComputePicHash();
Console.WriteLine(theFile.Chapters[0].Picture.PictureHash); <-- successfuly shows picture hash 1
theFile.Chapters[1].Picture.ComputePicHash();
Console.WriteLine(theFile.Chapters[1].Picture.PictureHash); <-- successfuly shows picture hash 2

CooPzZ commented 2 years ago

Try loading the pictures, or saving them off and viewing.
I didn't notice the error before, that occurred in the status bar - Parameter is not valid which happens on loading the chapters. Loading the PictureData straight to a picture box the same error occurs.

Looking at the data, New Test to compare the good file with same images to the broken one:

you'll see the picture length is correct,
The data is not and hence cannot load - assuming missaligned output.

Should be:

Broken one:

Video (ChapImages-InvalidParameter.mp4) and those files added (TestFromABC-Orig - T-AllImages.m4b, TestFromABC-Orig - T-1stImageOnly.m4b)

Zeugma440 commented 2 years ago

Thanks to you, I realized the FNV1a hash implementation I've been relying on to compare picture data was utterly useless.

I've fixed that, plus the issue itself.

CooPzZ commented 2 years ago

Almost - yes now the picture is available but it's not on the chapter I specified if not the first one. ie: I added a pic to chapter 2 only and it loaded on chapter 1? I tried with more chapters and with seperations and it reverts them all to starting from chapter 1.

The only solution I can imagine to make that work would be for ATL to auto-generate a blank picture and insert it on chapters with no declared picture to maintain titles and pictures ordered the very same way... but creating media out of thin air sounds real messy. I'd rather stick to creating warnings in the log IMHO.

Is this-this issue?

And sorry otherwise looks good.

CooPzZ commented 2 years ago

After doing the above the clear tags is also not working when changing chapters, doesn't remove the images again by the looks and then the file is messed up -- 😣 Track.Remove(ATL.AudioData.MetaDataIOFactory.TagType.ANY)

I'm sorry, I would write all these tests up, but don't have the time at the moment - maybe next week. 🤞

Zeugma440 commented 2 years ago

Almost - yes now the picture is available but it's not on the chapter I specified if not the first one. ie: I added a pic to chapter 2 only and it loaded on chapter 1? I tried with more chapters and with seperations and it reverts them all to starting from chapter 1.

The only solution I can imagine to make that work would be for ATL to auto-generate a blank picture and insert it on chapters with no declared picture to maintain titles and pictures ordered the very same way... but creating media out of thin air sounds real messy. I'd rather stick to creating warnings in the log IMHO.

Is this-this issue?

And sorry otherwise looks good.

Exactly, it's that issue. The MP4/M4A structure isn't meant to store chapters without images. I can't do anything except "cheating" by inserting blank pictures, but I won't do that yet, as that use case still sounds theoretical to me.

Zeugma440 commented 2 years ago

After doing the above the clear tags is also not working when changing chapters, doesn't remove the images again by the looks and then the file is messed up -- 😣 Track.Remove(ATL.AudioData.MetaDataIOFactory.TagType.ANY)

I'm sorry, I would write all these tests up, but don't have the time at the moment - maybe next week. 🤞

Don't worry, it doesn't look too difficult to repro. I'll look into it ASAP.

Thanks for your patience anyway. As the song says, Through all the doubt Somehow, they knew And stone by stone, they built it high 🎶

CooPzZ commented 2 years ago

Exactly, it's that issue. The MP4/M4A structure isn't meant to store chapters without images. I can't do anything except "cheating" by inserting blank pictures, but I won't do that yet, as that use case still sounds theoretical to me.

I'm thinking in editing this actually makes sense to do now - it should be minimal bytes for a blank image, but the added images are where you put them. But, only do this if there is min 1 image added to a chapter somewhere. You could still show it as a null image when viewed in ATL if your blank image is used. This is better for making sense while editing and outputs as expected. If not the Warning needs to describe what happened.

Otherwise I think, instead of the warning, an Error needs to occur to prevent adding without images on some chapters as the format doesn't support it and they won't go to the expected chapter, if not done in a row from chapter 1. I'd call that fair in that you are all in or not because of the format.

Zeugma440 commented 2 years ago

After doing the above the clear tags is also not working when changing chapters, doesn't remove the images again by the looks and then the file is messed up -- 😣 Track.Remove(ATL.AudioData.MetaDataIOFactory.TagType.ANY)

I tried multiple combinations of what you're describing but I can't bring the file to get messed up. Everytime I clear the data, no matter which chapter has pictures and which doesn't, VLC manages to read it properly.

I guess I'll have to wait for precise repro steps 😋

For now I'll release everything we've done so far while keeping that issue open.

I'm thinking in editing this actually makes sense to do now - it should be minimal bytes for a blank image, but the added images are where you put them. But, only do this if there is min 1 image added to a chapter somewhere. You could still show it as a null image when viewed in ATL if your blank image is used. This is better for making sense while editing and outputs as expected. If not the Warning needs to describe what happened.

Otherwise I think, instead of the warning, an Error needs to occur to prevent adding without images on some chapters as the format doesn't support it and they won't go to the expected chapter, if not done in a row from chapter 1. I'd call that fair in that you are all in or not because of the format.

I've brought that one outside as a specific issue -> #163

CooPzZ commented 2 years ago

Unit Tests

        readonly string imagePath1 = @"M:\Temp\Audio\image1.jpg";
        readonly string imagePath2 = @"M:\Temp\Audio\image2.jpg";
        readonly string imagePath3 = @"M:\Temp\Audio\image3.jpg";
        readonly string imagePath4 = @"M:\Temp\Audio\image4.jpg";

       [TestMethod, TestCategory("snippets")]
        public void CS_RemoveTag()
        {
            //string fileToTestOn = audioFilePath;
            string fileToTestOn = "M:\\Temp\\Audio\\TestFromABC-Test.m4b";
            System.IO.File.Delete(fileToTestOn);
            System.IO.File.Copy("M:\\Temp\\Audio\\TestFromABC-Orig.m4b", fileToTestOn);
            Track theFile = new Track(fileToTestOn);
            double tDuration = theFile.DurationMs; System.Console.WriteLine("Pre Duration: " + tDuration);
            double dLenght = FileLength(fileToTestOn); System.Console.WriteLine("Pre File Length: " + dLenght);
            theFile.Remove(ATL.AudioData.MetaDataIOFactory.TagType.NATIVE);
            //theFile.Save(); //Dont need.
            theFile = new Track(fileToTestOn);
            double dPostLenght = FileLength(fileToTestOn);
            System.Console.WriteLine("POST Duration: " + theFile.DurationMs.ToString());
            System.Console.WriteLine("POST File Length: " + dPostLenght);

            Assert.AreEqual(tDuration, theFile.DurationMs, "Duration should be the same.");
            Assert.IsTrue(dLenght > dPostLenght, "File should be smaller.");
            Assert.AreEqual(9526213, dPostLenght, "File should be 9526213 once tags are removed.");
        }

        [TestMethod, TestCategory("snippets")]
        public void CS_AddChap2Image_then_RemoveTag()
        {
            //string fileToTestOn = audioFilePath;
            string fileToTestOn = "M:\\Temp\\Audio\\TestFromABC-Test.m4b";
            System.IO.File.Delete(fileToTestOn);
            System.IO.File.Copy("M:\\Temp\\Audio\\TestFromABC-Orig.m4b", fileToTestOn);
            Track theFile = new Track(fileToTestOn);
            System.Console.WriteLine("# Initial Details #");
            double tDuration = theFile.DurationMs; System.Console.WriteLine("Duration: " + tDuration);
            double dLenght = FileLength(fileToTestOn); System.Console.WriteLine("File Length: " + dLenght);
            System.Console.WriteLine("Chapters: " + theFile.Chapters.Count.ToString());
            System.Console.WriteLine("Chapters(1) Image: " + (theFile.Chapters[0].Picture != null));
            System.Console.WriteLine("Chapters(2) Image: " + (theFile.Chapters[1].Picture != null));

            System.Console.WriteLine("# Chap 2 Image added #");
            theFile.Chapters[1].Picture = PictureInfo.fromBinaryData(System.IO.File.ReadAllBytes(imagePath2));
            theFile.Save();
            theFile = new Track(fileToTestOn);
            System.Console.WriteLine("Duration: " + theFile.DurationMs);
            System.Console.WriteLine("File Length: " + FileLength(fileToTestOn));
            System.Console.WriteLine("Chapters: " + theFile.Chapters.Count.ToString());
            System.Console.WriteLine("Chapters(1) Image: " + (theFile.Chapters[0].Picture != null));
            System.Console.WriteLine("Chapters(2) Image: " + (theFile.Chapters[1].Picture != null) );

            //Switch these Assertions for expected editing.
            //Assert.IsTrue(theFile.Chapters[0].Picture == null, "Picture should not exist in Chap 1."); 
            //Assert.IsTrue(theFile.Chapters[1].Picture != null, "Picture should exist in Chap 2.");
            Assert.IsTrue(theFile.Chapters[0].Picture != null, "Picture should should exist in Chap 1 due to MP4 format limitation.");
            Assert.IsTrue(theFile.Chapters[1].Picture == null, "Picture is no longer in Chap 2 due to MP4 format limitation.");

            System.Console.WriteLine("# Remove Tags #");
            theFile.Remove(ATL.AudioData.MetaDataIOFactory.TagType.NATIVE);
            theFile = new Track(fileToTestOn);
            System.Console.WriteLine("Duration: " + theFile.DurationMs.ToString());
            System.Console.WriteLine("File Length: " + FileLength(fileToTestOn));
            System.Console.WriteLine("Chapters: " + theFile.Chapters.Count.ToString());
            if (theFile.Chapters.Count > 0)
            {
                System.Console.WriteLine("Chapters(1) Image: " + (theFile.Chapters[0].Picture != null));
                System.Console.WriteLine("Chapters(2) Image: " + (theFile.Chapters[1].Picture != null));
            }

            Assert.AreEqual(tDuration, theFile.DurationMs, "Duration should be the same.");
            Assert.IsTrue(dLenght > FileLength(fileToTestOn), "File should be smaller.");
            Assert.AreEqual(9623601, dLenght, "File should be 9526213 once tags are removed - As per test CS_RemoveTag.");
        }

Results: using latest code.

CooPzZ commented 2 years ago

FYI: I've done a fork and begun adding some more holistic unittests for the Track object on MP4s including the ones above. https://github.com/Zeugma440/atldotnet/compare/main...CooPzZ:atldotnet:AddTrackM4BUnitTests Added #FAILED on tests not currently working. Source files are still in the share.

Zeugma440 commented 2 years ago

Awesome. Thanks a lot, I'm gonna work on that this week.

Warning To ensure the ATL project doesn't get flagged/banned by Microsoft, please make sure that additional test files

Do not have copyrighted material
Have a reasonable size, i.e. 2MB tops (we're using Git !)

CooPzZ commented 2 years ago

Thanks, and no i didn't intend to add them to the repo for those reasons. Ill make another testfile in the meantime based on the original problem format.

Zeugma440 commented 2 years ago

Got it reproduced. Both of your unit tests go together really well and underline a structural issue I didn't see until now.

I'm in the middle of finding a sustainable solution for this. I'll also integrate both unit tests to the official test project, probably by truncating and zeroing audio data on our test file ("Say baby do you wanna lay down by my side" 😁 )

I might use this opportunity to code a few dedicated methods to automatically "clean up" copyrighted audio files and make them Github-compatible 😉

Zeugma440 commented 2 years ago

Almost there... the issues highlighted by your tests are now fixed, but I still have to integrate these tests into the libray's standard unit tests. I'll publish a new version when it's done.

For the record, I also need to explain why test 2 doesn't create a file with the same size as test 1, but a smaller file (!)

The source test file has a bogus padding atom (free) that is completely empty. The size of this padding atom is 8 bytes, that is the size of its header 😆
Test 1 has just one step : remove metadata => resulting file is 9526123 bytes long
Test 2 has two steps : edit chapters and remove metadata => resulting file is 9526115 bytes long (i.e. 8 bytes less)

ATL doesn't remove padding atoms when removing metadata, because padding exists to ease medata expansion without expanding file size. That explains why test 1 keeps that 8-byte padding atom.

Test 2, however, starts by editing chapters, which automatically rewrites all metadata and removes that useless padding atom in the process.

=> I'll document that in the target unit test

Zeugma440 commented 1 year ago

That's done. The whole thing has been released today (v4.11)

CooPzZ commented 1 year ago

It’s a bit interesting to me that the result is not the same, in that different edits can have a different result in consideration of a buffer already existing. But I trust you understand this bit better.

Cleanup in a couple of comments to change - from my fork, I don’t think I’ll do a pull request for that. https://github.com/CooPzZ/atldotnet/commit/ba5ce193b1f94a4d01983b47ee3f1ce43a3da21b

And sorry about this, I'm still getting weird bug/errors happening and cannot reflect the same in unit testing, however I think I found a way to test for it. Seems to be mostly if I remove the tag first. Last commit for the unittests. https://github.com/CooPzZ/atldotnet/commit/5e7ed406090aa407af917d0d73e1b16bbbb617d6

CooPzZ commented 1 year ago

And it's wierd in the app - it has other results that I cant get in the unittest. In this error I'm only updating the Title, Description and Artist (removed all the code to update the rest of the fields) after removing the tag first manually.

If you want to look at the results: TestFromABC-Orig - AfterClear-1stSave.m4b TestFromABC-Orig - AfterClear-2ndSave-ChangeDesc.m4b Logs are there too.

Zeugma440 commented 1 year ago

Reopened as per latest feedback

Zeugma440 commented 1 year ago

Fixed ! There were remaining issues, some generic and others specific to the MP4/M4A format.

The MDAT size declaration warning has also been fixed and added as a check inside a unit test (see 5d823bff243d37c5e660d6e934b0b40b411155a6)

As for your unit tests (looks like you've worked hard on that 😮 )

MP4Other : Most of them are now integrated as standard tests
MP4 : You're making many assumptions about file size evolution that need to be taken with a grain of salt. As ATL rewrites metadata, it also rewrites "structural" atoms along with it, sometimes using slightly different technical conventions than the ones that had been used to generate the original file in the first place. That's why adding and removing data can't always make the file come back to the exact same size.

A better way to do that might be to check if the size has evolved within reasonable bounds (e.g. if you've added a 2MB picture and removed it, new file size should be original file size +/- 1%).

Zeugma440 commented 1 year ago

Available in today's v4.12

CooPzZ commented 1 year ago

Fixed ! There were remaining issues, some generic and others specific to the MP4/M4A format.

Oh good, I thought I was doing something weird after those updates..

One more test - on removing the tag first, things go a bit weird again (like before) with the chapter images, you'll see that the image disappears but, it is still in the file and you can no longer access it, even with another clear. Wasn't sure about the 3. code as I'm not sure what will happen with the removal again and I'm assuming it should be the same as the first Remove. https://github.com/CooPzZ/atldotnet/commit/97ba702472385f7d26dc29d6af2eac41a5bb1bcf

MP4 tests were a way to test every field was working as expected - (mostly sanity tests for me, that could be adapted to all file types using Track object), I understand that there may be differences now but hadn't coded that in, adapt it if you like..

One test in there that was weird and kept failing and is maybe specific to this file only - is on a tag removal the IsVBR flagged changed. A normal save didn't change it. MP4. public void CS_RemoveTagLongBook() //resultTag.IsVBR = true; //#FAILS: Not sure why this flips -- uncomment to skip issue.

Zeugma440 commented 1 year ago

Here we go for another round

Zeugma440 commented 1 year ago

I'm on it.

One test in there that was weird and kept failing and is maybe specific to this file only - is on a tag removal the IsVBR flagged > changed. A normal save didn't change it. MP4. public void CS_RemoveTagLongBook() //resultTag.IsVBR = true; //#FAILS: Not sure why this flips -- uncomment to skip issue.

Could you add the "long book" file to the shared drive, please ?

Zeugma440 / atldotnet