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.

Sign up for GitHub

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

Jump to bottom

[WIP] OSC support via liblo #2064

Closed
wants to merge 2 commits into from
Closed

[WIP] OSC support via liblo #2064

wants to merge 2 commits into from

Conversation

iffyloop
Copy link

This pull request adds OSC support (provided by the oscpkt library) to Mixxx. This has been discussed under the "OSC Support" topic of the development stream on Mixxx's Zulip instance. Please feel free to provide any further feedback and ideas of changes that should be incorporated here.
Thanks!

@iffyloop
Copy link
Author

@harryhaaren I'm still fairly new to threading and networking in C++, so thanks for bringing up the blocking issues. If you or anyone else could give me some guidance as to what would be involved in creating a dedicated OSC thread, that would be appreciated. Would that interfere with the other Mixxx controller threads? In that case, I'm wondering if it would be better to switch libraries to liblo and use it to create a non-blocking server:
http://liblo.sourceforge.net/examples/nonblocking_server_example.c.html

In response to your question about making the socket "block forever" - that's what happens if you set the timeout to -1. I tried this yesterday and everything seemed to work fine (but I didn't test with any hardware controllers plugged in - I'm guessing it might have blocked those) until I closed the application, at which point Mixxx just froze and stopped responding.

@iffyloop
Copy link
Author

I think mapping each group/key to a ControlProxy makes sense, but I'm wondering if there would be the same performance overhead if the map grew large from many different controls being addressed. Could you explain why ControlProxy is such a heavyweight class?

@harryhaaren
Copy link

Re threads; example is Ctlra controller, creates one thread for any amount of Ctlra controllers: https://github.com/openAVproductions/mixxx/blob/ctlra/src/controllers/ctlra/ctlraenumerator.cpp#L16

I expect the design to map best to using on thread per OSC socket, because that allows blocking the thread on the socket read - causing it to go to sleep until the kernel wakes it up because it has work to do.

I see no advantage to switching to LibLO per-se, using -1 to achieve non-blocking is just a good as using a library that supports it using API calls.

Re; threads and interfering with other Mixxx threads;
A ControlProxy uses "atomic" writes to achieve a single consistent state - this means that writing a value from one thread will just over-write the previous value - but you'll never get "half" of the previous value, and half of the new value (due to threading issues). This makes it easy to use ControlProxy's in lots of threads, and nobody needs to manage things bar using a ControlProxy->set() function to update the value itself.

