Realtime recording note analysis - #6
Draft
novikov-alexander wants to merge 27 commits into
Draft
Conversation
Author
|
I apologize for the formatting changes. I noticed that some indentations were made with tabs and others with spaces. Additionally, there were extra spaces on new lines. Visual Studio automatically standardized it, but I can attempt to revert it back. |
Member
|
This is very interesting! There's a lot to digest here but I've given it a quick test and it clearly works. pYin can produce streamed output if initialised with the fixed-lag option set, though I think only for fundamental frequency data - the note segmentation is always produced at the end if I remember correctly. I think that the SV feature-extraction model running code is prepared to "stall" while providing input to the plugin, in order to wait for the record input to provide more data, so in theory there might be a possibility of doing the whole thing as a continuous stream through to a single output layer - only without the note segmentation which presumably would involve running another extraction at the end. But I'm not confident about that. It's possible that your approach might work better even if it does look clumsier, just because it produces all output layers and doesn't risk changing its mind about their contents at the end. |
Author
|
That's very pleasant to read! I think I need to:
What other important points would you suggest we consider? Thank you! |
novikov-alexander
force-pushed
the
alnovi/recordAnalyse
branch
from
September 5, 2025 04:40
4386080 to
dd76c12
Compare
novikov-alexander
force-pushed
the
alnovi/recordAnalyse
branch
from
September 5, 2025 04:57
dd76c12 to
5306b57
Compare
novikov-alexander
force-pushed
the
alnovi/recordAnalyse
branch
from
September 5, 2025 06:07
e221a69 to
08892ec
Compare
novikov-alexander
marked this pull request as draft
September 5, 2025 06:22
novikov-alexander
force-pushed
the
alnovi/recordAnalyse
branch
5 times, most recently
from
March 16, 2026 14:34
c269f90 to
ecd449e
Compare
novikov-alexander
force-pushed
the
alnovi/recordAnalyse
branch
from
March 16, 2026 14:37
ecd449e to
bd7a622
Compare
Undo after any realtime chunk was a use-after-free: temp analysis layers are never added to a view, but cleanup called removeLayerFromView() on them, which pushes a RemoveLayerCommand holding a raw Layer* onto the undo stack, and then deleted the layer. Delete directly with deleteLayer(force) instead. This also stops recording filling the undo stack with bogus entries and marking the document dirty. MainWindow's recordCompleted handler used disconnect(sender, &signal, nullptr, nullptr), which removes *all* receivers: it was permanently destroying MainWindowBase's record-duration display and the recordCompleted -> analyseNow() connection. Only the first recording worked. Disconnect by stored Connection handle instead. The runner read the never-written "record-analysis" key rather than the "recording-analysis-mode" the menu writes, so realtime analysis ran regardless of mode, and on completion both it and the full re-analysis raced over the same layers. Route recordCompleted through analyseAfterRecording(), which stands down while the realtime handler is live. m_analysedFrames was only reset in fileClosed(), which recording a second time does not always reach, so the next recording's first chunk started from the previous recording's end frame -- silently swapped by Selection into a large backwards range that wiped the new pitch track. Reset it in newFileLoaded() and refuse empty ranges. Other correctness fixes: - RealtimeAnalyser::cleanup() no longer restarts pending work; every caller is tearing state down, and from a destructor it connected signals to a half-destroyed object. - finishChunk() is generation-aware, so a stale callback can no longer release the in-flight slot while a newer chunk is running. - Pending selections merge rather than replace, so the frames between a running chunk and the newest request are no longer skipped. - Completion handlers also fire if the model is already complete when we connect; otherwise a fast transform left m_inFlight stuck true and realtime analysis silently dead for the session. - processNoteEvents() removes existing notes absorbed into a merge transitively, not just those directly overlapping the incoming ones, which used to leave duplicates stacked under the merged note. - findOverlapGroups() is a sort-and-sweep rather than a rescan on every match, which was cubic over the whole note track once per chunk. - std::max(..., 0LL) would not compile where int64_t is long. - Restore libc++ hardening on macOS. - Decouple "Auto-Analyse New Audio" from the recording mode again. Add test-tony-main covering OverlapProcessor: grouping semantics against a brute-force transitive-closure reference, a scaling guard for the old cubic loop, merging and weighted frequency, and both patch paths including idempotence across repeated overlapping chunks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
pYIN's two outputs disagree about what their timestamps mean, and the realtime path was treating them identically. The host feeds absolute block timestamps to the plugin (FeatureExtractionModelTransformer.cpp, m_plugin->process with frame2RealTime(blockFrame)). pYIN's smoothedpitchtrack is a FixedSampleRate output that echoes that timestamp back verbatim (PYinVamp.cpp:592), and the transformer's FixedSampleRate branch reconstructs the frame from it, so those frames are already absolute. Its notes output is VariableSampleRate and derives timestamps from a frame index counting from zero, ignoring the host timestamps (PYinVamp.cpp:695-699), so those frames are relative. processPitchEvents and processNoteEvents both added contextStart, which is correct for notes and doubles the offset for pitch. During recording the pitch track therefore drifted off past the record head, further the longer the take ran, and only looked right afterwards because Document::refreshModel regenerates the derived layers when recording stops. processPitchEvents no longer shifts. contextStart remains the boundary of the region being replaced. The differing frame conventions are now documented on both methods and pinned by tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR is a rough draft for a real-time note analysis feature. Such a feature would be helpful for vocal and instrumental real-time performance analysis, particularly in music performance teaching or studio recording quality assurance.
The idea of the PR is to create temporary layers that are filled via the pYIN plugin as record target emits
recordDurationChanged. The filled layer models are then copied to layers attached to the view. Analysis occurs with some overlap to ensure that notes are not split by analysis window boundaries. Given that consecutive windows overlap, I propose merging notes in the overlapping areas.This PR serves as a proof of concept which works amazingly well. However, I'm not satisfied with the constant creation of temporary layers. I believe it is possible to reuse the same temporary layers again and again. Moreover, I'm unsure if there's a way to continuously retrieve features from pYIN. If continuous retrieval is possible, my current approach might not be the best, and it could be better to establish a reactive stream of features from pYIN to the note layers.
I acknowledge that there is some buggy code that needs refactoring, and the UI in the menu should be improved. I'm ready to split the PR into smaller chunks or perform additional refactoring as necessary.
Additionally, I'm uncertain whether the new functionality should be implemented within
analyseNow,reanalyseSelection, or as a separateanalyseRecordingfunction, as I have done.I would appreciate any recommendations on this topic and am prepared to enhance this feature or related aspects, as I have spare time over the next couple of months.