-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Sample Track Recording Stage One #5990
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! 🎩
Linux
Windows
macOS🤖{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://output.circle-artifacts.com/output/job/a63b2a2d-f02b-4b83-8235-cdb92f29af95/artifacts/0/lmms-1.2.0-rc6.1231+gb2f3a1f86-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/Reflexe/lmms/1626?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/0f86efe9-e3ad-4d7a-9f38-5831d542b9d2/artifacts/0/lmms-1.2.0-rc6.1231+gb2f3a1f86-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/Reflexe/lmms/1624?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://output.circle-artifacts.com/output/job/c724e479-cedb-48eb-affc-ba13fa6ce6fc/artifacts/0/lmms-1.2.0-rc6.1231+gb2f3a1f86-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/Reflexe/lmms/1628?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/m58xebqefwmpqok0/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44344781"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/2b35vlj7jhr3ov4a/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/44344781"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://output.circle-artifacts.com/output/job/b8b29d25-eaf5-4b78-a156-eaec12fed67a/artifacts/0/lmms-1.2.0-rc6.1231+gb2f3a1f86-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/Reflexe/lmms/1627?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "b2f3a1f86e95abb4f1565b7be86e69187028d028"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still going to review this with more care when I get home, but decided to take a peak at it from my phone and did a first pass review just for fixing code style (stuff like explicit blocks for one line ifs, spacing, camelCase variable names). Added all as suggestions if you'd like to just batch commit them.
Is there any reason you decided to use a braced-init-list instead of the expression-init-list for those new members?
My initial comments:
|
@PhysSong You are right. I forgot to remove it before pushing. |
Fully agree with doing backends 1-by-1. SDL is a good start since it's available everywhere and is the default, so recording will work ootb for most users. |
I compiled and ran this PR today on Linux and it seemed to work pretty well for me. Used the SDL backend (with pipewire), recorded a bit of Guitar music. Things that worked for me:
Things that did not work for me:
I think lmms tries to save the sample track, but it has no associated file name. See Another feature that would be really nice is if I had a way to save the sample track I recorded with lmms (maybe I decide I need it as an extra wav/mp3/something or want to post-process it somewhere else). All in all: A big plus from my side! Looking forward to seeing this feature integrated in lmms once it's done! |
@sebageek Thanks for the feedback. I will look into both. In any case. feel free to open an issue about being able to save individual Samples (if there isn't one open already).
|
Hey @Reflexe, nice to hear that it was helpful && that the second issue is easy to fix! I also opened #6022 for the feature request. Regarding the first issue: The record button apparently dates back to a stub implementation in 2008 and is only now appearing as it checks if the current audio backend is implementing capturing. So, how about using this button for its original intention? It could just allow the user to record samples without any other track playing (essentially the same as the two record buttons in the piano roll). On the other hand while I could imagine this functionality useful.. if this is not needed I'd opt for removing the button. |
I think you should remove |
I have compiled this branch ... I am about to PR that fix Issue #4668 , but also changes some things how capture works in goal to enable input capture callback only if needed. Also see my questions on Discord ... |
Now recording is working if merge this branch over my capture branch: RecordingStageOne.mp4Is this correct? |
4a62327
to
c14a357
Compare
Rebased on master, remove apply master gain related changes. |
c14a357
to
6158e76
Compare
I think at some point we should switch from set/clear record to a proper record button but that's for the future IG |
d01abd2
to
e5c795c
Compare
Tried doing this today, sadly it isn't working for me on Arch. When I place down a sample section then right click it, I get no "set/clear record" option at all. Would be really simple if I could grab the exe's in some way but none of the .exe's can be downloaded, same story with the appimage file. 2021-09-21.17-07-36.mp4 |
@Mayravixx That version doesn't contain the code from this PR - it's instead the latest |
audioPort as nullptr by default.
"played" (recorded).
actual data starts after that.
instead of hacking sampleBuffer ()->startFrame (). That solves a bug with `startFrame ()` being negetive in recording some cases.
For final variant - yes. For stage one - no: only one thing be added - samples to sample track not only imported, but may be recorded too. My opinion: this PR problem is that stage one is treated as last one. Why it is problem? LMMS device drivers are not ready to final variant - but device drivers can not be improved without testing possible: are waiting for this stage one be in master.
So in this case there are no difference in complexity (only variant):
(All after I treat as off-topic)
I mean folder is in track configuration, so changes the file name pattern.
OK, but
In PR "Sample Track Recording Stage Five - Final" . P.S. |
I mean, there is a way to have audio recording working in most common audio backend used in LMMS, SDL. There is also proposed solution for saving recordings, without writing base64 blob into project file. Rudimentary, but it is. Other stuff, like recording in other audio API (JACK is already in another PR), autosaving, project folders, can be done later. |
OK, I compiled this PR (but only for my Linux Debian Stable 64-bit). It seems, that audio is recorded to memory and saved in project file as huge data="...." line . In this case #7627 is a workaround , but it should be tested. Now I will check, how Audio Recording is implemented ... P.S. |
@JohannesLorenz, I have just checked with Bitwig. In the project folder there is a sub folder called "recordings" where the files go that have been recorded with Bitwig. There's also a sub folder called "samples" where the samples go that come from somewhere else, e.g. samples that have been loaded from disk and which are used in a drum machine. Perhaps this is some nice compromise so that we still do not have any vinyls in our folders. 😉 |
It seems that #7627 only exports the content of sample clips into files, i.e. it will not replace the recorded data with references to the exported files. This means that the data would still be stored in unwieldy Saving the data as Example for several versions:
|
Sorry, I don't understand what you're trying to say. Can you please elaborate? |
I meant to say that relying on the auto-save feature might lead to losing lot of work if the auto-save is not "quick" enough, e.g. if you have done a lot of recordings between the last auto-save point and the next one that is not reached anymore due to a crash. If the recordings are streamed to disk then you will have at least all your recordings and might only lose part of some recordings if LMMS crashes during the recording. The auto-save files are also another example where storing everything in memory and then in |
I have just also checked how Bitwig handles projects which have not been saved yet. The are assigned a GUID as the "project name" and their data is stored in a temporary folder, e.g. the recordings are stored under I guess if you then save a project for the first time then all that stuff is moved into the actual project folder together with a newly created save file. |
The same accounts for the rest of the project file, too. I assume you say that it's more easy to create the notes and the instrument/effect tunings of the last 1 hour than the records of the last 1 hour? In this case, we could allow 2 autosave options (How often should records be saved? How often should the rest be saved). However, I am not against the proposal of saving it while recording - it's just more overhead to code that 😬 Possibly the audio thread would smash everything into a ringbuffer, and another thread fetches it and writes it on disk. I imagine it's more ugly to split the data into chunks and find a good naming for those chunks. At this point though, I am also not sure why we should split it... |
I would like to mention that I have made 3 PRs about audio exporting:
I would suggest moving the discussions about project folders to #7538 (since that pull request is a project folder implementation) |
It is 1-st thing, that should be implemented and it is surprise for me that it is not done yet ... P.S. But about question: "Where and how place recorded files?" |
Audio Recording implementation is not here, but already in master: And this implementation is already in lmms-1.3.0-alpha.1.102 |
So implementation with storing recorded data in project file is not acceptable by LMMS team. P.S. |
Where was this decided? I didn't hear about this |
There are ~149 hidden items, and one is: "tresf commented on Jul 17, 2024 •" P.S. |
I implemented 2 out of 3 ideas mentioned here. I have made 3 PRs that allow audio exporting for this PR.
Yes, this seems to be true. I don't see why this was mentioned after 5 months, but it is a good reminder. |
So it is deadlock like situation:
May be (manual) this PR integration into one of Yours , @szeli1 , PR be a solution? P.S. |
Also it seems some of this PR changes are also in #7567 ... |
To me it looks like this PR is stalled because it tries to do too many things at once. There are at least two big tasks in making recording available:
The general flow/loop in an audio driver would then look as follows:
This PR is concerned with both tasks described above whereas #7567 is attempting to make (one) input buffer available via the Jack driver. The first task would be responsible for making all available inputs available to LMMS (so that users do not have a make a single rather limiting choice when setting up the audio driver) and the second task would be responsible for extending LMMS so that it can make use of them. It would likely make sense to work on these tasks separately and to work on them in a bottom-up way in a collaborative effort. |
Does it?
Can you explain (in a few words, please) why this isn't already functioning in
Well, @sakertooth just removed this capture support in PortAudio #7444, quoting "... input device is useless as of right now ..." so I agree that we're -- as a collective -- limiting choice, but I can't see anything in this PR specifically that is "limiting choice when setting up the audio driver". The code reads pretty backend agnostic and touches a minimal set of files. When this was discussed on Discord, the hold-up was the storage of the recorded data, as pointed out in several comments above. That's my high-level understand of it, happy to be corrected (preferably in a few words, please). |
IMO it does because it seems to build on several things that are not fleshed out yet, like for example:
Only the SDL driver seems to return If I understand correctly there is already rudimentary support for multiple inputs and outputs with the I also was not able to find any usage of
My comment with regards to "make a single rather limiting choice when setting up the audio driver" was about the current state of the SDL driver. If you activate and configure it then you can select a single input in the dialog. It is limiting in that I can only use one input and have to decide which one to use upfront. The option is also in the wrong place. The driver should make all inputs available and users should be able to select them in the application and not in some preferences/configuration screen.
My understanding is that it is indeed one of the open questions and if it is solved then the state will be that only the SDL driver will allow the recording of audio from a single source that has to be configured in the driver configuration screen and that therefore cannot even be switched without restarting the application. So it would be very rudimentary support that's not very comfortable for the users. However, it would at least be a step in the direction and might highlight the places that need work. P.S.: I hope I did not use too many words. It reminded me of how I sometimes tell ChatGPT to be concise. 😅 😉 |
I've only ever been able to use a single output device with SDL so I'm not sure how that's relevant, but assuming the rest is, can you please make specific recommendation? Also, can you chime in on #7444 and tell @saker to stop removing input devices ? (Edit @sakertooth confirmed in #7444 prior to writing this that the input devices will be staying). |
If you have several inputs you will find them listed in the combo box on the SDL driver's configuration screen. They are enumerated when the driver is initialized. I also assume that it is possible to process all of them during the processing done in the audio driver loop. So my recommendation is to devise a concept for how to forward the different input buffers to the application, e.g. as described here: #5990 (comment).
Unfortunately I am not familiar with that PR and only did a very quick scan as I now want to wind down my involvement in the project to an absolute minimum. The way I understand it is that the handling of the inputs caused some problem in the implementation of the PortAudio driver. If it can only be solved by removing the input handling then this might point to a problem with the greater design revolving around audio devices, inputs, etc., i.e. the design might not work for all different ways audio driver APIs might work. So removing the input handling might be feasible until the bigger problem is solved. IMO it should be well explained and documented though why they absolutely must be removed and what's exactly causing the problem. Otherwise it is just taking stabs in the dark and a potential back and forth that leads nowhere. So I cannot reliably tell if it is wrong to remove the input handling. |
Oh, I didn't get that impression at all. Anyway I'll leave the conversation to be discussed over there.
Hmm, I was under the impression that only the default output device was selectable. That's how it was when testing #7444.
So is the argument here that we need to be able to record from multiple inputs without restarting before this can be merged? If so, I disagree.
I'll try to avoid roping you into new PRs, but I thought it was extremely relevant to your point. |
For the SDL driver it should look something like shown here: #5990 (comment) #7444 is about the PortAudio driver though and its configuration screen might look different from the SDL driver's.
No, that's not my argument. As stated here an "incomplete" merge might point to the places where work is needed even if it would offer rather rudimentary functionality: #5990 (comment). However, I believe that "the perfect is the enemy of the good" so it would be counter-productive to wait with a merge until everything is implemented perfectly. It's a trade-off with regards to what we want to present to the users though. It should be somewhat polished, doesn't need to be perfect and should not make the users run away. 😅
Thanks, I appreciate your consideration. I just happened to notice that being involved in lots of discussions would run counter to my farewell. 😅 |
I have now unsubscribed from this issue due to the aforementioned reasons. Just so that nobody wonders why I won't react anymore although I have been quite active in this issue before. I wish you good luck with getting this feature into a good state for the users! 🙂 Edit: @tresf, why did you downvote this? I consider it good etiquette to not just vanish without any explanations from threads where I have been quite active. Obviously I will not do this everywhere. |
This branch needs to be hijacked to a master branch (no disrespect to the author). The reason is that we have already 3 PRs (#7567, #7538 and #7627) that must be merged into this branch, but we don't want to make PRs in @Reflexe 's fork (because we will not see them and we may not even have permission there). @Reflexe are you OK with this? Can you please push your branch to LMMS/lmms and open a new PR there? Otherwise I would just do it myself (the commit authors will remain unchanged). Note: A rebase could be done at the same time to fix the merge conflict. |
recording.mov
This is the first stage of the Sample Track Recording feature and was intended to only work on one backend (SDL) with no fancy features like looping and visualization.
It has been tested very quickly on my setup with a monitor interface.
Note: the changes are from #4994.