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

Sample Track Recording Stage One #5990

Draft
wants to merge 48 commits into
base: master
Choose a base branch
from

Conversation

Reflexe
Copy link
Member

@Reflexe Reflexe commented Apr 17, 2021

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.

@LmmsBot
Copy link

LmmsBot commented Apr 17, 2021

🤖 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"}

Copy link
Contributor

@IanCaio IanCaio left a 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?

include/Mixer.h Outdated Show resolved Hide resolved
include/SampleRecordHandle.h Outdated Show resolved Hide resolved
src/core/Mixer.cpp Outdated Show resolved Hide resolved
src/core/Mixer.cpp Outdated Show resolved Hide resolved
src/core/Mixer.cpp Outdated Show resolved Hide resolved
src/gui/SampleTCOView.cpp Outdated Show resolved Hide resolved
src/tracks/SampleTrack.cpp Outdated Show resolved Hide resolved
src/tracks/SampleTrack.cpp Outdated Show resolved Hide resolved
src/tracks/SampleTrack.cpp Outdated Show resolved Hide resolved
include/Mixer.h Outdated Show resolved Hide resolved
@PhysSong
Copy link
Member

My initial comments:

  • If I remember correctly, we concluded to remove shouldApplyMasterGain from pushInputFrames before.
  • I think it's okay to add more backends in this PR.

@Reflexe
Copy link
Member Author

Reflexe commented Apr 18, 2021

@PhysSong You are right. I forgot to remove it before pushing.
Hopefully will do it after work (in about 12h).
About other backends: I think we can first merge this, and then add pulseaudio and jack one by one. That will make testing more focused and wilp make review much easier.

@enp2s0
Copy link
Contributor

enp2s0 commented Apr 19, 2021

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.

@PhysSong PhysSong mentioned this pull request May 10, 2021
11 tasks
@sebageek
Copy link

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.
I had to search a bit for "how to actually record something", but after I found out that I could arm painted tracks for being recorded everything became clear. Maybe the o rec symbol could be accompanied by painting the track background orange/red so I know I'll record over it on the next run.

Things that worked for me:

  • saving and loading (without saving resources) - sample tracks were there on load
  • recording, cutting, moving, duplicating sampletracks

Things that did not work for me:

  1. Using the "Record samples from audio-device" button - It does nothing for me.
    No recording or anything will happen. I can only record audio using the "record while playing song / BB-Track" button. Am I holding it wrong?

  2. saving "with resources" (click in save dialog) breaks the saving process
    Directories will be created, but save will be aborted. The error seen on the console is as follows:

QFile::copy: Empty or null file name
ERROR: Failed to copy resource

I think lmms tries to save the sample track, but it has no associated file name. See DataFile::copyResources() in ./src/core/DataFile.cpp.

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!

@Reflexe
Copy link
Member Author

Reflexe commented May 17, 2021

@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).

  • The second issue was very simple, I will open another PR for the fix.

  • The first issue, however, IDK really know how to solve because I don't really know what is the meaning of the button (Maybe we better just remove it?) I have no idea what could it mean. The only thing I could think about is to play the song like every other track has been muted (e.g. only record w/o playing anything). But that will require some heavy modifications of the recording code I am not sure are in scope for this PR. CC: @PhysSong . @Umcaruje ?

@sebageek
Copy link

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.

@PhysSong
Copy link
Member

PhysSong commented Jun 5, 2021

I think you should remove applyMasterGainToInputBuffer function as well, it's not used anymore after the removal of shouldApplyMasterGain.

@firewall1110
Copy link
Contributor

I have compiled this branch ...
Is real recording implemented or stage 1 means some preparations for this?

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 ...
Let's collaborate!

firewall1110 added a commit to firewall1110/lmms that referenced this pull request Jul 2, 2021
@firewall1110
Copy link
Contributor

Now recording is working if merge this branch over my capture branch:

RecordingStageOne.mp4

Is this correct?

@Reflexe Reflexe force-pushed the feature/recording-stage-one branch from 4a62327 to c14a357 Compare July 31, 2021 18:35
@Reflexe
Copy link
Member Author

Reflexe commented Jul 31, 2021

Rebased on master, remove apply master gain related changes.
Left to apply styling issues. Will do it shortly.

@Reflexe Reflexe force-pushed the feature/recording-stage-one branch from c14a357 to 6158e76 Compare July 31, 2021 18:55
@JGHFunRun
Copy link
Contributor

I think at some point we should switch from set/clear record to a proper record button but that's for the future IG

@Reflexe Reflexe force-pushed the feature/recording-stage-one branch 2 times, most recently from d01abd2 to e5c795c Compare August 21, 2021 18:13
@JGHFunRun
Copy link
Contributor

@LmmsBot

@Mayravixx
Copy link

Mayravixx commented Sep 21, 2021

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

