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

CO Renaming (Pt. 5): Latency Control #12022

Merged
merged 5 commits into from
Sep 29, 2023
Merged

Conversation

Holzhaus
Copy link
Member

Part of #11931.

Moves the latency control into the [App] group.

@Holzhaus Holzhaus added the control objects Issues and bugs specifically in regard to mixxx `ControlObjects` label Sep 24, 2023
@Holzhaus Holzhaus added this to the 2.4.0 milestone Sep 24, 2023
@JoergAtGithub
Copy link
Member

It would be future proof to call it output_latency instead of just latency. We currently do not use the input-latency value that portaudio reports, but for more complex multi device sync scenarios it might be needed too.

@Holzhaus
Copy link
Member Author

It would be future proof to call it output_latency instead of just latency. We currently do not use the input-latency value that portaudio reports, but for more complex multi device sync scenarios it might be needed too.

@daschuer @Swiftb0y opinions?

@Swiftb0y
Copy link
Member

very much in favor.

@daschuer
Copy link
Member

Yes, you hit a sore point. Potaudio the output latency is the
"Internal buffering + Host API reported latency" the buffer size of one cycle also often regerenced as latency is not included.

Therfore it looks like the use in the Akai script is wrong.
I guess the want to compensate the buffer size (frames per buffer)
Do we even have a control for that?
Do other scripts compensate a buffer sycle?

I am in favor for the remaining. Do you want to fix the Akai script as well?

src/engine/enginemixer.cpp Outdated Show resolved Hide resolved
src/preferences/dialog/dlgprefsound.cpp Outdated Show resolved Hide resolved
@Holzhaus
Copy link
Member Author

I opted for output_latency_ms.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

I think the Akai mapping is still broken. The reported output_latency can be long and is unrelated to the internal processing.
I have found out where the "audio_buffer_size" CO is gone that is probably the correct use here:
https://github.com/mixxxdj/mixxx/pull/11121/files

But does the script really need that compensation? Why do we not see the use case in other mappings?

Did you consider how to continue?

@@ -29,7 +29,7 @@
[EffectRack1_EffectUnit4_Effect4],parameter4_set_default,0
[EffectRack1_EffectUnit2_Effect2],parameter2_link_type,0
[QuickEffectRack1_[Channel3]_Effect1],parameter5_set_zero,0
[Master],latency,5.80499
[App],output_latency_ms,5.80499
Copy link
Member

Choose a reason for hiding this comment

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

My dump button is broken. Does yours work? I will file a bug.

Copy link
Member Author

@Holzhaus Holzhaus Sep 29, 2023

Choose a reason for hiding this comment

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

My dump button is broken. Does yours work? I will file a bug.

Last time it check it did. It creates a file in ~/.mixxx. It does not create a save file dialog.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, right it is working. I just needed to look in my testing .mixxx folder 🤦

@Holzhaus
Copy link
Member Author

I think the Akai mapping is still broken. The reported output_latency can be long and is unrelated to the internal processing.
I have found out where the "audio_buffer_size" CO is gone that is probably the correct use here:
https://github.com/mixxxdj/mixxx/pull/11121/files

But does the script really need that compensation? Why do we not see the use case in other mappings?

Did you consider how to continue?

Yes, I considered it and decided that I won't mess with the mapping in this PR. I updated the control, but I don't plan other changes. The bug was not caused by this PR and I don't own this controller, so I cannot properly test changes.

@daschuer
Copy link
Member

OK, what puzzles me is the question if we need to revive the "audio_buffer_size" for such use cases. The bug in the Akai mapping itself is there or not, probably hardly notable. Do you have there a good idea for a bug report regarding the that?

@Holzhaus
Copy link
Member Author

Holzhaus commented Sep 29, 2023

OK, what puzzles me is the question if we need to revive the "audio_buffer_size" for such use cases.

The use case is "implementing beatjumps in JavaScript instead of using the proper Mixxx COs" ?

@daschuer
Copy link
Member

The use case is "implementing beatjumps in JavaScript instead of using the proper Mixxx COs" ?

Ok, that is clearly not a valid use case. So there is nothing else to do.
Thank you for this PR.

@daschuer
Copy link
Member

We can also ignore the Lint issues. Someone really needs to put some love on this mapping.

@daschuer daschuer merged commit 17362a7 into mixxxdj:2.4 Sep 29, 2023
12 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code quality control objects Issues and bugs specifically in regard to mixxx `ControlObjects` controller mappings ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants