Use OSC Messaging to avoid calling QMessageBox from the core

[Wallacoloo]

This implements two new classes: core/Messenger.cpp and core/OscMsgListener.cpp with the goal of reducing the coupling between the core & gui.

The former class allows any thread to broadcast a message to a number of OscMsgListeners in a thread-safe manner, using Open Sound Control for the underlying format & @fundamental 's rtosc library. Upon broadcast, messages are added to a queue, where the listener thread later dequeues & processes them. It is not a lock-free implementation, but no user/callback code is ever run while the queue is locked, so deadlocks are impossible & it's relatively low-latency (better than having the core directly call into unknown gui functions, in any case). If anybody finds a good lock-free queue and set implementation, they could be dropped in and this is solved.

Currently, the only two forms of communication this new system is used for is broadcasting initialization messages (i.e. splash-screen status updates) & warning/error messages. It behaves nearly identically to current master, but the core no longer has a single usage of QMessageBox - the QMessageBoxes are instantiated in gui/GuiApplication.cpp instead. The differences are:

• Initialization messages arising inside Engine::init() aren't processed until after Engine::init() returns, which means there are no splash screen updates for the first few seconds. This is because Engine::init() actually still initializes portions of the GUI and therefore must be run in the gui thread, thereby blocking gui updates.
• Error messages when opening a project through the command line are not displayed to the terminal. Previously, they were selectively routed to a QMessageBox::Warning or stderr based on if the gui existed. They could be added back in by implementing OscMsgListener in core/main.cpp or another file that logs error messages to the console.

---

Feedback is appreciated. Although this looks like a large PR, that's mainly due to the addition of rtosc to the source tree. The end goal is in having the gui manipulate the core only through OSC commands & never by directly calling into it, but a lot needs to happen before that's feasible.

Note: this fixes the following issues:

• Zyn GUI crashes LMMS in Master #2269 (and its several duplicates)
• Command-line render segfaults if ~/.lmmsrc.xml cannot be written #2245

[tresf]

@Wallacoloo why are string endpoints are used instead of enum types? This is purely cosmetic, but I always find a switch/case preferable from a readability perspective over a if/elseif/else.

I assume this is intentionally done by design and I think I remember you asking @fundamental this same question on IRC, but wanted to check as seeing strcomp's in our code via https://github.com/LMMS/lmms/pull/2252/files#diff-1947d23e91609eea3558d73579d8a23eR362.

Another cosmetic item...

[...] InitMsg = "/status/initmsg"

I would recommend Info over InitMsg, personally (unless they're different?)

Last, do we have to worry about any namespace issues here? Shall we prefix the enpoint paths with lmms via /lmms/status/initmsg ?

Sorry if these questions all sounds rudimentary, just trying to offer some feedback. :)

-Tres

[Wallacoloo]

@tresf good questions. I suppose enum endpoints are feasible. The only
downside I see is that the messages are less human-readable, but OSC is a
binary protocol anyway & not designed to be human parsable. So enums might
do the job just fine. I'd like @fundamental's opinion on that.

An alternative solution is to use a map<string, osc handler> and process
the message by looking it up in the map and calling the appropriate handler.

Namespacing shouldn't be an issue, but if we decide to stick with strings
instead of enums, it wouldn't hurt.

While the questions are rudimentary, that's not a bad thing. OSC is new to
me as well, and I'm fairly certain I'm using rtosc in a different manner
than @fundamental intended, as I had trouble adapting his example code to
work with lmms.

[tresf]

... I'm sure you've seen this by now, but Travis windows build is upset per:

[ 29%] Building C object lib/rtosc/CMakeFiles/rtosc.dir/src/rtosc.c.obj
/home/travis/build/LMMS/lmms/lib/rtosc/src/rtosc.c: In function 'rtosc_message_ring_length':
/home/travis/build/LMMS/lmms/lib/rtosc/src/rtosc.c:496:5: error: 'for' loop initial declarations are only allowed in C99 or C11 mode
     for(int i=0; i<4; ++i)
     ^

and again on

/home/travis/build/LMMS/lmms/lib/rtosc/src/rtosc.c:574
/home/travis/build/LMMS/lmms/lib/rtosc/src/rtosc.c:612

[midi-pascal]

