Conversation
Summary of ChangesHello @Alex2772, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the examples suite by adding a new 'synced audio playback' demonstration. It provides a practical, interactive application of the AUI Framework's audio capabilities, showcasing how to synchronize multiple audio streams, control their properties dynamically, and offer visual feedback on audio activity. This addition serves as a valuable reference for developers looking to implement complex audio interactions within their applications. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new example for synchronized audio playback. The implementation is well-structured and demonstrates several features of the AUI framework. I've found a few issues related to the correctness of the audio analysis and consistency in volume handling, along with a minor suggestion for improving code robustness. My detailed feedback is in the comments below.
| float prevFrame = std::numeric_limits<float>::infinity(); | ||
| float accumulator = 0.f; | ||
| for (int channel = 0; channel < channelCount; ++channel) { | ||
| for (size_t i = 0; i < mSamplesBuffer.size(); i += channelCount) { | ||
| float currentFrame = mSamplesBuffer[i + channel]; | ||
| AUI_DEFER { prevFrame = currentFrame; }; | ||
| if (prevFrame == std::numeric_limits<float>::infinity()) { | ||
| continue; | ||
| } | ||
| accumulator += glm::distance(currentFrame, prevFrame); | ||
| } | ||
| } |
There was a problem hiding this comment.
The prevFrame variable is initialized outside the channel loop. This means that for multi-channel audio, the analysis for the second channel (and subsequent channels) will incorrectly use the last sample from the previous channel for its initial calculation. This can lead to inaccurate gain analysis. To fix this, prevFrame should be re-initialized inside the loop for each channel.
float accumulator = 0.f;
for (int channel = 0; channel < channelCount; ++channel) {
float prevFrame = std::numeric_limits<float>::infinity();
for (size_t i = 0; i < mSamplesBuffer.size(); i += channelCount) {
float currentFrame = mSamplesBuffer[i + channel];
AUI_DEFER { prevFrame = currentFrame; };
if (prevFrame == std::numeric_limits<float>::infinity()) {
continue;
}
accumulator += glm::distance(currentFrame, prevFrame);
}
}| _<IAudioPlayer> player; | ||
| _<GainAnalysis> gainAnalysis; | ||
|
|
||
| AProperty<aui::audio::VolumeLevel> volume = 255; |
There was a problem hiding this comment.
The volume property is initialized to 255, but the underlying type aui::audio::VolumeLevel supports a range up to 256. Using 255 prevents the use of the full volume range, especially if the maximum is intended. Consider initializing to 256 to be consistent with the type definition and allow for maximum volume. This is also related to the slider logic where 255.f is used for scaling.
AProperty<aui::audio::VolumeLevel> volume = 256;| switch (column) { | ||
| case 0: return Horizontal { | ||
| _new<AView>() AUI_LET { | ||
| it->setFixedSize({10_dp, 0}); | ||
| AObject::connect(track->gainAnalysis->lastFrame, it, [&it = *it](float frame) { | ||
| it AUI_OVERRIDE_STYLE { | ||
| BackgroundSolid { glm::mix(glm::vec4(AColor::BLACK), glm::vec4(AColor::GREEN), frame) }, | ||
| }; | ||
| }); | ||
| }, | ||
|
|
||
| Centered { Label { layer == 0 ? "Base" : "Layer {}"_format(layer) } }, | ||
| } AUI_OVERRIDE_STYLE { LayoutSpacing { 4_dp } }; | ||
| case 1: return Slider { | ||
| .value = AUI_REACT(aui::float_within_0_1(float(*track->volume) / 255.f)), | ||
| .onValueChanged = [=](float v) { track->volume = v * 255.f; }, | ||
| }; | ||
| } | ||
| return nullptr; |
There was a problem hiding this comment.
The switch statement on column is implicitly exhaustive because ranges::view::iota(0, 2) only produces 0 and 1. However, the return nullptr; after the switch makes the control flow less clear. For better robustness and readability, consider adding a default case to the switch to handle unexpected values and make the exhaustiveness explicit.
switch (column) {
case 0: return Horizontal {
_new<AView>() AUI_LET {
it->setFixedSize({10_dp, 0});
AObject::connect(track->gainAnalysis->lastFrame, it, [&it = *it](float frame) {
it AUI_OVERRIDE_STYLE {
BackgroundSolid { glm::mix(glm::vec4(AColor::BLACK), glm::vec4(AColor::GREEN), frame) },
};
});
},
Centered { Label { layer == 0 ? "Base" : "Layer {}"_format(layer) } },
} AUI_OVERRIDE_STYLE { LayoutSpacing { 4_dp } };
case 1: return Slider {
.value = AUI_REACT(aui::float_within_0_1(float(*track->volume) / 255.f)),
.onValueChanged = [=](float v) { track->volume = v * 255.f; },
};
default:
return nullptr;
}| .value = AUI_REACT(aui::float_within_0_1(float(*track->volume) / 255.f)), | ||
| .onValueChanged = [=](float v) { track->volume = v * 255.f; }, |
There was a problem hiding this comment.
The volume slider logic uses 255.f for scaling. This is inconsistent with the aui::audio::VolumeLevel type which has a range of [0, 256]. Using 255.f prevents the slider from reaching the maximum possible volume. To ensure the full volume range is accessible, please use 256.f for scaling.
.value = AUI_REACT(aui::float_within_0_1(float(*track->volume) / 256.f)),
.onValueChanged = [=](float v) { track->volume = v * 256.f; },
1 participant
No description provided.