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 with Jack backend #7567

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mmeeaallyynn
Copy link

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

Based on #5990

@JohannesLorenz
Copy link
Contributor

@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 mmeeaallyynn force-pushed the sample-track-recording-jack branch from aab14d8 to f83a2f1 Compare November 24, 2024 11:11
@mmeeaallyynn
Copy link
Author

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

Copy link
Contributor

@JohannesLorenz JohannesLorenz left a comment

Choose a reason for hiding this comment

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

What I did:

  1. Functional code review
  2. Style review
  3. Testing review

Possible further reviews:

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

@@ -341,6 +343,11 @@ class LMMS_EXPORT AudioEngine : public QObject
AudioDevice * tryAudioDevices();
MidiClient * tryMidiClients();

const AudioDevice* audioDev() const
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggesting to put the whole function in one line to save space (at least the coding conventions allow it).


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

Choose a reason for hiding this comment

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

Can you please explain exactly why this was necessary to be in this PR?

Copy link
Contributor

@JohannesLorenz JohannesLorenz Dec 29, 2024

Choose a reason for hiding this comment

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

Ah, this is mentioned in #5990 : #5990 (comment) . I still wonder if it could break anything else?

@@ -70,6 +70,9 @@ class SampleRecordHandle : public PlayHandle
PatternTrack* m_patternTrack;
SampleClip * m_clip;

// The offset from the start of m_track that the record has
// started from.
TimePos m_startRecordTimeOffset;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
TimePos m_startRecordTimeOffset;
const TimePos m_startRecordTimeOffset;

@@ -286,11 +298,17 @@ void AudioJack::renamePort(AudioPort* port)
}


int AudioJack::setBufferSizeCallback(jack_nframes_t nframes, void* udata)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if this is allowed to do allocations? I just ask because some callbacks must happen in realtime-context and thus are not allowed to do slow things like new[].

My guess is that realtime-safety does not matter when a buffer size changes, but I could not tell from the docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to Perplexity AI it is called in a non-realtime thread. However, I wasn't able to find that information in the pointers it gave either. 🤔 So it should be taken with a big grain of salt.

On the other hand, it would be strange if that information was communicated on the realtime audio thread because it is very likely that clients need to allocate memory as well if that event happens.

Perplexitiy blurb Yes, the callback that informs about changed buffer sizes in the JACK audio API is called outside of the audio thread. This callback, known as the buffer size callback, is executed in a separate non-real-time thread[1][2].

The buffer size callback is registered using the jack_set_buffer_size_callback function. It is called whenever the size of the buffer that will be passed to the process callback is about to change[1]. This callback is designed to allow clients that depend on knowing the buffer size to prepare for the change before it occurs.

It's important to note that:

  1. The buffer size callback is not called from within the real-time audio processing thread.
  2. The normal process cycle is suspended during the execution of this callback, causing a gap in the audio flow[2].
  3. Because it's not in the real-time thread, the callback can perform operations that are not real-time safe, such as allocating memory or touching previously unreferenced memory[2].

This design allows for more flexibility in handling buffer size changes without interfering with the real-time audio processing thread, which has strict timing requirements.

Citations:
[1] https://jackaudio.org/api/group__ClientCallbacks.html
[2] https://jackclient-python.readthedocs.io/en/0.3.0/
[3] spatialaudio/jackclient-python#81
[4] https://docs.rs/jack/latest/jack/struct.Client.html
[5] https://stackoverflow.com/questions/78429446/which-thread-handles-a-realtime-callback-in-jack-audio/78882670
[6] https://linuxmusicians.com/viewtopic.php?t=22620
[7] https://devforum.play.date/t/c-api-sound-callbacks-happen-on-another-thread/4414
[8] https://www.rncbc.org/drupal/node/2162

// disable the record buttons.
if (!Engine::audioEngine()->captureDeviceAvailable())
{
for (auto &recordAction : {m_recordAccompanyAction, m_recordAction})
Copy link
Contributor

Choose a reason for hiding this comment

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

I was able to test with the "m_recordAccompanyAction" (I guess that's the record button with the play sign), but when I pressed m_recordAction (I guess that's the record button without play sign), just nothing happened - the cursor did not move and nothing was recorded.

Do you know why? What steps are required to make any use of m_recordAction?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, this is also mentioned in #5990 .

@JohannesLorenz
Copy link
Contributor

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.

{
auto thisClass = static_cast<AudioJack*>(udata);
delete[] thisClass->m_inputFrameBuffer;
thisClass->m_inputFrameBuffer = new jack_default_audio_sample_t[thisClass->channels() * nframes];
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider to use an std::vector<jack_default_audio_sample_t> that's simply resized in this block. This would also spare you the manual deletion in the destructor and would thus make the code a bit more memory safe.

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.

3 participants