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

Refactor - StoneyDSP Library #69

Open
nathanjhood opened this issue Feb 23, 2024 · 4 comments · Fixed by #70, #72, #74, #77 or #76
Open

Refactor - StoneyDSP Library #69

nathanjhood opened this issue Feb 23, 2024 · 4 comments · Fixed by #70, #72, #74, #77 or #76
Assignees
Labels
enhancement New feature or request

Comments

@nathanjhood
Copy link
Owner

The existing implementation is still a little messy; previous attempts at improving things within the codebase have lead to some complexity that is probably not necessary, given that the project is mostly just academic in nature.

Moreover, I've gotten to grips with making API's and modules with C++ and CMake in graceful ways that should now allow me to build my own library/extension for JUCE.

My intention therefore is to begin parting out the relevant source code for modularisation. This is already somewhat considered in some of the design aspects of the C++ used code, but there is currently no larger library or framework supplying these files to (potential) consumers.

The C++ module code shall be supported by a possibly optional CMake API, designed to provide consumers with easy CMake targets to link with in their projects, allowing them to easily pull in the C++ modules from various sources such as vcpkg, or even have system installations of the library, with which to build.

Furthermore, the proposed work would also allow some scope for the creation, development, and testing of a custom vcpkg registry from which the modules could be supplied.

Ideally, this external project should be able to do a CMake find_package(StoneyDSP 1.0.0 CONFIG REQUIRED) and then a target_link_libraries(Biquads PRIVATE stoneydsp::stoneydsp_audio stoneydsp::stoneydsp_graphics), and then only require the standard ProcessBlock() and MainComponent from the JUCE audio plugin template.

I have begun work on the library itself, and will be using this project as a test/example of the library.

@nathanjhood nathanjhood self-assigned this Feb 23, 2024
@nathanjhood
Copy link
Owner Author

On reflection (been a while), I know that the existing OS implementation is quite hacky; It is not a requirement for the base DSP to work, but rather an extension that further demonstrates some points made in the analysis/write up.

Since both open issues are related to the OS implementation, and it is this implementation which is holding back progress somewhat, I've moved that implementation over to a backup branch for visitors to continue referencing, and have updated the main branch to reflect the other latest changes - because I wanted to get multi-platform CI/CD off the ground, above and beyond just oversampling on a Windows-only plugin.

I will "circle back" around to the oversampling at a slightly later date - readers, if curious about this, please check the backup branch for the meantime.

@nathanjhood nathanjhood added the enhancement New feature or request label Mar 14, 2024
@nathanjhood nathanjhood pinned this issue Mar 14, 2024
@nathanjhood
Copy link
Owner Author

I have merged a v1.2.0 which rolls back the GUI and the oversampling, in favour of multi-platform builds validated with Tracktion Pluginval via CTest, over GitHub Actions.

Now, this project can be deployed for all platorms.

However, in the context of this ticket, I consider only half of the work to be complete: I moved some of the codebase into the StoneyDSP library modules, which are currently still local to the project - these should obviously be centralized, perhaps delivered over a CDN or package manager, sourced from the StoneyDSP library repo - and much of the implementation is still sitting in the Biquads unit files.

Ideally, those unit files (i.ie., everything under include and src), will end up being very very generic across all of my plugins. I forsee the possibility that only the encapsulating namespace, and root unit files (Biquads.hpp/cpp) need change from project to project.

It may be possible to do some kind of aliasing, like using or some sort of typedef or preprocessor macro, to override what AudioPluginAudioProcessorEditor evaluates to. That might allow the root unit files to contain all (or most) of the unique information, while the Processor, Wrapper, Parameters, and Editor be largely generic from project to project.

In regards to this Biquads implementation, I have moved the atomic coefficient object and a general Biquads class (not used - just for reference for the moment) into the stoneydsp_audio module, and called on a few constants and other helpers from stoneydsp_core.

In continuation, I would ideally like to have the transforms be placed into the stoneydsp_audio module also, and the coefficient calculations either go along with it, or go under stoneydsp_core/maths.