The source file has ".c" extension so it is compiled as a C source.
Declaring the loop iterator inside the for() is a C++ feature.
That's why it fail - I think -.
Just renaming it with the '.cpp' extension should do the job - I did not tried though -

[grejppi]

The source file has ".c" extension so it is compiled as a C source.
Declaring the loop iterator inside the for() is a C++ feature.
That's why it fail - I think -.

It has been a C feature since C99, like the error message says.

Anyway, I am getting this from make:

...
[ 21%] Built target lmmsobjs
Scanning dependencies of target lmms
[ 21%] Building CXX object src/CMakeFiles/lmms.dir/core/main.cpp.o
[ 21%] Building CXX object src/CMakeFiles/lmms.dir/lmms_automoc.cpp.o
make[2]: *** No rule to make target 'lib/rtosc/librtosc.a', needed by 'lmms'. Seis.
CMakeFiles/Makefile2:990: recipe for target 'src/CMakeFiles/lmms.dir/all' failed
make[1]: *** [src/CMakeFiles/lmms.dir/all] Error 2
Makefile:137: recipe for target 'all' failed
make: *** [all] Error 2

which, very strangely, didn't happen when I just decided to try Ninja instead of Make (cmake -G "Ninja").

Also, #1991 is going to introduce a dependency on rtosc too, as a submodule.

[Wallacoloo]

I'll look into those build errors when I get home from work today. I realized I didn't address @tresf's question about the InitMsg path. This is distinct from general "information" messages in that they are specifically to let the system know when different components have been initialized (currently just use for splash-screen messages).

Come to think of it, a better solution would be to replace those messages with a unique endpoint for each component of the system. i.e. broadcast an otherwise-empty message to /status/init/mixer to indicate that the mixer system has been initialized.

[fundamental]

per enums vs. strings, the idiomatic OSC that I've seen in other programs seems to be similar to REST within the webdev community. So that means you're generally expected to see /hierarchical/subsystem/field-or-action arguments-to-apply-to-field with no arguments typically being the "get this field" message, so string/symbol arguments are fairly typical and so is encoding the action into the path of the message. Since most of the messages have only one or two arguments at most it's pretty easy to read OSC in its serialized form. Since that's the case I typically try to avoid obfuscating things with enums as it's a bit faster to debug stuff when dumping memory, but that's somewhat of a personal preference.

Per getting rid of the if tables, the best option is using something like the rtosc::Ports class, though it currently does have some restrictions on the patterns of the paths. If you're interested in using it to make a dispatch table in the future I'd try to organize some thoughts on how you'd want it to look and open a feature request on rtosc's tracker.

@Wallacoloo, if you want a lock free queue, there's already a ringbuffer implementation sitting in the rtosc code. You're going to need to know that there's only one reader/writer pair per ringbuffer which might be a problem with the current state of lmms, but once that's figured out an implementation is in place.

[Wallacoloo]

Ok, looks like it's passing the Travis build now! I'm going to refactor the init message stuff (use the endpoint to indicate which component has initialized rather than doing that with a translated user message)

@fundamental you may want to consider looking at, and maybe cherry-picking commits 7e8497b (and d991bba) - the rtosc CMakeLists file doesn't seem to handle mingw all that well since it's detected as both WIN32 and CMAKE_COMPILER_IS_GNUCC.

[Wallacoloo]

@tresf for gcc 4.x and 5.x, yes. For other compilers, I don't know. clang probably implements -std=c++0x to maintain gcc-compatibility, but I don't know about MSVC or ICC.

[tresf]

@Wallacoloo thanks. Yeah, AFAIK, I've been taking the c++0x recommendations. I suppose MSVC is potentially important if this makes it upstream, but from an LMMS perspective, I think c++0x would cover all of our bases.

[Wallacoloo]

Ok, I've fixed this all up.

[Wallacoloo]

@fundamental it's a runtime issue on Windows. The windows runtime apparently uses %I instead of supporting %z because, you know, Microsoft... http://stackoverflow.com/questions/174612/cross-platform-format-string-for-variables-of-type-size-t

Anyways, you were already casting the other sizeof(RESULT) argument to an int instead of printing it as a size_t, so it doesn't seem like a big deal.

[fundamental]

@Wallacoloo sigh, classic MS... the workaround is now incorporated into rtosc's HEAD

