Bugzilla – Bug 2009
Windows and Linux: Intermittent truncation of the end of a recording
Last modified: 2018-11-09 10:08:12 UTC
This bug was reported by a user who was sometimes losing the the last word of recorded speech. In my tests on Windows, the problem is intermittent on both Windows 7 and 10, but occurs more frequently on Windows 7 (which is still used by lots of people). The steps to reproduce all use record on new line, but append record is also affected.
Steve confirmed that the bug also occurred on Linux. Mac is untested as far as I'm aware.
This is a regression on 2.2.2
Testing with 2.3.0 with Mojave 10.14 1) I seem to get consistent full (exact 3.000) three second recordings from the first set of steps. 2) I detect no truncation on the second set of steps Looks OK (and highly accurate) on Mac ---------------------------------------------- If this bug remains at P3 then it needs a releases note - and ideally a workaround
(In reply to Peter Sampson from comment #3) > Testing with 2.3.0 with Mojave 10.14 > > 1) I seem to get consistent full (exact 3.000) three second recordings from > the first set of steps. > > 2) I detect no truncation on the second set of steps > > Looks OK (and highly accurate) on Mac > > ---------------------------------------------- > > If this bug remains at P3 then it needs a releases note - and ideally a > workaround I set it at P3, but I really think it's a P2. Users really shouldn't be losing part of what they think they have recorded.
Looks like the bug was introduced by this commit: Author: Paul Licameli <paul.licameli@gmail.com> Date: 5 months ago (01/06/2018 09:31:40) Commit hash: 9e2937feedd6eb61ec7e38e6b3d630f4028f9889 Children: c0fb140257 Parent(s): 20f3f76e2f 2892dfd956 Punch and roll recording menu item, tentative shortcut Shift+D ("do-over") Add to recording preferences for pre-roll and crossfade Define EXPERIMENTAL flag Recording options allow crossfade data for start of recording Stop playback of pre-rolled tracks at the right time Allow for preRoll in start-stream options Contained in branches: (HEAD detached at 9e2937fee), wasapi, undofocus, trackselection, trackname, temp, slug, slow, samplerate, master, macros, getinfo, effectfocus, details, contract, clipshortcuts, checkbox Contained in tags: Audacity-2.3.0
(In reply to David Bailes from comment #5) > Looks like the bug was introduced by this commit: > > Author: Paul Licameli <paul.licameli@gmail.com> > Date: 5 months ago (01/06/2018 09:31:40) > Commit hash: 9e2937feedd6eb61ec7e38e6b3d630f4028f9889 > Children: c0fb140257 > Parent(s): 20f3f76e2f 2892dfd956 > > Punch and roll recording > > menu item, tentative shortcut Shift+D ("do-over") > Add to recording preferences for pre-roll and crossfade > Define EXPERIMENTAL flag > Recording options allow crossfade data for start of recording > Stop playback of pre-rolled tracks at the right time > Allow for preRoll in start-stream options > > Contained in branches: (HEAD detached at 9e2937fee), wasapi, undofocus, > trackselection, trackname, temp, slug, slow, samplerate, master, macros, > getinfo, effectfocus, details, contract, clipshortcuts, checkbox > Contained in tags: Audacity-2.3.0 The above commit was a merge. Within that merge bug was introduced by this commit: Author: Paul Licameli <paul.licameli@audacityteam.org> Author date: 5 months ago (25/05/2018 13:01:14) Committer: Paul Licameli <paul.licameli@gmail.com> Commit date: 5 months ago (01/06/2018 09:28:58) Commit hash: a0256e935c1b60359b0b0291422018e7a5286421 Children: 11fd3dcc9d Parent(s): f9cd5595d5 Recording options allow crossfade data for start of recording I've updated the first git sha.
In commit a0256e935c1b60359b0b0291422018e7a5286421 the line: mRecordingSchedule = {}; // free arrays was added to the lambda in the definition of cleanup at the start of AudioIO::StopStream(). A fix for this bug is to remove that line. Given that mRecordingSchedule is initialized near the start of StartStream(), I think this is a safe fix. I'm not familiar enough with the code to understand why this line of code caused the problem, but did notice that AudioIO::StopStream() does get called several times at the end of a recording.
(In reply to David Bailes from comment #7) > In commit a0256e935c1b60359b0b0291422018e7a5286421 > the line: > mRecordingSchedule = {}; // free arrays > was added to the lambda in the definition of cleanup at the start of > AudioIO::StopStream(). > > A fix for this bug is to remove that line. Given that mRecordingSchedule is > initialized near the start of StartStream(), I think this is a safe fix. > > I'm not familiar enough with the code to understand why this line of code > caused the problem, but did notice that AudioIO::StopStream() does get > called several times at the end of a recording. Shall I commit this fix, or does someone else who is more familiar with the code want to have a look at it?
I have a suspicion that "aggregate initialization" of the RecordingSchedule object isn't doing what I thought, with the Windows compiler, and some fields are getting undefined values instead of being reinitialized according to the in-class member initializers. See if this fixes it instead: don't remove that line, but add an explicit, empty constructor to struct RecordingSchedule in AudioIO.h : RecordingSchedule() {} And maybe add this too for good measure: RecordingSchedule(const RecordingSchedule&) = default;
(In reply to Paul L from comment #9) > I have a suspicion that "aggregate initialization" of the RecordingSchedule > object isn't doing what I thought, with the Windows compiler, and some > fields are getting undefined values instead of being reinitialized according > to the in-class member initializers. The bug shows up on Linux as well as Windows, so it would have to be both compilers. In addition, the statement mRecordingSchedule = {}; is used in StartStream, which causes no obvious problems. In the lambda in cleanup at the start of StopStream, I replaced mRecordingSchedule = {}; by individual statements to initialize the data members of mRecordingScheldule. This did not fix the bug. In the same place, I replaced mRecordingSchedule = {}; simply by mRecordingSchedule.mCrossfadeData.clear(); and not reinitializing the other data members. This fixed the bug. I think this is a good enough fix, as it does free the arrays, and I would like to commit this so Steve can check whether this also fixes the problem on linux. Is this OK? I had a quick look to see if I could work out which or which combination of the reinitializations of the other data members was causing the bug, but it's very time consuming, as the bug is intermittent, so I gave up. > > See if this fixes it instead: don't remove that line, but add an explicit, > empty constructor to struct RecordingSchedule in AudioIO.h : > > RecordingSchedule() {} > > And maybe add this too for good measure: > > RecordingSchedule(const RecordingSchedule&) = default; I did try these, but they made no difference.
Good then: push the fix that just clears the arrays.
(In reply to Paul L from comment #11) > Good then: push the fix that just clears the arrays. https://github.com/audacity/audacity/commit/bb7b95023fb2af9a9055b940bbc4aec97790e8f6 I've tested this on Windows 7 and 10.
Testing on W10 with audacity-2.3.1-alpha-207-4c76e598d5859dc172e063d37287cc510fa7850f This looks to be fixed OK(is) (OK enough?) on Windows. I tested using both sets of steps. With use case-1 The recordings look visually very close to the 3 seconds, but forensic examination shows they are always consistently a very small fraction of a send short. I tried ten recordings wyh the following resultant times. 2.977 2.976 2.974 2.978 2.974 2.985 2.975 2.975 2.985 2.975 Is this close enough ?
(In reply to Peter Sampson from comment #13) > Testing on W10 with > audacity-2.3.1-alpha-207-4c76e598d5859dc172e063d37287cc510fa7850f > > This looks to be fixed OK(is) (OK enough?) on Windows. > > I tested using both sets of steps. > > With use case-1 The recordings look visually very close to the 3 seconds, > but forensic examination shows they are always consistently a very small > fraction of a send short. > > I tried ten recordings wyh the following resultant times. > 2.977 > 2.976 > 2.974 > 2.978 > 2.974 > 2.985 > 2.975 > 2.975 > 2.985 > 2.975 > > Is this close enough ? It's certainly close enough for the use case which was reported and resulted in the bug being logged. I checked 2.1.3, with the latency correction set to 0, and the times as slightly higher than 3.0. So it may never have been exact.
Looks good on Linux, so closing as QuickFixed
Fixed rateher than quickfixred as this is a regression on 2.2.2 (not 2.3.0) and thus not a bug introduced in the 2.3.1 dev cycle
