Implement EQ Preset MHOD (type 7)

[ThatBlockyPenguin]

Intro

First off, I want to say a huge thank you for all the work put into this project - it has been incredibly useful to me!

I also want to note that C# is not a language I am overly confident in, so whilst I have tested these changes on my own iPod and can confirm that they work (at least on my 1st gen Mini), if you notice any non-standard code practices or oddities in how I've implemented things, please let me know and I will be happy to adjust them.

---

TLDR;

This PR adds support for Equalizer (EQ) presets and addresses a bug regarding track data saving.

1. Implement EQ Preset MHOD (type 7)
   Added the logic to parse and write MHOD type 7 objects. This allows the library to recognize and preserve Equalizer preset data within the iTunesDB.
2. Fix Track.SetDataElement() bug
   Calling Track.SetDataElement() with data == null didn't set _isDirty to true. I fixed this by adding _isDirty = true; after _childSections.Remove(mhod);

---

Background

I found out that iTunes can set per-song EQ presets and sync these to iPods, which then use them if their own EQ is set to "flat", and immediately checked to see if Clickwheel supported it. I saw that it didn't, so I dug further and found that iTunes stores the EQ preset in an MHOD of type 7.
I didn't find any further information on this though.

Slightly concerned, I grabbed my iPod, cloned this library and wrote some test code. Well, lo and behold, after writing some EQ data from iTunes, I was able to read it from Clickwheel as a string MHOD. iPodLinux confirmed that this should be the case, and it turned out that each of the EQ presets has an ID, from 100, up to 121. I carefully mapped each of these IDs to their names from iTunes, and built some code infrastructure around it. This is the resultant PR.

When testing this, I found that when clearing an EQ preset by setting it to null, the change wouldn't be reflected on my iPod. Looking into it, I saw the bug I mentioned in SetDataElement, so I fixed that at the same time. If you'd rather, I can break that out into its own separate PR (literally just a one-line fix).

[ThatBlockyPenguin]

Whoops - was unaware that renaming the branch would close the PR!

[dstaley]

Thank you so much for this! Before I review, would you mind taking a look at the diff? It looks like Git might have converted line endingings in your PR, making it have more changes than necessary.

[ThatBlockyPenguin]

Ah, sorry about that, all sorted now!

[dstaley]

Thanks for cleaning that up! Before I start requesting changes I have a few questions:

1. Did you reference any documentation or code for the structure of the mhod string? I'd love to be able to update the included docs with the specifics and want to make sure I'm crediting the right place.
2. I believe iPod supports custom EQ, and I'm wondering if it's also stored in mhod 7. I can test this in a few days, but wanted to mention it to you in case you wanted to also give it a try. You can setup a custom EQ curve in iTunes and then assign it to a song on iPod. If it's stored in the same place we'll likely need to update the parser to accommodate that, and introduce an API to define a custom EQ curve. It's possible that custom EQ was added in a later DB version and doesn't use mhod 7, so this might take a little digging to figure out. I think the first question to answer is "Does mhod type 7 change when a custom EQ is set?"
3. Can you explain a little bit about why you chose to implement this as a ConvertibleUnicodeMHOD as opposed to just a plain EQPresetMHOD? If there isn't another mhod that would obviously use the ConvertibleUnicodeMHOD logic I think I'd prefer keeping things simple and just putting the existing logic into a EQPresetMHOD. This is especially true if custom EQ curves use mhod 7 with a different structure than a string.

[ThatBlockyPenguin]

Hey, no worries!
Just FYI, where it makes sense, every link I've given below links directly to the relevant section of the page using text fragment URLs.

1. Did you reference any documentation or code for the structure of the mhod string? I'd love to be able to update the included docs with the specifics and want to make sure I'm crediting the right place.

I frequently referenced the iPodLinux wiki's page on the iTunesDB file throughout my research, although they didn't seem to have any information on how the EQ preset was actually stored, other than that it was type 7 and likely a string.
I figured it out by simply looping through every track on my iPod (filtering by Select() to ensure the result of calling GetDataElement(7) was not null), and printed it, along with the track's name to the console. Initially, nothing was printed - none of my tracks had EQs, so I know that null=no preset.

