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

Add sample track recording support #4531

Closed
wants to merge 91 commits into from
Closed

Add sample track recording support #4531

wants to merge 91 commits into from

Conversation

PhysSong
Copy link
Member

@PhysSong PhysSong commented Aug 9, 2018

This is the continuation of #3947 which has done by @Reflexe. Due to the merge conflict and some more reason, @lukas-w have rebased the original commits and pushed a new branch feature/recording.
You can contribute to this work by pushing commits to feature/recording or opening a pull request against this branch.
Thanks to @Reflexe for the work, @lukas-w for rebasing commits and many reviewers!

Bugs

  • TripleOscillator preset previews show errors and don't stop due to nonexistent samples/empty.wav
  • w2_invsinehalf.flac has a furry tail in BitInvader due to resampling(maybe an issue with Graph?)
  • Visualization gets weird when resizing tracks while recording

Enhancements

  • (optional) Add more backends
    • ALSA
    • CoreAudio
    • Sndio
    • libsoundio
  • (optional) Refactor
  • Decide when to resample data
  • Optimize

@Reflexe
Copy link
Member

Reflexe commented Aug 9, 2018

About the first one, I'm not sure how to do it. I think that the best way would be to just have an empty.wav. However, the current way of loading audio files returns empty data as a sign of failure.

About the second bug (re-sampling), would it be better to just drop automatic resampling (resampling higher to match the current output device) and stick with resampling on-the-fly?

About the third, I think I should implement an attribute for every track that "locks" it from the user. Track that's being recorded or manipulated in some way should not disappear (removed), resized or moved to another track, copied and so on.


Edit:

Fixed first issue here: #4536

I could not verify the second and the third issue for the latest feature/recording, I think i've fixed that already . It currently looks good to me. @karmux Could you verify it? Thank you.


About adding more backends: I'm not a fan of that idea since I've already tried it with PulseAudio and SDL and their behaviour were not consistent and had bugs so I've disabled them both in a few platforms. I think that focusing on one backend would help us debug and make sure we release a stable code.

Edit @PhysSong: removed old download links.

@PhysSong
Copy link
Member Author

would it be better to just drop automatic resampling (resampling higher to match the current output device) and stick with resampling on-the-fly?

That's what we used to do. Resampling also makes some people can think LMMS hangs when exporting.

I think that focusing on one backend would help us debug and make sure we release a stable code.

Since this pull request targets to the development branch(unusual name master though), I think we don't need to concern about stability issues now.

@LmmsBot
Copy link

LmmsBot commented Sep 5, 2018

Downloads for this pull request

Generated by the LMMS pull requests bot.

@PhysSong
Copy link
Member Author

Reminder: 18a7f28 and 88dca57 should be dropped once #4586 gets merged.

@Reflexe
Copy link
Member

Reflexe commented Feb 21, 2019

Do we want to merge it? I don't want this work to go to waste.
(All of the bugs has been fixed)

@zonkmachine
Copy link
Member

(All of the bugs has been fixed)

Great! So what's left to do? Adding more backends and optimization?

@Reflexe
Copy link
Member

Reflexe commented Feb 24, 2019

Great! So what's left to do? Adding more backends and optimization?

IIRC, I worked a lot to optimize the recording and adding more backends is not that useful for now (Alsa is very different that Pulse and modern APIs).
The only thing left to do is maybe make the arming button more friendly (maybe a led, like the mute button).

@PhysSong
Copy link
Member Author

I'll rebase commits against current master to produce newer test binaries.

SampleBuffer.

That solves problem with recording that caused samples to be moved and
messed up.

Thanks @-BaraMGB for helping.
Previously, begining record in any position would result the data being
in the end of the TCO.

@see #3947 (comment)

Thanks to @-BaraMGB :)
`updateMenu`, use a virtual function overriden by every Track
implemetation.
input channel configurations. And support recording of one channel and
converting it to two.

The user would be able to set their input config per track, or globally.
For each recorded sampletrack, the recorded will get the channel config
and will duplicate the samples from one channel to the other in a case
of mono input.
options unchecked will be considered as global default.

Suggested by @-tresf, Thanks!
lukas-w and others added 18 commits March 11, 2019 16:58
Fixes regression from f765773 (presumably
a copy/paste mistake)
Fixes a regression in 84a1f35.
Fixes a regression in 7bc0056
that breaks showing file names.
* Enable WeakJACK only if JACK is available.

* Try `pkg_check_modules` first when using WeakJACK, too.
Fixes a regression in 063b7c9.
* Bundle 64-bit version of JACK for 64-bit installer.
* Change some CMake logic for NSIS
Since it may cause quality loss, we have decided to remove it and just
use on-the-fly resampling while playing.
Also renames misleading `shouldLockMixer` to `syncWithMixer`.
@PhysSong
Copy link
Member Author

You can find newest test binaries here: https://github.com/PhysSong/lmms/releases/tag/record

@Reflexe
Copy link
Member

Reflexe commented Mar 17, 2019

@PhysSong Great job with SampleBuffer! Do you think we need more refactoring on other parts of this PR?

Copy link
Member

@Reflexe Reflexe left a comment

Choose a reason for hiding this comment

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

I will fix these in the next days.

include/MemoryManager.h Show resolved Hide resolved
@@ -168,19 +184,17 @@ class LMMS_EXPORT SampleBuffer : public QObject, public sharedObject

inline f_cnt_t frames() const
{
return m_frames;
dataReadLock();
Copy link
Member

@Reflexe Reflexe Mar 17, 2019

Choose a reason for hiding this comment

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

I think I should use the RAII pattern here too.

include/SampleBuffer.h Show resolved Hide resolved
* @param begin Beginning of an InputIterator.
* @param end End of an InputIterator.
* @param syncWithMixer Should we call requestChangeInModel?
* @return
Copy link
Member

Choose a reason for hiding this comment

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

What does it returns? (success)

include/SampleBuffer.h Show resolved Hide resolved
include/SampleBuffer.h Show resolved Hide resolved
@@ -343,7 +343,12 @@ void audioFileProcessor::setAudioFile( const QString & _audio_file,

void audioFileProcessor::reverseModelChanged( void )
{
m_sampleBuffer.setReversed( m_reverseModel.value() );
if (m_reverseModel.value () != m_isCurrentlyReversed) {
Copy link
Member

Choose a reason for hiding this comment

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

it loadSettings calls this functions, m_isCurrentlyReversed won't be initialized (at least, not for the loaded buffer; it'll be initialized to m_reverseModel's current value) on line 269.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't know how to solve that. Maybe just a isLoading attribute. :\

@PhysSong
Copy link
Member Author

PhysSong commented May 6, 2019

@Reflexe How is your work going on? Do you need some help?

@Reflexe
Copy link
Member

Reflexe commented May 31, 2019

Closed in favour of the rebased and refactored version at #4994

@Reflexe Reflexe closed this May 31, 2019
@rubiefawn rubiefawn mentioned this pull request Sep 5, 2019
9 tasks
@tresf tresf deleted the feature/recording branch October 3, 2019 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants