Skip to content
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

OOP implementation #138

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from
Open

OOP implementation #138

wants to merge 18 commits into from

Conversation

jmarcosfer
Copy link
Collaborator

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
Copy link
Collaborator Author

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.

@jmarcosfer jmarcosfer added this to In progress in 1.0 Roadmap via automation Mar 7, 2024
@jmarcosfer jmarcosfer added enhancement New feature or request api labels Mar 7, 2024
@jmarcosfer jmarcosfer moved this from In progress to On Review in 1.0 Roadmap May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api enhancement New feature or request
Projects
1.0 Roadmap
On Review
Development

Successfully merging this pull request may close these issues.

None yet

1 participant