indiependente / autoEqMac

EqMac preset generator powered by AutoEq.
MIT License
23 stars 1 forks source link

Parse fixed band EQ from its own file instead of headphone README #28

Closed douglascamata closed 2 years ago

douglascamata commented 2 years ago

I saw that you are parsing each headphone's README to look for preamp. I do not recommend that you do this. The README isn't structured information, it is free text that can change at any time, so the implementation becomes fragile.

My recommendation is that you grab this information from FixedBandEQ text file, like this one: https://github.com/jaakkopasanen/AutoEq/blob/647d0422ac346a5616eaf6ddda0420317121941b/results/oratory1990/harman_over-ear_2018/HIFIMAN%20Sundara%20(Dekoni%20heepskin%20Earpads)/HIFIMAN%20Sundara%20(Dekoni%20heepskin%20Earpads)%20FixedBandEQ.txt. You can be sure that the structure of this file won't be changing anytime soon. It should be stable and possibly help you simplify the parsing code.

When I wrote #24 I thought you were already using the FixedBandEQ file that I linked. I read your source code again and realized you weren't.

indiependente commented 2 years ago

Hello @douglascamata,

Thanks for reporting this issue. At the time of writing the script I wasn't aware of the text files hence why I went with parsing the MD files.

Of course having a more structured and reliable source is much more convenient.

This would require a major refactoring of the data fetching mechanism, so bear with us while we work on it 🙂

douglascamata commented 2 years ago

This would require a major refactoring of the data fetching mechanism, so bear with us while we work on it 🙂

Thanks and no worries at all about timing. If you want to split the work into smaller issues I can even help you out with the implementation. Golang is my daily job since years and I love contributing to open source. 👍

indiependente commented 2 years ago

Hi @douglascamata ,

I just wanted to give you more context around the issue you raised.

Context

I had another look and I can confirm the EQ preset data is fetched from the txt files. Here is how it works:

graph LR
    A[Start] -->|Fetch Index MD metadata| B(Store EQMetas)
    B --> C(Select preset)
    C -->|Get EQMeta| D(Download preset)
    D -->|Fetch TXT preset| E(Download preamp)
    E -->|Fetch MD preamp| F(Store as JSON)
    F --> G[End]

The reason why at startup the script fetches data from the index MarkDown file is simply because there's no other place I'm aware of that keeps track of all the available presets in autoEQ. So that MarkDown acts as a repository of presets metadata.

Hence why the EQMetadata struct holds the data found in the MD file for each available preset, including the link to the TXT file.

Once the user selects which preset to download, the tool will get the metadata previously stored in a map and use the saved link to download the preset raw data from the text file and finally save it locally following the JSON format.

You can find the above process in the server_test.go file.

However, at the time this tool was created, the text file only held information regarding each band in the EQ, but no info related to the global preamp gain in dB, hence the need to fetch that single value from somewhere else, and that's how it ended up parsing the MD file.

Proposed solution

So considering now that information is available in the fixed band txt file, it will be wise to capture it at that point, avoiding the subsequent HTTP call for the MarkDown file. I wouldn't delete the MD global preamp parsing code as it could be used as a fallback mechanism for when the value isn't available in the fixed band txt file (unless we can be 100% sure it always is).

This will be a simple fix that won't require much work 😃

douglascamata commented 2 years ago

Thanks for the great explanation! The proposed solution sounds good to me.

I think it's safe to assume the preamp values will now be there 100% of the time for two reasons:

  1. They were added since Nov 2020 (I verified it, here's the commit: https://github.com/jaakkopasanen/AutoEq/commit/a917820a4cb39ee205a304e365b646a16a0118d6#diff-b6369edbed76da6044705c90ef6052f5c0734f5f5a2364d359f9326c83dba938). Indeed before this tool was created. Before that commit, only the Parametric EQs had preamps in the file.
  2. They are mandatory for a correct equalization to avoid clipping.

Buuut, it doesn't hurt to have a backup solution. 👍