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

Initial implementation of MTC #6164

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

jonata
Copy link
Contributor

@jonata jonata commented Mar 24, 2024

Resolves: #6159, #5418, #37, #2167, #1023, helps #5038

This enables Audacity to send MTC through PortMidi. Of course, I'm not sure if the TimeToolBar is the best place to put it, but for the user perspective, it make sense that it would be "sent" by the time toolbar. This adds a checkbox on a context menu and also adds an option on Playback panel on Preferences. We would need to find a way to expose the video framerate selection for the user, though. Another thing to consider is that it's selecting the "Default" device to send. Maybe we should expose an option for the user to choose..?

To test, open the audio of a video in Audacity and open XJadeo. In XJadeo, select the Sync mode to MTC (Portmidi) and enable the option on Audacity. If video is not sinchronized, that's probably because the video framerate is hardcoded to 29.97. Launching XJadeo with the option "-M 1" should work around this.

Of couse, PLEASE, give me feedback on the code as I'm a Python developer, not C++. Thanks!!

  • I signed CLA
  • The title of the pull request describes an issue it addresses
  • If changes are extensive, then there is a sequence of easily reviewable commits
  • Each commit's message describes its purpose and effects
  • There are no behavior changes unnecessary for the stated purpose of the PR

Recommended:

  • Each commit compiles and runs on my machine without known undesirable changes of behavior

@LWinterberg
Copy link
Member

image
Right-clicking the bulk of the time toolbar opens the time format picker; even knowing it would be there I needed a while to figure out that I'd need to right-click the place where the time display is not. I don't think it's viable to have a right-click menu in there.

image
I have no strong opinion on where in the preferences this option goes; though given that it a) probably also should be sent during recording and b) it is called MIDI time code, I'd say it probably has a better home inside the "MIDI devices" section of the preferences (which may just get renamed to just "MIDI" as well).

Code-wise, I'm not sure if this code wants to be part of the Time toolbar, maybe it should be its own little lib-mtc library instead. @crsib or @vsverchinsky may have stronger opinions on that.

@jonata
Copy link
Contributor Author

jonata commented Apr 2, 2024

Thanks for the response. I agree with you. Just explaining more about why I did this way, but I agree with you:

  • For MTC, we would need to select what is the video FPS in some way. That's why I put a dropdown menu on Time Toolbar. And as you mentioned, it's naturally hidden because in practice, not all users would need to use this feature. But the user would need to be able to select the FPS for each video he wants to sync. And I don't think a good idea to put this option only on Preferences window, as the user would need to select the FPS every time he opens a project. But maybe a dropdown menu on main menubar?
  • For the settings option, as I mentioned, although it is a MIDI protocol, this feature is not related to "music" (as the user would expect by looking into the MIDI panel).
  • For the Toolbar placement of the code, that's mainly because I'm not familiar with Audacity code and C++ in general. I just thought that it would be easier for the user to select there, as I noted, the FPS for the loaded video.

Thanks!!!

@LWinterberg
Copy link
Member

We could structure the menu similar to the snapping options right next-door and put things into submenus instead:

image

This design assumes you still want your #316 in still, and further assumes that a 30fps timecode would be added. When switching between different formats, the MTC would need to slightly lie (eg sending NTSC drop frames even though non-drop frames are selected) but overall it'd keep things in sync.

@jonata
Copy link
Contributor Author

jonata commented Apr 2, 2024

Wow, awesome! Yes, that is an excellent solution. I'll look into that.

@@ -152,6 +152,7 @@ void PlaybackPrefs::PopulateOrExchange(ShuttleGui & S)
S.TieCheckBox(XXO("Always scrub un&pinned"),
{UnpinnedScrubbingPreferenceKey(),
UnpinnedScrubbingPreferenceDefault()});
S.TieCheckBox(XXO("Send MTC (MIDI Time Code)"), {"/AudioIO/SendMTC", true});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would make a named BoolSetting object in this MidiIO.cpp, declared in a header, to avoid repetition of the string literal. See other examples. Specify the preference path and default value in just one place.

static_cast<unsigned char>(minutes), static_cast<unsigned char>(seconds),
static_cast<unsigned char>(frames), 0xF7};

Pm_WriteSysEx(portMidiStream, 0, message);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t like having all this in a toolbar file, whose purpose is UI.

Rather add to MidiIO.cpp and its header file and make calls into that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where could it trigger the MTC send function?

bool isPortMidiInitialized = false;
void InitializePortMidi();
void TerminatePortMidi();
void UpdatePortMidiAccordingToPreferences();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function isn’t reached when the state of the checkbox is changed in PlaybackPrefs.

You could call it from the Commit method of the preference page.

Again, move the functions that do all the real business to a file whose job is not GUI. Neither the toolbar nor preference page should depend on the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add methods to sync with external programs
3 participants