I then used iTunes to add an EQ preset to a few tracks - you can do this by right clicking on a track, pressing "Song Info", going to the "Options" tab, and there finding the "Equaliser" dropdown. I set one track on my iPod to "Accoustic", the first preset on the list (besides "None").
When re-running my code, the name of that song was printed to the console, alongside the string #!#100#!#. I thought that was interesting, so I added the remaining equaliser presets to other tracks and found that each time, it was encoded as a string that started and ended with "#!#", with a three digit number in the middle.
This three digit number always corresponded to the index of the preset in the dropdown menu on iTunes, plus 100. As I believe I mentioned, I only have a 1st gen Mini, so it's entirely possible that other models encode this differently, but I am encouraged by the fact that iPodLinux states that every MHOD type under 15 is a string.

I've also verified that adding an EQ preset to a track via Clickwheel also shows up in iTunes.

2. I believe iPod supports custom EQ, and I'm wondering if it's also stored in mhod 7. I can test this in a few days, but wanted to mention it to you in case you wanted to also give it a try. You can setup a custom EQ curve in iTunes and then assign it to a song on iPod. If it's stored in the same place we'll likely need to update the parser to accommodate that, and introduce an API to define a custom EQ curve. It's possible that custom EQ was added in a later DB version and doesn't use mhod 7, so this might take a little digging to figure out. I think the first question to answer is "Does mhod type 7 change when a custom EQ is set?"

Ah, sorry to disappoint, but according to (again, you guessed it) the iPodLinux wiki ^{source 1 source 2}, whilst iTunes does in fact save custom EQs to the iPod, no iPod was known to actually make use of it, at least, not when that article was written. This is corroborated by multiple forums, such as Reddit and Apple Discussions. (Note: the link mentioned in that comment no longer works, however, a copy was saved on the Wayback Machine here).

3. Can you explain a little bit about why you chose to implement this as a ConvertibleUnicodeMHOD as opposed to just a plain EQPresetMHOD? If there isn't another mhod that would obviously use the ConvertibleUnicodeMHOD logic I think I'd prefer keeping things simple and just putting the existing logic into a EQPresetMHOD. This is especially true if custom EQ curves use mhod 7 with a different structure than a string.

Well I did start by writing an EQPresetMHOD, but I realised that what I was really after was a way to read a string MHOD and get back an object, which I thought may end up being useful if other MHOD types end up being saved as encoded strings. Looking again at that article on the iPodLinux wiki though, I don't see any MHOD types under 15 that stand out as being likely to require it.

If you'd prefer, I could remove ConvertibleUnicodeMHOD and Helpers.IStringConvertible, keeping the static Encode and Decode methods in EQPreset, and just call those from Track.EQPreset's getter and setter (using GetDataElement(MHODElementType.EQPreset) and SetDataElement(MHODElementType.EQPreset, value))?

[ThatBlockyPenguin]

Something more like this perhaps?

[dstaley]

Okay finally had some time to sit down and do some digging! Turns out mhod 7 will also store the custom preset name.

So, a built-in preset will have a value of the format: #!#100#!# (Acoustic)

And a custom preset is just the string name of the preset: No Bass At All

I think given this we should probably update the API to simply return a string value. For the built in formats we can create a string enum to make it easier for the built-in presets:

