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

Fix pan and IRateConverters #281

Merged
merged 15 commits into from
Nov 12, 2023
Merged

Fix pan and IRateConverters #281

merged 15 commits into from
Nov 12, 2023

Conversation

Raffaello
Copy link
Owner

@Raffaello Raffaello commented Nov 12, 2023

Summary by CodeRabbit

  • New Features
    • Introduced a new version 0.16.0 with updated project description.
    • Added new member variables and functions to the IMixer class and related classes.
    • Introduced a new overload of the reset function, and new functions isActive and updateChannelsVolumePan_.
  • Bug Fixes
    • Fixed the stereo audio output by swapping the order of parameters in the output_channel function calls.
    • Improved the panning behavior of the audio mixer by swapping the calculation of m_volL and m_volR.
    • Enhanced the buffer size used for audio data storage by updating the resizing of the m_buf buffer.
  • Chores
    • Updated GitHub Actions workflow file to modify the configuration related to pull requests.

@Raffaello Raffaello self-assigned this Nov 12, 2023
Copy link

coderabbitai bot commented Nov 12, 2023

Walkthrough

The changes primarily involve renaming and reorganizing functions and variables related to audio processing in the sdl2-hyper-sonic-drivers project. The isPlaying function is renamed to isActive across multiple files, and the m_sampleRate variable is renamed to m_freq. Additionally, the stereo audio output and the audio mixer's panning behavior are modified.

Changes

File(s) Change Summary
CMakeLists.txt Updated project version and description.
sdl2-hyper-sonic-drivers/examples/... Increased delay before playing sounds and replaced drv.isPlaying with drv.isActive. Replaced isPlaying() with isActive() in loop conditions.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/converters/... Swapped order of parameters in output_channel function calls.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/mixer/Channel.cpp Swapped calculation of m_volL and m_volR and updated initialization of m_converter.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/... Added new member variables and functions, removed some functions and variables, and introduced m_reverseStereo.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/sdl2/Renderer.cpp Updated resizing of m_buf buffer.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/... Renamed isPlaying function to isActive and modified stop function.
.github/workflows/sonarcloud.yml Modified configuration related to pull requests.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/... Renamed m_sampleRate to m_freq and updated corresponding calculations and assertions.

Poem

🎵 With a hop and a skip, we've made a flip, 🐇

From isPlaying to isActive, a nifty script! 📜

In the spirit of fall, we've changed it all, 🍂

Celebrating the code, standing tall! 🎉


Tips