Re: ControlProxy creation; Creating any class is usually expensive - in this case the ControlProxy is "linked" to a ControlObject, which has to be found. I believe the current implementation is using a HashTable, so the the "Group/Key" pair get hashed and then looked up (this isn't too expensive usually). What can be particularly expensive is calling malloc() or new in the C++ case. Although the code here doesn't directly do that, we can't be sure that the ControlProxy() constructor function doesn't allocate memory internally. For this reason, if you know you'll need something in "normal" usage of a program, its usually better to identify it, and cache it for use, instead of allocating and de-allocating it every time it is required.
Most importantly: measure if there is a real world impact on performance due to this!

@harryhaaren
Copy link

Left a comment in the Zulip on caching ControlProxy* instances in a hash table after they're used once - this will make the commonly used controls more performant to access, and the not-used controls will not consume any memory: https://mixxx.zulipchat.com/#narrow/stream/109171-development/topic/OSC.20Support

@daschuer
Copy link
Member

Did you considered to use liblo instead of oscpkt? The other draft implementations is using this, and IMHO we should use only one if there is no technical limitation.

@daschuer
Copy link
Member

Could you explain why ControlProxy is such a heavyweight class?

The required Hash table lookup is the heavy part. The object itself avoids to doing it over and over again because it just wraps a shared pointer to the control and adds the capability of receive the valueChange signal. I have created a PR introducing a light version of ControlProxy without the valueChange hook #1717 but this does not relrase you from the hash table lookup and the required string math.

@daschuer
Copy link
Member

This PR:
#1675
Introduced the Mpris D-Bus interface to Mixxx and uses a thread to write a file https://github.com/davidhm/mixxx/blob/39dc90135a2f3d373c05ab944198536fc8fbfbac/src/broadcast/filelistener/filelistener.cpp
Maybe this helps.

@iffyloop
Copy link
Author

@daschuer I'm a little confused about the end of your message "we should use only one if there is no technical limitation." Are you talking about using only one thread? Or are you saying we should switch to liblo?
The reason I mentioned liblo earlier is that I think it can handle threading for us (something like http://liblo.sourceforge.net/examples/example_server.c.html). I think maybe we would just have to call lo_server_thread_start in OscController::open and lo_server_thread_free in OscController::close. Then we could get rid of OscController::poll and let liblo call our OSC methods, right? I'm happy to try that approach, but it would be helpful if you could give me some guidance on how to correctly include liblo via Mixxx's build system.
Also, if the hash table lookup is the heavy part of using the ControlProxy class, then wouldn't the caching proposal by @harryhaaren be just as slow?

@daschuer
Copy link
Member

Or are you saying we should switch to liblo?

Yes, #2062 uses liblo, so maybe it suits for all OSC topics. But I am unsure because I have never worked with it. At least it is under active development http://liblo.sourceforge.net/NEWS.html

The reason I mentioned liblo earlier is that I think it can handle threading for us ...

That looks easy, hopfuly.

liblo is setup in #2062 for Ubuntu. We need to add it to the Windows and Mac CI to be able to successful build it.

I think your approach an #2062 can live side by side. Do we need here a preferences page as well? Maybe we can use one common page for client ant server.

Also, if the hash table lookup is the heavy part of using the ControlProxy class, then wouldn't the caching proposal by @harryhaaren be just as slow?

Since the Hash Table scales well for many entries, I expect only a minor benefit if you set up your own hash table. With the single parameter solution, we have the chance to get entirely rid of the hash table and save the ControlProxy inside the callback.

@daschuer
Copy link
Member

Should this relay become a controller? We have a model for each hardware controller in Mixxx to allow a mapping via GUI or Java.

In the OSC receive case we offer a fixed API anyone can use. Is there a need for a OSC controllrt model?

@iffyloop iffyloop changed the title [WIP] OSC support via oscpkt [WIP] OSC support via liblo Mar 26, 2019
@iffyloop
Copy link
Author

I just rewrote the majority of OscController.cpp to use liblo rather than oscpkt. There's no more polling now and liblo takes care of creating its own non-blocking thread. All controls are now addressable via their own path, so you can send messages like "/[Master]/crossfader 0.3" (path, double). If possible, types of value (e.g. int, float, bool, etc.) are automatically converted to double (so you can send "/[Channel1]/play true" and Deck 1 will start playing). If you want to query a parameter, you send "/[Master]/crossfader '?'" (path, string) - just like the Resolume docs @daschuer linked me to on Zulip.

@iffyloop
Copy link
Author

iffyloop commented Mar 26, 2019 via email

@iffyloop
Copy link
Author

iffyloop commented Mar 26, 2019 via email

@daschuer
Copy link
Member

I think the controller model, does actually not fit well here.
The OSC Server is represented as a controller here. Normally the clients are represented as a controller.
In this case it would be TouchOSC, but we cannot do it because Mixxx is not aware any OSC controller.

@iffyloop
Copy link
Author

iffyloop commented Mar 26, 2019 via email

@daschuer
Copy link
Member

I am sorry I have not yet found the time to test you branch. Here only some thoughts that might be wrong:

If we look at TouchOSC it is a OSC Server and Client at the same time. A control has a unique single Unique address, that can be auto generated from the structure in TouchOSC or adopt the namespace of the controlled Application.

In this case of a fully customizable Controller, it is reasonable to use the Mixxx Namespace for it. It is somehow like
#2062 with its own preferences page. Here we need to enter the server port and the Clint's IP address and portnumbet. The current implementation of the client side is currently not flexible enough, since we cannot simply send every single CO update from Mixxx.

Here I have an idea from your implementation: How about using the temporary ControlProxy to register a OSC callback for all following OSC commands and register a value change callback at the control proxy to send OSC commands back.
This would be a nice plug and play solution :-)

If we think of more complicated control task where for example one control on the controller will control two controls of Mixxx, the a OSC Controller would be handy.

This can be implemented by copying the HID controller implementation where every hid command is parsed by a script.

We may also consider to use a Xml mapping file that would allow to map the controller namespace to the Mixxx Namespace.

@daschuer
Copy link
Member

IMHO Mixxx can make use of both interfaces one for easy interfacing other customizable applications and one for map other static applications, that requires a mapping just like midi controllers.

I am fine with merging a solution that is somewhere on the way to it.

@daschuer
Copy link
Member

Is it Ok to have the co namespace in the OSC address root? I am unsure.

@iffyloop
Copy link
Author

@daschuer Sorry - I'm still a bit confused your previous comments. Is my implementation redundant now that #2062 is about to be merged? I don't want to clutter the Mixxx codebase if someone else has already implemented the same functionality.

@daschuer
Copy link
Member

Both PR target different issue. This here is a OSC Server, the other one is a OSC Client.
For TouchOSC for instane we need both.

Can you have a look to the other PR and descide how a integrated solution will look like. If we have an idea about it we can tweak both towards it and merge them finally.

Pegasus-RPG
Pegasus-RPG requested changes Mar 27, 2019
View reviewed changes
Copy link
Member

@Pegasus-RPG Pegasus-RPG left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question about how OSCEnumerator is structured. Comments inline. #2062 looks like it integrates OSC endpoints more deeply with Mixxx, rather than the "arm's length" approach the Controller subsystem offers. I've never used OSC so can't speak to which is more appropriate for the DJ use case, though my gut feeling is the deeper integration would be more flexible.

build/depends.py
@@ -58,6 +58,13 @@ def configure(self, build, conf):
def sources(self, build):
return ['controllers/midi/portmidienumerator.cpp', 'controllers/midi/portmidicontroller.cpp']

# TODO: how to make this optional? (see OSC in features.py)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answer: Check to see how HSS1394 does it. I believe you would move this liblo stuff to features.py, so it's only acted on if the feature is turned on.

build/depends.py
@@ -1529,7 +1536,7 @@ def depends(self, build):
return [SoundTouch, ReplayGain, Ebur128Mit, PortAudio, PortMIDI, Qt, TestHeaders,
FidLib, SndFile, FLAC, OggVorbis, OpenGL, TagLib, ProtoBuf,
Chromaprint, RubberBand, SecurityFramework, CoreServices, IOKit,
QtScriptByteArray, Reverb, FpClassify, PortAudioRingBuffer]
QtScriptByteArray, Reverb, FpClassify, PortAudioRingBuffer, LibLO]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

src/controllers/dlgcontrollerlearning.cpp
@@ -401,6 +401,11 @@ void DlgControllerLearning::visit(BulkController* pBulkController) {
Q_UNUSED(pBulkController);
}

void DlgControllerLearning::visit(OscController* pOscController) {
qWarning() << "ERROR: DlgControllerLearning does not OSC devices.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"does not yet support OSC devices"

src/controllers/osc/oscenumerator.cpp
#include "controllers/osc/oscenumerator.h"

OscEnumerator::OscEnumerator() {
m_devices.push_back(new OscController());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems strange to me. I understand this to mean that when an OSCEnumerator is instantiated, it creates a single OSCController. The intent of the controller subsystem is to query available controllers in queryDevices() using whatever API is required to see what OSC endpoints are on the network and return that list there. That allows users to hit a 're-scan' button in the GUI which would call queryDevices() again to get an updated list. As well, there is supposed to be one OSCController instance per endpoint. Does OSC not fit this paradigm?

@iffyloop
Copy link
Author

iffyloop commented Mar 28, 2019 via email

@iffyloop
Copy link
Author

iffyloop commented Mar 28, 2019 via email

@daschuer
Copy link
Member

Do we agree to the following future implementation?

  1. Mixxx should provide a mapping free OSC Server Socket for all ControlObjects with Mixxx addressing.
  2. Mixxx should provide exactly one Client for responses on this socked, optimized for TouchOSC like servers also Mixxx addressing.
  3. Via 2. It should be possible to subscribe to other sting or binary blob info. Using a subscription protocol (TouchOSC) and a config file.
  4. We will provide additional Server and separated Client pairs to receive OSC command with any addressing. This can be parsed with JS only just like HID controllers?
  5. Adopt the Midi Xml mapping file for OSC (or a future solution discussed) to allow point and click mappings

I think all this can be done one by one.
What do you think?

@iffyloop
Copy link
Author

@daschuer I definitely agree on impementation ideas 1 & 2 above. I think 3-5 would be wonderful features to have but are probably too involved for me to implement given the amount of time I have right now. Am I correct in assuming that the other OSC support PR adds feature 2? If so, would it be best for me just to fork your daschuer/mixxx repository and try to build feature 1 into its osc_support branch?

@daschuer
Copy link
Member

Great, it is reasonable to to it step by step so go ahead. Please just take the rout that suits best to you.

@iffyloop
Copy link
Author

iffyloop commented Apr 7, 2019

@daschuer I'm almost done with the new OSC server implementation that can exist alongside the existing OSC client we've been discussing. Since I built this into your fork of Mixxx that has the osc_support branch, would it be better for me to create a PR on your fork rather than this repository?

@daschuer
Copy link
Member

daschuer commented Apr 7, 2019

If you are willing to take responds for all OSC branches, It is probably reasonable that you issue your own PR which includes @jakobbraun work. We can then close #2062 with a reference to your PR.

@iffyloop
Copy link
Author

iffyloop commented Apr 7, 2019 via email

@daschuer
Copy link
Member

daschuer commented Apr 7, 2019

mixxxdj/mixxx

@iffyloop iffyloop mentioned this pull request Apr 8, 2019
Closed
@iffyloop
Copy link
Author

iffyloop commented Apr 8, 2019

Thanks @daschuer. I've created a new PR #2078. Do you want to keep this (#2064) PR or should we delete it?

@github-actions
Copy link

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions bot added the stale Stale issues that haven't been updated for a long time. label Oct 20, 2020
@Be-ing Be-ing changed the base branch from master to main October 23, 2020 23:27
@ronso0 ronso0 marked this pull request as draft February 14, 2021 21:56
@iffyloop iffyloop closed this by deleting the head repository Sep 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Pegasus-RPG Pegasus-RPG Pegasus-RPG requested changes

Assignees
No one assigned
Labels
stale Stale issues that haven't been updated for a long time.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iffyloop @harryhaaren @daschuer @Pegasus-RPG