-
-
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 with Jack backend #7567
base: master
Are you sure you want to change the base?
Conversation
@mmeeaallyynn While the PR diff itself looks correct, the commit list (30 commits by different people) looks bad. Do you mind doing a |
aab14d8
to
f83a2f1
Compare
Thanks for the hint, I hope it looks good now. |
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.
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)
@@ -341,6 +343,11 @@ class LMMS_EXPORT AudioEngine : public QObject | |||
AudioDevice * tryAudioDevices(); | |||
MidiClient * tryMidiClients(); | |||
|
|||
const AudioDevice* audioDev() const |
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.
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()) |
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.
Can you please explain exactly why this was necessary to be in this PR?
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.
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; |
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.
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) |
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.
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.
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.
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:
- The buffer size callback is not called from within the real-time audio processing thread.
- The normal process cycle is suspended during the execution of this callback, causing a gap in the audio flow[2].
- 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}) |
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 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
?
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.
Oh, this is also mentioned in #5990 .
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]; |
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.
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.
This adds support for the Jack backend for recording Sample Tracks.
Based on #5990