@DomClark
Copy link
Member

@Mayravixx That version doesn't contain the code from this PR - it's instead the latest master version (see the commit hash in the title bar - 8a9a2fa).

LostRobotMusic and others added 8 commits June 21, 2024 17:36
This noise generator doesn't work properly when multiple noise sources are being generated simultaneously.
…#7236)

## Add support for "factorysample:" prefix
Add support to upgrade files with `src` tags that are prefixed with "factorysample:".

## Fix "bassloopes" typo
Fix projects that still reference files with the typo "bassloopes" in their name.

The upgrade is implemented in its own method because it is unrelated to the BPM renaming even if it is technically very similar in its solution.

Introduce the helper method `mapSrcAttributeInElementsWithResources` which replaces the `src` attribute in elements with resources (samples and AFP) if it can be found in a map.
* removed debian folder

* removed debian entries from check-strings

* fixup verify script too
Fixes an issue where sorted arpeggios over multiple notes used a largely
unusable algorithm. piano-octave-arp instead of octave-arp-piano.

Fixes LMMS#6499
Fixes LMMS#4491
When launching the wine VST process various wrappers may be involved,
when they exit the VST process becomes orphaned. This breaks the
PollParentThread mechanism which is responsible for cleaning up
processes in case of a crash. Because of this 64bit VST process exits
prematurely, in other words 64bit VST is currently broken in a typical
wine configuration.

A solution suggested by Lukas W is to set the PR_SET_CHILD_SUBREAPER
flag which makes the kernel reparent such process to lmms and
PollParentThread then works as intended.

Co-authored-by: Lukas W <lukaswhl@gmail.com>
* Add architecture to macOS cache keys

* Only save Homebrew cache if lock file has changed
@messmerd
Copy link
Member

It seems to work without major issues.

However, there appears to be quite a bit of latency. Here's me tapping on my mic on each quarter note of the metronome (120 BPM) starting at the 2nd measure:
Screenshot from 2024-06-23 15-39-21

Is there anything that can be done about this?

The audio was also recorded in the left channel only rather than upmixed to stereo which might be more desirable.

@michaelgregorius
Copy link
Contributor

@messmerd, what are your audio settings? What latency is set there.

Also, to reduce any human latency: can you please try to record the metronome sound itself coming from the speakers?

@Rossmaxx
Copy link
Contributor

I tried to reproduce a crash but couldn't. I used to get a crash before but strangely it disappeared. Can anyone check if recording still crashes?

@Rossmaxx
Copy link
Contributor

If i get a confirmation, I'll merge in 2 days unless someone objects

@tresf tresf marked this pull request as draft July 17, 2024 17:48
@tresf
Copy link
Member

tresf commented Jul 17, 2024

As @StakeoutPunch observed and reported on Discord, this feature has a blocker:

While I doubt I have any right to block a PR, I think that storing the recorded data in the project file as base64 is not an appropriate implementation.

Some workarounds have been proposed:

  • Prompt user where to store file after a recording has been made
    --OR--
  • Automatically store recorded samples in samples directory
    --OR--
  • Implement "portable" project file format

Each come with some major drawbacks (usability, portability/clutter, effort, respectively).

@PhysSong
Copy link
Member

Implement "portable" project file format

Possibly related: #5735, #3981.

@michaelgregorius
Copy link
Contributor

Some workarounds have been proposed:

* Prompt user where to store file after a recording has been made--OR--

* Automatically store recorded samples in `samples` directory--OR--

* Implement "portable" project file format

Each come with some major drawbacks (usability, portability/clutter, effort, respectively).

Prompting users after each recording would be very cumbersome and should be optional if it is implemented.

REAPER stores recordings in a global, configurable folder if there is no saved project yet. The file names contain date and time and other things to make them unique. These files are kept until they are explicitly deleted.

It is possible to configure the behavior of REAPER, i.e. you can configure where recordings are stored for projects that have already been saved. This way it is possible to have every recording in the project folder from the get-go.

Most other DAWs provide several options with regards to working with files, e.g. audio files/samples, MIDI clips, etc:

  • References to files: if you pull in a file from anywhere in the file system it's only referenced. It stays at its original location and the data is streamed from that location.
  • Project consolidation: this is an action that users can choose. Files that are outside of a project folder are copied/moved to a location inside the project folder.
  • Portable project: this would collect all assets wherever they are located and put them into some kind of archive format, e.g. zip up everything.

@firewall1110
Copy link
Contributor

This #5990 and (Fix hang on exit with SDL2 audio backend) #6070
May be have hidden conflict:
now audio call back is always active, but after #6070 must be activated before recording (and turned off after)

…tage-one

Conflicts:
* src/core/audio/AudioSdl.cpp
…tage-one

Merge origin's master so that the SDL related changes are reflected.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Audio Sample Recording