[Wallacoloo]

[Edit: Wrong thread]

[fundamental]

@Wallacoloo I think you just attached those commits to the wrong pull request. They look like they belong to #2289

[Wallacoloo]

@fundamental Nah, that couldn't have been me... (Fixed)

Thank you very much for pointing that out. 😅

[unfa]

Hmm. I can't checkout this branch:

  git checkout Wallacoloo/osc-qmessagebox
  error: pathspec 'Wallacoloo/osc-qmessagebox' did not match any file(s) known to git.

[fundamental]

@unfa you'll want to add the remote repository first:

IIRC the commands should be

git remote add Wallacoloo https://github.com/Wallacoloo/lmms
git fetch

[unfa]

Ok, now I see that you missed this in your Wiki tutorial:

 git fetch

However I did that and still no success. I keep getting the same error.

[Wallacoloo]

Oops! Sorry @unfa - I added that step to the wiki.

Maybe try git checkout remotes/Wallacoloo/osc-qmessagebox? If that doesn't work, post the output of git branch -a and git remote -v and then maybe I can figure out which other steps I forgot to put in the wiki :/

[Wallacoloo]

Turns out you need to do git fetch Wallacoloo instead of git fetch.

[unfa]

Thanks, that seems to have worked. I'm building.

EDIT: I still have Zyn crashing LMMS at "Show GUI" action. I wonder if I'm really using the branch.

[michaelgregorius]

If we decide to maintain our own version of rtosc I assume that we should change the drop site? Or is @fundamental ok with this? 😉

[curlymorphic]

@Wallacoloo
Nice work Colin. The first step is often the hardest, and this is definitely a big step in the right direction. You have well documented the use of this system in your source code. I cant wait to try it out.

To clarify all future error/warnings should be made using
Engine::messenger()->broadcastError and Engine::messenger()->broadcastWarning functions?

[michaelgregorius]

Right. We could try to get @tobydox to maintain builds of rtosc if we wanted to. However, rtosc is still young enough to where it undergoes regular API changes, which means that might not give us enough control over ensuring the right version of the library. Alternatively, we could include rtosc in the source tree as a git submodule. Downside is that developers may initially be confused, as you have to clone the repository with --recursive (or some flag like that) for it to work, and we also can't maintain our own patches against rtosc quite as easily (still possible - we would just have to apply them dynamically during the build process).

There seem to be several implementations of the OSC protocol. If rtosc is not that stable yet we could also consider to use one of the alternatives. For example liblo is used by several other well known sound applications (Ardour, QTractor, Rosegarden, ZynAddSubFX) and will therefore likely be quite stable because all these applications will have a large interest in its stability.

With a stable implementation it might also be possible to just add it as a normal dependency instead of pulling it into LMMS' source tree with all the technical inconveniences that this would bring.

[michaelgregorius]

GitHub is showing a warning that there is no newline at the end of the file and GCC will complain as well. 😉 The same applies for OscMsgListener.h.

[Wallacoloo]

Yeah, yeah, yeah.... Good catch :P (fixed)

[Wallacoloo]

@michaelgregorius

There seem to be several implementations of the OSC protocol. [...] With a stable implementation it might also be possible to just add it as a normal dependency instead of pulling it into LMMS' source tree with all the technical inconveniences that this would bring.

The current version of #2311, which further builds off of the commits in this PR, is such that rtosc was replaced with liblo. It does make the dependencies much easier to manage, and liblo is easier to interact with than rtosc (more-or-less by design). The major downside, pointed out by @fundamental, is that you lose the "realtime" part of rtosc. It's not possible to create an OSC message in liblo without making lots of heap allocations, which is technically problematic when we're broadcasting messages from the core.

Now, the existing broadcast mechanism built on rtosc still uses dynamic allocation in some areas (queuing messages), so we still have a ways to go. Because of the way the Messenger.h interface is structured though, it would be entirely possible to not construct the OSC messages in the caller thread (e.g. mixer threads), but instead just copy the arguments into a private queue and create + broadcast the messages in a separate thread. This would allow one to use liblo without any realtime issues, I believe, and sounds like a better design than what I have going right now. I imagine it won't achieve quite the same performance as possible with rtosc because of the extra copying and thread sync, but that might be worth it, especially if it simplifies the act of initially implementing OSC support.

Another option is to hunt for an existing OSC library that has similar realtime properties as rtosc but is also available through standard packaging systems. Personally, I'm fond of the idea in my 2nd paragraph after having typed it out. What do you think?

[curlymorphic]

On the subject of rtosc dependency, It will be needed for the ZASF 2.5 upgrade, when the bugs are ironed out. So is liblo. I am only stating this as I dont feel the difficulties in the rtosc packaging to be a reason to change the messaging system used.

Another option is to hunt for an existing OSC library that has similar realtime properties as rtosc but is also available through standard packaging systems

IIRC the lack of such a library was why rtosc was developed.

Personally, I'm fond of the idea in my 2nd paragraph after having typed it out. What do you think?

I have no experience with either libraries or implementing messaging systems in general, So I feel I am really not in a good position to comment.

[michaelgregorius]

@Wallacoloo It might be worth giving a try. To me your description sounds like you want to implement a facade which abstracts away the actual implementation of the communication (rtosc, liblo).

However, I wonder whether a complete implementation of such a mechanism wouldn't result in writing your own realtime safe messaging system. You will have a writer that uses the messenger interface to put messages into a queue without having to dynamically allocate memory. This queue is then read in a synchronized fashion by at least one reader which reads the messages and multiplexes / broadcasts them to several listeners. Or am I interpreting your comment completely wrong?

[Wallacoloo]

@michaelgregorius

You will have a writer that uses the messenger interface to put messages into a queue without having to dynamically allocate memory. This queue is then read in a synchronized fashion by at least one reader which reads the messages and multiplexes / broadcasts them to several listeners.

That's close to what I was suggesting. Here it is listed out explicitly:

1. Writer puts a field indicating the type of the message + associated arguments into a queue (would probably be done using a tagged union), without allocating dynamic memory.
2. Reader thread pops the information and constructs an actual OSC message from this, free to dynamically allocate memory if need be, since it isn't running in an audio thread.
3. Reader thread distributes the OSC message to all the listeners.

You're right though - at that point, encoding/decoding to OSC is just unneeded overhead. Hmm.

[fundamental]

