search

MTG / essentia.js

JavaScript library for music/audio analysis and processing powered by Essentia WebAssembly
https://essentia.upf.edu/essentiajs
GNU Affero General Public License v3.0
627 stars 41 forks source link

OOP implementation #138

Open jmarcosfer opened 4 months ago

jmarcosfer commented 4 months ago

Addresses #64

Main goal

Modified code_generator.py, as well as corresponding parts of .cog files, to produce a OOP implementation of the library, where each algorithm is a class with a configure, compute, and delete method (this last one is provided by Embind and needed to destroy the underlying C++ object from the JS side).

Open design questions

  1. Parameter-less algorithms like this (there are currently 38 algorithms without configuration parameters), perhaps should be configure-less? I guess internally it resets the algorithm state, but it's strange from a JS user perspective: maybe just exposing a reset method instead?
  2. If we care about making the library as similar as possible to the upstream Python bindings (and I would argue maybe we should care more about having an idiomatic JS interface), we could make algorithm classes "callable" (algorithmInstance() instead of algorithmInstance.compute()) by implementing a base EssentiaAlgorithm TS class which extends the JS Function class, as explained here.

Issues

Separate create and configure methods: failed for 3 algorithms

In the OOP implementation, algorithm create and configure are separate

FrequencyBands::FrequencyBands(const std::vector<float>& frequencyBands, const float sampleRate) {
    _frequencybands = AlgorithmFactory::create("FrequencyBands", "frequencyBands", frequencyBands, "sampleRate", sampleRate);
}
void FrequencyBands::configure(const std::vector<float>& frequencyBands, const float sampleRate) {
    _frequencybands->configure("frequencyBands", frequencyBands, "sampleRate", sampleRate);
}

However this approach failed to compile for algorithms ["MultiPitchMelodia", "PitchMelodia", "PredominantPitch"] with the following error, as if too many parameters were passed to the configure call. Perhaps related to this upstream issue, maybe @dbogdanov will know what's happening.

src/cpp/includes/essentiajs.cpp:3898:28: error: no matching member function for call to 'configure'
        _predominantpitchmelodia->configure("binResolution", binResolution, "filterIterations", filterIterations, "frameSize", frameSize, "guessUnvoiced", guessUnvoiced, "harmonicWeight", harmonicWeight, "hopSize", hopSize, "magnitudeCompression", magnitudeCompression, "magnitudeThreshold", magnitudeThreshold, "maxFrequency", maxFrequency, "minDuration", minDuration, "minFrequency", minFrequency, "numberHarmonics", numberHarmonics, "peakDistributionThreshold", peakDistributionThreshold, "peakFrameThreshold", peakFrameThreshold, "pitchContinuity", pitchContinuity, "referenceFrequency", referenceFrequency, "sampleRate", sampleRate, "timeContinuity", timeContinuity, "voiceVibrato", voiceVibrato, "voicingTolerance", voicingTolerance);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~
/emsdk/upstream/emscripten/system/local/include/essentia/configurable.h:158:3: note: candidate function not viable: requires 32 arguments, but 40 were provided
  CONFIGURE P(2) P(3) P(4) P(5) P(6) P(7) P(8) P(9) P(10) P(11) P(12) P(13) P(14) P(15) P(16)

Modular structure and imports

As a side-effect, we're forced to re-structure the library's main modules (both WASM and TS interface), since now algorithms are no longer methods of a parent EssentiaJS class, but each is a class at root module-level.

Limitations:

On the TypeScript API, we're forced to find some other way to get the EssentiaWASM instance (which used to be passed to the TS constructor: new Essentia(EssentiaWASM). For now I've opted for a top-level util function called ready which expects to be passed the EssentiaWASM instance and is also in charge of calling essentia::init().

Opportunities:

We could re-think how the library gets imported, which has usually been confusing and tedious to get right, depending on the platform you're in. Ideally importing the library would:

  1. be as similar as possible between JS platforms
  2. hide from the user the retrieval and loading of the EssentiaWASM backend instance

This could arguably be on it's own PR, but I think it makes sense to address it here since we're already forced to re-structure the module altogether.

Tests

Took the time to write a first test (manual for now) for this new interface, using the FrequencyBands algorithm as test case. This can be used in the future to design test structures and edge-cases for #66.

Nice-to-haves

Automatic memory management from the JS side

Something equivalent to TensorFlow.js' tidy function would be nice to keep track of Essentia algorithm instances in use inside of the function's scope and automatically delete them as soon as that tidy-like function returns. Not sure how hard this would be to implement. See here and their source code for more info on how TensorFlow.js manage memory.

jmarcosfer commented 4 months ago

CI workflow is currently failing on new above-mentioned tests, on this and this test cases related to #137 types issue. This failure is expected for now.