public enum StandardEQPreset : string
{
  Acoustic = "Acoustic",
  BassBooster = "Bass Booster",
// ...

And then update the EQPreset field to be a string.

songWithBuiltInPreset.EQPreset; // "Acoustic", StandardEQPreset.Acoustic
songWithCustomPreset.EQPreset; // "No Bass At All"

The parser can use the presence of the magic string to determine if it should be parsed as a built-in preset integer value or whether it should just be treated as the string.

[dstaley]

Actually, thinking on it a little more I wonder if we just skip parsing all together and just treat it as a string mhod, but make the enum with the magic values as an easy way to set it?

[ThatBlockyPenguin]

Turns out mhod 7 will also store the custom preset name.

Oh, that's interesting - I hadn't played around at all with custom presets, as I'd read that no iPod was known to make use of them. Do any of your iPods seem to be able to use your custom EQs? It would be very interesting if we find that one model can.

After tinkering around a bit, I can confirm that my Mini 1G also stores custom EQ names, but definitely doesn't use them. It also doesn't create the iTunesEQPresets file.

public enum StandardEQPreset : string

It seems that enums must be numerical:

Actually, thinking on it a little more I wonder if we just skip parsing all together and just treat it as a string mhod, but make the enum with the magic values as an easy way to set it?

Personally, I would prefer if Track.EQPreset returned an object with information, such as the name of the EQ, whether or not it is custom, etc. That way, if you're designing some UI around it, for example, you can use myTrack.EQPreset.Name. This would also make it easier to tell the difference between built-in presets and custom presets, by way of myTrack.EQPreset.IsCustom.

Your call though of course.

[dstaley]

Do any of your iPods seem to be able to use your custom EQs?

Nope! I'm somewhat confident that this isn't something that has ever worked given the amount of documentation saying as such. iTunes also didn't sync an iTunesEQPresets file to my fifth generation iPod nano, so I think we can safely assume custom EQ presets don't actually work on iPod.

it seems that enums must be numerical:

ahhh bummer :( I read a blog post that had a code sample showing it and I assumed it was available. Turns out the blog post was explaining that it would be nice if C# supported it, not that it was supported 😅

Personally, I would prefer if Track.EQPreset returned an object with information, such as the name of the EQ, whether or not it is custom, etc.

I think I'm mostly on board with this! Any thoughts on what the API would look like to assign a custom EQ name to a track in this case?

// reading
track.EQPreset.Name; // "Acoustic"
track.EQPreset.ID; // 100
track.EQPreset.IsCustom; // false

track.EQPreset.Name: // "No Bass At All"
track.EQPreset.IsCustom; // true

// writing
var track = new NewTrack
{
  EQPreset = /* set custom or built-in preset */

I think EQPreset = new EQPreset("No Bass At All") could work? So essentially if an EQPreset is instantiated with just a string name we'd assume it's a custom EQPreset.

[ThatBlockyPenguin]

ahhh bummer :( I read a blog post that had a code sample showing it and I assumed it was available. Turns out the blog post was explaining that it would be nice if C# supported it, not that it was supported 😅

Hah, I think we read the same blog post xD

I think I'm mostly on board with this! Any thoughts on what the API would look like to assign a custom EQ name to a track in this case?

Your example looks good to me.

I think EQPreset = new EQPreset("No Bass At All") could work? So essentially if an EQPreset is instantiated with just a string name we'd assume it's a custom EQPreset.

I do wonder if a factory method might be clearer here though - for example EQPreset.Custom(string name), then if we implement a private constructor that takes string name, string encodedValue, bool isCustom, the built-in presets could pass something like new("Accoustic", "#!#100#!#", false), and the factory method would pass new(name, name, true)?

So that would mean setting an EQPreset would look like:

var track = new NewTrack
{
  EQPreset = EQPreset.Accoustic
  [...]
}

Or

var track = new NewTrack
{
  EQPreset = EQPreset.Custom("No Bass At All")
  [...]
}

[ThatBlockyPenguin]

Also, what are your thoughts on using [CallerMemberName] to get the name of the built-in preset being instantiated (which is what my current code does) - I have just realised I could use nameof() instead?

[dstaley]

I think that API looks good:

// assign built-in
var track = new NewTrack
{
  EQPreset = EQPreset.Accoustic
  /* ... */
}

// assign custom
var track = new NewTrack
{
  EQPreset = EQPreset.Custom("No Bass")
  /* ... */
}

// read
var name = track.EQPreset.Name;
var isCustom = track.EQPreset.IsCustom;

As far as [CallerMemberName] I think that's just to know the name of the method that calls the method the attribute is annotated for? Happy to learn more from you but I don't think we need either CallerMemberName or nameof based on this API so far.

[ThatBlockyPenguin]

I think that API looks good:

Great! I'll get to work on that.

As far as [CallerMemberName] I think that's just to know the name of the method that calls the method the attribute is annotated for?

When used as in EQPreset right now:

public static readonly EQPreset Acoustic = new(100);
/* ... */
private EQPreset(int id, [CallerMemberName] string name = "") : this(name, id) {}

name will be set to "Acoustic" - the name of the field.

I don't think we need either CallerMemberName or nameof based on this API so far.

You make a good point. I was trying to avoid typing the same text twice, once for the field name and once for the preset's name string, but I realise that this isn't always what we want, for example, when the field name is SmallSpeakers, the actual name of the preset should be what is displayed in iTunes: "Small Speakers" (with a space).