@Wallacoloo pretty much any encoding is going to result in some memory or CPU encoding. I'd just look at the trade offs for maintainability seeing as the cost in cpu cycles tends to be relatively small and developer hours are limited. Personally I stuck with OSC because it makes it easier to inspect a system (e.g. http://fundamental-code.com/zyn-ports/ ) and it's a preexisting format which makes it possible to interface with the application externally (e.g. https://github.com/fundamental/oscprompt ). For my own work it has saved tons of time by making it easier to debug issues with a messaging system (which due to the complexity of multi-threaded lockless programming are bound to come up). It's all about what makes the most sense for a given application.

[Wallacoloo]

OK, well I would be fine sticking with the rtosc implementation. So then the bit to figure out is how to bring in the dependency.

My thoughts are:
First, fork fundamental/rtosc into lmms/rtosc, where we can maintain any lmms-specific patches.
Then, add lmms/rtosc as a submodule to the lmms/lmms repository under the libs directory. As part of the cmake configuration procedure, we can check for the existence of libs/rtosc, and if it doesn't exist, issue an error saying the user needs to git submodule update --init --recursive.

[Wallacoloo]

@curlymorphic

To clarify all future error/warnings should be made using Engine::messenger()->broadcastError and Engine::messenger()->broadcastWarning functions?

I apologize for not replying to that question. Any time it's worth showing the error/warning to the user, the answer would be "yes". If it's a non-fatal warning that the user isn't likely to care about (e.g. "failed to load xxx.png needed by theme"), it's probably better to log those to the console.

Eventually, we could tag these with unique error codes so that they could all be broadcast via the messenger and the gui could properly choose when and how to notify the user based on the error code.

[tresf]

git submodule update --init --recursive.

We did this with our Zyn repo a while back and the number of complaints made Toby revert the decision and just maintain the code in two repos. I'm not sure if this was a good idea or not given the complexity this can add to the codebase.

Perhaps we do the best of both worlds and check for git and run the command at configure time? I know that is a bit of coddling... 😕

[fundamental]

@tresf That's the approach that I took with zyn: https://github.com/fundamental/zynaddsubfx/blob/master/CMakeLists.txt#L7-L19

[Wallacoloo]

@fundamental @tresf There are a few reasons why cmake or make shouldn't implicitly access the web though (e.g. metered connections). It's also inconsistent with how we handle the rest of our dependencies.

If we use submodules, I'd prefer we error in the exact same manner as our other non-optional dependencies. E.g. if Qt isn't installed, we currently error with something like "Qt: not found. Install qt from your package manager to resolve." Adding one more step for installing dependencies doesn't seem especially problematic to me. We just say "rtosc: not found. Run git submodule update --init --recursive to resolve."

What kind of complaints did @tobydox get regarding the use of submodules?

[tresf]

What kind of complaints did @tobydox get regarding the use of submodules?

http://sourceforge.net/p/lmms/mailman/message/32164398/

#315 (comment)

http://comments.gmane.org/gmane.comp.audio.lmms.devel/4981

...etc

[lukas-w]

My proposal regarding the /extern directory was meant for dependencies already existing in the codebase only. I'm strongly opposed to introducing new foreign code in our repo. The existing ones are bad enough.

Despite my comment from 2014 in #315, I vote for submodules. One of the arguments against using it was

bzr-git import module does not support submodules, and this would certainly mean I [dobey] would have to disable any daily builds in my PPA for Ubuntu.

But IIRC, bzr-git doesn't work for projects that have used submodules at any time in their history. We have used them.
I also realised there are just no good alternatives. If someone knows one, I'd love to hear about it.

@Wallacoloo's arguments are quite convincing in my ears. We're talking about an external dependency here. As with every dependency it's necessary to install it from a package manager or compile it yourself before compiling LMMS. By adding the dependency as a submodule we make people's life easier by having them just run git submodule update --init --recursive instead of searching for and downloading the external project's sources themselves. This gets even better when we have more than just one project. You can fetch them all with one command.

We can always drop submodules and switch to something better if it turns out to be too ugly. However, submodules are still lots better than including the dependency.

[jackokring]

Lock free queues: A simple method I have heard of is to use a function pointer to decide queue access. Each writer tries to write a function pointer into a shared variable, (the processor snoop bus kind of does a behind the scenes lock, to prevent cache too and fro). A function pointer must have a bit length of an atomic size for this to work. After the write the writing thread sleeps. The queue transit thread executes the function pointer, and performs the writer's list attachment using the global shared array of the intended write of each writer, and nulls the array element of the writer, and on return writes a sleep myself function pointer value to the function pointer, and then sleeps. So on wake up the queue transit thread has a know duty. (Could use an atomic size writer ID and switch?)

When a writer wakes up it checks to see if its global slot is nulled. If it is, queuing was successful. If it is not, queuing may have not been successful, and so must be tried again. And this is the important point, it must use the same sequencing number in the data structure linked to from its global element in the writer global array. So the transit thread has one more duty before sleep, and that is to delete (low CPU load) duplicates.

A multiple destination reader is handled by having the transit thread queue to many linked lists, one for each output destination. As the element needs to be written to the tail of the list, so that the reader can read the head (note the head does not but can be advanced due to sequence number and writer ID). This is achieved by having the list tail element point to the head element in a circular loop, and indexing the list starting from the tail. This makes append easy. The reader must check for duplicates based on writer and sequence ID, and retry after sleep.

A tail insert may introduce a duplicate, new element points to head, backup old pointer to tail pointer (should be done first), advance pointer to tail, using backup extend list to point to new element. Atomic writes in that order keep the list append operation safe. The element may be removed before the last operation completes, and so hence the backup. And also it would be good to have two flags indicating inserted (pre applied) and removed (post applied). These are so that removal knows to wait for insertion completion, and the destructor knows to wait for removal completion if an insert has been done.

A head remove is index via tail pointer, and make tail of list pointer point to found head's next element.

If an element points to itself it is considered a null empty queue, and initialization must make the tail pointer point to one element behaving as such. (the null element).

Very complex indeed, and I have no tested source. But I do think it worth explaining the complexity of some of the underlying issues that are keeping this pull open.

EDIT: To get the final thing out, a reader may have to send the null element to itself. 🎯 and the input mux can be eliminated for a single 1 to 1. An output cross thread sync (i.e. 1 to 1) may be instanced in a sender thread so working as a blocking proxy, allowing the actual sender to not block.

EDIT: As the insertion into the output sync list has to happen before the removal can complete, a flag in the element before an item in the list has to be set after correct insertion, and removal has to retry until this flag is set. At all cost avoid memory address write collisions, and so this is why the one mux ID write is the bottleneck. A mux hierarchy may solve this.

EDIT: As an insert started flag in the previous element in the 'tail circular list' would also be necessary. The opposite of using removal started/completed may look as good, and may be better for lowering queue sizes and latency, but does have the dual flag reset issue, an so a single flag "removing in progress" flag would be better. Removal is also a faster operation than insertion, so the virtual, (but not a real, with the process overhead) kernel "lock" would block (or re-issue) for less time. But as the insertion has to step forward the tail pointer too, it is the one which is not semi-atomic. The removal has just one pointer update to fix it in the list as complete. So in has to be "insertion in progress". And bigger buffers perhaps slow other "trashing" on the queue, and it's better?!

Using a head and a tail pointer with dual access may look simpler logically, but moving the tail back in the list needs double linking (link list not bad in this context due to lower cache line collisions as opposed to a linear addressed buffer), This requires a greater virtual locking time, on short queues, (throwing more yield, slowing), and more complex signalling.

The null element (maybe sequence ID zero, and a self pointing element is never removed), is re-inserted in the removal procedure, and never passed on.

64 bit really complicates the issue when using a 32 bit bus, and does a 64 bit atomic write exist? That's for the compiler aficionado.

EDIT: The trick is that the 'insertion in progress' flag implies a removal fail (via an insertion overwrite completion), but in that case the overwrite will link to the element after the one after the removed, and complete one write later with a tail pointer move, (or move the tail pointer first ... and use the previous value of it to help fill in the link). So semi-atomically (link to speculative next element filled in before linking to active element). So if the one before the tail has not yet been linked post the tail pointer move, and it is the head also (circular), the tail will still point correctly. A two item circular list with one removed. Removing an element from a single element circular list, works and the self reference is detected and a no read is declared. Any part inserted element which has not been fixed up, is not findable as the head. And contrary to my stipulation before, duplicates do not occur.

A FINAL EDIT: jumping the pointer to the head, for removal may race, and recover the tail, so if the pointer to the tail has moved after indexing a head for removal, a removal has to be rescheduled, and declared void. I think this algorithm is new and I declare this post GPL .as per the LMMS source.

Or use output mux object, on each input, and spread the contention chance, and make the transport thread just round loop over all the outputs of the input syncs.

Continued jackokring#8 for briefness here.

[Wallacoloo]

Very complex indeed, and I have no tested source. But I do think it worth explaining the complexity of some of the underlying issues that are keeping this pull open.

Locks are already used quite a bit inside the lmms audio processing code. I think the thing keeping this PR open is that nobody has stepped up to finish the OSC integration - it would not be very good to begin implementing OSC messaging but then abandon it before it's feature-complete or otherwise useful (e.g. UI integration). Doing so would just mean extra baggage that has to be carried around.

To be clear, I've decided that I'm not cut out for that role; I'm not going to be doing any further OSC development with lmms in the foreseeable future - I'm not comfortable making integral changes to a large codebase that doesn't have any form of regression testing, and adding testcases at this stage of development is equally intimidating (it appears to me as a catch-22, since testing requires better core/gui separation, but OSC is what aims to provide that feature).

[jackokring]

@Wallacoloo I saw a delete before detachment from the engine in Opulenz the other day. Does the engine do any writeback into a possibly reallocated object? I just don't know the code well enough.

[Umcaruje]

Taking the critical label off this, since #1758 was sucessfully resolved.

[Umcaruje]

@Wallacoloo what will happen with these OSC related Pull Requests, now that you can't/won't work on fully implementing OSC messaging in LMMS?

[Wallacoloo]

@Umcaruje That's a group decision, I suppose. If/when needed, I will gladly update or rebase these PRs against future changes to the codebase.

But until then (i.e. until somebody starts working on OSC stuff again), it seems reasonable to close these PRs to declutter the tracker and reopen them later if needed. Feel free to close these if you agree.

[tresf]

it seems reasonable to close these PRs to declutter the tracker and reopen them later if needed. Feel free to close these if you agree.

Closing for now. 👍