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

Improved comments in enginecontrol and use of std::size_t for bufferSize accross the codebase #13819

Merged

Conversation

JoergAtGithub
Copy link
Member

This is a spin-off of #13172 (comment) to keep the changes not related to Macros out of that PR.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

two quick notes:

  1. I'm fairly certain size_t is the C nomenclature, the proper C++ name/type live in std, so std::size_t.
  2. While I prefer to use std::size_t when doing indexing especially in realtime code for compatibility with std containers, it does go against Qt's mantra of using int (at least in their external APIs). Previous experiments have also shown that MSVC is apparently worse at autovectorizing buffer indexes that use size_ts rather than ints, which was the reason to typedef SINT to int rather than size_t.

I'd prefer if you can take care of 1.
2. I just wanted to mention so you're aware even though I don't personally think its a strong argument against size_t.

@JoergAtGithub JoergAtGithub force-pushed the optimize_enginecotrol_and_process branch from 539df3f to 748163d Compare November 1, 2024 13:58
@JoergAtGithub JoergAtGithub changed the title Improved comments in enginecontrol and use of size_t for bufferSize accross the codebase Improved comments in enginecontrol and use of std::size_t for bufferSize accross the codebase Nov 1, 2024
@JoergAtGithub JoergAtGithub force-pushed the optimize_enginecotrol_and_process branch from 748163d to 7f2247f Compare November 1, 2024 14:30
@daschuer
Copy link
Member

daschuer commented Nov 1, 2024

typedef std::ptrdiff_t SINT;

And Qt used int in Qt5 an ssize_t form Qt6
https://doc.qt.io/qt-6/qttypes.html#qsizetype-typedef

We have also a pending discussion to pass all buffers as a std::span, wich includes std::size_t a size.

This however goes against our alternative attempt to pass buffers without the stereo assumption. This issue is also visible in the #13654

So maybe we want an own span type with a frame size?

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

couple more comments. LGTM otherwise though

src/encoder/encodermp3.h Outdated Show resolved Hide resolved
Comment on lines 63 to 64
SINT numSamplesLeft = bufferSize;
while (numSamplesLeft > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

was this a confused clang-format or where is the bogus indentation from?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is not clang-format itself. I've clang-format support enabled in VisualStudio and it formats it like this:
grafik
but if I ran pre-commit, it looks like this:
grafik

I will not address this in this PR!

Copy link
Member

Choose a reason for hiding this comment

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

I will not address this in this PR!

Not forcing you to, though I wonder whether it makes sense to just enforce yourself and skip clang-format...

Copy link
Member Author

Choose a reason for hiding this comment

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

This I can do, but not fixing the script

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! CI fails now as expected.

Copy link
Member

Choose a reason for hiding this comment

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

yeah thats fine for me. Since it only runs on the diff we shouldn't be bothered by it again after the merge, right?

src/encoder/encodermp3.cpp Outdated Show resolved Hide resolved
src/engine/channelmixer.cpp Show resolved Hide resolved
src/engine/channels/enginedeck.cpp Outdated Show resolved Hide resolved
src/engine/filters/enginefilterdelay.h Outdated Show resolved Hide resolved
@Swiftb0y
Copy link
Member

Swiftb0y commented Nov 1, 2024

So maybe we want an own span type with a frame size?

In the long-term, yes. As I pointed out previously std::mdspan would likely be the good solution here, we just need to wait for GCC to implement it (or use Kokkos::mdspan).

@Swiftb0y
Copy link
Member

Swiftb0y commented Nov 1, 2024

I think transitioning to std::size_t and std::span in the meantime is a good enough solution. In actual audio code, we may want to leverage std::simd anyways (which is currently being proposed for standardization in C++26).

…f the name at many related parts of the code
@JoergAtGithub JoergAtGithub force-pushed the optimize_enginecotrol_and_process branch from 7f2247f to 220ce2e Compare November 2, 2024 09:23
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

LGTM otherwise. Thank you.

src/engine/channels/enginedeck.cpp Outdated Show resolved Hide resolved
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

Thank you. LGTM.

@Swiftb0y Swiftb0y merged commit c7bf876 into mixxxdj:main Nov 3, 2024
12 of 13 checks passed
@JoergAtGithub JoergAtGithub deleted the optimize_enginecotrol_and_process branch November 3, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants