Sample Track Recording with Jack backend #7567

mmeeaallyynn · 2024-10-27T10:50:12Z

This adds support for the Jack backend for recording Sample Tracks.

Based on #5990

JohannesLorenz · 2024-11-20T22:29:47Z

@mmeeaallyynn While the PR diff itself looks correct, the commit list (30 commits by different people) looks bad. Do you mind doing a git rebase master, followed by a git push -f? Another way would be a git reset master, followed by a git commit and a git push -f.

mmeeaallyynn · 2024-11-24T11:13:20Z

Thanks for the hint, I hope it looks good now.

JohannesLorenz

What I did:

Functional code review
Style review
Testing review

Possible further reviews:

Second functional review (optional)
Consolidation with other developers (as other developers were active here already)

include/AudioEngine.h

src/core/DataFile.cpp

include/SampleRecordHandle.h

src/core/audio/AudioJack.cpp

src/gui/editors/SongEditor.cpp

JohannesLorenz · 2024-12-29T19:09:43Z

This should possibly not be merged into master, but into #5990, though other devs should consolidate this decision. I also wrote a comment: #5990 (comment). Nonetheless, the comments can be fixed here, too.

src/core/audio/AudioJack.cpp

firewall1110 · 2025-02-16T09:27:59Z

This should possibly not be merged into master, but into #5990, though other devs should consolidate this decision.

I think, it is bad idea, that only confuse author:

how I understand, Sample Track Recording Stage One #5990 has blocker (recorded date are saved in project file - not acceptable ... );
Sample Track Recording Stage One #5990 is staled and also last PR (Sample Track Recording Stage One #5990) active developer is not more active (Farewell! #7659 , and Sample Track Recording Stage One #5990 is not in my open pull requests )

This PR should be reviewed (possibly - improved) independently (and this is hard - testing is near not possible).

P.S.
It was not so bad at 2024.12.29 ...

firewall1110 · 2025-02-16T12:02:39Z

To author:

If You decide to integrate #5990 into this (this would be adequate to PR title), You should complete SampleRecordHandle.h , SampleRecordHandle.cpp - enabling recording to file, not only in memory.

Only in this case You will be able save recorded date to audio file (not to project file - as it is done in #5990 )

You should also make comment in #5990 , if You decide to integrate #5990 into this PR.

P.S.
My opinion is, that You should only enable audio input in Jack driver and may be change jack driver configuration gui (see #7421 ) if this is not already done. But it is only my opinion.

JohannesLorenz · 2025-02-16T13:12:13Z

@mmeeaallyynn Can you please solve the comments and then close this PR and instead open a PR from your branch against the #5990 branch? Or do you not want to continue contributing to this topic and someone else needs to do it?

michaelgregorius · 2025-02-16T13:13:32Z

Sample Track Recording Stage One #5990 is staled and also last PR (Sample Track Recording Stage One #5990) active developer is not more active (Farewell! #7659 , and Sample Track Recording Stage One #5990 is not in my open pull requests )

While I am indeed not actively coding on LMMS anymore I'd like to point out that #5990 was not started by me anyway and that it rather was worked on collaboratively at a certain point. So me leaving should hopefully not really stall this PR.

I have added this comment though where I describe what I think stalls that PR: #5990 (comment)

JohannesLorenz · 2025-03-15T19:14:28Z

I changed the target branch to upstream (LMMS/lmms #7786 ), but this did not work too well. It looks like GH does a complete rebase and fails with that. It's probably better to open a new PR which starts at #7786 (feature/recording-stage-one) and simply adds #7786 . @mmeeaallyynn Can you please do that?

Edit: It might work just to git rebase upstream/feature/recording-stage-one (assuming the remote is named "upstream") and then git push -f.

JohannesLorenz · 2025-03-22T19:30:13Z

Edit: It might work just to git rebase upstream/feature/recording-stage-one (assuming the remote is named "upstream") and then git push -f.

I'll just try it out myself now...

JohannesLorenz · 2025-03-31T21:33:53Z

After this PR has been refactored to not contain the #7786 changes, I think it is really independent of #7786 . Actually, this PR just adds a feature for Jack that we already have for SDL.

So I think this should directly go from master to master - i.e. only add those 2 Jack files to master. This is atomic enough. Can anyone agree?

michaelgregorius · 2025-04-08T18:27:24Z

@JohannesLorenz, continuing here so that it does not feel like writing on the margin of a piece of paper. 😆

Quoting your comment from the commit comment:

@michaelgregorius I updated the code with ecd6a63. It looks like this code does not make any assumptions on the channel number (at least, not more than the previous code). I guess you don't require an assert like in your last paragraph?

The way that this could break is if there are not exactly two input channels, e.g. if there were three channels. That's because SampleFrame currently represents a stereo pair.

However, for the Jack driver it does not seem to be possible to get into this situation (at least via the GUI) because the number of channels cannot be changed to something else than two anyway. So the only way to break this would be to manually edit the configuration file so that it contains the number 3. In that case the channels method would return 3 because it gets initialized to that value in the AudioJack constructor. This in turn would break the []-operator of SampleFrame here:

https://github.com/mmeeaallyynn/lmms/blob/ecd6a63a0f89da09cbb0583b4d107bbd56b0e312/src/core/audio/AudioJack.cpp#L390

To guard against this it might make sense to add an assertion which asserts that the result of channels is 2.

src/core/audio/AudioJack.cpp

JohannesLorenz · 2025-04-28T20:31:29Z

So the only way to break this would be to manually edit the configuration file so that it contains the number 3. In that case the channels method would return 3 because it gets initialized to that value in the AudioJack constructor.

I don't even think that would work, since the config file value is clamped in AudioJack's CTOR.

Would you still suggest to add an assert? If yes, likely in the CTOR?

michaelgregorius · 2025-05-01T10:08:38Z

So the only way to break this would be to manually edit the configuration file so that it contains the number 3. In that case the channels method would return 3 because it gets initialized to that value in the AudioJack constructor.

I don't even think that would work, since the config file value is clamped in AudioJack's CTOR.

Would you still suggest to add an assert? If yes, likely in the CTOR?

Ah, you're right. It seems that for some reason I missed the clamping. Although my next question then would be: why is it even read from the configuration if it is clamped between [DEFAULT_CHANNELS, DEFAULT_CHANNELS], i.e. if it is always set to DEFAULT_CHANNELS anyway regardless of what is stored in the configuration? It seems that the configuration has no effect anyway and that the whole clamp statement can be replaced with simply DEFAULT_CHANNELS. Does the configuration value have an effect in some other place?

On a related note: what the intention of the spinbox in AudioJack::setupWidget::setupWidget if it cannot be changed and will always display the value of DEFAULT_CHANNELS? Does it add meaningful information?

IMO an assert should not be necessary as the code currently enforces a single value anyway.

JohannesLorenz · 2025-05-01T11:26:37Z

So the only way to break this would be to manually edit the configuration file so that it contains the number 3. In that case the channels method would return 3 because it gets initialized to that value in the AudioJack constructor.

I don't even think that would work, since the config file value is clamped in AudioJack's CTOR.
Would you still suggest to add an assert? If yes, likely in the CTOR?

Ah, you're right. It seems that for some reason I missed the clamping. Although my next question then would be: why is it even read from the configuration if it is clamped between [DEFAULT_CHANNELS, DEFAULT_CHANNELS], i.e. if it is always set to DEFAULT_CHANNELS anyway regardless of what is stored in the configuration? It seems that the configuration has no effect anyway and that the whole clamp statement can be replaced with simply DEFAULT_CHANNELS.

It earlier clamped between DEFAULT_CHANNELS and SURROUND_CHANNELS, and I was hoping you could tell me why you changed it 😃

Does the configuration value have an effect in some other place?

No, from what I checked.

michaelgregorius · 2025-05-01T12:01:44Z

Ah, you're right. It seems that for some reason I missed the clamping. Although my next question then would be: why is it even read from the configuration if it is clamped between [DEFAULT_CHANNELS, DEFAULT_CHANNELS], i.e. if it is always set to DEFAULT_CHANNELS anyway regardless of what is stored in the configuration? It seems that the configuration has no effect anyway and that the whole clamp statement can be replaced with simply DEFAULT_CHANNELS.

It earlier clamped between DEFAULT_CHANNELS and SURROUND_CHANNELS, and I was hoping you could tell me why you changed it 😃

Oops! 😅 The longer explanation can be found in #7156. The short story is that surround sound did not work/compile anyway so I removed it while introducing the SampleFrame class. Looks like I did this rather mechanically and did not realize that I could also simplify the code in the constructor while doing so.

Does the configuration value have an effect in some other place?

No, from what I checked.

In that case it could even be considered to remove the configuration for now.

michaelgregorius · 2025-05-01T19:21:59Z

@JohannesLorenz, I have pushed 3ab8cc5 which removes the reading of the configuration parameter, the clamping and which also removes the LCD spinbox which cannot be changed anyway.

A next step might be to make the setup dialog similar to the SDL dialog, i.e. to enable the users to choose the first and the second input from a list. I have started to implement something in that direction:

Bildschirmaufnahme_20250501_210151.webm

This would give users control over their inputs from within LMMS, i.e. they would not have to use external tools like qpwgraph or Helvum to route the device inputs to the two inputs that LMMS currently creates. The downside of that approach would be that they'd have to restart LMMS each time they want to use a different input.

The final step (and best solution) would be to have the equivalent of this combo box available for each individual track input. That way users could create for example eight audio tracks, set them all to record, set eight different inputs for them (all from within LMMS) and then record them all at once.

JohannesLorenz · 2025-05-01T19:26:52Z

Thanks. I appended a cleanup commit and triggered CI (the CI does not start automatically in this PR, probably because the OP is not from an LMMS dev).

This would give users control over their inputs from within LMMS, i.e. they would not have to use external tools like qpwgraph or Helvum to route the device inputs to the two inputs that LMMS currently creates.

Isn't the same question valid for the output? For example, I use bluetooth headphones, and when I remove them and connect them again, I have to use qjackctl to manually connect the headphones.

However, if the question is about output, too, then it might be out-of-scope for this PR.

michaelgregorius · 2025-05-01T19:42:26Z

This would give users control over their inputs from within LMMS, i.e. they would not have to use external tools like qpwgraph or Helvum to route the device inputs to the two inputs that LMMS currently creates.

Isn't the same question valid for the output? For example, I use bluetooth headphones, and when I remove them and connect them again, I have to use qjackctl to manually connect the headphones.

However, if the question is about output, too, then it might be out-of-scope for this PR.

Yes, the same applies to the outputs. Here the users should be able to directly select the outputs for the master channel.

JohannesLorenz · 2025-05-30T18:10:44Z

A next step might be to make the setup dialog similar to the SDL dialog, i.e. to enable the users to choose the first and the second input from a list. I have started to implement something in that direction

I was hoping you were doing this next step, because I have no idea what you have in mind 😄 Also, I still think this is independent of this PR, so I would rather prefer not to block this PR (which already has 2 approvals). Unless you think you can finish whatever you plan here relatively soon.

michaelgregorius · 2025-05-31T08:35:06Z

A next step might be to make the setup dialog similar to the SDL dialog, i.e. to enable the users to choose the first and the second input from a list. I have started to implement something in that direction

I was hoping you were doing this next step, because I have no idea what you have in mind 😄 Also, I still think this is independent of this PR, so I would rather prefer not to block this PR (which already has 2 approvals). Unless you think you can finish whatever you plan here relatively soon.

It is definitively independent of this PR and therefore I have now approved it.

There is now a branch which contains some first preparation with regards to the input and output selection via combo boxes. It can be found here: https://github.com/michaelgregorius/lmms/tree/InputOutputComboBoxesForJackDriver

It already allows you to select the input and output ports and stores the selection in the configuration. However, the stored values are not used yet during the initialization of the actual Jack driver yet. In the end the stored port names must be used to search if they exist in the list of ports that are currently present and the connection must be made if that is the case. Otherwise some fallback is needed.

I am not sure if I will finish this as I now really want to wind down my involvement in the project as announced some time ago in the discussions page. If the remaining work can be done quickly I might finish it but otherwise anybody else is free to use it as a starting point to finish everything off.

JohannesLorenz · 2025-05-31T09:45:03Z

Thanks for explaining.

I guess in this case, it's best to make a draft PR and copy your explanation into the PR's OP.

I will merge this branch in the next 12 hours and change the merge target from feature/recording-stage-one, to master like explained here. If anyone thinks this is wrong, scream now 😱

michaelgregorius · 2025-05-31T09:48:22Z

I guess in this case, it's best to make a draft PR and copy your explanation into the PR's OP.

I think I will create an issue which coarsely describes the goal and then create a WIP PR for it.

Also, refactor buffer resizing into one function, and avoid useless static method.

Remove the reading and saving of the channel number configuration in several places as it will default to `DEFAULT_CHANNELS` all the time anyway. Remove the LCD spin box which does not allow to make any changes to the channel numbers anyway.

michaelgregorius · 2025-05-31T13:35:30Z

Edit: I have just noticed the comment in the commit message which references #7786. The question would be how to deal with the extended dialog. Should I comment out everything with regards to recording, rebase on master and make a PR for master or should I keep it based on the recording branch?

@JohannesLorenz, I am a little bit confused now. Is this PR intended as a preparation for sample recording with Jack or was it intended to merge the full feature?

I have two branches now which add a dialogue that allows users to select inputs and outputs. I think one of the branches was based on this PR's branch and it has recordings functionality and one was rebased onto the current master and there is no recording functionality.

Here's how the dialog looks like:

I was able to record from one device and playback on another.

However, letting the user choose inputs does not really make sense if the feature is not implemented/merged yet. Are there any plans with regards to that?

JohannesLorenz · 2025-05-31T15:47:13Z

@michaelgregorius This was indeed just preparation, similar to SDL and PortAudio. You probably have 2 branches because I needed to force push this PR's branch due to the changed target branch - I guess you figured it all from the commit message :)

Right now, master needs to be merged into recording - feel free to do it, or I will do it in a few hours.Then, it is up to you: Either merge to master only with the outputs for now, or base on our recording branch with inputs and outputs.

I hope this helps with the confusion :)

michaelgregorius · 2025-05-31T16:57:14Z

@JohannesLorenz, yes it has helped with the confusion. 😅

I have now created #7918 with the accompanying PR #7919 because selecting the outputs from a list already works for master and brings benefits to the users.

I will create another PR which will reintroduce the input selection as well. That PR would then have to be merged once the recording feature is done.

michaelgregorius · 2025-05-31T17:12:43Z

The input selection branch is #7920.

sakertooth · 2025-06-01T06:50:50Z

Uhh, is m_inBuf even used? Unless we are planning to use it in the future?

michaelgregorius · 2025-06-01T07:26:39Z

Uhh, is m_inBuf even used? Unless we are planning to use it in the future?

Hmm, good catch. In current master the inputs are directly read from the Jack buffers into m_inputFrameBuffer:

lmms/src/core/audio/AudioJack.cpp

Lines 377 to 386 in 3f8b86a

    
           for (int c = 0; c < channels(); ++c) 
        
           { 
        
           	jack_default_audio_sample_t* jack_input_buffer = (jack_default_audio_sample_t*) jack_port_get_buffer(m_inputPorts[c], nframes); 
        
           	for (jack_nframes_t frame = 0; frame < nframes; frame++) 
        
           	{ 
        
           		m_inputFrameBuffer[frame][c] = static_cast<sample_t>(jack_input_buffer[frame]); 
        
           	} 
        
           } 
        
           audioEngine()->pushInputFrames (m_inputFrameBuffer.data(), nframes);

It it looks like m_inBuf can be removed.

sakertooth · 2025-06-01T07:37:51Z

It it looks like m_inBuf can be removed.

Cool, done in #7922.

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]>

mmeeaallyynn force-pushed the sample-track-recording-jack branch from aab14d8 to f83a2f1 Compare November 24, 2024 11:11

JohannesLorenz reviewed Dec 29, 2024

View reviewed changes

JohannesLorenz mentioned this pull request Dec 29, 2024

Sample Track Recording Stage One #5990

Closed

michaelgregorius reviewed Dec 30, 2024

View reviewed changes

src/core/audio/AudioJack.cpp Outdated Show resolved Hide resolved

JohannesLorenz mentioned this pull request Mar 15, 2025

Sample Track Recording Stage One (by Reflexe, LMMS/lmms continuation) #7786

Draft

JohannesLorenz changed the base branch from master to feature/recording-stage-one March 15, 2025 19:10

JohannesLorenz force-pushed the sample-track-recording-jack branch 2 times, most recently from ebc3ad4 to 75ea709 Compare March 22, 2025 19:47

michaelgregorius reviewed Apr 8, 2025

View reviewed changes

src/core/audio/AudioJack.cpp Show resolved Hide resolved

michaelgregorius approved these changes May 31, 2025

View reviewed changes

JohannesLorenz changed the base branch from feature/recording-stage-one to master May 31, 2025 09:48

mmeeaallyynn and others added 6 commits May 31, 2025 12:01

enable recording for the jack backend

0039089

Fixup merge error

5b7ef10

Make "m_inputFrameBuffer" a vector

bcba201

m_inputFrameBuffer: Use vector of SampleFrame

9690c7b

Also, refactor buffer resizing into one function, and avoid useless static method.

Jack: Remove channel number configuration

631fdef

Remove the reading and saving of the channel number configuration in several places as it will default to `DEFAULT_CHANNELS` all the time anyway. Remove the LCD spin box which does not allow to make any changes to the channel numbers anyway.

Remove further useless things

1900fc2

JohannesLorenz force-pushed the sample-track-recording-jack branch from 3ab8cc5 to 1900fc2 Compare May 31, 2025 10:03

Fix rebase error

58e7e8a

JohannesLorenz merged commit 352ef8c into LMMS:master May 31, 2025
11 checks passed

Uh oh!

Sample Track Recording with Jack backend #7567

Sample Track Recording with Jack backend #7567

Uh oh!

Conversation

mmeeaallyynn commented Oct 27, 2024

Uh oh!

JohannesLorenz commented Nov 20, 2024

Uh oh!

mmeeaallyynn commented Nov 24, 2024

Uh oh!

JohannesLorenz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JohannesLorenz commented Dec 29, 2024

Uh oh!

Uh oh!

firewall1110 commented Feb 16, 2025

Uh oh!

firewall1110 commented Feb 16, 2025

Uh oh!

JohannesLorenz commented Feb 16, 2025

Uh oh!

michaelgregorius commented Feb 16, 2025

Uh oh!

JohannesLorenz commented Mar 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohannesLorenz commented Mar 22, 2025

Uh oh!

JohannesLorenz commented Mar 31, 2025

Uh oh!

michaelgregorius commented Apr 8, 2025

Uh oh!

Uh oh!

JohannesLorenz commented Apr 28, 2025

Uh oh!

michaelgregorius commented May 1, 2025

Uh oh!

JohannesLorenz commented May 1, 2025

Uh oh!

michaelgregorius commented May 1, 2025

Uh oh!

michaelgregorius commented May 1, 2025

Uh oh!

JohannesLorenz commented May 1, 2025

Uh oh!

michaelgregorius commented May 1, 2025

Uh oh!

JohannesLorenz commented May 30, 2025

Uh oh!

michaelgregorius commented May 31, 2025

Uh oh!

JohannesLorenz commented May 31, 2025

Uh oh!

michaelgregorius commented May 31, 2025

Uh oh!

Uh oh!

michaelgregorius commented May 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohannesLorenz commented May 31, 2025

Uh oh!

michaelgregorius commented May 31, 2025

Uh oh!

michaelgregorius commented May 31, 2025

Uh oh!

sakertooth commented Jun 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michaelgregorius commented Jun 1, 2025

Uh oh!

sakertooth commented Jun 1, 2025

Uh oh!

JohannesLorenz left a comment •

edited

Loading

JohannesLorenz commented Mar 15, 2025 •

edited

Loading

michaelgregorius commented May 31, 2025 •

edited

Loading

sakertooth commented Jun 1, 2025 •

edited

Loading