The various distinct pieces would be filed under stoneydsp_audio/filter/2_pole in individual files and class declarations, and then called and used in stoneydsp_2PoleFilterBLT, or something like that.

The ProjectInformation namespace/object at the root - which is of course different from project to project - can sit next to our sneaky typedef/macro/using/alias, which would do something like:

namespace StoneyDSP {
namespace Biquads {

using AudioPluginAudioProcessor = Biquads;

}
}

...before it includes the other unit files.

Then, the Wrapper object's member - the DSP to be used - wouldn't need to be changed from project to project. Instead, we just point our alias at some other pre-constructed class from the library.

I am not sure how well this might work, but I will investigate it while continuing with this task.

Still to be done, then:

  • Move transformations and calculations into modules declarations folder
  • Rebuild Biquads library module using the newly declared transformations and calculations
  • Refactor the existing plugin (particularly Wrapper) to call the new Biquads library module, which in turn uses the library coeffs and transformations modules!

With the above complete, and the "genericizing" of the project files a secondary goal, I will re-evaluate the status of this ticket.

@nathanjhood
Copy link
Owner Author

EDIT: I guess I'm sort-of thinking about trying a PIMPL-like way of connecting a predefined set of interfaces using a compile-time variable...

@nathanjhood
Copy link
Owner Author

StoneyDSP::Audio::Biquads has been successfully extrapolated into a class, defined within the stoneydsp_audio module.

This concludes on half of the refactoring task at hand - the moving of our class from outside of the project files, and into an external library/module.

The other half which still persists, is to now move stoneydsp_audio - and stoneydsp_core - back over into StoneyDSP, and have this Biquads project fetch our library from something like vcpkg...

stoneydsp_audio as it currently stands is actually a whole unique set of files in the Biquads project root folder under modules. This is not how we want it; as I make further improvements to the DSP implementation, I'd like those changes/improvements to populate downstream to all consumers of my Biquads class. Following the single source of thruth principle, this leads to some more conclusions on how the DSP library shall be managed, maintained, versioned, and distributed - which perfectly demonstrates why I picked a refactor of this project as a means of beginning the DSP library:

  • This Biquads project currently just calls a Biquads class instance, and expects to have certain methods in place which are being called in the Biquads project files - namely, things like process(juce::AudioBuffer, juce::MidiMessages), setFrequency(SampleType), and setTransformationType(StoneyDSP::Audio::Biquads::BiLinearTransformationType).
  • So long as our class method's call signatures never change, then this Biquads project should continue functioning no matter what changes I make to the Biquads class implementation over in StoneyDSP.
  • This means we've established a nice clear interface model for the Biquads DSP processor class, derived from actual usage requirements, and now we simply endevour to maintain support for the usage-case as demonstrated here, in the Biquads project.
  • This guidance will most likely apply to all future work on the other projects which are scheduled to be refactored to use the StoneyDSP in the same way as this. New scenarios will inevitably arise, but nonetheless we now have a strong template for the work ahead.
  • A StoneyDSP git submodule is not really an ideal choice IMO, since StoneyDSP also contains several submodules of it's own - including JUCE, and CMake! - which means that we will have to go to greater lengths to avoid multiple inclusions of JUCE and guard aganst various edge cases...
  • a vcpkg package would be a much better choice for distro/acquiring of StoneyDSP (though users may still opt for a submodule, as long as they are aware of our recommendation of acquiring via vcpkg instead). I may say this with some confidence, since my entire CI/CD for both Biquads and for StoneyDSP is all driven by acquiring JUCE itself via vcpkg. Those have all been passing validation successfully for some time already, and there are lots of useful integrations available for vcpkg already such as git workflow actions, for caching vcpkg packages...
  • Another point in favour of vcpkg over git submodules is version requirement specifications. For example, as of writing, when consuming the JUCE project as a git submodule, we pull in version 7.0.10, but when acquiring via vcpkg, we can pull in a whole range of earlier versions customarily, but the latest available JUCE release via vcpkg is 7.0.9. This might seem inconsequential on paper, but it actually has big ramifications on all of the CI/CD and dependability for future users trying to build this project. Using git submodules for JUCE and StoneyDSP acquisition, in one years' time, I can git clone this project and have no idea what versions I've pulled in for those two packages, no idea if any breaking changes were shipped in the intervening time, and no idea at all whether I will still be able to build this project the way I left it.
  • By specifying dependency version requirements on vcpkg, I am safeguarding the future of this project (I believe).

