-
-
Notifications
You must be signed in to change notification settings - Fork 994
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
will fix issue #523 JACK transport support with ExSync API draft #6066
base: master
Are you sure you want to change the base?
Conversation
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Windows
Linux
macOS🤖{"platform_name_to_artifacts": {"Windows": [{"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://14116-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.121%2Bg217a98b0e-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14116?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://14120-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.121%2Bg217a98b0e-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14120?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/8bom1aqi29cagkvy/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/39758894"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/v43q6m6okcfdeaec/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/39758894"}], "Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://14117-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.121%2Bg217a98b-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14117?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://14119-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.121%2Bg217a98b0e-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/14119?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "878a14ed64bf7d7d2c5c4dbafbea5c943cfc9892"} |
Some files for testers in zip: |
Some video illustration (on the start : ALSA ; than JACK): jackd_sync_v02_red.mp4And finally main PR goals: |
Is LMMS developers command core interested in this PR? |
Sorry for the late response. We ust haven't paid any attention to this PR, among a few others, and idk who should be reviewing this, but from my instinct, i would like to invite @michaelgregorius |
Don't delete PRs just because they are stale. Someone else might work on top of it. Also, tbh i don't understand what the feature is. |
Forgot to mention this too, you can add fixes #523 and it'll automatically close on merge. |
It synchronizes LMMS' transport with the JACK transport and vice versa. There's a demonstration video by @firewall1110 in this comment: #6066 (comment). IMO the following needs to be done before this PR is reviewed:
My first gut feeling after quickly going over the code is that it is a bit all over the place because there is no abstraction of the synchronization feature, i.e. there is no interface that LMMS defines which describes the sync features that are supported by LMMS. An abstraction would be useful if other audio back ends also support synchronization. In that case there would be an implementation of the interface for JACK and for all other back ends that support it. It would also keep the JACK related code mostly in one place and not scattered with |
I understand :I will wait for @LMMS/people ( @LMMS/developers make wrong link) decision. About my implementation:
A:(4) every IPC type can be used for synchronization - LMMS can introduce (may be - should) LMMS synchronization protocol (for example: named shared memory with struct {unsigned char status; unsigned long time /* in usec */ ; }) B:(1) I assumed new feature implementation with small compact steps: PR : <1:JackTransport> <2:PortMidi MTC> <3:ShaMem IPC> <4:ExSync.h + code refactoring> ... This is 1-st step <1:JackTransport> abstraction is <4:ExSync.h + code refactoring> But once more "The @LMMS/developers have to decide if the feature is wanted and if they want to maintain it." |
Now I think about " keep the ExSync related code mostly in one place" but with the same goal : nothing should be added in compiled code if
So questions are: |
Look at |
If I understand this correctly, this is similar to VST sync, but for JACK? Maybe this could be given a nice generic transport/timeline info interface which would let VST, JACK, and also CLAP make use of it. Then we wouldn't need a bunch of different code all performing similar tasks patched into |
Is the "ExSync API" something you've designed, or is there API documentation for it somewhere on the internet that I can read? |
VST sync provides LMMS transport info to VST plugins similar to how this PR provides transport info to JACK. It does not allow VST plugins control over the LMMS transport as this PR does, but if we refactored LMMS's messy transport implementation to provide a generic transport provider/controller, we could implement VST sync and this "ExSync" in terms of that quite easily. Then we wouldn't have to perform shotgun surgery by modifying multiple places in several files whenever some new LMMS feature needs access to transport information. I'm not saying this PR has to be the PR that cleans up LMMS's transport mess, but it also shouldn't introduce any new messes. As it stands, this PR is pretty messy and indecipherable. The LMMS coding conventions are not followed at all, everything is written in C style, and it just feels unorganized and confusing in my opinion. It will need to be cleaned up before it can be merged. |
@messmerd ! Thank You for comments! Quote: "Is the "ExSync API" something you've designed... ?" Yes, it is my designed API: (title) "... with ExSync API draft". In core/ExSync.cpp (for using in driver implementation: protected interface): P.S.1 |
Quote: "VST sync provides LMMS transport info to VST plugins similar to how this PR provides transport info to JACK." In this case Vestige code should be able somehow to implement struct ExSyncHandler and connect it to ExSync.o ... Quote: "The LMMS coding conventions are not followed at all" Quote: "everything is written in C style" Is C style not allowed by LMMS coding conventions? I think - no, but @messmerd - Your are LMMS member and Your opinion must be respected. Question is about - do I understand You ? P.S. But before I should prove that such kind of construction is useful at all (and only after that find better implementation) - but I am not sure... |
So another 10 days ... (Edited:) Stopped as it seems not more actual My plan 1-st step is: (Edited 2024.08.14) This plan is bad : my intuition bring me better one, but work anyway stopped After this code will enter the state, covered by LMMS coding conventions, but still will be in "C with classes" style. P.S. Stop me if I am wrong ... Now I am about add additional complexity, not needed for communication with Jack. |
You know what would be nice, develop the "ExSync" API on your own and we can submodule it. It doesn't need to be this coupled into LMMS afaik. Or does it use any LMMS specific features? |
Quote: "Or does it use any LMMS specific features?" (@Rossmaxx) Much worse ( ;) ) ... But seriously : it is adapter from LMMS "transport+" (and I think - it is LMMS specific feature) to another application : now only to jackd. P.S. |
Oh i see. @DomClark does have an IPC refactor in mind, which might end help with jack transport too, instead of using another highly coupled API, which led us into this mess in the first place. @firewall1110 sorry to say, but your implementation looks way too messy. Better to have a cleaner implementation with better interfaces. Not closing for now, as there's still scope for a jack transport implementation. |
@Rossmaxx , so I should better stop and wait? |
I would ask dom for that, I said it like that because introducing a new API on your own will very quickly lead to an unmaintainable mess, which is what led to the VST code being so messy. And your PR doesn't really look either. |
That said, I've got a feel at times that jack, being feature rich, has lots of complex features, implementing everything might mess up. It's out of scope, so I'll leave a better explanation of my idea in #1467 |
Quote: "Not closing for now, as there's still scope for a jack transport implementation." So this PR is not more actual for LMMS team. 10 days alert to close this PR. (Edited 2024.08.15) Canceled |
Please do not close this PR. <3 |
@tresf : P.S. |
Have you copied |
We do too! <3
Understood, this was not obvious. Please edit your original description to link to the PR which this supercedes.
Generally speaking, no, never do this. The original PR's copyright information and license must be kept. In some edge-case instances, a license can change but this is a bit off-topic here. The reason I asked to NOT close this PR is because this comment suggests that you were prepared to improve it: #6066 (comment). If this is no longer true, the team will respect your wish to close this PR. |
The problem is that there are very few developers and I guess many of them also work day jobs. Most/all developers work on this project in their spare time. This spare time is scarce and therefore it can take time until a PR is reviewed. One way to deal with this is to directly address people for a code review or to remind them if they have started one and potential changes have been incorporated with no progress afterwards. It's definitively not ideal and a hassle but that's reality. On the other hand, it's not like your code did not get any review or attention. It rather seems that you are unhappy with the results?
It should be okay to add new APIs. But they have to be discussed and worked out. It must be defined how LMMS interfaces with an API and vice versa. And if there is more than one potential implementation (as is the case for this PR) then this must be abstracted with some kind of interface. Doing so ensures that the actual implementations are confined in a contained space and not spread all over the code. It's what I have alluded to in my comment here: #6066 (comment) |
Quote: "It rather seems that you are unhappy with the results?" I asked question: "if the feature is wanted" I had no direct answer so I continue this on my own risk. But this: It seems I simply understand this wrong |
It seem's that something happens with MinGW build system ... |
You have two solutions at least.
|
By the way, if |
You are right, @michaelgregorius , I will change this in next commit.
Jack header should not be in ExSync.h : it is only made , using jack API. (+ moving JACK headers from ExSync.h to ExSync.cpp) @michaelgregorius ! @PhysSong ! Thank You for help! |
New plan in this PR (1) Now (in 1-st iteration) bad plan (3) Document and may be change names for "wires" [all after exSyncGetHandler() declaration] is completed. Next steps: All 1-st iteration done in ExternalSync.h/ExternalSync.cpp P.S.
If I by mistake add new PR - than "This is fate". |
Can we use |
(1) All changes make sense only if JACK support is compiled, but some parts are programmed as be placed in ExSinc.h/ExSync.cpp
(2) I use C style static functions and variables and violate LMMS coding style for them by prefix cs_ - this guarantee name collision never be happen.
(3) Introducing completely new thing can't be done perfect:
(4) When audio interface is not JACK , than ExSync button can't be activated and than clicked "Master" be cleaned. (GUI part should be improved but I hope it is not problem).
(5) When ExSync is not activated, in Slave and Duplex mode external messages are received but LMMS not react;
(6) When ExSync is activated LMMS react to external messages in Slave and Duplex mode , and send messages in Master and Duplex mode.
(7) ExSync activation/deactivation don't perform any synchronization or driver activation/inactivation : this seems as bug, but allow more freedom by playing with ExSync buttons .