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

Multi channel with phantom clients #1913

Merged
merged 0 commits into from
Feb 16, 2022
Merged

Multi channel with phantom clients #1913

merged 0 commits into from
Feb 16, 2022

Conversation

dcorson-ticino-com
Copy link
Contributor

Multichannel Input using phantom clients

Dual Mono-in/Stereo-out added. Note this is not yet working for delay
panned servers. I don't understand the code, still looking on that.

There is no change to the protocol, works with old clients (of course they can't input
dual mono, but the old client can receive and manipulate the channel).
Using version number 3.9.9 and requires that on the server and client to enable dual mono.

Also compiled with the new ASIOSDK2.3.3 seems to be working fine.

I'm thinking an addition to the protocol between the dual mono input
client and server might be nice to be able to give the phantom channel a
different name and instrument. That could still work with old clients at
the receiving end. Right now I am just appending -L and -R to the name.

Short description of changes

Multichannel Input using phantom clients

Context: Fixes an issue?

Does this change need documentation? What needs to be documented and how?

Status of this Pull Request
Draft and testing, note delay pan is not yet implemented for Dual Mono-in/Stereo-out

What is missing until this pull request can be merged?
Testing, testing, testing
Agreement that we should do multichannel like this
Checklist

I've verified that this Pull Request follows the general code principles
[x ] I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

@dcorson-ticino-com
Copy link
Contributor Author

Thanks for the tip Noam !
For me GitHub (and git in general) is maximally opaque.

@dcorson-ticino-com
Copy link
Contributor Author

dcorson-ticino-com commented Jul 14, 2021

Oh, great. I pulled in the new ASIOSDK too, but it is, of course, not formatted according to out rules so the Clang-format check fails :(
EDIT: and the files are apparently not to be found either. This is no fun :(

@npostavs
Copy link
Contributor

I don't think you should be including the ASIOSDK code at all, rather update the lines in windows/deploy_windows.ps1 to point to the new one.

@ann0see
Copy link
Member

ann0see commented Jul 14, 2021

I don't understand the code, still looking on that.

cc: @DetlefHennings

@dcorson-ticino-com
Copy link
Contributor Author

dcorson-ticino-com commented Jul 14, 2021

Went back to old ASIOSDK and did some corrections on the phantom fader behaviour.
Looks better now, also updated on my server Classical>Strings Server.
Clang-format on github still doesn't agree with Clang-format locally.
Just did a check, the communications bandwidth used in Dual-Mono is exactly the same as for Stereo, as expected.

@ann0see ann0see marked this pull request as draft July 14, 2021 17:54
@DetlefHennings
Copy link

For lack of time I did not yet look into any details. But as far as I understand, the idea behind seems to be similar to having a stereo mixer with multiple panned mono inputs in front of a Jamulus in stereo-in/stereo-out mode.
To comply with a delay panned session, the panning of the mono inputs could also be done by delays instead of volume differences.

@dcorson-ticino-com
Copy link
Contributor Author

Oops, just saw that the phantom's mutes are not working.
Will have to wait 'til I am back from vacation.
In the mean time please test at your leisure.

@ann0see
Copy link
Member

ann0see commented Sep 5, 2021

@dcorson-ticino-com could you please check if you can fix the coding style check?

@dcorson-ticino-com
Copy link
Contributor Author

@dcorson-ticino-com could you please check if you can fix the coding style check?

I do not understand why the local style check and the github style check do not give the same results.
As it is I have to change the white spaces in a couple of lines after each compilation.

@pljones
Copy link
Collaborator

pljones commented Sep 5, 2021

@dcorson-ticino-com could you please check if you can fix the coding style check?

I do not understand why the local style check and the github style check do not give the same results.
As it is I have to change the white spaces in a couple of lines after each compilation.

The Github one is, by definition, correct. You probably have a different version of clang-format. I don't remember the required minimum but I do know that my linux server doesn't have it (as I run Ubuntu LTS rather than keeping it bleeding edge).

This is the main reason I want the style check run on every commit.

@dcorson-ticino-com
Copy link
Contributor Author

So what version of clang-format should I be using locally.
What I have now says v12.0.0
That is under Win10

@pljones
Copy link
Collaborator

pljones commented Sep 5, 2021

So what version of clang-format should I be using locally.
What I have now says v12.0.0

The same as the build process ideally. I'm not sure what that is. It actually looks like it might have been a bug in clang-format that makes it want to interpret the style the way it is, so a later version may be incompatible... Just one of those things. Easiest is to let Github tell you what the problems are and just fix them and not worry, in my view.

The style check step only takes about 20 seconds to run, so having it run on every commit - and not run the later steps if it failed - would be really useful.

@dcorson-ticino-com
Copy link
Contributor Author

dcorson-ticino-com commented Sep 5, 2021

Easiest is to let Github tell you what the problems are and just fix them and not worry, in my view.

The problem is that it wants changes in white space so it is always a pain to find what it really wants, exactly how many spaces, tabs, I don't remember exactly.
Possible, but a pain.
And this PR still has problems with muting that I have not been able to resolve yet.
I hope that October will give me more free time to look after this.

@pljones
Copy link
Collaborator

pljones commented Sep 5, 2021

We don't use tab characters.

It provides a patch that can be applied, so it's no work, too.

@dakhubgit
Copy link
Contributor

Stupid question: how would I be able to get Windows binaries? Looks like we'll be interjecting a virtual rehearsal this week because our room is occupied, and we have one pair of players with a Windows laptop where this would come in handy. I am in charge of the server and I know how to deal with that part (which is why I'd prefer a server-only option where a channel strip name "Jack & Jill" automatically splits the strip in two, obviously causing some strangeness particularly regarding "mute myself" with the unwitting client getting split).

@pljones
Copy link
Collaborator

pljones commented Sep 15, 2021

how would I be able to get Windows binaries?

See the list of "checks" below all these comments and click "Details" on one of them (best in a new tab/window). Then, over to the right, above the big black box of "Details" for whatever you clicked, there's a label saying "Artifacts (6)". Had you arrived here soon enough, you could click on one of those and download the ZIP file.

But, as it was 14 Jul when the last change was made, the build has now expired.

So, currently, you'd need to clone https://github.com/dcorson-ticino-com/jamulus/tree/MultiChannel and build it.

@softins
Copy link
Member

softins commented Sep 15, 2021

Stupid question: how would I be able to get Windows binaries? Looks like we'll be interjecting a virtual rehearsal this week because our room is occupied, and we have one pair of players with a Windows laptop where this would come in handy. I am in charge of the server and I know how to deal with that part (which is why I'd prefer a server-only option where a channel strip name "Jack & Jill" automatically splits the strip in two, obviously causing some strangeness particularly regarding "mute myself" with the unwitting client getting split).

