search

ebu / ear-production-suite

The EAR Production Suite is a set of VST® plugins and tools for producing immersive and personalizable audio content suitable for any Next Generation Audio codec. It is based on the Audio Definition Model (ITU-R BS.2076) and the ITU ADM Renderer (ITU-R BS.2127) and enables monitoring on any ITU-R BS.2051 loudspeaker configuration.
https://ear-production-suite.ebu.io/
GNU General Public License v3.0
101 stars 19 forks source link

BEAR TF selection review #270

Closed rsjbailey closed 4 months ago

rsjbailey commented 5 months ago

Sorry, popping this in as a PR to your PR branch to hopefully make it easier to diff. Review comments below. TL/DR is that I wanted to check you'd thought about which threads access the DataFileManager (I can see the processor restart is synchronised but wasn't sure about access to the filemanager). I'm also not sure if you've checked the linux build (I think it'd work in the same way as the windows one from a path perspective, but I'm not sure)

Other than that, the I've suggested a couple of changes to how things are stored in the DataFileManager. I'm only presenting that as a PR as I had to do make the changes to properly reason about it, so figured I might as well push. I'll leave it to you whether to accept it or not, it's about the same amount of code just a slightly different approach. Reasoning follows.

Looking at it again I think my version doesn't now handle the case where a file with the same name changes during a session, but I'm not sure if that is likely to happen in real life. (requiring a restart to pick up changed files as opposed to new files doesn't seem massively unreasonable) - I think it'd only work in the current implementation if the metadata changed anyway.

Synchronisation

There doesn't seem to be any synchronisation of access to the DataFileManager, and it's a little tricky to figure out if that's an issue.

I think we’re OK, but I wanted to check it had been thought about - With the exception of prepareToPlay, it looks as though it’s only ever accessed via the main (gui) thread. I don't think you'd ever get parameter updates triggering the config save from the audio thread as the parameters that trigger it are not automatable, so shouldn't come in via the audio callback. It may still happen if you're using the built-in host UI rather than our GUI to make the changes. I'm not sure in that case if it's possible to have anything else happening concurrently from the main thread (as the GUI wouldn't be open by definition), but I got a bit lost following callbacks. Might be worth doing a build with the thread sanitizer and prodding everything.

I’m also not actually sure whether prepareToPlay is called from the main thread or not, it might be, I’d probably have to go diving through the VST3 docs to find out, or check with a debugger.

DataFileManager

Going through the DataFileManager, I had a couple of thoughts:

shared_ptr

Using a shared_ptr for the DataFile seems unnecessary:

If the only reason is for it to be nullable, using an optional<DataFile> in those cases makes more sense, as value semantics are generally easier to reason about.

Manual sort/find on DataFiles

As the DataFiles held in a vector, there’s a fair bit of complexity around presenting them as a sorted list. The files are sorted first by origin and then by filename. Looking them up by key (file path), also needs a manual find_if.

A map makes sense here as the lookup becomes part of the container API, and the files are kept sorted by default. It’s slightly complicated by the fact that the sort criteria (sorted by file’s origin and then filename) and the key sort criteria (just a comparison on file path) are not the same. So you can’t just throw everything in a sorted container.

If you keep release/custom files in two containers, it does work as you can just concatenate them at the one point you need them together (generating the list for the UI). It also makes some other stuff simpler as you can do the default check by just checking the size of the release container, for instance. It adds a minor complication of having to do lookup in two places, but that can be factored out so it only happens in one function.

flat_map is a container adapter from boost that takes any container with sequencial storage (vector, array), stores sorted key/value pairs in it, gives them a map-like interface. It is appropriate here as we’re hardly ever inserting new elements & only have a small number of files. We already have boost as a dependency and it’s becoming part of the standard library for C++23, so seems fine to drop in, although std::map would also work (unordered_map wouldn’t as it’s not sorted)

You could probably do it with a set instead if you just treat anything with the same filename as equal & wind up with slightly nicer code, but it feels a bit less obvious what’s going on then.

firthm01 commented 5 months ago

The possible synchronisation/threading issue needs checking. Might need to wrap any changes in a mutex Safe in current implementation

firthm01 commented 5 months ago

I think overall this is much cleaner, but it breaks some previous functionality. The intention was that if any of the installed files were to change (overwritten, deleted, or new ones installed), the user would be able to just reopen the plugin UI to see those changes reflected. I think if that could be addressed, this would be the one to go with.

UPDATE: Tested overwriting files, deleting files and installing new files. Installing new files works when the UI is refreshed. Deleted files are not removed, and overwritten files are not updated (labels/desc remain as before). Resolved by 0639ee1