Chat with CodeRabbit Bot (@coderabbitai)

  • If you reply to a review comment from CodeRabbit, the bot will automatically respond.
  • To engage with CodeRabbit bot directly around the specific lines of code in the PR, mention @coderabbitai in your review comment
  • Note: Review comments are made on code diffs or files, not on the PR overview.
  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Commands (invoked as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Note: For conversation with the bot, please use the review comments on code diffs or files.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • The JSON schema for the configuration file is available here.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 3

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 1ceb2a6 and 48159d2.
Files selected for processing (5)
  • sdl2-hyper-sonic-drivers/examples/pcm-example.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/converters/CopyRateConverter.hpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/converters/LinearRateConverter.hpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/converters/SimpleRateConverter.hpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/mixer/Channel.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
  • sdl2-hyper-sonic-drivers/examples/pcm-example.cpp
Additional comments: 1
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/converters/SimpleRateConverter.hpp (1)
  • 95-101: Ensure that the reverseStereo flag is being set correctly throughout the codebase. Also, verify that the swapping of channels doesn't cause any unintended side effects in the audio output.

@Raffaello
Copy link
Owner Author

@coderabbitai help

Copy link

coderabbitai bot commented Nov 12, 2023

CodeRabbit Commands

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

@Raffaello
Copy link
Owner Author

@coderabbitai review

@Raffaello Raffaello added the enhancement New feature or request label Nov 12, 2023
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 7

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 48159d2 and 0fb30a7.
Files selected for processing (39)
  • CMakeLists.txt (1 hunks)
  • sdl2-hyper-sonic-drivers/examples/adl-example.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/examples/mid-example.h (1 hunks)
  • sdl2-hyper-sonic-drivers/examples/mt32-example.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/examples/pcm-example.cpp (3 hunks)
  • sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (2 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/IMixer.cpp (2 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/IMixer.hpp (3 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/mixer/Channel.cpp (2 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/sdl2/Mixer.cpp (6 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/sdl2/Mixer.hpp (2 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/sdl2/Renderer.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/PCMDriver.cpp (2 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/PCMDriver.hpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/IPCMFile.cpp (2 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/IPCMFile.hpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/VOCFile.cpp (4 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/WAVFile.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/IHardware.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/PCSpeaker.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/PCSpeaker.hpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/mt32/MT32.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/OPL.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/mame/MameOPL3.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/scummvm/dosbox/DosBoxOPL.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/scummvm/mame/MameOPL2.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/scummvm/nuked/NukedOPL3.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/woody/WoodyOPL.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/audio/IMixerMock.hpp (2 hunks)
  • sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/audio/sdl2/TestRenderer.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestIAudioDriver.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestMIDDriver.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestPCMDriver.cpp (4 hunks)
  • sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/hardware/TestPCSpeaker.cpp (2 hunks)
Files skipped from review due to trivial changes (11)
  • CMakeLists.txt
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/PCMDriver.hpp
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/IPCMFile.hpp
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/VOCFile.cpp
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/WAVFile.cpp
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/PCSpeaker.hpp
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/scummvm/dosbox/DosBoxOPL.cpp
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/scummvm/nuked/NukedOPL3.cpp
  • sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestIAudioDriver.cpp
  • sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/hardware/TestPCSpeaker.cpp
Additional comments: 46
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (1)
  • 46-46: Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
- bool isPlaying() const noexcept override;
+ bool isActive() const noexcept override;
sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestMIDDriver.cpp (1)
  • 88-101: The isPlaying method has been renamed to isActive. Ensure that this change is reflected throughout the codebase and that the new method name accurately represents its functionality. Also, verify that the change does not affect the logic of the test case.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/PCSpeaker.cpp (1)
  • 90-96: The function isPlaying has been renamed to isActive. Ensure that all calls to this function throughout the codebase have been updated to match the new function name.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.cpp (1)
  • 253-259: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [256-260]

The function isPlaying has been renamed to isActive. Ensure that all calls to this function throughout the codebase have been updated to match the new function name.

sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/mt32/MT32.cpp (1)
  • 131-136: Ensure that the change from m_mixer->getOutputRate() to m_mixer->freq is intentional and that m_mixer->freq is always initialized before this call. This change could affect the audio stream configuration.
- m_mixer->getOutputRate()
+ m_mixer->freq
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.cpp (1)
  • 234-240: The function name isPlaying has been changed to isActive. Ensure that all calls to this function throughout the codebase have been updated to match the new function name.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (1)
  • 33-33: Ensure that all calls to the previous isPlaying() function throughout the codebase have been updated to isActive(). Also, verify that the new isActive() function's behavior aligns with the intended use in all contexts where it's called.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/sdl2/Mixer.hpp (2)
  • 32-50: The new and renamed functions should be reflected in the documentation and all places where these functions are called should be updated accordingly. Also, ensure that the new reset function is implemented correctly and that it doesn't introduce any side effects.

  • 64-65: Moving updateChannelsVolumePan_ to the protected section might affect its accessibility in derived classes or friend classes. Ensure that this change doesn't break any existing functionality.

sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/audio/sdl2/TestRenderer.cpp (1)
  • 63-69: Ensure that the change from isPlaying() to isActive() does not alter the expected behavior of the loop. If isActive() has a different implementation, it might cause the loop to run longer or shorter than intended.
- while (drv1.isPlaying())
+ while (drv1.isActive())
sdl2-hyper-sonic-drivers/examples/mt32-example.cpp (1)
  • 44-50: The change from middrv.isPlaying() to middrv.isActive() may alter the behavior of the loop. Ensure that the isActive() function provides the expected behavior in this context.
- while (middrv.isPlaying())
+ while (middrv.isActive())
sdl2-hyper-sonic-drivers/examples/adl-example.cpp (1)
  • 62-65: Ensure that the change from isPlaying() to isActive() does not introduce any unintended side effects. The loop now continues while the driver is active rather than playing, which might alter the behavior of the loop.
- } while (adlDrv.isPlaying());
+ } while (adlDrv.isActive());
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (2)
  • 62-70: The isPlaying() function has been replaced with isActive(). Ensure that this change is reflected throughout the codebase and that the new function provides the expected behavior.

  • 404-410: The isPlaying() function has been replaced with isActive(). Ensure that this change is reflected throughout the codebase and that the new function provides the expected behavior.

sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/sdl2/Renderer.cpp (1)
  • 26-30: The change from m_mixer->getBufferSize() to m_mixer->buffer_size is noted. Ensure that m_mixer->buffer_size is always correctly initialized before this call. Also, verify that this change doesn't affect the rest of the codebase where getBufferSize() might have been used.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/OPL.cpp (1)
  • 33-38: Ensure that m_mixer->freq is correctly initialized and has the expected value before passing it to the EmulatedStream constructor. Also, verify that the change from m_mixer->getOutputRate() to m_mixer->freq doesn't introduce any issues, as it alters the frequency at which the audio stream's callback is initialized.
sdl2-hyper-sonic-drivers/examples/mid-example.h (1)
  • 32-37: The change from isPlaying() to isActive() may alter the behavior of the loop. Ensure that the isActive() method in the MIDDriver class accurately reflects the desired state for this loop to continue.
-    while (midDrv.isPlaying()) {
+    while (midDrv.isActive()) {
        utils::delayMillis(1000);
    }
sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/audio/IMixerMock.hpp (3)
  • 34-36: Ensure that all calls to the reset function throughout the codebase have been updated to match the new overloads.

  • 44-47: Ensure that all calls to the isActive function throughout the codebase have been updated to match the new overloads.

  • 63-63: The new function updateChannelsVolumePan_ has been added. Ensure that it is being used correctly in the codebase.

sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/IPCMFile.cpp (2)
  • 13-19: The switch statement now uses the local variable bitsDepth instead of the member variable m_bitsDepth. Ensure that bitsDepth is correctly initialized and updated before this switch statement.

  • 30-36: The PCMSound object is now constructed using the local variable freq instead of the member variable m_sampleRate. Ensure that freq is correctly initialized and updated before this object creation.

sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/woody/WoodyOPL.cpp (1)
  • 20-27: Ensure that the m_mixer->freq is always initialized before these lines of code are executed. If m_mixer->freq is not initialized, it could lead to undefined behavior.
sdl2-hyper-sonic-drivers/examples/pcm-example.cpp (3)
  • 42-46: The loop condition has been changed from drv.isPlaying(wavSound) to drv.isActive(wavSound). Ensure that the isActive function provides the expected behavior in this context.

  • 51-55: The loop condition has been changed from drv.isPlaying(vocSound) to drv.isActive(vocSound). Ensure that the isActive function provides the expected behavior in this context.

  • 70-73: The loop condition has been changed from drv.isPlaying() to drv.isActive(). Ensure that the isActive function provides the expected behavior in this context.

sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/IMixer.cpp (3)
  • 8-13: The constructor is now initializing freq and buffer_size directly. Ensure that all calls to this constructor throughout the codebase have been updated to match the new signature.

  • 20-24: The setChannelGroupVolume function now calls updateChannelsVolumePan_. Ensure that this does not introduce any side effects or performance issues, especially if setChannelGroupVolume is called frequently.

  • 26-35: Two new functions, getChannelGroupPan and setChannelGroupPan, have been added. Ensure that these functions are used correctly throughout the codebase and that their error handling (if any) is appropriate.

sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestPCMDriver.cpp (4)
  • 20-31: The function isPlaying has been replaced with isActive. Ensure that this change is reflected throughout the codebase and that the new function isActive is correctly implemented.

  • 41-52: The stop function now accepts an optional parameter. Ensure that this change is correctly implemented and reflected throughout the codebase.

  • 62-73: The stop function is called without any parameters. Ensure that the function is correctly implemented to handle this case.

  • 89-100: The function isActive is called after the stop function. Ensure that the isActive function correctly reflects the state of the driver after the stop function is called.

sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/IMixer.hpp (4)
  • 16-26: The new member variables max_channels, freq, buffer_size, and bitsDepth have been added. Ensure that these are initialized correctly in the constructor and used appropriately throughout the class. Also, the copy constructor and copy assignment operator have been deleted to prevent shallow copying of the object, which is a good practice for classes managing resources.

  • 45-60: The reset function now has two new overloads, one accepting a uint8_t and another accepting a mixer::eChannelGroup. The isActive function has also been overloaded to accept a mixer::eChannelGroup. Ensure that these new overloads are used correctly throughout the codebase.

  • 75-85: The getChannelGroupVolume, setChannelGroupVolume, getChannelGroupPan, setChannelGroupPan functions have been added. Also, a new member variable m_reverseStereo has been added with a corresponding function toggleReverseStereo. Ensure that these new functions and the variable are used correctly throughout the codebase.

  • 87-97: The updateChannelsVolumePan_ function has been added. Also, the m_group_settings array has been added. Ensure that these new additions are used correctly throughout the codebase.

sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/mixer/Channel.cpp (1)
  • 17-20: The initialization of m_converter now uses m_mixer.freq instead of m_mixer.getOutputRate(). Ensure that m_mixer.freq is always correctly set before this function is called.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/sdl2/Mixer.cpp (8)
  • 86-98: The reset function has been overloaded to reset channels based on a group. This could be useful for managing groups of channels together.

  • 130-133: The isChannelActive function has been renamed to isActive and now checks if a specific channel is active. This change should be reflected in all calls to this function.

  • 144-152: A new isActive function has been added to check if any channel is active. This could be useful for managing the state of the mixer as a whole.

  • 155-164: The isActive function has been overloaded to check if any channel in a specific group is active. This could be useful for managing groups of channels together.

  • 231-235: The updateChannelsVolumePan_ function has been added to update the volume and pan of all channels. This could be useful for managing the volume and pan of all channels together.

  • 237-239: The init_ function has been modified to use freq and buffer_size parameters. This change should be reflected in all calls to this function.

  • 246-254: The desired audio specs have been updated to use freq and buffer_size parameters. This could affect the audio output quality.

  • 286-288: The callback function has been modified to use a scoped lock. This could improve thread safety.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 0fb30a7 and 9890945.
Files selected for processing (1)
  • .github/workflows/sonarcloud.yml (1 hunks)
Additional comments: 3
.github/workflows/sonarcloud.yml (3)
  • 18-19: The branches key under push is commented out. This means that the workflow will run on push events to all branches. If you want to limit this to specific branches, uncomment and specify the branches.

  • 21-29: The pull_request event is now active, and the workflow will run on pull requests. However, it will ignore changes to the paths specified under paths-ignore. Ensure that this is the intended behavior.

  • 31-31: The permissions key is commented out. This means that the workflow will have the default permissions. If you want to limit the permissions, uncomment and specify the permissions.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 0

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 9890945 and b1ae500.
Files selected for processing (6)
  • sdl2-hyper-sonic-drivers/examples/pcm-example.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/IPCMFile.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/IPCMFile.hpp (1 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/VOCFile.cpp (2 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/WAVFile.cpp (1 hunks)
  • sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/hardware/TestPCSpeaker.cpp (1 hunks)
Files skipped from review due to trivial changes (3)
  • sdl2-hyper-sonic-drivers/examples/pcm-example.cpp
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/WAVFile.cpp
  • sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/hardware/TestPCSpeaker.cpp
Additional comments: 5
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/IPCMFile.cpp (1)
  • 30-36: The change from m_sampleRate to m_freq in the audio::PCMSound constructor call is noted. Ensure that this change is reflected everywhere m_sampleRate was previously used, and that the new m_freq variable is correctly initialized and used.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/VOCFile.cpp (2)
  • 74-79: The calculation of m_freq has been updated and the old calculation for m_sampleRate has been commented out. Ensure that this new calculation is correct and that it doesn't introduce any bugs in the system. Also, the assertion on line 79 should be updated or removed if it's no longer relevant.

  • 137-143: The calculation of m_freq, m_bitsDepth, and m_channels has been updated. Ensure that these changes are correct and that they don't introduce any bugs in the system.

sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/IPCMFile.hpp (2)
  • 20-21: The function getSampleRate now returns m_freq. Ensure that this change is reflected wherever this function is called, and that the semantics of m_freq match those of the original m_sampleRate.

  • 29-29: The member variable m_sampleRate has been renamed to m_freq. Ensure that this change is reflected wherever this variable is accessed or modified.

Copy link

sonarcloud bot commented Nov 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 3 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Review Status

Actionable comments generated: 1

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between b1ae500 and fe17e41.
Files selected for processing (2)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/sdl2/Mixer.cpp (7 hunks)
  • sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/IHardware.cpp (1 hunks)
Additional comments: 7
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/sdl2/Mixer.cpp (7)
  • 1-5: The inclusion of <ranges> is new. Ensure that the project's minimum supported compiler version supports C++20, which introduced this header.

  • 87-99: The new reset overload for channel groups is a good addition for better control over channel groups. However, ensure that the getChannelGroup function is thread-safe or consider making it const if it doesn't modify any class members.

  • 131-134: The renaming of isChannelActive to isActive is a breaking change. Ensure that all calls to this function throughout the codebase have been updated to match the new name.

  • 145-151: The new isActive overload without parameters is a good addition for checking if any channel is active. However, ensure that the isEnded function is thread-safe or consider making it const if it doesn't modify any class members.

  • 153-161: The new isActive overload for channel groups is a good addition for better control over channel groups. However, ensure that the getChannelGroup and isEnded functions are thread-safe or consider making them const if they don't modify any class members.

  • 242-251: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [234-251]

The init_ function now accepts freq and buffer_size parameters. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.

  • 283-285: The callback function now locks m_mutex at the start. This is a good practice to prevent data races, but ensure that no functions called within this function also try to lock m_mutex, as this could lead to a deadlock if the mutex is not recursive.

@Raffaello Raffaello merged commit db7430b into master Nov 12, 2023
8 checks passed
@Raffaello Raffaello deleted the fix_pan branch November 12, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

1 participant