audio redesign #2407
audio redesign #2407
Conversation
|
Continuing from #2400 (reply in thread) regarding ASIO SDK inclusion and modification:
We currently don't include (as in: distribute) ASIO SDK at all, as far as I can see. It is downloaded during build.
Hmm, maybe I'm confusing something, but Regarding minor fixes and replacements: I don't think we are allowed to do even that. We should really avoid copying that code into the Jamulus source.
I think this only means that only final releases of the SDK must be used for public software, doesn't it? |
But it's code is included in the build right?
Yes! I forgot about that one! I removed the copyright header there since there where quite some changes to the file (And the License agreement explicitly tells not to refer to Steinberg in modified code.). Maybe I should include a "based on" disclaimer?
The main part of the ASIOSDK2 is "example" code, so at least we should be allowed to change that code.
I agree that the License agreement is not quite clear, but as far as I can see the restriction on changes only apply to redistribution of the SDK itself. |
Yes, that's my understanding as well. We currently haven't got a licence from Steinberg (which I'm hoping will be easy enough to get), and we'd need to comply with a couple of minor things in their terms once we do. But right now I don't immediately see a problem. The relevant clauses in their agreement seem to be 3, 4 and 5. BTW I'm also assuming none of this prevents us from distributing Jamulus under the GPL, since ASIO would be a proprietary or GPL-incompatible "library" and thereby covered either by the system library exception or requiring an exception statement to be added. But we can look at that detail later. |
|
Not related to the licensing: The CI fails.
|
…ADME.MD) and Replaced ASIOSDK2 by own vesion in the windows folder (Copied generic ASIO headers from ASIOSDK2, created new asiodrivers.cpp/h and modified sound.cpp/h and jamulus.pro file accordingly).
Done.
Please clarify. What Powershell scripts for which automated Binary compilation? The JamulusWin files are just added for building in Visual studio during development. The normal autobuild scripts should still work too... |
|
Not sure: the style check has failed again. https://github.com/jamulussoftware/jamulus/runs/5264558226?check_suite_focus=true Let‘s wait until the autobuild scripts are run. You‘ll be able to check what failed if you click on the Checks tab in your PR 😀 |
|
It seems to me as if these changes weren’t enough (but I have the feeling that there was an improvement: https://github.com/jamulussoftware/jamulus/runs/5264767533?check_suite_focus=true) |
|
@pgScorpio I started drafting a reply to your points, but realized that we might get lost in the details. I agree with some of your interpretations of the licensing situation, yet it will leave uncertainty. Therefore I want to go back one step again: Why do we have to change the way we include ASIO SDK? Does it fix an actual bug or issue, if so, which part of the change? |
Strange indeed... Somehow "clang-format -i windows/*.h" missed something, since a clang-format with the Visual Studio LLVM Even stranger... The remote check seems to do strange things with pointer parameters: changed to: So sometimes changing 'type* name' to 'type * name' but not consistant! So how to overcome these differences in remote and local checks? |
As a whole: (Without getting lost in details again.) And finally. This is the whole idea of my PR! Getting the whole sound handling throughout the software implemented in a well structured manner, so maintenance becomes a lot easier and less error prone. |
Is this done in code based on the example from steinberg? |
| JamulusWin.* files for builing the windows version using visual studio 2019 (by pgScorpio) | ||
|
|
||
| #================================================================================================== | ||
| # WARNING: | ||
| # The vcx project files in this folder are heavily modified to make them movable. | ||
| # DO NOT REGENERATE VCX FILES FROM Qt! | ||
| # When the Jamulus pro file is changed, the vcx files need (often manual) editing too! | ||
| # | ||
| # In Visual Studio NEVER use ABSOLUTE PATHS but always use the appropriate variables. | ||
| # In Visual Studio NEVER use PERSONAL PATHS but always use appropriate environment variables. | ||
| # | ||
| # Do NOT directly open the JamulusWin.vcxproj or JamulusWin.sln files by double-clicking ! | ||
| # ALWAYS start Visual studio using this script from Powershell using./JamulusWin.ps1 --startvs | ||
| #================================================================================================== | ||
|
|
||
|
|
||
| JamulusWin.ps1 is a PowerShell script to setup the environment needed for builing with Visual Studio. | ||
| JamulusWin.ps1 also handles all Custom Build Steps during building of the project. (Modify when needed.) | ||
|
|
||
| Run ./JamulusWin.ps1 without parameters for command options info. | ||
|
|
||
| You should check the variables defined in JamulusWin.ps1 to reflect your Qt installation | ||
|
|
||
| You can modify the 'User Defined functions' in JamulusWin.ps1 if needed. | ||
|
|
There was a problem hiding this comment.
This should go into COMPILING.md if we merge the vs files.
| # Visual Studio Tool Script by pgScorpio | ||
| #================================================================================================== | ||
| Add-Type -AssemblyName 'PresentationFramework' | ||
| #================================================================================================== |
There was a problem hiding this comment.
Could you write a script which gets the files from qmake and modifies them?
Why should we include vs files directly in the repo? They should be EASILY generated by qmake - or a script using qmake. If there are manual steps, they should be documented in COMPILING.md
There was a problem hiding this comment.
Could you write a script which gets the files from qmake and modifies them? Why should we include vs files directly in the repo? They should be EASILY generated by qmake - or a script using qmake. If there are manual steps, they should be documented in COMPILING.md
Of coarse anything is possible, but it would not be easy. Don't forget these files are in my repo because I use them for development and I want them under version control too. They don't need to be included when merging to master.
windows/asio.cpp
Outdated
| asio.cpp | ||
|
|
||
| asio functions entries which translate the | ||
| asio interface to the asiodrvr class methods |
There was a problem hiding this comment.
As mentioned by @hoffie
How can we avoid including code based on Steinberg's work? Could you at least show clearly what you changed (and why). We should still require the Steinberg code to be downloaded (this ensures we're compatible with Debian for example)
There was a problem hiding this comment.
As mentioned by @hoffie How can we avoid including code based on Steinberg's work? Could you at least show clearly what you changed (and why). We should still require the Steinberg code to be downloaded (this ensures we're compatible with Debian for example)
I already changed the code to include the ASIOSDK2 as before, but just using two headers from the common folder. Also I rearanged some of my code and changed the filenames so they don't match SDK filenames to avoid confusion.
windows/asio.h
Outdated
| //--------------------------------------------------------------------------------------------------- | ||
|
|
||
| /* | ||
| Steinberg Audio Stream I/O API |
There was a problem hiding this comment.
As above. This is COPYRIGHTED by Steinberg.
windows/asiodrivers.cpp
Outdated
| } | ||
| } | ||
|
|
||
| bool CAsioDrivers::loadDriver ( long index ) |
|
We should solve some generic questions around this before continuing... |
I totally agree on that !! As said before my time to spend on this is limited, and at the moment I've got the feeling I'm spending more time on these discussions than on coding. Also I notice it are basically the same questions that are asked again an again. Do we really understand the ASIO code and the Licence agreement? There are only 2 header files in the ASIOSDK that specify the ASIO version: iasiodrv.h that specifies the iASIO interface to the driver dll (function definitions). Updating to a new ASIO version would just mean replacing these two headers, and maybe some code change to our own code if we want to use any new functionality, but notice that the iASIO interface has never changed (at least not since 2008), just support for 64 bit systems and stream synchronization was added later (AFAIK by just adding things like enum values and struct definitions for 64bit values.) and all these years the ASIO interface always stayed backwards compatible. These two header files are almost unchanged, and can be fully reverted if necessary (Though the licence nowhere says you are not allowed to modify the files, as long as you don't state they are from Steinberg, but "based on ASIOSDK2") All other files are "selecting the current driver" (opening a dll and set a pointer to it's interface) and glue between the application and IASIO. Until now we literally used the sample code in the SDK (But you are not restricted to using the SDK examples at all, ) Filename similarities in the windows folder and the SDK are just because the way I did the redesign, so these files can easily be renamed if we want. They have nothing to do with the SDK, nor with the ASIO specification, since they just, transparently, pass the iASIO interface. So please make some decisions first, then I will decide if I will continue with this PR or not. |
…from the SDK's common folder) Shuffled some code around and renamed files to avoid confusion with ASIOSDK files. Excluded ASIOSDK from clang. Excluded macOS build files from git.
|
I think the general consensus was that the current ASIO implementation is not optimal. changing the ASIO interface would be good. However your changes look dangerous (license wise) and probably do not fulfill some unwritten Jamulus standards (You changed the Build process for developing locally via VS, but probably this should not be pushed directly to master. Better document it somewhere such that further contributors can do it on their own). @jamulussoftware/maindevelopers could you please comment on any decision how to handle these changes? |
Not only the ASIO implementation.... (macOS would need quite some redesign too.) But concerning ASIO I still don't understand what the real problem with ASIO is. Are we obliged to use ALL the (example) code in the SDK?? Did anybody ask Steinberg if it is OK to only use the necessary dll interface header files from the SDK?
The modified VS project files where just added to my personal repo for personal use, they where never intended to merge to master...
The idea was to get a more uniform implementation of all sound code to prevent platform specific bug/impossibilities (As there seems to be a lot.), but this is very hard if we have to stick with the current implementation for ASIO. Again I don't see why we have to use a 'spaghetti' example code... Almost every SDK contains additional example code, but it is hardly ever used as-is. Normally you only use the library headers, and this is exactly what I do with the ASIO SDK.
'improving the current code' is exactly what I intended to do, but this is very hard if a lot of people are reluctant to change any code at all (and there should be BIG changes!). So perhaps indeed I better close the PR?
Yes any advise is welcome. I have a lot of ideas (Based on over 30 year experience), but do you still want me to contribute? |
That would be nice !
At this moment the only CSound implementation that is not yet tested at all is iOS. Though I did adapt the iOS files to match the new structure as much as possible, but I have no clue on how to compile them, so debugging is also impossible, and so I didn't commit my last changes yet. As far as all the other OS'es concerns we maybe could already merge them (At least they are already working, but may need the finishing touch on error messages and other gui texts).
Yes, I agree, but please test the result and do not try to check every changed line, since too much has changed, it's a re-design!
My idea too ;=)
I think this redesign is not yet ready for 3.9.0 and since it is a major redesign. I want it thoroughly tested first, and I think it would be logical if it becomes a 4.0.0 than.
As mentioned before, this is a major change in the framework that affects CSoundbase, CSound of all OS'es and also CClient and it's dialogs. So I'am afraid there is no way to submitting individual buildable parts. Concerning the sound redesign it's all or nothing I'm afraid (at least the first step). |
Concerning the autobuild scripts: they aren‘t designed to run locally. In order to run them on GitHub, you must push to a branch named like this: autobuild/sound-redesign
Yes sure. It’s fully alive and people use it with success. You can however rely on others testing it or the simulator.
I‘m afraid that this means your test device isn’t suitable for testing the current Android version (do you have a tablet? VM?). The mobile GUI isn’t well adapted, but that’s another issue.
Before merging at least one person of the maintenance team MUST fully understand the changes. We need to ensure new code is understood such that it can be changed/explained to other contributors if necessary. Concerning the split out: Usually the probability of merging small PRs is a lot higher than big ones (see JSON-RPC which took about half a year although it was 99% finished). Some even don’t get merged. So I think it’s worth splitting it in mergable, independent parts (maybe you can find some)? Moreover I suggest you to quickly read a bit about git rebase, how to handle conflicts and how to use branches if you don’t feel comfortable with git yet. |
It seems like you've changed your fork's repository name. That's not necessary and might be rather confusing. The important part is changing the branch name (i.e. sound-redesign -> autobuild-sound-redesign; @ann0see suggested autobuild/sound-redesign, that would work as well).
I completely understand that sentiment (I've been through larger refactorings/redesigns both in Jamulus and outside of Jamulus). At the same time, the Jamulus code base has grown with bugfixes of several years. Testing is surely important, but even before that we should also be very certain which part is changed for which reason in order to avoid removing existing bugfixes/workarounds with no replacement. I agree that splitting refactorings/redesigns into parts is a hard, maybe near-impossible task. At the very least we should try to split the actual refactoring from intended changes (bug fixes, new features or even GUI re-designs; that's probably what you've meant by "at least the first step"). If that's not sanely possible in some parts, that'll likely be OK, but it still needs a detailed description what has been done for what reason in the commit messages.
That's good to now. At the same time my fear that Jamulus development and sound-rebase development may diverge further, making it even harder to review/diff/avoid losing other fixes. Github already shows 10 file conflicts at the current stage, so as @ann0see suggested, it's essential for you to rebase to current master regularly.
I understand that it's more work on the author side of things, but I fear there's no way around that.
If fixes depend on the redesign, that's fine, then maybe this could be a way forward:
(Purely anecdotal, feel free to skip: I've recently started adding support for armhf Debian builds. While doing so, I quickly noticed that the autobuild logic had grown to a point where it was hard to reason about. I immediately felt the urge to improve that, to refactor it, simplify it, rename it, etc. So I went ahead and did that. I did all of that in a single evening. I suspect that's how you've got started with the sound redesign as well. Then I realized that a diff of thounds of lines would not be acceptable for a PR which essentialy only adds another build target. Therefore, I went ahead and split those changes into individual PRs. First, I submitted fixes for the existing behavior, e.g. build parallelization and error checking. Then, I found a way to convert each platform individually and submited one PR per platform [this may not be possible in your case, though]. With this almost finished, I can now submit another PR to do further cleanups which haven't been possible earlier. This process has been taking several weeks now, but I'm more than confident that this was essential to get all of this accepted. The individual reviews also lead to discussions and improvements for parts which I didn't think about it. Would we have accepted this as a single huge diff ("because it works"), we would have missed all of this.) I'm writing such a detailed answer because I highly appreciate the work you do and because it would be a pity if it took too long to merge that so it everyone can profit from it. |
OK, I see. I reverted the repository name and, hopefully, now changed branch name to autobuild/sound-redesign correctly. |
I'm surely willing to give it a try, but I could use some more advise (and maybe some help too.). I know there are some changes/additions that can be merged without problem, but I'm afraid to get a lot of comments and questions why since they would hardly have any effect at that moment, but they would be just preparations for future steps and I don't wanna spend a lot of time explaining every step, since merging step by step would already take me a lot of extra time I don't really have, Also I have a lot of professional experience that taking small steps often leads to more bugs, because several steps would need workarounds that need to be removed again later, both giving extra chance on bugs. |
I can use some help with this one too. |
|
@pgScorpio The git parts of docs/TRANSLATING.md are also applicable to code development. They are quite detailed, so have a read through them and let us know what questions remain. |
I don't see an Action run on your repo yet. Maybe you have to enable actions manually once?
Yes, you'd need one branch per PR.
Personally, I think individual Issues make sense for two reasons:
For work where code has already been written, a PR with a proper description is sufficient in my opinion. |
|
@pgScorpio I might be able to help you a bit with git via a video conferencing tool (preferably in the evenings if time permits). However, my git experience is mostly based on what I learnt from contributing to this project, so it's not perfect at all. |
|
Help !!! I don't know what went wrong, renaming branch/repo issues or trying to rebase issues, but I completely lost all my local changes (my local folder completely reverted to master). Also, though I just renamed my branch, I now see 2 branches listed, one as sound-redesign and one as autobuild/sound-redesign, where sound-redesign seems to contain the latest changes that I committed today (after the rename!), Now I only can get my last committed files back by downloading the "sound-redesign" zip, but even cloning my own repo again (with github desktop) gets the master files again, not my own sound-redesign repo, and so I also I can't push any changes to my own sound-design repo again. So I'm completely lost now, not knowing what to do to get things working again... |
Any help is welcome ! |
|
I'm from the same time zone ;-). |
|
BTW: I've just back-upped the branch of this PR to my repo: https://github.com/ann0see/jamulus/tree/backup/pg/sound-redesign |
So I guess that should be alright? |
|
I think you'll need to do do a little clean-up in your repo:
Now. And only now. rebase your working branch. I'd suggest to delete the autobuild/sound-redesign branch via GitHub UI first. (do this by clicking on branches and then there should be a delete button). Afterwards do the following:
Now follows the difficult part (the actual rebasing process):
|
Aaaaarghhh !!! Then followed your instructions by the letter. And again all changes lost !! I'll try again tomorrow.... |
|
Ok. So you didn’t Set up your ash key on GitHub. Replace [email protected]:user[…] by https://github.com/user[…] |
Or better set up you ash key on GitHub... ;-) |
Aha! Good point. I once did, but in the mean time I have a new PC and didn't think about the ssh keys, so I had to add a new key on github :=(( |
|
Got further now, but now I get: EDIT: @ann0see Could this be because the PR is closed ? Maybe that also explains why autobuild does not kick in ?? |
|
No. I doubt it. also this PR isn’t from your master branch. I think you need to push via SSH to have full access. |
?? I just used the commands you gave me. Aren't they all using SSH then ? |
|
It seems as if it’s not the case for origin: Conveys that you’re using https (maybe only for origin not for the upstream remote. You‘ll need to update the origin remote: https://pandammonium.org/how-to-change-a-git-repository-from-https-to-ssh/) |
Short description of changes
Redesign of all sound handling for different platforms as discussed here.
The user interface should be the same for all platforms as much as possible.
No sound related platform dependent code in CClient. (Also all interface related settings should eventually be handled in CSound).
Context: Fixes an issue?
Could also fix: #1378, #2344, #2238, #2496, #2519
Also better solution for: #2168, #2215
Does this change need documentation? What needs to be documented and how?
A new sound interface (CSoundBase) should be well documented as this PR evolves.
Status of this Pull Request
Work in progress.
Initial stage (Replacing ASIOSDK2 for Windows) could already be merged after reviewing and testing.
A lot of following steps still to be taken. See the discussion.
What is missing until this pull request can be merged?
Merging should be done in steps, as will be discussed in this PR.
Checklist