Skip to content

Conversation

@JohannesLorenz
Copy link
Contributor

@JohannesLorenz JohannesLorenz commented Mar 15, 2025

This PR continues #5990 from Reflexe (and others).

The problem with #5990 was that we cannot make PRs against it (at least, no PRs on our upstream LMMS/lmms) and at least #7567 must be merged into this PR (possibly also #7627 or a follow-up of that branch).

Note: When this is merged, authorship must be preserved to Reflexe, so we probably need to squash on commandline (using --author=...) and then fast-forward the resulting squash commit. Other users should be added to the commit message as "Co-authored-by".


Note: #5990 already contains a long discussion. If you want to add a discussion comment, please check:

  1. Is it offensive or starting a flame war? If yes -> Don't comment
  2. Does the comment directly reflect to a specific code line? If yes -> Make a code line comment
  3. Otherwise -> Make a discussion comment, please keep it descriptive and - if possible - not too long ❤️

I will use this top post to keep track of all known issues that can not be attached to a specific code line.


Open issues:

  1. Record button has no effect: Sample Track Recording Stage One #5990 (comment) (not fixed by d8c2716 ?)
  2. Samples must be properly saved, possibly using adding sample exporter #7627 (PR by @szeli1 , review assigned to @JohannesLorenz )
  3. Sample Track Recording with Jack backend #7567 must be merged -> Already merged to master, was independent.
  4. AudioEngine::pushInputFrames is very likely not realtime-safe, since it calls requestChangesInModel, which uses a mutex. IMO not a blocker, since LMMS itself is not (yet), and being able to record non-realtime safe is better than not being able to record at all. From my tests, there was no notable latency.

Reflexe and others added 30 commits October 23, 2021 21:02
instead of hacking sampleBuffer ()->startFrame ().

That solves a bug with `startFrame ()` being negetive in recording some
cases.
Instead of hiding the record action buttons, disable them and indicate the issue on the tooltip.
…tage-one

Conflicts:
* src/core/SampleRecordHandle.cpp

Also fixed the setting of the font size in `SampleClipView::paintEvent`.
Move the recording symbol and the "Rec" string to the lower left of the
clip so that it does not overlap with the clip name if it is shown.

Make the "Rec" string translatable and increase the font size.
Up to now the SDL audio driver attempted to use the default recording
device. This might not be what users want or expect, especially since the
actually used device is not visible anywhere. So if recording does not
work for the users they have no way to find out what's wrong.

Extend the settings screen of the SDL driver with a combo box that allows
to select the input device to be used. Store the selected device name in
a new attribute called "inputdevice" in the "audiosdl" section of the
configuration file.

Use the information from the configuration when attempting to inialize
the input device. Fall back to the default device if that does not work.
Provide the setting "[System Default]" which instructs the SDL driver to
use the default device of the system as the input device. In the
configuration file this option is represented as an empty string. This
should play well with the current existing configuration of the users.
Let users configure the output device that's used by the SDL driver.
Code-wise the implementation is very similar to the input device
configuration.

Use a `QComboBox` instead of a `QLineEdit` for `m_device` and rename it
to `m_playbackDeviceComboBox`.

Rename `s_defaultInputDevice` to `s_systemDefaultDevice` because it is
used in the context of playback and input devices.
Make sure that labels are always shown by setting the row wrap policy of
the form layout to wrap long rows.
Rename "Device" to "Playback device" to make clear what the setting
refers to.
Introduce const expressions to get rid of repeated strings with a risk
of typos.
Make the option to record samples more prominent by providing a pixmap
button that is always shown and that can be used to turn recording on or
off. The corresponding widget which constitutes of the button and a label
is always positioned at the bottom left of the sample clip.

Technical details
------------------
The recording widget is built in the private method `buildRecordWidget`.
The method `adjustRecordWidget` is used to move the recording widget to
the correct position relative to the sample view. It is called after
construction and whenever the sample clip is resized (see `resizeEvent`).

Add the method `SampleClip::getRecordModel` so that the pixmap button can
be configured to act on it.
Only show the recording widget in the sample clip if the audio engine has
a device configured that can capture audio. For simplicity of the code,
i.e. no nullptr checks, the widget is always created but only shown if
capture capabilities are available.

Rename the context menu entry "Set/clear record" to "Toggle record" and
only enable it if capture is possible.

In `SampleClipView` the test for the availability of a capture device is
abstracted behind the helper method `recordingCapabilitiesAvailable`.

Technical details
------------------
Add the method `captureDeviceAvailable` to `AudioEngine` because clients
should not be concerned with driver details. Drivers should mostly be
used by the audio engine and therefore hidden to clients. Add a private
`const` version of the method `audioDev` so that `captureDeviceAvailable`
can be implemented in a `const` correct way.

Use `captureDeviceAvailable` in the constructor of `SongEditorWindow`.
Remove the recording button that was added to the sample clip view with
commit 48c4dce.

This commit simply reverts the aforementioned one.
szeli1 and others added 8 commits June 21, 2024 17:36
Fixes an issue where sorted arpeggios over multiple notes used a largely
unusable algorithm. piano-octave-arp instead of octave-arp-piano.

Fixes #6499
Fixes #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 <[email protected]>
* Add architecture to macOS cache keys

* Only save Homebrew cache if lock file has changed
…tage-one

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

Merge origin's master so that the SDL related changes are reflected.

// Skip empty resources. In some cases, there might be an
// alternative way of actually representing the data (e.g. data element for SampleTCO)
if (attribute.isEmpty())
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a more specialized solution to this which may not cause regressions in completely different areas?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copying my analysis from Discord here:

Usually, in LMMS, SampleClips contain only a file name, e.g. "test.wav". When you normally save an LMMS savefile, the saved SampleClip will just contain the string "test.wav", but when you save into a folder, you want the whole "test.wav" to be copied to the folder - so far so good.

Now, in this PR, sample files are not saved yet (this is where cssli's PR #7627 will come into play). So this routine tries to find the resource of our recorded SampleClip, but it does not exist, so it cannot copy any resource, which will lead to the error described in #5990 (comment) :

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

My conclusion would be that this piece of code can (and should) be removed after we have implemented saving of the SampleClips using csslis's PR #7627.

Thanks to @steven-jaro for helping with this issue.

@firewall1110
Copy link
Contributor

For me title
"Sample Track Recording Stage One (by Reflexe, LMMS/lmms continuation)"
is confusing.
I understand, that all remember name "Sample Track Recording Stage One".
But may be after ~month can remove this Stage One" :
"Sample Track Recording (by Reflexe, LMMS/lmms continuation)"

Or even should be find better title: de facto this PR only add UI to control recording in memory, implemented years(!!!) ago.

P.S.
Finally some progress!

Record button has no effect

I understand, that it is about removed button on clip "armed to record"
It is not about {Editor::}m_recordAction ...
(I am working with this m_recordAction in #7520 context: I need this button to enable - disable Audio Input ... This work is not yet ready to PR)

@AW1534
Copy link
Member

AW1534 commented Mar 18, 2025

Just got a BSOD running this branch. Can't really confirm it was even caused by LMMS (although the only other app I had open was discord), or provide logs so this probably means nothing, but I'm just putting this out there.

@steven-jaro
Copy link

steven-jaro commented May 23, 2025

Hi! So, a couple of weeks ago, it becane my goal to finish EVERYTHING that it is left to do from this PR so that the branch feate/recording-stage-one can be finally merged with the main branch. To summarize, there are 4 issues left to solve from this PR and are mentioned by Johannes Lorenz in the original post of this PR.
These are the plans I have to resolve every issue:

  1. The issue 1 is about the recording button not doing anything. This issue is basically solved in my PR Removed the first recording button from SongEditor as it has no use #7899, I am just waiting for reviews so it can be merged with the branch feature/recording-sage-one.
  2. I am currently working in issue number 4 in Real time safe recording with ring buffer stage one #7903 and I think I already have 50% of the code done. I belive I will have it done by June.
  3. Issue 2 only needs review and testing to be finished. szeli1 is the person working the most on this issue in PR adding sample exporter #7627, so if he needs any help to finish his PR, I would try to help.
  4. Issue 3 seems to be almost solved, it is being finished in PR Sample Track Recording with Jack backend #7567 by mmeeaallyynn. I think his PR also needs review and test to be merged with the master branch in this case.

In summary: Issue 1 (#7899 by steven-jaro), 2 (#7627 by szeli1) and 3 (#7567 by mmeeaallyynn) seem to be almost finished but need review and test. The issue that needs the most work to be done is issue 4 and I am working on it on #7903. If somebody wants to help me, joing the discord server and look for Audio-recording-state thread in #dev-only.

JohannesLorenz added a commit that referenced this pull request May 31, 2025
Enable the `AudioJack` to take recorded input and push it to the `AudioEngine`. This adds functionality for `AudioJack` which already exists for `AudioSdl` and `AudioPortAudio`. Note that sample track recording in the LMMS core is not completed before #7786 .

This PR also removes the reading and saving of the channel number configuration in several places as it will default to `DEFAULT_CHANNELS` all the time anyway.

Co-authored-by: Johannes Lorenz <[email protected]>
Co-authored-by: Michael Gregorius <[email protected]>
@michaelgregorius
Copy link
Contributor

michaelgregorius commented May 31, 2025

Once the recording feature is merged #7920 should be merged as well. It allows users to select and persist recording inputs for the Jack driver without having to use external connection management tools.

Edit: #7920 has been closed and superseded by #7919. That PR can also be merged before the recording feature as it brings the Jack driver setup to the same state the SDL driver already is in.

sakertooth pushed a commit to sakertooth/lmms that referenced this pull request Jun 1, 2025
Enable the `AudioJack` to take recorded input and push it to the `AudioEngine`. This adds functionality for `AudioJack` which already exists for `AudioSdl` and `AudioPortAudio`. Note that sample track recording in the LMMS core is not completed before LMMS#7786 .

This PR also removes the reading and saving of the channel number configuration in several places as it will default to `DEFAULT_CHANNELS` all the time anyway.

Co-authored-by: Johannes Lorenz <[email protected]>
Co-authored-by: Michael Gregorius <[email protected]>
@JohannesLorenz
Copy link
Contributor Author

FYI I merged master into this branch. This means that now, additionally to PortAudio and SDL, Jack is also a possible source for recording.

@michaelgregorius
Copy link
Contributor

For anybody wanting to (conveniently) test with Jack as the input source:

  1. Checkout the branch of this PR: git checkout feature/recording-stage-one
  2. Create a new temporary branch, e.g. recording-with-jack-dialog: git checkout -b recording-with-jack-dialog
  3. Add https://github.com/michaelgregorius/lmms.git as a remote and fetch it.
  4. Merge the branch https://github.com/michaelgregorius/lmms/tree/InputOutputComboBoxesForJackDriver into the temporary branch recording-with-jack-dialog.
  5. Compile and start the application.
  6. Open the setup dialog, select Jack as the driver. Select you inputs and outputs.
  7. Record.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.