I've been making improvements to the audio processor structure while de-coupling the DSP from it. I have realized some redundant areas in the previous design and removed/refactored quite a lot of that away already, but I'm not totally happy with a few of the design decisions with regards to ownership, and initialization.

Which class owns which, and then which class initializes which? I've de-tangled several looping idioms in the constructors that started to look very dodgy when looking at them a bit closer. However, I've done only about half of the work involved in what I've discovered along those lines. (currently I'm quite unhappy about all the un-managed pointers to the main Audio Processor class being passed in to initialize-construct it's own members... in some cases, quite redundandtly...)

I am thinking that the APVTS and the entire parameter layout should all be defined and looked after in our Parameters unit, which may pass a couple of methods to the Audio Processor which instantiates it, allowing the Audio Processor to access the APVTS (and vice versa), while providing a level of indirection between our parameter pointers and the audio processor class.

The parameters class/object can then also be made accessible to the audio processor wrapper, where the parameter values can be exchanged atomically with the audio-thread class instances. It will be necessary to consider carefully how this is done, as we can potentially reduce complexity quite a bit by simply considering the chain of dependencies between each of these class objects, and which shall need to be defined first.

It should not be at all necessary to pass multiple un-managed *this calls from the audio processor to multiple underlying references of itself - and via a forward declaration, even!

Regarding the Biquads Audio Processor:

There are sketchy aspects of this design which I think are valid concerns, not least because this was my first ever serious coding project and I'd held on to lots of things I'd learned at the time, but probably misunderstood, or over-used, or can now think of solutions which are more aligned to the way I have done things elsewhere within the codebase and/or want to pursue as I move forward.

Regarding the Biquads DSP:

It is very hard for me to validate the Biquads class as a whole, from top to bottom, even using unit tests and a debugger. The data structure is much too large and it's members too inter-related. I aim to continue refactoring the DSP such that the coefficient calculations will become a set of namespaced math functions, templated, and that the blinear transformations will be something like a DSP class, defined seperately. Ideally, both shall hold and manage array-based containers of coefficients (instead of unique declarations per coefficient), and the Biquads DSP class will define a method which passes the calculated coefficient array to the bilinear transform array, possibly atomically.

Looking at both concerns above, what actually happens is, the Biquads Audio Processor is defined here in the Biquads project, and so those concerns will remain associated with this project; if I am not finished with that part of the work by the time I have vcpkg (or something else) serving and fetching our modules from the library, then I will make future epics here in this repo for fulfilling those tasks.

The concerns relating to the DSP library shall not really be applicable to the management this repository - even if this is where we discover them, and figure out how to fix them. Instead, since the DSP library shall be fetched remotely from the StoneyDSP project, those concerns shall become epics and issues over on the StoneyDSP repository, instead.

I am also looking at the potential idea of having each StoneyDSP module exist as a submodule, such that the StoneyDSP org holds the StoneyDSP repo, plus a repo each for stoneydsp_core and stoneydsp_audio, making it slightly easier to version-control each one, to manage these epics and issues in a more specific fashion, and also perhaps make it slightly easier for non-vcpkg acquisitors to pick and choose which modules they want from the library, without being lumped with the whole thing.

In either case, I find the most interesting element in the work to be the nature of the provider/consumer relationship in this coding project. As I de-couple the DSP from the plugin files, I will need to be mindful not to break the interface that I have defined for the Biquads class, including as I dive deeper into de-coupling parts of the DSP from itself internally. This Biquads project, being the consumer, must not be broken by upstream changes in the StoneyDSP library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment