Add native score-to-video sync#33323
Conversation
|
Updated the mixer integration for the score-to-video patch. What changed:
Verification:
|
|
Follow-up fix for bottom-docked Video panel clipping:
|
|
Follow-up video panel sizing change:
Verification:
|
|
Fixed two playback behavior bugs from testing:
Verification:
|
|
Video panel layout follow-up:
Verification:
|
ToranadoMusic
changed the title
|
PR readiness update: refreshed the description to reflect the current implementation, scope, known limitations, and a concrete tester checklist. Also moved Video solo behavior into |
|
Less formal comment this time. CLA Has Now Been signed, however, working out a few more bugs with exporting video alongside the new Video Export feature in 4.7. Also attempting to make it more compatible with the system we are now semi-required to download alongside of Musescore in order to do video export and having it decode the video to better implement the audio into the normal audio workflow. |
|
It looks like this PR needs a rebase ( |
29c48ae to
5a675da
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request implements project-level video attachment support: new types and IProjectVideoSettings, ProjectVideoSettings persistence with MSC read/write, NotationProject wiring, playback controller integration for master mute/solo, mixer Video channel and controls, a VideoPanel QML UI and VideoPanelModel bridge, timeline timecode/hit-point meta rows, appshell actions/shortcuts, build updates for Qt Multimedia, and unit tests for settings persistence. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/CMakeLists.txt`:
- Line 21: The unconditional find_package(Qt6 REQUIRED COMPONENTS Multimedia)
should be guarded by the playback build option; wrap the find_package call so it
only runs when MUE_BUILD_PLAYBACK_MODULE is enabled (the same option that
controls the consumer target_link_libraries(... Qt6::Multimedia) in the playback
QML CMake). Modify src/CMakeLists.txt to check MUE_BUILD_PLAYBACK_MODULE before
calling find_package(Qt6 COMPONENTS Multimedia) (or make the component optional)
so non-playback builds do not require Qt6::Multimedia during configuration.
In `@src/engraving/infrastructure/mscreader.cpp`:
- Around line 246-249: readVideoSettingsJsonFile currently calls
fileData("videosettings.json") unconditionally which triggers error paths/log
spam when the file is absent; modify MscReader::readVideoSettingsJsonFile to
first check fileExists(u"videosettings.json") and only call fileData(...) when
present, otherwise return an empty ByteArray (or appropriate default) so no
false errors are logged by the reader. Ensure you use the existing
MscReader::fileExists and MscReader::fileData helpers to locate the change.
In `@src/playback/qml/MuseScore/Playback/mixerpanelmodel.cpp`:
- Around line 52-55: videoVolumeFromDb currently converts dB to linear with
muse::db_to_linear(volume).raw() which can return a tiny non-zero value for the
dB value that represents exact silence (-60 dB produced by videoVolumeToDb(0)),
causing saved volume=0 to be bumped; fix by special-casing that sentinel dB
value: if volume equals the dB value produced by videoVolumeToDb(0) (or the
constant -60 dB if used), return 0.f immediately, otherwise continue to return
std::clamp(muse::db_to_linear(volume).raw(), 0.f, 1.f); update videoVolumeFromDb
to reference videoVolumeToDb/video silent-constant and muse::db_to_linear so the
intent is clear.
In `@src/playback/qml/MuseScore/Playback/VideoPanel.qml`:
- Around line 277-282: The onTextEditingFinished handler uses
parseInt(newTextValue) without a radix; change the call in the
onTextEditingFinished function to parseInt(newTextValue, 10) to ensure decimal
parsing and then assign videoModel.offsetMs as before (preserving the existing
isNaN check).
In `@src/project/internal/projectvideosettings.cpp`:
- Around line 68-70: ProjectVideoSettings::read currently calls
QJsonDocument::fromJson(...).object() without error checking and ignores the
persisted VIDEO_SETTINGS_VERSION; update read to parse with a QJsonParseError,
verify error.error == QJsonParseError::NoError before using the document, then
validate the root JSON contains a "version" that matches VIDEO_SETTINGS_VERSION
(or handle known migration paths) and that "attachment" exists and is an object;
only after those checks assign m_attachment =
attachmentFromJson(rootObj.value("attachment").toObject()); reference
QJsonDocument::fromJson, QJsonParseError, VIDEO_SETTINGS_VERSION,
ProjectVideoSettings::read, m_attachment, and attachmentFromJson when locating
the code to change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1596b7e7-1c75-4443-a12c-9532680a41ae
📒 Files selected for processing (35)
src/CMakeLists.txtsrc/app/configs/data/shortcuts.xmlsrc/app/configs/data/shortcuts_azerty.xmlsrc/app/configs/data/shortcuts_mac.xmlsrc/appshell/appshelltypes.hsrc/appshell/internal/applicationuiactions.cppsrc/appshell/qml/MuseScore/AppShell/NotationPage/NotationPage.qmlsrc/appshell/qml/MuseScore/AppShell/appmenumodel.cppsrc/appshell/qml/MuseScore/AppShell/notationpagemodel.cppsrc/appshell/qml/MuseScore/AppShell/notationpagemodel.hsrc/engraving/infrastructure/mscreader.cppsrc/engraving/infrastructure/mscreader.hsrc/engraving/infrastructure/mscwriter.cppsrc/engraving/infrastructure/mscwriter.hsrc/playback/internal/playbackcontroller.cppsrc/playback/internal/playbackcontroller.hsrc/playback/qml/MuseScore/Playback/CMakeLists.txtsrc/playback/qml/MuseScore/Playback/MixerPanel.qmlsrc/playback/qml/MuseScore/Playback/VideoPanel.qmlsrc/playback/qml/MuseScore/Playback/internal/MixerAuxSendsSection.qmlsrc/playback/qml/MuseScore/Playback/internal/MixerTitleSection.qmlsrc/playback/qml/MuseScore/Playback/mixerchannelitem.hsrc/playback/qml/MuseScore/Playback/mixerpanelmodel.cppsrc/playback/qml/MuseScore/Playback/mixerpanelmodel.hsrc/playback/qml/MuseScore/Playback/videopanelmodel.cppsrc/playback/qml/MuseScore/Playback/videopanelmodel.hsrc/project/CMakeLists.txtsrc/project/inotationproject.hsrc/project/internal/notationproject.cppsrc/project/internal/notationproject.hsrc/project/internal/projectvideosettings.cppsrc/project/internal/projectvideosettings.hsrc/project/iprojectvideosettings.hsrc/project/tests/CMakeLists.txtsrc/project/tests/projectvideosettingstest.cpp
|
@ToranadoMusic, what's your username on MuseScore.org for the account you used to sign the CLA? If you're not sure, you can find out your username at https://musescore.org/en/user. |
|
@shoogle I have signed the CLA with the account toranmorton |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/project/internal/projectvideosettings.cpp (1)
45-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize
hitPointsinsetAttachment().
attachmentFromJson()sortshitPoints, but Line 51 stores caller order verbatim.Timeline::hitPointMeta()then iteratesattachment.hitPointsdirectly insrc/notationscene/widgets/timeline.cpp:1493-1518, so newly added/edited hit points can render in the wrong order until the score is saved and reopened. Canonicalize before the equality check so live behavior matches the persisted model.Proposed fix
void ProjectVideoSettings::setAttachment(const VideoAttachmentSettings& attachment) { - if (m_attachment == attachment) { + VideoAttachmentSettings normalized = attachment; + std::stable_sort(normalized.hitPoints.begin(), normalized.hitPoints.end(), + [](const VideoHitPointSettings& a, const VideoHitPointSettings& b) { + return a.timeMs < b.timeMs; + }); + + if (m_attachment == normalized) { return; } - m_attachment = attachment; + m_attachment = std::move(normalized); m_settingsChanged.notify(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/project/internal/projectvideosettings.cpp` around lines 45 - 52, The setAttachment problem is that it stores attachment.hitPoints in caller order so live rendering differs from the persisted, sorted model; update ProjectVideoSettings::setAttachment to canonicalize (sort) attachment.hitPoints the same way attachmentFromJson does (use the same comparator or shared helper used elsewhere) before comparing to m_attachment and before assigning m_attachment and calling m_settingsChanged.notify(); ensure you normalize hitPoints so Timeline::hitPointMeta which iterates attachment.hitPoints sees the same order as the persisted model.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/notationscene/widgets/timeline.cpp`:
- Around line 1544-1545: The long initializer for videoPositionMs violates
uncrustify formatting; refactor the expression in timeline.cpp by breaking the
computation into clearly indented subexpressions or intermediate variables
(e.g., compute ticks = seg->measure()->tick().ticks(), timeSec =
score()->utick2utime(ticks), timeMs = std::lround(timeSec * 1000.0) and then use
std::max(0, static_cast<int>(timeMs) + attachment.offsetMs)) so the line wraps
conform to the repo codestyle while preserving the same semantics for
videoPositionMs.
- Line 1517: The synthetic video-metadata rows are being given a segment pointer
(seg) when calling addMetaValue, which incorrectly routes clicks into the
segment-selection branch in Timeline::mousePressEvent; update the addMetaValue
calls for these synthetic rows (the calls around the occurrences using the local
variable seg at the shown sites) to pass nullptr for the segment parameter
(instead of seg) so clicks target only the measure metadata rather than
selecting the measure's first segment.
- Around line 1507-1510: The code currently clamps negative score-relative times
to zero so pre-roll hit points get mapped to measure 1; instead detect the raw
delta (hitPoint.timeMs - attachment.offsetMs) and if it's negative skip/continue
before computing scoreTimeSeconds and tick. Update the logic around
scoreTimeSeconds/tick/measure (references: hitPoint.timeMs, attachment.offsetMs,
scoreTimeSeconds, tick, score()->utime2utick, score()->tick2measure, measure,
currentMeasureIndex) to early-return or continue when the hit point occurs
before the score start rather than clamping to zero.
In `@src/playback/qml/MuseScore/Playback/videopanelmodel.cpp`:
- Around line 100-118: In VideoPanelModel::formatTimecode the code computes
totalFrames using the fractional framesPerSecond but then splits frames/seconds
using roundedFrameRate, causing drift for non-integer FPS (e.g., 29.97). Fix by
computing totalFrames with the same integer rate: compute roundedFrameRate first
(as you already do) and replace the totalFrames calculation to use
roundedFrameRate (e.g. totalFrames = floor((videoPositionMs / 1000.0) *
roundedFrameRate + 0.5)) so seconds and frames remain aligned; keep the rest of
the decomposition (frames, totalSeconds, seconds, minutes, hours) unchanged.
---
Outside diff comments:
In `@src/project/internal/projectvideosettings.cpp`:
- Around line 45-52: The setAttachment problem is that it stores
attachment.hitPoints in caller order so live rendering differs from the
persisted, sorted model; update ProjectVideoSettings::setAttachment to
canonicalize (sort) attachment.hitPoints the same way attachmentFromJson does
(use the same comparator or shared helper used elsewhere) before comparing to
m_attachment and before assigning m_attachment and calling
m_settingsChanged.notify(); ensure you normalize hitPoints so
Timeline::hitPointMeta which iterates attachment.hitPoints sees the same order
as the persisted model.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c06d651d-e7fc-40b1-9637-02da9584e74f
📒 Files selected for processing (10)
src/notationscene/qml/MuseScore/NotationScene/timelineview.cppsrc/notationscene/widgets/timeline.cppsrc/notationscene/widgets/timeline.hsrc/playback/qml/MuseScore/Playback/VideoPanel.qmlsrc/playback/qml/MuseScore/Playback/videopanelmodel.cppsrc/playback/qml/MuseScore/Playback/videopanelmodel.hsrc/project/internal/projectvideosettings.cppsrc/project/internal/projectvideosettings.hsrc/project/iprojectvideosettings.hsrc/project/tests/projectvideosettingstest.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/project/internal/projectvideosettings.h
- src/project/tests/projectvideosettingstest.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/engraving/style/textstyle.cpp`:
- Around line 1830-1832: allTextStyles() currently skips
TextStyleType::VIDEO_HIT_POINT but editableTextStyles() later reinserts
VIDEO_HIT_POINT after TextStyleType::SYSTEM and removes DYNAMICS; clarify intent
by either aligning the rules or documenting why VIDEO_HIT_POINT is excluded
globally but reintroduced for the editor: add a short comment in allTextStyles()
explaining why VIDEO_HIT_POINT is omitted (e.g., it is editor-only / rendered
differently at runtime), and mirror that rationale as a comment in
editableTextStyles() where VIDEO_HIT_POINT is inserted (and note the DYNAMICS
removal), or if the omission was accidental, change allTextStyles() to include
VIDEO_HIT_POINT so both functions are consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cdb4aef7-94e8-4c0d-9e90-ccefda6a413f
📒 Files selected for processing (25)
src/CMakeLists.txtsrc/appshell/internal/applicationuiactions.cppsrc/appshell/internal/applicationuiactions.hsrc/appshell/qml/MuseScore/AppShell/NotationPage/NotationPage.qmlsrc/appshell/qml/MuseScore/AppShell/appmenumodel.cppsrc/appshell/qml/MuseScore/AppShell/appmenumodel.hsrc/appshell/qml/MuseScore/AppShell/notationpagemodel.cppsrc/appshell/qml/MuseScore/AppShell/notationpagemodel.hsrc/engraving/style/styledef.cppsrc/engraving/style/styledef.hsrc/engraving/style/textstyle.cppsrc/engraving/types/types.hsrc/engraving/types/typesconv.cppsrc/notationscene/qml/MuseScore/NotationScene/abstractnotationpaintview.cppsrc/notationscene/qml/MuseScore/NotationScene/abstractnotationpaintview.hsrc/notationscene/widgets/editstyle.cppsrc/notationscene/widgets/editstyle.hsrc/notationscene/widgets/editstyleutils.cppsrc/notationscene/widgets/timeline.cppsrc/playback/qml/MuseScore/Playback/CMakeLists.txtsrc/playback/qml/MuseScore/Playback/VideoPanel.qmlsrc/playback/qml/MuseScore/Playback/mixerpanelmodel.cppsrc/playback/qml/MuseScore/Playback/videopanelmodel.cppsrc/playback/qml/MuseScore/Playback/videopanelmodel.hsrc/project/internal/projectvideosettings.cpp
✅ Files skipped from review due to trivial changes (2)
- src/engraving/types/types.h
- src/notationscene/qml/MuseScore/NotationScene/abstractnotationpaintview.h
🚧 Files skipped from review as they are similar to previous changes (7)
- src/CMakeLists.txt
- src/appshell/qml/MuseScore/AppShell/NotationPage/NotationPage.qml
- src/playback/qml/MuseScore/Playback/videopanelmodel.h
- src/playback/qml/MuseScore/Playback/CMakeLists.txt
- src/playback/qml/MuseScore/Playback/mixerpanelmodel.cpp
- src/project/internal/projectvideosettings.cpp
- src/notationscene/widgets/timeline.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/engraving/style/textstyle.cpp (1)
1857-1864: 💤 Low valueConsider adding a defensive check for SYSTEM not found.
While SYSTEM should always be present in
_editableTextStyles, adding a defensive check would make the code more robust against future changes.🛡️ Optional defensive code
+ bool found = false; for (auto it = _editableTextStyles.begin(); it != _editableTextStyles.end(); ++it) { if (*it == TextStyleType::SYSTEM) { _editableTextStyles.insert(++it, TextStyleType::VIDEO_HIT_POINT); + found = true; break; } } + if (!found) { + LOGE() << "SYSTEM text style not found in editableTextStyles; VIDEO_HIT_POINT not inserted"; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/engraving/style/textstyle.cpp` around lines 1857 - 1864, The loop that reinserts TextStyleType::VIDEO_HIT_POINT assumes TextStyleType::SYSTEM exists in _editableTextStyles; add a defensive check so you only call _editableTextStyles.insert(++it, TextStyleType::VIDEO_HIT_POINT) when SYSTEM was actually found (e.g., track a found flag or use std::find to locate TextStyleType::SYSTEM), and avoid advancing/inserting if SYSTEM is not present to prevent invalid iterator use.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/engraving/style/textstyle.cpp`:
- Around line 1857-1864: The loop that reinserts TextStyleType::VIDEO_HIT_POINT
assumes TextStyleType::SYSTEM exists in _editableTextStyles; add a defensive
check so you only call _editableTextStyles.insert(++it,
TextStyleType::VIDEO_HIT_POINT) when SYSTEM was actually found (e.g., track a
found flag or use std::find to locate TextStyleType::SYSTEM), and avoid
advancing/inserting if SYSTEM is not present to prevent invalid iterator use.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ff2429f7-7fcf-4615-9c2c-c9790fab919c
📒 Files selected for processing (1)
src/engraving/style/textstyle.cpp
08a0bc4 to
3148bd5
Compare
|
Linux build: Hangs on splash screen. Ubuntu 26.04 |
Check the new build to see if it fixed it |
New build opens. Video panel is blank and lacks any controls or interface to load/display video. Operation of any video capability is unclear. |
Ok, I tried something, let me know how it goes, I'll be gone for a week, so i won't be able to make any updates until after that, but I will make any changes needed whenever I get back. |
It looks to be the same as before. I am assuming that there should be a viewer, controls, and a folder browser to load a video. But it comes up as a blank tab, as before. Does this PR access Qtmultimedia from the distro or is it containerized in the appimage? No worries on the time table. I'm just happy to help test and troubleshoot for Linux or general use so long as you need it. |
|
Ubuntu 24.04: |
7 participants
#33442
Summary
Adds a native score-to-video workflow to MuseScore Studio. Users can attach one reference video to a score, open a dockable Video panel, keep the video synced to MuseScore playback, control the video audio from the Mixer through a dedicated Video channel, place absolute video hit points, and view SMPTE-style timecode/hit-point information in the Video panel and Timeline.
Motivation
Composers working on film, games, animation, theater, dance, and other picture-locked media often need to write notation against video. Today that workflow usually requires running MuseScore beside a separate video player or DAW, which makes playhead sync, offset handling, save/load, hit-point spotting, and audio control fragile.
Implementation
videosettings.jsoninside the score package.Ctrl+Alt+Vto open/toggle the Video panel.PlaybackController, so it works even when the Mixer panel is not open.HH:MM:SS:FFformatting based on the stored video frame rate.Validation
MuseScoreStudio,project_test,playback_qml, andnotationscenetargets build successfully.project_testandplayback_testpass under CTest with the Qt runtime onPATH.git diff --checkreports no patch whitespace errors.Contributor Checklist