Various fixes and additions related to recording the mix to a file #1035

JosepMaJAZ · 2016-11-01T00:31:14Z

launchpad bugs:

#1637786 : Note: this one requires fixing the compilation of libsndfile, at least on windows (config.h, CPU_CLIPS_NEGATIVE)
#1415758
#1605921
#1605923
#1638115
#1638120
#1605922
#1415720

Be-ing · 2016-11-01T01:38:07Z

src/engine/sidechain/enginerecord.cpp

@@ -308,6 +308,10 @@ bool EngineRecord::openFile() {
 #endif
        if (m_pSndfile) {
            sf_command(m_pSndfile, SFC_SET_NORM_FLOAT, NULL, SF_TRUE);
+            // Warning! Depending on how libsndfile is compiled autoclip may not work.
+            // Ensure CPU_CLIPS_NEGATIVE and CPU_CLIPS_NEGATIVE is setup properly in the build.


It seems like CPU_CLIPS_NEGATIVE shouldn't be mentioned twice here.

daschuer · 2016-11-01T10:43:07Z

Thank you for this PR.

It is a bad idea to reuse the quality slider to set the sample format.
I would prefer to have an extra combobox for it and use "WAVE_format" preference option.

We may also consider to replace the quality slider, with a dynamic region that allows to set the recording parameter in established options for each format.
It would allow us to enable VBR encoding for ogg and mp3 later.

daschuer · 2016-11-01T10:46:36Z

The other parts are looking good. Would you mind to split the quality related changes out into a new PR?
This way we can merge the backed issues soon.

JosepMaJAZ · 2016-11-01T11:09:49Z

Ok, fine. I'll do the split now.

daschuer

I have added some findings to improve the code a bit more.

daschuer · 2016-11-01T20:49:13Z

src/preferences/dialog/dlgprefrecord.cpp

@@ -117,21 +117,30 @@ DlgPrefRecord::DlgPrefRecord(QWidget* parent, UserSettingsPointer pConfig)

    slotApply();
    // Make sure a corrupt config file won't cause us to record constantly.
+    // TODO: What does this do exactly? I thougth it stopped the recording when showing
+    // the preferences page but it does not (and i would preffer that keeps not stopping it).
    m_pRecordControl->set(RECORD_OFF);


This code is only passed when Mixxx is initialized. This line should be removed.

I am not sure if I understand you correctly. Do you mean that I have to remove the comments I added, or that I have to remove the "set", because it is useless?
In fact, I am not sure if the initial value is set anywhere. Probably recordingmanager should do it in its constructor.

All control objects are 0.0 = RECORD_OFF by default so the whole block with comments can be removed.

daschuer · 2016-11-01T21:00:39Z

src/recording/recordingmanager.cpp

@@ -100,17 +112,25 @@ void RecordingManager::startRecording(bool generateFileName) {
        m_recordingLocation = m_recording_base_file + "."+ encodingType.toLower();
        m_pConfig->set(ConfigKey(RECORDING_PREF_KEY, "Path"), m_recordingLocation);
        m_pConfig->set(ConfigKey(RECORDING_PREF_KEY, "CuePath"), m_recording_base_file +".cue");
+
+        m_recReady->set(RECORD_READY);
+
    } else {
        // This is only executed if filesplit occurs.


How about putting the else branch into a new function like
splitContinueRecording()?

I kept the structure there, but yes, my initial intention was to separate it into its own method. I could do it for clarity

daschuer · 2016-11-01T21:16:20Z

src/recording/recordingmanager.cpp


 // Only called when recording is active.
 void RecordingManager::slotBytesRecorded(int bytes)
 {
    // auto conversion to long
    m_iNumberOfBytesRecorded += bytes;
-    if(m_iNumberOfBytesRecorded >= m_split_size)
+    m_iNumberOfBytesRecordedSplit += bytes;
+    if(m_iNumberOfBytesRecordedSplit >= m_split_size)


It will likely split the file > m_split_size. How about adding here the a fixed offset that allows to process one buffer + closing frames, instead of tweaking the m_split_size value in the preferences?

At least a comment about the issue should be placed here

I don't understand that sentence. I will explain what I do just in case:

enginerecord emits a signal with the number of bytes processed (i.e. the size of the buffer that the process call worked on).
on recordingmanager, we collect that size and increment a variable, so that we can compare how big the file is (in order to split it), but also in order to emit a signal to the UI (recordings, in the library), where it shows the total amount recorded.

The implementation as it was, needs the amount to be reset once we reach split_size, because we create a new file and start from 0, but then, we were telling to the UI that we started again too.
With these separated variables, we keep the current file size in one variable, and the total size recorded in the other.

Yes, I understand it in general. Here we have the issue, that we have to split the file before it is bigger than the limit. It is done by actually using smaller limits than the user has selected.
This was hard to understand for me in the first place.

A comment like:
// The m_split_size limit is tweaked that the user value is not exceeded in any case
will help. But using an offset here might be better.

The split size would probably never be exactly the size requested. Reasoning is that the size is incremented by process' buffer size (In my tests, this was around 12K frames, so 48KB for a 16bit stereo file). As such, I don't see any problem in having the constants rounded down already, and not try to infer an offset that is mostly by convenience.

Ok, than just add a comment here.

daschuer · 2016-11-01T21:19:46Z

src/recording/recordingmanager.cpp

+    else if(fileSizeStr == SPLIT_120MIN)
+        return 120*60;
+    else // Do not limit by time for the rest.
+        return 999999999;


use -1 for unlimited

Yes and no. Maybe for the method, but not for the comparison. If I were to use -1 (or 0, which is what I initially wanted to use), then the if ( x > split_time ) would need to be if ( split_time != -1 && x > split_time).
Sure, this way it is not really unlimited, and the method is only called once each second, but I would prefer the simpler if.

Ah OK, 999999999 should than be a const variable or just use INT_MAX

INT_MAX sounds good. Will do all these changes tomorrow.

daschuer · 2016-11-01T21:21:15Z

src/recording/recordingmanager.cpp

     else
         return SIZE_650MB;
 }
+long RecordingManager::getFileSplitTime()


"long" has different size on different architectures. int should be big enough.

My fault, I copied it from the other method (previous to changing the other method's size)

daschuer · 2016-11-01T21:21:51Z

src/recording/recordingmanager.cpp

     else
         return SIZE_650MB;
 }
+long RecordingManager::getFileSplitTime()


getFileSplitSeconds()

daschuer · 2016-11-01T21:30:58Z

src/recording/recordingmanager.cpp

@@ -145,23 +167,37 @@ QString& RecordingManager::getRecordingDir() {
 }

 // Only called when recording is active.
-void RecordingManager::slotDurationRecorded(QString durationStr)
+void RecordingManager::slotDurationRecorded(quint64 duration)


We have a nice class mixxx::Duration with 64 bit nano seconds resolution that includes some convenient helpers for printing and debugging. Maybe it is worth to use this here.

I was just adapting the code to what we had. Duration represents seconds, so it really doesn't need that much precision. It's just that it already was quint64 in the enginerecord class.

daschuer · 2016-11-01T21:42:31Z

src/engine/sidechain/enginerecord.cpp

+        updateFromPreferences();  // Update file location from preferences.
+        if (openFile()) {
+            qDebug() << "Splitting to a new file: "<< m_fileName;
+            m_pRecReady->set(RECORD_ON);


This can be an (unlikely) race condition if the user has stopped the recording in between.
It can be solved by using connectValueChangeRequest() and write the value finally by setAndConfirm()

This is done for broadcasting as well:

mixxx/src/engine/sidechain/enginebroadcast.cpp

Line 60 in 8d191bf

m_pStatusCO->connectValueChangeRequest(

In addition, status and enable is separated into two COs

I am unsure if I follow your reasoning fully here, but anyway, you seem to suggest a conceptual change.

If your scenario is:

mixxx is recording
process is called
split is required
new mode set to split
(time passes)
process is called
the split part is entered
user stops recording. This should be calling the encodingmanager to stop, and so set the mode to off
since we had already entered into the if, but not yet called the set on (we were opening the file), the stop request is ignored.

If you want to ensure thread synchronization, the only bet is synchronizing the whole method.

Since it was that way before anyway and the case is unlikely, it is not required to do this in this PR. But a bullet prove solution would be nice on the long run.

What do you mean with "synchronizing the whole method"?
For my feeling using two separate COs will works, but we have to verify this.

daschuer

I have just tested this PR and it works like a charm.
I only see two compiler warnings.

I think it is a known issue that I get
INDEX 01 00:00:44
in a split cue file
Do we have a bug for it?

We have discussed to Improve the PlayerInfo class and make it real-time along with a OSC client here:
http://www.mixxx.org/forums/viewtopic.php?f=6&t=8003&start=20
Maybe you have some Ideas and fun to help Jakob with building on Windows.

Please give a hint when it is ready to merge.

daschuer · 2016-11-03T19:52:10Z

src/recording/recordingmanager.cpp

 {
-    if(m_durationRecorded != durationStr)
+    if(m_secondsRecordedSplit != duration)


This produces a warning:
warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

daschuer · 2016-11-03T19:53:16Z

src/recording/recordingmanager.cpp

-        m_durationRecorded = durationStr;
-        emit(durationRecorded(m_durationRecorded));
+        m_secondsRecordedSplit = duration;
+        if(duration >= m_split_time)


Here as well:
warning: comparison between signed and unsigned integer expressions [-Wsign-compare]

I think I didn't see on launchpad the bug about the imprecisions with the track start positions. It was on my plan to fix that too, but in the end I couldn't get how to work it out.

Later on I thought that maybe we don't need to be precise. It is enough with being ahead instead of too late.
That would fix the 00:00:44 that you say, as well as the first song start, which always starts too late. In fact, for the first song, I was thinking on avoiding the counter in metaDataHasChanged until we get a track from getCurrentPlayingTrack for the first time.
So, with that, and assuming that the change took place between the previous call and now, we coud set the time to that of the previous call.

An improved PlayerInfo could help to determine this better, but we also want to avoid filling the cue with several changes forth and back when moving the faders/crossfader as an effect. I guess that's part of the reason why the counter was added initially, although it's not a perfect solution.

As for the warniings, I've just changed that and will commit it later on. Compiling on windows it's quite difficult to get anything out of the console output (except when it directly fails).

… on new splits, and improves the time precision of first track of the initial cue.

JosepMaJAZ · 2016-11-04T22:46:20Z

With the latest commit (small improvement on start time of cue files) I would consider the changes ready. Further improvement of start time needs improvement in the PlayerInfo class and changing the decision of what is considered a new track (so that the lifecounter is no longer needed).

daschuer · 2016-11-05T19:26:24Z

Thank you! I have just tested this branch an it works nicely.
LGTM!

However have two finding:

If I stop a Track, to insert silence, nothing is added to the cue file. Could this be an intended behavior, to identify interviews in a radio-show or such?
I have stepped into libsndfile and dither is disabled by default.
I will file bugs for them.

daschuer · 2016-11-05T20:14:42Z

The new bugs are here:
https://bugs.launchpad.net/mixxx/+bug/1639495
https://bugs.launchpad.net/mixxx/+bug/1639499

Before merge, you have to become a Mixxx contributor.
Please sign

https://docs.google.com/a/mixxx.org/spreadsheet/viewform?formkey=dEpYN2NkVEFnWWQzbkFfM0ZYYUZ5X2c6MQ

and comment here when done.
Thank you.

JosepMaJAZ · 2016-11-05T20:40:51Z

Hi.
In reference to the dither issue, i've added a comment to the bug in lauchpad. Basically, it is not usable.
In reference to the "pause", or "break" track, seems reasonable and easy. I'll take a look after dinner and if it works I'll commit it too.

And I've accepted and submitted the contributor agreement.

daschuer · 2016-11-06T21:23:08Z

Thank you, LGTM! I think this PR is big enough for a first merge.
I am happy to review more ;-)

Be-ing · 2017-09-27T15:11:41Z

I'm not sure what's going on with the preferences pane. Maybe some widgets aren't getting hidden when selecting different codecs?

JosepMaJAZ · 2017-09-27T15:15:23Z

You are right. I saw this on recent builds but I haven't checked why yet. It is as if there are extra radios that shouldn't be there. They are grnerated dynamically in code. El dia 27/09/2017 17:11, "Be" <notifications@github.com> va escriure:

…

I'm not sure what's going on with the preferences pane. Maybe some widgets aren't getting hidden when selecting different codecs? [image: image] <https://user-images.githubusercontent.com/9455094/30921486-40170d0c-a36c-11e7-9d1c-f68df7be52ec.png> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#1035 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJdFf3NUIFvccXmofw0XOXpabwjnEciXks5smmWzgaJpZM4Klq0p> .

JosepMaJAZ · 2017-09-30T11:06:57Z

Weird, I did a full rebuild again (I already needed to do it some weeks ago for other weird crashes and waveform misbehaviour) and the bug has gone. Try deleting lin64_build, .sconf_temp and cache dirs and well as sconsign.dblite and rebuild it to see if that also fixes it for you. I also checked out the most recent changes, but I don't see any change that could have fixed this.

JosepMaJAZ added 7 commits October 30, 2016 03:03

launchpad bug #1605923 correct path in cue file when recording to wav

67ebbf6

launchpad bug #1637786 recording to wav heavily distorts on saturation

b0007b1

better fix for relative path in cue file

6447a0b

fix for bug #1638120 : close cue file

a836414

fix bug #1605921 : Improve recording split size options

b33faf2

fixes bugs #1638115 and#1605922 related to recording and splitting

5bbe8fe

Bug #1415720 save wav/aiff in 16/24 or 32bit float

e53ca01

Be-ing reviewed Nov 1, 2016

View reviewed changes

typo

c082dee

splitting wav fileformat changes to another branch

53ef61b

daschuer reviewed Nov 1, 2016

View reviewed changes

changes from the PR review

773cc21

daschuer requested changes Nov 3, 2016

View reviewed changes

JosepMaJAZ added 2 commits November 3, 2016 22:54

more review changes (signed/unsigned)

1c020de

some improvements in track time in cue files. Fixes the 00:00:44 time…

3e4902c

… on new splits, and improves the time precision of first track of the initial cue.

daschuer merged commit 5257679 into mixxxdj:master Nov 6, 2016

JosepMaJAZ deleted the fix-clip-recording branch April 8, 2017 13:05

esbrandt mentioned this pull request Jun 24, 2017

WIP Manual 2.1.x updates mixxxdj/manual#59

Merged

37 tasks

daschuer mentioned this pull request Jan 26, 2021

use Windows dependencies from vcpkg #3594

Closed

Various fixes and additions related to recording the mix to a file #1035

Various fixes and additions related to recording the mix to a file #1035

Conversation

JosepMaJAZ commented Nov 1, 2016

Choose a reason for hiding this comment

daschuer commented Nov 1, 2016

daschuer commented Nov 1, 2016

JosepMaJAZ commented Nov 1, 2016

daschuer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daschuer left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JosepMaJAZ commented Nov 4, 2016

daschuer commented Nov 5, 2016

daschuer commented Nov 5, 2016

JosepMaJAZ commented Nov 5, 2016

daschuer commented Nov 6, 2016

Be-ing commented Sep 27, 2017

JosepMaJAZ commented Sep 27, 2017 via email

JosepMaJAZ commented Sep 30, 2017 via email

daschuer left a comment •

edited