I think the easiest would be if you forked the repo dcorson-ticino-com/jamulus into your own github account. You could then on your fork go into "Actions", then click on the left on Auto-Build. The click on the grey "Run workflow" button, and in the box that drops down, select the "MultiChannel" branch. Then click on the green "Run workflow" button. That should kick off a build for all platforms. When it's finished, you can download the artifacts.

@dakhubgit
Copy link
Contributor

I think the easiest would be if you forked the repo dcorson-ticino-com/jamulus into your own github account.

My server setup script pulls a "deployment" branch from my fork anyway, so triggering a build from my fork looks like low-hanging fruit once I know how this is done.

Will try, thanks!

@softins
Copy link
Member

softins commented Sep 15, 2021

I think the easiest would be if you forked the repo dcorson-ticino-com/jamulus into your own github account.

My server setup script pulls a "deployment" branch from my fork anyway, so triggering a build from my fork looks like low-hanging fruit once I know how this is done.

Will try, thanks!

Your fork of jamulussoftware/jamulus will not contain the MultiChannel branch from dcorson-ticino-com/jamulus. That is why I mentioned you need also to fork the latter repository. I haven't ever tried to fork two different repos with the same name, so I'm not sure whether Github allows you to, and what happens if you do.

Alternatively, and this might actually be a lot better, you can add dcorson-ticino-com as an additional remote on the command line and fetch the branch from there:

git remote add dcorson https://github.com/dcorson-ticino-com/jamulus.git
git fetch dcorson MultiChannel:MultiChannel
git checkout MultiChannel
git push --set-upstream origin MultiChannel

You need to specify both local and remote branch names in the fetch. The above procedure avoids tracking dcorson instead of your origin.

You then have your own branch on Github that is a snapshot of dcorson-ticino-com:MultiChannel, and you can run the workflow on that.

@softins
Copy link
Member

softins commented Sep 15, 2021

In fact, there is a better procedure that doesn't involve setting dcorson as a remote. You can fetch from the pull request:

git fetch upstream pull/1913/head:MultiChannel
git checkout MultiChannel
git push --set-upstream origin MultiChannel

This assumes that you have remotes called origin for your fork, and upstream for jamulussoftware/jamulus.

@dakhubgit
Copy link
Contributor

Well thanks for the pointers, but my git-fu certainly is up to dealing with multiple remotes (my local work tree already has both my own and the original repositories as upstream). In fact, I am a lot more fuzzy about how to do things with a web interface than just via the commandline, so I am not going to make use of your advice. But it certainly should come in handy for other potential testers!

@softins
Copy link
Member

softins commented Sep 15, 2021

Well thanks for the pointers, but my git-fu certainly is up to dealing with multiple remotes (my local work tree already has both my own and the original repositories as upstream). In fact, I am a lot more fuzzy about how to do things with a web interface than just via the commandline, so I am not going to make use of your advice. But it certainly should come in handy for other potential testers!

Ok, that's cool. The bottom line is that once you have got the MultiChannel branch into your own fork on Github, you can run the workflows on it to produce the binares.

@ann0see
Copy link
Member

ann0see commented Jan 28, 2022

This PR still seems to be interesting, but is stale. @dcorson-ticino-com I think if someone wants to take it over it’s ok for you. I‘d like to put it on a feature branch.

@dcorson-ticino-com
Copy link
Contributor Author

I am still interested in this, it is one of the few new features that I think is really useful, but am waiting for a version or so to see how the refactoring of the program turns out. As it is the muting seems to be done at a low level and will be difficult to get right. As the separation of UI and engine goes forward maybe this will get simpler.
In any case this has shown that this can work. I will probably start gain from scratch though.

@ann0see ann0see changed the base branch from master to feature/dcorson/MultiChannel February 16, 2022 20:56
@ann0see ann0see merged commit 23d80f0 into jamulussoftware:feature/dcorson/MultiChannel Feb 16, 2022
@ann0see ann0see force-pushed the feature/dcorson/MultiChannel branch from 89b3d0e to 23d80f0 Compare February 16, 2022 20:57
@ann0see
Copy link
Member

ann0see commented Feb 16, 2022

Merged to a feature branch.

@pljones
Copy link
Collaborator

pljones commented Feb 16, 2022

Merged to a feature branch.

Thanks for writing the comment -- I'll try to remember this time 😁

@ann0see
Copy link
Member

ann0see commented Feb 16, 2022

By the way: We should discuss what to merge to a feature branch when. Opened #2396

@dcorson-ticino-com dcorson-ticino-com deleted the MultiChannel branch March 23, 2022 18:03
@dcorson-ticino-com dcorson-ticino-com restored the MultiChannel branch March 23, 2022 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

7 participants