review ADLFile #285
review ADLFile #285
Conversation
WalkthroughRefactors ADL handling to use 16-bit counts/indices, adds ADL version/offset accessors, expands driver track parameter widths to uint16_t, adds an ADL playback example and SDL test harness changes (new tests and event loop), updates several header includes (cstring/atomic), bumps project version and Sonar key, upgrades GitHub Actions cache to v4, and updates .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as ADLPlay / Test harness
participant SDL as SDL (Window & Events)
participant Mixer as Audio Mixer
participant ADLFile as ADLFile
participant OPL as OPL Device
participant Driver as ADLDriver
User->>App: launch example/test
App->>SDL: init window & events
App->>Mixer: init audio mixer
App->>ADLFile: load ADL (read header/data)
App->>OPL: create OPL device
App->>Driver: construct ADLDriver (OPL + Mixer)
loop playback
SDL->>App: input (LEFT/RIGHT/ESC)
App->>Driver: play(track) / stop
Driver->>OPL: write programs / tones
Driver->>Mixer: push audio frames
Mixer-->>SDL: audio callback
end
User-->>SDL: ESC / quit
App->>SDL: destroy window / quit subsystems
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (2 hunks)
- sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/files/westwood/TestADLFile.cpp (1 hunks)
Files skipped from review due to trivial changes (1)
- sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp
Additional comments: 1
sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/files/westwood/TestADLFile.cpp (1)
- 59-59: The expected number of tracks has been updated from a previous unspecified value to 200. This is a significant change and should be verified to ensure that the
ADLFileclass is intended to support this many tracks. If this is a new feature, ensure that the implementation inADLFileis capable of handling 200 tracks without issues.Verification successful
The search results indicate that the
getNumTracksmethod is indeed present in theADLFileclass and that there are several test cases with different expected values for the number of tracks, including the value 200 that was mentioned in the review comment. This suggests that theADLFileclass is expected to handle 200 tracks, at least in the context of testing.Based on the information provided:
- The
ADLFile::getNumTracksmethod exists.- The test suite includes a test that expects 200 tracks, which aligns with the change mentioned in the review comment.
Therefore, the change in the test suite to expect 200 tracks from the
ADLFileclass appears to be consistent with the test cases present in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that the ADLFile class has been updated to support 200 tracks. rg --type cpp "getNumTracks" src/HyperSonicDrivers/files/westwood/ADLFile.cppLength of output: 166
Script:
#!/bin/bash # Re-run the search for the getNumTracks method in the ADLFile.cpp file with the correct path. rg --type cpp "getNumTracks" HyperSonicDrivers/files/westwood/ADLFile.cppLength of output: 158
Script:
#!/bin/bash # Find the ADLFile.cpp file and search for the getNumTracks method. fd ADLFile.cpp --exec rg "getNumTracks" {}Length of output: 256
sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/files/westwood/TestADLFile.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (1)
4-8: Missing standard headers for used APIs: add and .std::format and std::filesystem are used later; compilation will fail without these includes.
Apply this diff:
#include <iostream> #include <memory> +#include <format> +#include <filesystem> #include <SDL2/SDL.h>
🧹 Nitpick comments (4)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/midi/IMidiChannelVoice.cpp (1)
18-21: Prefer std::clamp with widened arithmetic for clarity and safety.Avoids template gymnastics on std::min<uint8_t> and makes the bounds explicit while keeping the intermediate computation in int.
Apply this diff:
- return std::min<uint8_t>(static_cast<uint8_t>(m_volume * m_channel->volume / 127), 127); + // clamp the product after widening to int, then narrow to uint8_t + return static_cast<uint8_t>( + std::clamp<int>( + (static_cast<int>(m_volume) * static_cast<int>(m_channel->volume)) / 127, + 0, 127 + ) + );sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (3)
386-407: Event loop responsiveness: poll before sleeping and avoid noisy default logging.Polling first reduces input latency; the default branch prints every event type and can spam logs.
Apply this diff:
- while (drv.isPlaying()) - { - utils::delayMillis(200); - SDL_Event e; + while (drv.isPlaying()) + { + SDL_Event e; while (SDL_PollEvent(&e)) switch (e.type) { @@ default: - std::cout << "event: " << e.type << std::endl; + break; // ignore } + utils::delayMillis(10); }
417-467: vocdune2filestest(): remove unused variable, add tiny sleep in loop, and ensure SDL cleanup.
- device is unused.
- Tight loop may spin hot while audio plays; add a small delay.
- Add SDL_QuitSubSystem on exit, like the ADL test.
Apply this diff:
- auto device = devices::make_device<devices::Adlib, devices::Opl>(mixer); drivers::PCMDriver drv(mixer); @@ - while (drv.isPlaying()) + while (drv.isPlaying()) { - //utils::delayMillis(200); + utils::delayMillis(5); SDL_Event e; while (SDL_PollEvent(&e)) @@ QUIT: SDL_DestroyWindow(window); + SDL_QuitSubSystem(SDL_INIT_VIDEO); + SDL_QuitSubSystem(SDL_INIT_EVENTS);
478-481: Main calls adldune2filestest(): consider gating test harness under a build option.Keeps test code out of production binaries and avoids accidental UI popups.
Would you like a small CMake option (e.g., -DBUILD_DRIVER_TESTS=ON) plus ifdef guards to compile/enable these entry points only in test builds?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp(4 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/midi/IMidiChannelVoice.cpp(1 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp
🧰 Additional context used
🪛 GitHub Check: Codacy Static Code Analysis
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/midi/IMidiChannelVoice.cpp
[warning] 3-3: sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/midi/IMidiChannelVoice.cpp#L3
Include file: not found. Please note: Cppcheck does not need standard library headers to get proper results.
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp
[warning] 39-39: sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp#L39
Include file: <HyperSonicDrivers/files/VOCFile.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (6)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/midi/IMidiChannelVoice.cpp (2)
3-3: Good switch to for std::min.Header choice is correct for the current implementation.
3-3: Codacy warning “ not found” looks spurious.This is a standard header. If the warning persists, it’s likely a toolchain configuration issue on Codacy rather than a code problem.
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (4)
134-135: Mix_Hook vs Mix_HookMusic: confirm target SDL_mixer API.This block is commented-out, but “Mix_Hook” is not part of SDL_mixer 2.x public API. If you plan to enable this path, double-check against your SDL_mixer version and headers.
Would you like me to cross-check the exact function name against the version you’re targeting and propose a compatible snippet?
39-39: Include path verified
- The header
VOCFile.hppis present atsrc/HyperSonicDrivers/files/VOCFile.hpp, matching the include directive.- No changes are needed; the build system should resolve this path correctly.
359-415: Build configuration already enforces C++20The
sdl2-hyper-sonic-drivers/CMakeLists.txtincludes:set(CMAKE_CXX_STANDARD 20)which ensures the code is compiled with C++20 support, covering the usage of
std::format. No further changes are required.
359-415: adldune2filestest(): tighten initialization, remove dead loop, ensure cleanup, and confirm track indexing
- Add
#include <format>at the top to resolve uses ofstd::format.- Guard mixer startup: check the return of
mixer->init()(and optionallymixer->isReady()) and log+return on failure.- Eliminate the redundant
for (int f = 0; f <= 0; f++)loop—just setfndirectly.- After destroying the SDL window, call:
SDL_QuitSubSystem(SDL_INIT_VIDEO);SDL_QuitSubSystem(SDL_INIT_EVENTS);- Verify whether tracks are 0-based. If so, change
tofor (int i = 1; i < static_cast<int>(adlf->getNumTracks()); i++)for (int i = 0; i < static_cast<int>(adlf->getNumTracks()); ++i) // or at least document why track 0 is skippedSuggested diff:
+#include <format> void adldune2filestest() { auto mixer = audio::make_mixer<audio::sdl2::Mixer>(8, 44100, 1024); - mixer->init(); + if (!mixer->init() || !mixer->isReady()) { + utils::ILogger::instance->error("Mixer initialization failed", + utils::ILogger::eCategory::Application); + return; + } @@ - for (int f = 0; f <= 0; f++) - { - const std::string fn = "adl/KYRA1A.ADL"; + // Play a single ADL file + const std::string fn = "adl/KYRA1A.ADL"; utils::ILogger::instance->info(std::format("opening file: {}", fn), utils::ILogger::eCategory::Application); @@ - for (int i = 1; i < static_cast<int>(adlf->getNumTracks()); i++) + // TODO: confirm indexing; if tracks are 0-based, start at 0 + for (int i = 1; i < static_cast<int>(adlf->getNumTracks()); ++i) { … } @@ SDL_DestroyWindow(window); + SDL_QuitSubSystem(SDL_INIT_VIDEO); + SDL_QuitSubSystem(SDL_INIT_EVENTS); }
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (3)
70-71: Use fmt consistently or include
std::formatrequires<format>and C++20 library support. Given you're already usingfmt, preferfmt::formatthroughout or add the missing header and ensure C++20 is enabled.Option A (preferred): keep using
fmt::formateverywhere (already included viafmt/color.h).
Option B: if you keepstd::format, add:-#include <fmt/color.h> +#include <fmt/color.h> +#include <format>
73-75: Avoid busy-spin: wait for events with a short timeoutThe outer loop runs continuously and
SDL_PollEventcan return false, causing a tight spin that burns a core. UseSDL_WaitEventTimeoutor add a tiny delay.Minimal change:
- SDL_Event e; - while (SDL_PollEvent(&e)) + SDL_Event e; + while (SDL_PollEvent(&e)) { ... } + SDL_Delay(1); // prevent busy loop when no eventsOr preferred:
- SDL_Event e; - while (SDL_PollEvent(&e)) + SDL_Event e; + while (SDL_WaitEventTimeout(&e, 10)) { ... }
73-104: Graceful termination on window closeConsider handling
SDL_QUITto allow closing the window to stop playback without using Esc.- while (SDL_PollEvent(&e)) + while (SDL_PollEvent(&e)) { - if (e.type == SDL_KEYDOWN) + if (e.type == SDL_QUIT) { + adlDrv.stop(); + return; + } else if (e.type == SDL_KEYDOWN) { switch (e.key.keysym.sym) {sdl2-hyper-sonic-drivers/examples/CMakeLists.txt (1)
90-101: Linking SDL2 for event loop is correct; consider copying SDL runtime if neededAdding
SDL2::SDL2toADLPlayis appropriate since the example uses SDL event APIs. If you plan to run the installed example standalone, you may also need to copySDL2runtime alongside the executable on some platforms (Windows/macOS).Example (Windows-only, optional) to copy SDL2 DLL next to the exe:
add_dependencies(${EXAMPLE_EXE} ${EXAMPLE_DEPS}) target_link_libraries(${EXAMPLE_EXE} ${EXAMPLE_LINKS}) if(${EXAMPLE_COPY_LIB}) @@ endif() +if(WIN32) + add_custom_command(TARGET ${EXAMPLE_EXE} POST_BUILD + COMMAND ${CMAKE_COMMAND} -E copy_if_different + $<TARGET_FILE:SDL2::SDL2> + $<TARGET_FILE_DIR:${EXAMPLE_EXE}> + ) +endif()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
sdl2-hyper-sonic-drivers/examples/CMakeLists.txt(1 hunks)sdl2-hyper-sonic-drivers/examples/adl-play.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Codacy Static Code Analysis
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
168-171: Fix ADLFile to correctly support 16-bit track IDs for v3The root issue is that even if you widen the
getTrackAPI, the underlying containerm_headeris stillstd::vector<uint8_t>, so any values ≥256 are truncated as they’re read. We need to store full 16-bit values and expose them in the public API.Please apply these mandatory refactors:
• sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp
- Change the header storage to 16 bits
- Widen the
getTrackdeclaration--- a/src/HyperSonicDrivers/files/westwood/ADLFile.hpp +++ b/src/HyperSonicDrivers/files/westwood/ADLFile.hpp @@ - // TODO: This is wrong for v3 as it is uint16_t instead! - std::vector<uint8_t> m_header; + // Store full 16-bit IDs so v3 entries ≥256 aren’t lost + std::vector<uint16_t> m_header; @@ - uint8_t getTrack(const int track) const; + uint16_t getTrack(const int track) const;• sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp
- Widen the
getTrackdefinition--- a/src/HyperSonicDrivers/files/westwood/ADLFile.cpp +++ b/src/HyperSonicDrivers/files/westwood/ADLFile.cpp @@ - uint8_t ADLFile::getTrack(const int track) const + uint16_t ADLFile::getTrack(const int track) const { return m_header.at(track); }• sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp
- Ensure
readHeaderFromFile_still works, since it already reads 16-bit values via theread()callbackNo other call sites need changing: callers in ADLDriver already assign to
uint16_t, and tests compare to integer literals (<256) or to0xFF/0xFFFF, which remain correct.
♻️ Duplicate comments (4)
sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/files/westwood/TestADLFile.cpp (1)
23-23: Clarify semantics: getNumTracks now returns header capacity, not “actual subsongs”.The new expectations (120/250) mirror m_meta_version.num_headers. That’s fine, but several headers contain many empty/invalid entries, so iterating 0..getNumTracks()-1 will hit empty tracks. Please confirm all call sites that loop by getNumTracks() either:
- filter out invalid entries (0xFF for v1/v2, 0xFFFF for v3), or
- switch to a separate accessor that returns only “valid/occupied” headers.
Consider adding a companion test that asserts selected indices map to valid program IDs (< getNumTrackOffsets()) to prevent regressions.
Example test additions:
@@ TEST(ADLFile, ADLv2) - EXPECT_EQ(f.getTrack(2), 3); + EXPECT_EQ(f.getTrack(2), 3); + // Guard: header program id must be within available offsets + EXPECT_LT(static_cast<int>(f.getTrack(2)), f.getNumTrackOffsets()); @@ TEST(ADLFile, ADLv3) EXPECT_EQ(f.getVersion(), 3); EXPECT_EQ(f.getNumTracks(), 250); + // v3 IDs are 16-bit; ensure the chosen entry maps to a valid offset range + EXPECT_LT(static_cast<int>(f.getTrack(0)), f.getNumTrackOffsets());Also applies to: 36-36, 59-59, 81-81
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (3)
70-71: std::format vs fmt::format mismatch; include or standardize.You use fmt elsewhere; std::format requires and C++20 library support. For consistency, switch to fmt::format.
- ILogger::instance->info(std::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), ILogger::eCategory::Application); + ILogger::instance->info(fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), ILogger::eCategory::Application);
85-101: Handle LEFT/RIGHT by playing immediately and logging; avoid deferred restart.Currently you stop channels and adjust index, but rely on the outer loop to restart playback. Play immediately for snappier UX and log consistently.
case SDLK_RIGHT: { - adlDrv.stopAllChannels(); - track++; - if (track >= adlFile->getNumTracks()) - track = 0; + const auto numTracks = adlFile->getNumTracks(); + track = (track + 1) % numTracks; + adlDrv.stopAllChannels(); + adlDrv.play(track); + HyperSonicDrivers::utils::ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), numTracks), + HyperSonicDrivers::utils::ILogger::eCategory::Application + ); break; } case SDLK_LEFT: { - adlDrv.stopAllChannels(); - if (track > 0) - track--; - else - track = adlFile->getNumTracks() - 1; + const auto numTracks = adlFile->getNumTracks(); + track = (track + numTracks - 1) % numTracks; + adlDrv.stopAllChannels(); + adlDrv.play(track); + HyperSonicDrivers::utils::ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), numTracks), + HyperSonicDrivers::utils::ILogger::eCategory::Application + ); + break; }
111-114: Initialize SDL events/audio for reliability on all platforms.SDL_KEYDOWN is more reliable with SDL_INIT_EVENTS; some backends also expect SDL_INIT_AUDIO when using custom mixers. You already create a window; consider:
- if (SDL_Init(SDL_INIT_VIDEO) != 0) + if (SDL_Init(SDL_INIT_VIDEO | SDL_INIT_EVENTS | SDL_INIT_AUDIO) != 0) return -1;
🧹 Nitpick comments (8)
.github/workflows/sonarcloud.yml (1)
56-71: Good move to actions/cache@v4; consider aligning other core actions.Cache bump looks right. For consistency and maintenance, also consider upgrading:
Optional, but it avoids older runner behavior and deprecation churn.
Suggested minimal change:
- - uses: actions/checkout@v2 + - uses: actions/checkout@v4 @@ - - name: Set up JDK 17 - uses: actions/setup-java@v1 + - name: Set up JDK 17 + uses: actions/setup-java@v4 with: java-version: 17 + distribution: temurinsdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (4)
114-124: Track count = header capacity; consider exposing “occupied tracks” as well.m_num_tracks = m_meta_version.num_headers aligns with updated tests, but it blurs capacity vs. actual content. Suggest keeping this as-is for compatibility and adding an additional computed count (e.g., getNumOccupiedTracks()) that scans m_header for entries < m_meta_version.num_track_offsets (and != 0xFF/0xFFFF). This helps UIs/players skip empties without hardcoding sentinels.
Sketch:
+int ADLFile::getNumOccupiedTracks() const noexcept +{ + const auto maxId = m_meta_version.num_track_offsets; + int last = -1; + for (int i = 0; i < m_meta_version.num_headers; ++i) { + const auto id = m_header[i]; + if (id < maxId) last = i; + } + return last + 1; // 0..last inclusive +}
140-156: Remove stale commented logic or gate behind a debug helper.The commented “find last subsong” block is obsolete and can mislead future readers. Either delete or move into a small local debug helper (disabled by default). Keeps implementation intent clear.
284-289: Make header reading explicit by width; fix typos.The TODO accurately captures the need. Two small, actionable tweaks:
- Provide width via a template rather than passing a reader function; it’s faster and clearer.
- Typos: “greter” → “greater”.
Example:
-void ADLFile::readHeaderFromFile_(const int header_size, std::function<uint16_t()> read) +template<typename T> +void ADLFile::readHeaderFromFile_(const int header_size) { - m_header.resize(header_size); - // TODO: pass the byte size instead of a function read and read at once, - // and do read in LE when is greter than 1, basically if version 3 - for (int i = 0; i < header_size; i++) - { - m_header[i] = read(); - } + m_header.resize(header_size); + for (int i = 0; i < header_size; i++) { + if constexpr (sizeof(T) == 1) m_header[i] = readU8(); + else m_header[i] = readLE16(); + } assertValid_(m_header.size() == header_size); }And call with:
- readHeaderFromFile_<uint8_t>(m_meta_version.num_headers) for v1/v2
- readHeaderFromFile_<uint16_t>(...) for v3
307-315: Typo: parameter name “data_heder_size”.Minor readability nit: rename to data_header_size.
-void ADLFile::readDataFromFile_(const int data_offsets, const int data_heder_size) +void ADLFile::readDataFromFile_(const int data_offsets, const int data_header_size) { m_dataSize = size() - data_offsets; @@ - m_dataHeaderSize = data_heder_size; + m_dataHeaderSize = data_header_size; }sdl2-hyper-sonic-drivers/examples/adl-play.cpp (3)
55-55: Use a larger signed index for tracks and fix the stale comment.uint8_t silently wraps and the comment says “starting from 2” while the value is 5. Prefer size_t or int and start from 0 unless there’s a reason.
- uint8_t track = 5; // starting from 2 + size_t track = 0; // 0-based start
65-72: Optional: auto-advance when a song ends.As written, when a track finishes, you replay the same track. Consider auto-advancing:
- if (!adlDrv.isPlaying()) - { - adlDrv.play(track); - ILogger::instance->info(fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), ILogger::eCategory::Application); - } + if (!adlDrv.isPlaying()) { + const auto n = adlFile->getNumTracks(); + track = (track + 1) % n; + adlDrv.play(track); + ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), n), + ILogger::eCategory::Application + ); + }
96-102: Add an explicit break after the LEFT case.Not required since it’s the last case, but adding break avoids future fall-through bugs if more cases are appended.
case SDLK_LEFT: { adlDrv.stopAllChannels(); if (track > 0) track--; else track = adlFile->getNumTracks() - 1; + break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.github/workflows/ci-windows.yml(1 hunks).github/workflows/sonarcloud.yml(1 hunks)sdl2-hyper-sonic-drivers/examples/adl-play.cpp(1 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp(3 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp(2 hunks)sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/files/westwood/TestADLFile.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp
🧰 Additional context used
🧬 Code graph analysis (1)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
ADLFile(94-133)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (3)
ADLFile(35-35)ADLFile(36-36)track(44-44)
🪛 GitHub Check: Codacy Static Code Analysis
sdl2-hyper-sonic-drivers/examples/adl-play.cpp
[warning] 1-1: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L1
Include file: <HyperSonicDrivers/audio/sdl2/Mixer.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 2-2: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L2
Include file: <HyperSonicDrivers/hardware/opl/OPL.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 3-3: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L3
Include file: <HyperSonicDrivers/hardware/opl/OPLFactory.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 4-4: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L4
Include file: <HyperSonicDrivers/utils/algorithms.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 5-5: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L5
Include file: <HyperSonicDrivers/files/westwood/ADLFile.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 6-6: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L6
Include file: <HyperSonicDrivers/drivers/westwood/ADLDriver.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 7-7: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L7
Include file: <HyperSonicDrivers/utils/ILogger.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 8-8: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L8
Include file: <HyperSonicDrivers/devices/Adlib.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 9-9: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L9
Include file: <HyperSonicDrivers/devices/SbPro.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 10-10: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L10
Include file: <HyperSonicDrivers/devices/SbPro2.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 12-12: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L12
Include file: <spdlog/spdlog.h> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 13-13: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L13
Include file: <fmt/color.h> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 15-15: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L15
Include file: <SDL2/SDL.h> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 17-17: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L17
Include file: not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 18-18: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L18
Include file: not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 19-19: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L19
Include file: not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 20-20: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L20
Include file: not found. Please note: Cppcheck does not need standard library headers to get proper results.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build Windows (x64-windows, Release, 2022)
- GitHub Check: Build Windows (x64-windows, Debug, 2022)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
🔇 Additional comments (1)
.github/workflows/ci-windows.yml (1)
70-76: actions/cache@v4 upgrade LGTM.Cache key/restore-keys are kept stable across the matrix and scoped to vcpkg packages; this is a straightforward, correct bump.
Docstrings generation was requested by @Raffaello. * #285 (comment) The following files were modified: * `sdl2-hyper-sonic-drivers/examples/adl-play.cpp` * `sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp` * `sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp` * `sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/files/westwood/TestADLFile.cpp`
|
Note Generated docstrings for this pull request at #286 |
Docstrings generation was requested by @Raffaello. * #285 (comment) The following files were modified: * `sdl2-hyper-sonic-drivers/examples/adl-play.cpp` * `sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp` * `sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp` * `sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/files/westwood/TestADLFile.cpp` Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Raffaello
added
example
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (1)
351-367: Harden pcm_sound_append: check init result and remove unused variablemixer->init() is unchecked and s1b is never used.
Apply this diff:
void pcm_sound_append() { auto mixer = audio::make_mixer<audio::sdl2::Mixer>(8, 44100, 1024); files::WAVFile w1("test/fixtures/test_renderer_adlib_mame2.wav"); - - mixer->init(); + if (!mixer->init()) { + std::cerr << "Mixer init failed\n"; + return; + } auto s1 = w1.getSound(); - auto s1b = w1.getSound(); + // second copy was unused; keep a single read //auto s2a = utils::makeMono(s1); //auto s2b = utils::makeStereo(s1); auto s2 = utils::append(s1, s1); drivers::PCMDriver drv(mixer); drv.play(s2); while (drv.isPlaying()) { utils::delayMillis(100); } }
♻️ Duplicate comments (2)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (2)
81-83: Fix track navigation: remove double-advance risk and play immediately on arrow keysTrack is uint8_t and you only trigger a play when the current song ends. This causes sluggish UX and risks wrap/overflow issues. Use a larger index and play immediately after adjusting.
Apply this diff:
- uint8_t track = 5; // starting from 2 + size_t track = 0; // start from first track (0-based) @@ - do + // start initial track + adlDrv.play(track); + HyperSonicDrivers::utils::ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), + HyperSonicDrivers::utils::ILogger::eCategory::Application + ); + + do { - if (!adlDrv.isPlaying()) - { - adlDrv.play(track); - ILogger::instance->info(std::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), ILogger::eCategory::Application); - } + if (!adlDrv.isPlaying()) { + const auto numTracks = adlFile->getNumTracks(); + track = (track + 1) % numTracks; // auto-advance on song end + adlDrv.play(track); + HyperSonicDrivers::utils::ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), numTracks), + HyperSonicDrivers::utils::ILogger::eCategory::Application + ); + } @@ - case SDLK_RIGHT: + case SDLK_RIGHT: { - adlDrv.stopAllChannels(); - track++; - if (track >= adlFile->getNumTracks()) - track = 0; + const auto numTracks = adlFile->getNumTracks(); + track = (track + 1) % numTracks; + adlDrv.stopAllChannels(); + adlDrv.play(track); + HyperSonicDrivers::utils::ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), numTracks), + HyperSonicDrivers::utils::ILogger::eCategory::Application + ); break; } @@ - case SDLK_LEFT: + case SDLK_LEFT: { - adlDrv.stopAllChannels(); - if (track > 0) - track--; - else - track = adlFile->getNumTracks() - 1; + const auto numTracks = adlFile->getNumTracks(); + track = (track + numTracks - 1) % numTracks; + adlDrv.stopAllChannels(); + adlDrv.play(track); + HyperSonicDrivers::utils::ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), numTracks), + HyperSonicDrivers::utils::ILogger::eCategory::Application + ); break; }Note: this uses fmt::format; add
#include <fmt/format.h>or keep std::format and include .Also applies to: 91-97, 111-127
96-97: std::format without header; prefer fmt for consistencySame issue as the other file: add or switch to fmt::format.
Apply:
- ILogger::instance->info(std::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), ILogger::eCategory::Application); + ILogger::instance->info(fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), ILogger::eCategory::Application);And include
<fmt/format.h>(or<format>if you want to keep std::format).
🧹 Nitpick comments (6)
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (3)
400-440: Confirm intent: skipping track 0 in ADL playbackTracks iterate from 1 to getNumTracks()-1. If track 0 is valid (and some fixtures show it is), you’re skipping it.
If unintentional, change the loop to start from 0. If intentional, add a comment explaining why track 0 is skipped.
- for (int i = 1; i < adlf->getNumTracks(); i++) + for (int i = 0; i < adlf->getNumTracks(); i++)
545-600: Unreachable code after return: remove or #if 0There’s a large block after return 0 that never executes, which increases maintenance cost and confuses readers.
Either remove it or wrap with a compile-time guard:
- return 0; + return 0; +#if 0 SDL_Init(SDL_INIT_TIMER | SDL_INIT_AUDIO); ... return 0; #endif
134-135: Commented example uses Mix_Hook; other blocks use Mix_HookMusicTo avoid confusion when someone copy-pastes, keep the API consistent across examples or annotate which SDL_mixer version each block targets.
Add a brief comment indicating the intended SDL_mixer version or unify all examples to the same API (e.g., Mix_HookMusic for SDL_mixer 2).
sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/files/westwood/TestADLFile.cpp (1)
23-23: Clarify semantics of getNumTracks and add a guard test for V3 header >255getNumTracks now reflects metadata.num_headers (120 or 250), not “used” tracks. That’s fine, but it hides cases where header entries may be 0xFF or point past available offsets.
Consider adding a V3-specific test that asserts at least one header entry is >255 (if true for your fixtures), to catch potential truncation in getTrack for V3. I can draft it if you confirm a suitable fixture index.
Add something like:
// Ensure V3 header values can exceed 255 (guards against uint8_t truncation) const auto t = f.getTrack(200); // pick an index known >255 in LOREINTR EXPECT_GT(t, 255); EXPECT_LT(t, f.getNumTrackOffsets());Also applies to: 36-36, 59-59, 89-89
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (2)
361-369: Typo: data_heder_size → data_header_sizeMinor but worth fixing for readability.
-void ADLFile::readDataFromFile_(const int data_offsets, const int data_heder_size) +void ADLFile::readDataFromFile_(const int data_offsets, const int data_header_size) @@ - m_dataHeaderSize = data_heder_size; + m_dataHeaderSize = data_header_size;
338-349: Avoid per-element std::function overhead in readHeaderFromFile_Passing a std::function and invoking it in a loop adds overhead and inhibits inlining. A templated reader or a simple branch (v3 vs others) that bulk-reads then converts would be simpler and faster.
Sketch:
if (m_version == 3) { m_header.resize(header_size); for (int i=0;i<header_size;++i) m_header[i] = readLE16(); } else { std::vector<uint8_t> tmp(header_size); read(tmp.data(), header_size); m_header.resize(header_size); for (int i=0;i<header_size;++i) m_header[i] = tmp[i]; }This also guarantees LE reads for v3.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp(1 hunks)sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp(6 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp(4 hunks)sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/files/westwood/TestADLFile.cpp(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (3)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/IMixer.hpp (2)
make_mixer(100-103)make_mixer(100-100)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/devices/IDevice.hpp (2)
make_device(62-65)make_device(62-62)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/utils/algorithms.hpp (2)
delayMillis(50-53)delayMillis(50-50)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (1)
header_size(61-61)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/File.cpp (2)
read(44-50)read(44-44)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
ADLFile(115-154)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (3)
ADLFile(35-35)ADLFile(36-36)track(44-44)
🪛 GitHub Check: Codacy Static Code Analysis
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp
[warning] 39-39: sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp#L39
Include file: <HyperSonicDrivers/files/VOCFile.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
sdl2-hyper-sonic-drivers/examples/adl-play.cpp
[warning] 1-1: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L1
Include file: <HyperSonicDrivers/audio/sdl2/Mixer.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 2-2: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L2
Include file: <HyperSonicDrivers/hardware/opl/OPL.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 3-3: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L3
Include file: <HyperSonicDrivers/hardware/opl/OPLFactory.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 4-4: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L4
Include file: <HyperSonicDrivers/utils/algorithms.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 5-5: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L5
Include file: <HyperSonicDrivers/files/westwood/ADLFile.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 6-6: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L6
Include file: <HyperSonicDrivers/drivers/westwood/ADLDriver.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 7-7: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L7
Include file: <HyperSonicDrivers/utils/ILogger.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 8-8: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L8
Include file: <HyperSonicDrivers/devices/Adlib.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 9-9: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L9
Include file: <HyperSonicDrivers/devices/SbPro.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 10-10: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L10
Include file: <HyperSonicDrivers/devices/SbPro2.hpp> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 12-12: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L12
Include file: <spdlog/spdlog.h> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 13-13: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L13
Include file: <fmt/color.h> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 15-15: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L15
Include file: <SDL2/SDL.h> not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 17-17: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L17
Include file: not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 18-18: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L18
Include file: not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 19-19: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L19
Include file: not found. Please note: Cppcheck does not need standard library headers to get proper results.
[warning] 20-20: sdl2-hyper-sonic-drivers/examples/adl-play.cpp#L20
Include file: not found. Please note: Cppcheck does not need standard library headers to get proper results.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
🔇 Additional comments (1)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
115-134: Good: strict file layout validation and offset adjustmentValidating minimum size, version-dependent offsets, and then converting absolute to data-relative offsets with 0→0xFFFF sentinel is clear and safe.
Also applies to: 151-154
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp
Outdated
Show resolved
Hide resolved
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp
Outdated
Show resolved
Hide resolved
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp
Outdated
Show resolved
Hide resolved
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (2)
187-190: V3 header truncation risk: widen getTrack() result to 16-bit
getTrackreturnsuint8_tbut V3 header entries are 16-bit; values >255 will be truncated. This breaks program lookups for v3.Apply in this file:
- uint8_t ADLFile::getTrack(const int track) const + uint16_t ADLFile::getTrack(const int track) const { return m_header.at(track); }And in ADLFile.hpp (outside this file), update both the member and declaration:
-std::vector<uint8_t> m_header; // TODO: wrong for v3 +std::vector<uint16_t> m_header; -uint8_t getTrack(const int track) const; +uint16_t getTrack(const int track) const;If you want a follow-up, I can propagate type fixes across call sites and tests.
230-249: Bounds/sentinel check before decoding instrument bytesIf
getInstrumentOffsetreturns 0xFFFF (invalid) or an offset near the end, pointer arithmetic and reads will go OOB.Apply:
- const uint8_t* instr = m_data.get() + getInstrumentOffset(instrument); + const uint16_t off = getInstrumentOffset(instrument); + assertValid_(off != 0xFFFF); + // We read up to instr[10], ensure at least 11 bytes are available. + assertValid_(static_cast<uint32_t>(off) + 11 <= m_dataSize); + const uint8_t* instr = m_data.get() + off;
♻️ Duplicate comments (1)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (1)
82-99: Track navigation: use a wider index, start playback once, auto-advance on end, and play immediately on arrow keys
uint8_tcan wrap; prefer size_t.- Current loop restarts the same track when it ends instead of advancing.
- RIGHT/LEFT only adjust the index; playback resumes after a timeout tick, adding latency.
Apply this focused patch:
- uint8_t track = 0; + std::size_t track = 0; @@ - do + // Precompute and validate track count + const auto numTracks = static_cast<std::size_t>(adlFile->getNumTracks()); + if (numTracks == 0) { + spdlog::warn("No tracks in ADL file: {}", filename); + return; + } + + // Start initial track + adlDrv.play(track); + ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), + ILogger::eCategory::Application + ); + + do { - if (!adlDrv.isPlaying()) - { - adlDrv.play(track); - ILogger::instance->info(fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), ILogger::eCategory::Application); - } + if (!adlDrv.isPlaying()) { + track = (track + 1) % numTracks; // auto-advance + adlDrv.play(track); + ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), + ILogger::eCategory::Application + ); + } @@ - case SDLK_RIGHT: + case SDLK_RIGHT: { - adlDrv.stopAllChannels(); - track++; - if (track >= adlFile->getNumTracks()) - track = 0; + adlDrv.stopAllChannels(); + track = (track + 1) % numTracks; + adlDrv.play(track); + ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), + ILogger::eCategory::Application + ); break; } @@ - case SDLK_LEFT: + case SDLK_LEFT: { - adlDrv.stopAllChannels(); - if (track > 0) - track--; - else - track = adlFile->getNumTracks() - 1; + adlDrv.stopAllChannels(); + track = (track + numTracks - 1) % numTracks; + adlDrv.play(track); + ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), + ILogger::eCategory::Application + ); break; }Also applies to: 117-135
🧹 Nitpick comments (8)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (4)
39-40: Doc comment and code disagree on initial track indexThe comment says “initial track index is 5” but the code starts from 0. Align one of them.
Apply either:
- Update the comment to 0, or
- Initialize to 5 in code.
If you prefer code change:
- uint8_t track = 0; + uint8_t track = 5;
66-80: Guard against unhandled OPL types (device may remain null)If an unexpected OplType is passed,
devicestays null and subsequent usage is undefined. Add a default case and fail fast.Apply:
switch (type) { using enum OplType; @@ case OPL3: device = make_device<devices::SbPro2, devices::Opl>(mixer, emu); break; + default: + spdlog::error("Unhandled OPL type: {}", static_cast<int>(type)); + return; }
159-160: Initialize SDL audio too for portabilitySome SDL backends require
SDL_INIT_AUDIOfor mixer/device init to succeed reliably.Apply:
- if (SDL_Init(SDL_INIT_VIDEO | SDL_INIT_EVENTS) != 0) + if (SDL_Init(SDL_INIT_VIDEO | SDL_INIT_EVENTS | SDL_INIT_AUDIO) != 0) return -1;
87-90: Minor: early-return path logs but doesn’t indicate which mixerConsider adding the mixer type or filename for easier diagnostics; optional.
Example:
- spdlog::error("mixer not ready yet.."); + spdlog::error("Mixer not ready (type: SDL2). Cannot play '{}'.", filename);sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (4)
259-273: Iterate by number of entries, not header size in bytesUsing
V1_HEADER_SIZEas the loop bound happens to work (120), but it’s a byte size constant. UseV1_NUM_HEADERfor clarity and to avoid accidental misuse if constants change.Apply:
- for (int i = 0; i < V1_HEADER_SIZE; i++) + for (int i = 0; i < V1_NUM_HEADER; i++) { uint16_t v = readLE16();
316-327: readHeaderFromFile_: consider passing element width instead of std::function
std::function<uint16_t()>in a tight loop has overhead and hides byte-width semantics. Passing an element-size and reading inline (u8 vs le16) would be simpler and faster.Illustrative approach:
- Signature:
readHeaderFromFile_(int num_entries, int elem_bytes)- Body: branch once on
elem_bytesand fillm_header[i]usingreadU8()orreadLE16().
329-337: Offset-table read: small clarity nit
assertValid_(tell() == offset_start);is great. Consider a short comment stating thatoffset_startis absolute (from file start) per version metadata.
339-347: Fix inconsistent parameter naming in ADLFile.hppThe declaration of readDataFromFile_ in ADLFile.hpp uses a typo (
data_heder_size), while the definition in ADLFile.cpp usesdata_header_size. Although C++ doesn’t require parameter names to match, aligning them helps prevent confusion and improves IDE navigation.• File: sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp
• Line: 63Proposed diff:
--- a/sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp +++ b/sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp @@ -63,1 +63,1 @@ - void readDataFromFile_(const int data_offsets, const int data_heder_size); + void readDataFromFile_(const int data_offsets, const int data_header_size);This change harmonizes the parameter name between declaration and definition.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp(1 hunks)sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp(4 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp
🧰 Additional context used
🧬 Code graph analysis (2)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
ADLFile(115-145)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (3)
ADLFile(35-35)ADLFile(36-36)track(44-44)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (2)
header_size(61-61)data_offsets(63-63)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/File.cpp (8)
read(44-50)read(44-44)size(25-28)size(25-25)assertValid_(127-136)assertValid_(127-127)tell(30-33)tell(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Windows (x64-windows, Debug, 2022)
- GitHub Check: Build Windows (x64-windows, Release, 2022)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
🔇 Additional comments (1)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
135-145: Deriving counts from metadata: LGTMComputing counts from
m_meta_versionand usingcount_loop_for offsets is clean and version-agnostic.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (2)
391-398: Check mixer->init() result to avoid running with an uninitialized audio backendIf init fails, subsequent device/driver usage will misbehave.
void adldune2filestest() { auto mixer = audio::make_mixer<audio::sdl2::Mixer>(8, 44100, 1024); - mixer->init(); + if (!mixer->init()) { + utils::ILogger::instance->error("Mixer init failed", utils::ILogger::eCategory::Application); + return; + }
400-402: Guard SDL window creation; clean up subsystems on failureSDL_CreateWindow can return null; handle it and quit subsystems to avoid leaks.
- auto window = SDL_CreateWindow("a", 0, 0, 320, 200, 0); + auto window = SDL_CreateWindow("a", 0, 0, 320, 200, 0); + if (!window) { + utils::ILogger::instance->error(std::string("SDL_CreateWindow failed: ") + SDL_GetError(), utils::ILogger::eCategory::Application); + SDL_QuitSubSystem(SDL_INIT_VIDEO | SDL_INIT_EVENTS); + return; + } @@ -QUIT: - SDL_DestroyWindow(window); +QUIT: + SDL_DestroyWindow(window); SDL_QuitSubSystem(SDL_INIT_VIDEO); SDL_QuitSubSystem(SDL_INIT_EVENTS);Also applies to: 444-447
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (2)
226-250: Guard against invalid instrument offsets and out-of-bounds readsIf an instrument offset is 0xFFFF (invalid) or points near the end of the buffer, dereferencing m_data+offset will be undefined behavior.
Add sanity checks:
hardware::opl::OPL2instrument_t ADLFile::getInstrument(const int instrument) const { using hardware::opl::OPL2instrument_t; - - const uint8_t* instr = m_data.get() + getInstrumentOffset(instrument); + const uint16_t off = getInstrumentOffset(instrument); + assertValid_(off != 0xFFFF); + // We read up to instr[10]; ensure at least 11 bytes available + assertValid_(static_cast<uint32_t>(off) + 11u <= m_dataSize); + const uint8_t* instr = m_data.get() + off;
316-327: Critical Fix: Prevent 16-bit Header Truncation in ADLFile (V3)The current implementation of
readHeaderFromFile_reads 16-bit values for version 3 but stores them in astd::vector<uint8_t>, truncating any value > 255. Likewise,getTrack()returns an 8-bit value, masking true track IDs and leading to incorrect lookups for V3 ADL files. To eliminate this risk, we must widen the container and accessor to 16 bits.• In
src/HyperSonicDrivers/files/westwood/ADLFile.hpp:
– Change the member declaration at line 65
diff - std::vector<uint8_t> m_header; // TODO: This is wrong for v3 as it is uint16_t instead! + std::vector<uint16_t> m_header; // Store full 16-bit entries for V3 headers
– Update the accessor prototype at line 44
diff - uint8_t getTrack(const int track) const; + uint16_t getTrack(const int track) const;• In
src/HyperSonicDrivers/files/westwood/ADLFile.cpp:
– Widen the method signature at the definition (was line 187)
diff - uint8_t ADLFile::getTrack(const int track) const + uint16_t ADLFile::getTrack(const int track) const { return m_header.at(track); }• Verify that
readHeaderFromFile_’s loop remains correct—no per-element change needed since it already calls astd::function<uint16_t()> readand simply stores into the widenedm_header.• Update all call-sites and tests that consume
getTrack():
– InADLDriver.cpp(line 242) you can now directly assign touint16_t soundIdwithout truncation.
– Adjust anyTestADLFile.cppassertions to compare againstuint16_tvalues (GTest’sEXPECT_EQwill handle mixed integral types, but updating literals to0xFFuor casting touint16_tclarifies intent).By making these changes, V3 headers will be fully preserved, and downstream logic will work with the correct track IDs.
♻️ Duplicate comments (2)
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (1)
6-8: Includes for and —good fix to unbreak buildsAdding these headers resolves the previously flagged compile issues around std::format usage and std::filesystem iteration.
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (1)
82-101: Make track index type wider, auto-advance on song end, and play immediately on LEFT/RIGHT
- Use size_t (or int) instead of uint8_t to avoid 8-bit wrap concerns.
- Auto-advance to next track when a song ends.
- Start playback immediately when navigating with arrow keys for responsive UX.
- uint8_t track = 0; + size_t track = 0; @@ - if (!adlDrv.isPlaying()) - { - adlDrv.play(track); - ILogger::instance->info(fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), ILogger::eCategory::Application); - } + if (!adlDrv.isPlaying()) { + const auto numTracks = static_cast<size_t>(adlFile->getNumTracks()); + track = (track + 1) % numTracks; // auto-advance + adlDrv.play(static_cast<int>(track)); + ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), + ILogger::eCategory::Application + ); + } @@ - case SDLK_RIGHT: - { - adlDrv.stopAllChannels(); - track++; - if (track >= adlFile->getNumTracks()) - track = 0; - - break; - } + case SDLK_RIGHT: { + const auto numTracks = static_cast<size_t>(adlFile->getNumTracks()); + track = (track + 1) % numTracks; + adlDrv.stopAllChannels(); + adlDrv.play(static_cast<int>(track)); + ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), + ILogger::eCategory::Application + ); + break; + } @@ - case SDLK_LEFT: - { - adlDrv.stopAllChannels(); - if (track > 0) - track--; - else - track = adlFile->getNumTracks() - 1; - - break; - } + case SDLK_LEFT: { + const auto numTracks = static_cast<size_t>(adlFile->getNumTracks()); + track = (track + numTracks - 1) % numTracks; + adlDrv.stopAllChannels(); + adlDrv.play(static_cast<int>(track)); + ILogger::instance->info( + fmt::format("Playing track: {}/{}", static_cast<int>(track), adlFile->getNumTracks()), + ILogger::eCategory::Application + ); + break; + }Also applies to: 110-136
🧹 Nitpick comments (5)
sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (2)
341-350: Harden mixer init and drop the unused variable in pcm_sound_append()
- mixer->init() is unchecked; bail out if it fails.
- s1b is never used; remove it to silence warnings.
Apply:
void pcm_sound_append() { auto mixer = audio::make_mixer<audio::sdl2::Mixer>(8, 44100, 1024); files::WAVFile w1("test/fixtures/test_renderer_adlib_mame2.wav"); - mixer->init(); + if (!mixer->init()) { + spdlog::error("Mixer init failed"); + return; + } auto s1 = w1.getSound(); - auto s1b = w1.getSound(); + // auto s1b = w1.getSound(); // unused //auto s2a = utils::makeMono(s1); //auto s2b = utils::makeStereo(s1); auto s2 = utils::append(s1, s1); drivers::PCMDriver drv(mixer);Also applies to: 351-369
402-442: Remove the no-op outer loop (for f=0..0) and simplify file selectionThe loop executes exactly once; flatten for clarity.
- for (int f = 0; f <= 0; f++) - { - //const std::string fn = "adl/DUNE" + std::to_string(f) + ".ADL"; - const std::string fn = "adl/KYRA1A.ADL"; + { + const std::string fn = "adl/KYRA1A.ADL"; utils::ILogger::instance->info(std::format("opening file: {}", fn), utils::ILogger::eCategory::Application); auto adlf = std::make_shared<files::westwood::ADLFile>(fn); drv.setADLFile(adlf); for (int i = 1; i < adlf->getNumTracks(); i++)sdl2-hyper-sonic-drivers/examples/adl-play.cpp (1)
33-41: Doc mismatch: comment says “initial track index is 5” but code starts at 0Keep docs accurate to avoid confusion.
- * (initial track index is 5) and responds to key presses to control playback. + * Starts from track index 0 and responds to key presses to control playback.sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (2)
339-347: Minor: set m_dataHeaderSize before bounds assertions elsewhere depend on itNo functional issue here, but if future code adds checks during read, you might want m_dataHeaderSize set earlier. Optional.
- read(m_data.get(), m_dataSize); - m_dataHeaderSize = data_header_size; + m_dataHeaderSize = data_header_size; + read(m_data.get(), m_dataSize);
252-301: Version detection logic is clever but brittle; consider explicit signature checksCurrent heuristic relies on header entry magnitudes vs V3_HEADER_SIZE. Consider augmenting with additional invariants (e.g., minimum offset monotonicity, sentinel patterns) to reduce false positives across malformed inputs. Optional for now.
Happy to propose a dedicated detector that peeks both header and early offsets with clear invariants.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp(1 hunks)sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp(4 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp(5 hunks)sonar-project.properties(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
ADLFile(115-145)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (3)
ADLFile(35-35)ADLFile(36-36)track(44-44)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/File.cpp (8)
read(44-50)read(44-44)size(25-28)size(25-25)assertValid_(127-136)assertValid_(127-127)tell(30-33)tell(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
- GitHub Check: Build Windows (x64-windows, Release, 2022)
- GitHub Check: Build Windows (x64-windows, Debug, 2022)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
🔇 Additional comments (4)
sonar-project.properties (1)
2-2: SonarCloud projectKey update verified
No lingering references to the old key were found in the repository. Yoursonar.projectKeyis correctly set toRaffaello_sdl2-hyper-sonic-driversinsonar-project.properties, and the ripgrep check returned no instances of the previous key.• All occurrences of
sonar.projectKeynow point to the new value.
• No matches forRaffaello_sdl2-sonic-driverswere detected.Please ensure that:
- Your SonarCloud project’s key matches
Raffaello_sdl2-hyper-sonic-drivers.- Any CI/CD secrets or workflow inputs (e.g.
SONAR_PROJECT_KEY) are updated to this new key.sdl2-hyper-sonic-drivers/sdl2-hyper-sonic-drivers.cpp (1)
409-411: Verify track indexing (0-based vs 1-based) for ADLDriver::playThis loop skips track 0 by starting from 1; adl-play example uses 0-based indexing. Confirm intended behavior to avoid off-by-one or missing the first subsong.
Would you like me to scan ADLDriver and tests to confirm the expected indexing convention and generate a patch accordingly?
sdl2-hyper-sonic-drivers/examples/adl-play.cpp (1)
157-177: Solid: SDL init, window creation checks, and mixer failure cleanupInit/teardown paths are handled correctly; returning proper codes on failure is appreciated.
Also applies to: 221-224
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
115-121: Constructor flow/read ordering looks sound; deriving track count from metadata is correctValidating sizes, reading header/offsets/data, then deriving counts from version metadata removes the prior reliance on the (problematic) header count. Good progression.
Also applies to: 133-145
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (4)
63-63: Typo in parameter name:data_heder_size→data_header_sizeMinor spelling fix for clarity and consistency with
m_dataHeaderSize.- void readDataFromFile_(const int data_offsets, const int data_heder_size); + void readDataFromFile_(const int data_offsets, const int data_header_size);
65-65: Good change: header stored as 16-bit words; clarify unit in related APIsSwitching
m_headertostd::vector<uint16_t>aligns with 16-bit fields in the format. To avoid ambiguity, document whetherreadHeaderFromFile_’sheader_sizeargument is a count of 16-bit words or bytes. Consider renaming toheader_word_count(header only) in a future refactor for self-documenting intent.
75-78: Keep member types consistent with the (proposed) unsigned gettersIf you adopt unsigned return types for the offset counts, consider aligning the members as well and dropping negative sentinels (or use
std::optional<uint16_t>).- uint16_t m_num_tracks = 0; - int m_num_track_offsets = -1; - int m_num_instrument_offsets = -1; + uint16_t m_num_tracks = 0; + uint16_t m_num_track_offsets = 0; // consider std::optional<uint16_t> + uint16_t m_num_instrument_offsets = 0; // consider std::optional<uint16_t>If you keep
intmembers intentionally for a tri-state (unknown/known-zero/known-nonzero), please add a brief comment documenting the sentinel semantics.Also, ensure any assignment from counting helpers to
m_num_tracksclamps safely toUINT16_MAXto avoid silent narrowing.
81-86: Avoid signed/unsigned comparison in loop
vec.size()returnssize_t; iterating withint irisks signed/unsigned warnings and potential UB ifvec.size()exceedsINT_MAX. Usestd::size_tfor the index.- for (int i = 0; i < vec.size(); ++i) + for (std::size_t i = 0; i < vec.size(); ++i)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp(5 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
- GitHub Check: Build Windows (x64-windows, Release, 2022)
- GitHub Check: Build Windows (x64-windows, Debug, 2022)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
🔇 Additional comments (1)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (1)
40-42: It looks like attempts to display the file contents didn’t return output. To ensure we correctly inspect the backing member types, please verify manually that inADLFile.hppthe fieldsm_num_track_offsetsandm_num_instrument_offsetsare declared asint. If they’re indeed signed, consider aligning them touint16_t(or another unsigned type matching the file format) alongside changing the getters.Since the automated search didn’t reliably show callers depending on negative sentinels, please manually review:
- All uses of
getNumTrackOffsets()andgetNumInstrumentOffsets()to ensure no code relies on negative return values.- The declarations of backing fields
m_num_track_offsetsandm_num_instrument_offsetsfor type consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
226-250: Guard against invalid instrument offsets (0xFFFF) and OOB readsIf an instrument entry is invalid (0xFFFF after adjustment) or too close to the end of the data blob, this performs an out‑of‑bounds read. Add sentinel and bounds checks before dereferencing.
hardware::opl::OPL2instrument_t ADLFile::getInstrument(const int instrument) const { using hardware::opl::OPL2instrument_t; - - const uint8_t* instr = m_data.get() + getInstrumentOffset(instrument); + const uint16_t off = getInstrumentOffset(static_cast<uint16_t>(instrument)); + // Each instrument reads at least 11 bytes (highest index used below is 10) + constexpr uint32_t kInstrSizeBytes = 11; + if (off == 0xFFFF) { + assertValid_(false); // invalid instrument index + } + assertValid_(off + kInstrSizeBytes <= m_dataSize); + const uint8_t* instr = m_data.get() + off; return OPL2instrument_t( { .trem_vibr_1 = instr[0], .att_dec_1 = instr[7], .sust_rel_1 = instr[9], .wave_1 = instr[3], .scale_1 = 0, // ??? .level_1 = instr[5], .feedback = instr[2], .trem_vibr_2 = instr[1], .att_dec_2 = instr[8], .sust_rel_2 = instr[10], .wave_2 = instr[4], .scale_2 = 0, // ??? .level_2 = instr[6], .unused = 0, .basenote = 0 /// ??? } ); }If you prefer not to assert on invalid indices, we can return a default‑constructed OPL2instrument_t instead.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.cpp (2)
179-197: SMPTE division parsing is incorrect; high byte must be treated as signedWhen the MSB is set, the SMPTE FPS byte is signed (e.g., -24, -25, -29, -30). Current code masks with 0x7FFF and right-shifts, yielding a positive number and making the switch cases on negative constants unreachable.
Apply this fix to read the signed SMPTE value correctly:
- if (m_midi->division & 0x8000) + if (m_midi->division & 0x8000) { // ticks per frame - int smpte = (m_midi->division & 0x7FFF) >> 8; - int ticksPerFrame = m_midi->division & 0xFF; + const int8_t smpte = static_cast<int8_t>(m_midi->division >> 8); + const int ticksPerFrame = m_midi->division & 0xFF; switch (smpte) { case -24: case -25: case -29: case -30: logW("SMPTE not implemented yet"); break; default: logW(std::format("Division SMPTE not known = {}", smpte)); }
205-213: Thread-safety: data races on m_force_stop/m_paused/m_isPlayingprocessTrack reads m_force_stop and m_paused while play/stop/pause/resume write them without synchronization. This is UB under the C++ memory model.
Minimal safe options:
- Make m_force_stop, m_paused, and m_isPlaying std::atomic.
- Or guard all reads/writes with a mutex consistently.
Optionally switch to jthread stop_token to avoid custom flags; request_stop() in stop(), and check token in processTrack.sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp (1)
260-267: Bug: QueueEntry.id is uint8_t — truncates 16-bit track IDs and breaks retry logicThe PR widens track/sound IDs to 16-bit, but QueueEntry still stores id as uint8_t. When the queue is full or when retrying a skipped sound, IDs > 255 will be truncated, leading to playing the wrong program and misleading logs.
Apply this diff to carry full 16-bit IDs:
struct QueueEntry { - QueueEntry() : data(0), id(0), volume(0) {} - QueueEntry(uint8_t* ptr, uint8_t track, uint8_t vol) : data(ptr), id(track), volume(vol) {} + QueueEntry() : data(nullptr), id(0), volume(0) {} + QueueEntry(uint8_t* ptr, uint16_t track, uint8_t vol) : data(ptr), id(track), volume(vol) {} uint8_t* data; - uint8_t id; + uint16_t id; uint8_t volume; };Follow-up in ADLDriver.cpp is minimal (constructor call adapts automatically); see suggested tweak there for clarity.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.cpp (2)
85-103: Propagate 16-bit IDs through the queue and improve ring-buffer increment readabilityWith startSound_(uint16_t, ...), ensure QueueEntry.id is also uint16_t (see header fix). Additionally, the ring-buffer increment uses a terse idiom.
Apply this readability tweak (functionally equivalent):
- m_programQueue[m_programQueueEnd] = QueueEntry(trackData, track, volume); - ++m_programQueueEnd &= 15; + m_programQueue[m_programQueueEnd] = QueueEntry(trackData, track, volume); + m_programQueueEnd = (m_programQueueEnd + 1) & 0x0F;
234-249: Mandatory refactor: WidenQueueEntry.idto match the newuint16_ttrack typeThe grep shows that while you’ve widened the public APIs (
play()andstartSound_()) to accept auint16_ttrack, the internal queue still storestrackin an 8-bit field—truncating any value ≥256. You need to updateADLDriver.hppso theQueueEntryuses a 16-bitid.– ADLDriver.hpp (around line 261):
• Changeuint8_t id;→uint16_t id;
• Change the constructor fromQueueEntry(uint8_t* ptr, uint8_t track, uint8_t vol)→QueueEntry(uint8_t* ptr, uint16_t track, uint8_t vol)Suggested diff:
--- a/sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp +++ b/sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp @@ struct QueueEntry { - uint8_t id; + uint16_t id; @@ - QueueEntry(uint8_t* ptr, uint8_t track, uint8_t vol) - : data(ptr), id(track), volume(vol) {} + QueueEntry(uint8_t* ptr, uint16_t track, uint8_t vol) + : data(ptr), id(track), volume(vol) {}After this change, rebuild and test to ensure no further 8-bit truncations occur when queuing tracks.
♻️ Duplicate comments (1)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
325-333: Re: noexcept on readOffsetsFromFile_This implementation uses assertValid_ and can throw; combined with noexcept in the declaration leads to terminate on error.
See the header diff to remove noexcept.
🧹 Nitpick comments (7)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (2)
70-70: Prefer a narrower unsigned type for m_dataHeaderSizem_dataHeaderSize holds the header prefix within the data blob (≤ ~2 KiB for all versions). Using int invites sign/width mismatches when subtracting from uint16_t offsets.
Minimal change:
- int m_dataHeaderSize = 0; + uint16_t m_dataHeaderSize = 0;
72-74: Name/contract mismatch in count_loop_: clarify parameter and use an unsigned returnThe parameter represents the minimum valid offset (offset_start), not a “num_offs.” Also, the value returned is a count that fits in uint16_t for ADL; returning int is unnecessary.
Proposed tweaks (update declaration and definition):
- template<typename T> - int count_loop_(const int num_offs, const std::vector<T>& vec); + template<typename T> + uint16_t count_loop_(const int offset_start, const std::vector<T>& vec);And in the definition:
- int ADLFile::count_loop_(const int offs_start, const std::vector<T>& vec) + uint16_t ADLFile::count_loop_(const int offset_start, const std::vector<T>& vec) { - int tot = 0; + uint16_t tot = 0; constexpr T max_ = std::numeric_limits<T>::max(); - for (size_t i = 0; i < vec.size(); ++i) + for (size_t i = 0; i < vec.size(); ++i) { - if (vec[i] >= offs_start && vec[i] < max_) { + if (vec[i] >= offset_start && vec[i] < max_) { ++tot; } } return tot; }Also applies to: 80-87
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (3)
202-214: Make default branch explicitly unreachableThe enum covers both cases; the default is defensive but could mask future issues. Prefer asserting unreachable.
- default: - // unreachable code - return 0xFFFF; + default: + assertValid_(false); // unreachable + return 0xFFFF;
259-273: Use entry count constant instead of byte size in v3 detection loopThe loop reads 16‑bit entries but uses V1_HEADER_SIZE (bytes) as the iteration bound. It happens to be 120 which matches V1_NUM_HEADER, but using the proper “number of entries” constant improves readability and avoids accidental drift.
- for (int i = 0; i < V1_HEADER_SIZE; i++) + for (int i = 0; i < V1_NUM_HEADER; ++i)
335-343: m_dataSize width and upper‑bound checksize() returns uintmax_t; truncating to uint32_t is fine for ADL but add a guard to make the intent explicit, or widen m_dataSize to size_t.
Option A (guard and keep uint32_t):
- m_dataSize = size() - data_offsets; + const auto total = size(); + assertValid_(total >= static_cast<uintmax_t>(data_offsets)); + const auto remaining = total - static_cast<uintmax_t>(data_offsets); + assertValid_(remaining <= std::numeric_limits<uint32_t>::max()); + m_dataSize = static_cast<uint32_t>(remaining);Option B (widen the member type to size_t in the header and here).
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.cpp (1)
205-209: Remove redundantacquire()inplay()
The call tom_device->acquire(this)afterstop()is effectively a no-op, since:
IDevice::acquire(owner)
- on first call: runs
init(), setsm_acquired = trueandm_owner = owner.- on subsequent calls with the same
owner: returnsisOwned(owner)(always true) without side-effects.MIDDriver::stop()does not callrelease(), so the device remains acquired by this driver.Keeping the extra
acquire()is safe but redundant. To simplify, you can remove the check and early return:• File: src/HyperSonicDrivers/drivers/MIDDriver.cpp
Lines: 205–209- stop(); - if (!m_device->acquire(this)) { - return; - } + stop();sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.cpp (1)
271-289: Nit: logging typo and missing label in getProgram_() debugMinor, but makes logs easier to parse.
- logD(std::format("calling getProgram(prodIg={}){}", progId, offset)); + logD(std::format("calling getProgram(progId={}) offset={}", progId, offset));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
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/westwood/ADLDriver.cpp(2 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp(1 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp(5 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp(3 hunks)sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestIAudioDriver.cpp(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (5)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (1)
track(25-25)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp (2)
track(56-56)track(62-62)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (1)
track(44-44)sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestIAudioDriver.cpp (1)
track(11-11)sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/MIDDriverMock.hpp (2)
track(19-22)track(19-19)
sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestIAudioDriver.cpp (3)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (1)
track(25-25)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (2)
track(40-40)track(52-52)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp (2)
track(56-56)track(62-62)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp (4)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (1)
track(25-25)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (2)
track(40-40)track(52-52)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (1)
track(44-44)sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestIAudioDriver.cpp (1)
track(11-11)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.cpp (4)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (1)
track(25-25)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (2)
track(40-40)track(52-52)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (1)
track(44-44)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.cpp (2)
play(166-213)play(166-166)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (4)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (2)
track(40-40)track(52-52)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp (2)
track(56-56)track(62-62)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (1)
track(44-44)sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestIAudioDriver.cpp (1)
track(11-11)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (3)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (1)
track(25-25)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (2)
track(40-40)track(52-52)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp (4)
track(56-56)track(62-62)progId(70-70)progId(73-73)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.cpp (6)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.cpp (2)
play(234-249)play(234-234)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (1)
track(25-25)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (2)
track(40-40)track(52-52)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp (2)
track(56-56)track(62-62)sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestIAudioDriver.cpp (1)
track(11-11)sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/MIDDriverMock.hpp (2)
track(19-22)track(19-19)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (6)
track(44-44)programId(45-45)instrument(46-46)instrument(52-52)progId(47-47)data_offsets(63-63)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/File.cpp (8)
size(25-28)size(25-25)assertValid_(127-136)assertValid_(127-127)tell(30-33)tell(30-30)read(44-50)read(44-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
- GitHub Check: Build Windows (x64-windows, Debug, 2022)
- GitHub Check: Build Windows (x64-windows, Release, 2022)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
🔇 Additional comments (10)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (1)
25-25: API widening to 16‑bit track index looks good and consistent with ADL/MID changesSignature aligns with the 16‑bit track/offset model used across the PR. No issues spotted here.
sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestIAudioDriver.cpp (1)
11-11: Test mock kept in sync with interfaceThe override now matches IAudioDriver::play(uint16_t). Looks correct.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (1)
40-47: Public API updated to 16-bit — verified safe at call sitesI’ve confirmed that all
ADLFileaccessors now returnuint16_tand align with the on-disk format; any implicit conversions back toint(in tests) or comparisons against 8-bit sentinels still behave correctly:• ADLFile.hpp / ADLFile.cpp
getTrack,getTrackOffset,getInstrumentOffset,getProgramOffsetall returnuint16_tand index into vectors of matching element width.- Internally, for legacy ADLv<3 files, headers store
uint8_tvalues but are returned asuint16_t, so comparing against0xFF(255) still works; for ADLv3, 16-bit format uses0xFFFFsentinel.• Call sites:
- Tests frequently assign to
int track2 = f.getTrack(2)or useauto; convertinguint16_t→intis safe (no truncation).- No instances of narrowing to
uint8_tremain.- Drivers compare returned values (
soundId) to both0xFFand0xFFFFas intended.There are no lingering assumptions about 8-bit returns nor unsafe truncations. I’m happy to approve these changes.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (3)
94-114: Constructor doc is clear and matches the implemented flowHelpful overview of the parsing pipeline. No action required.
115-145: Construction flow looks correct; metadata‑driven sizing is a good improvementThe sequence (detect → read header via callback → read offsets → read data → compute counts → close → adjust offsets) is sound, and setting m_num_tracks from metadata avoids relying on potentially inconsistent v3 headers.
187-190: getTrack widened to 16‑bit — goodDirectly returns the widened header entry. Using at() preserves bounds checks.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (1)
40-40: play(uint16_t) override — consistent and non‑breakingMatches IAudioDriver’s widened signature; default remains 0. Good.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.cpp (1)
166-176: API widening to uint16_t looks good and is consistent with IAudioDriverSignature change aligns with IAudioDriver::play(uint16_t). The bounds check against numTracks is preserved.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp (2)
56-56: Public API widened to uint16_t: OKplay(uint16_t) matches IAudioDriver and related changes across the codebase.
62-63: Internal API widened to uint16_t: OK but verify downstream usagestartSound_(uint16_t, uint8_t) matches the broader 16-bit track IDs work.
Please ensure all call sites (e.g., QueueEntry construction) propagate 16-bit IDs without truncation (see next comment).
| void readHeaderFromFile_(const int header_size, std::function<uint16_t()> read); | ||
| void readOffsetsFromFile_(const int num_offsets, std::vector<uint16_t>& vec, const int offset_start) const noexcept; | ||
| void readDataFromFile_(const int data_offsets, const int data_heder_size); | ||
| void readDataFromFile_(const int data_offsets, const int data_header_size); | ||
|
|
There was a problem hiding this comment.
noexcept on readOffsetsFromFile_ conflicts with throwing asserts
readOffsetsFromFile_ is marked noexcept but calls assertValid_(), which throws. Any violation will std::terminate instead of surfacing a diagnostic. Drop noexcept here (and in the definition) to preserve error reporting.
Apply this diff in the header and mirror it in the cpp definition:
- void readOffsetsFromFile_(const int num_offsets, std::vector<uint16_t>& vec, const int offset_start) const noexcept;
+ void readOffsetsFromFile_(const int num_offsets, std::vector<uint16_t>& vec, const int offset_start) const;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void readHeaderFromFile_(const int header_size, std::function<uint16_t()> read); | |
| void readOffsetsFromFile_(const int num_offsets, std::vector<uint16_t>& vec, const int offset_start) const noexcept; | |
| void readDataFromFile_(const int data_offsets, const int data_heder_size); | |
| void readDataFromFile_(const int data_offsets, const int data_header_size); | |
| void readHeaderFromFile_(const int header_size, std::function<uint16_t()> read); | |
| void readOffsetsFromFile_(const int num_offsets, std::vector<uint16_t>& vec, const int offset_start) const; | |
| void readDataFromFile_(const int data_offsets, const int data_header_size); |
🤖 Prompt for AI Agents
In sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp
around lines 61-64, the method readOffsetsFromFile_ is declared noexcept but
calls assertValid_() which can throw; remove the noexcept specifier from the
declaration and update the corresponding definition in the .cpp to also drop
noexcept so thrown errors produce diagnostics instead of causing std::terminate.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (2)
62-64: Dropping noexcept from readOffsetsFromFile_ avoids terminate-on-assert.readOffsetsFromFile_ previously conflicted with throwing asserts; removing noexcept here (and in the cpp) fixes that. Thank you for addressing the earlier concern.
65-65: m_header widened to std::vector<uint16_t> fixes V3 truncation.This resolves the v3 WORD-width header issue and aligns with the new getTrack return type.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (1)
325-333: noexcept removal mirrored here — correct.readOffsetsFromFile_ asserts and reads from the stream; removing noexcept avoids std::terminate in error paths and matches the header change.
🧹 Nitpick comments (4)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (2)
75-77: Unify count types to avoid needless narrowing.Members are uint16_t but count_loop_ returns int; while safe given current maxima, returning uint16_t removes narrowing and clarifies intent.
Apply this diff in the header and the corresponding template definition below:
- template<typename T> - int count_loop_(const int num_offs, const std::vector<T>& vec); + template<typename T> + uint16_t count_loop_(const int offs_start, const std::vector<T>& vec) noexcept;And in the definition:
- template<typename T> - int ADLFile::count_loop_(const int offs_start, const std::vector<T>& vec) + template<typename T> + uint16_t ADLFile::count_loop_(const int offs_start, const std::vector<T>& vec) noexcept { - int tot = 0; + uint16_t tot = 0; constexpr T max_ = std::numeric_limits<T>::max(); for (size_t i = 0; i < vec.size(); ++i) { if (vec[i] >= offs_start && vec[i] < max_) { ++tot; } } return tot; }
81-93: Consider std::count_if for clarity (optional).Minor readability tweak: replace the indexed loop with a standard algorithm.
Apply this diff (and add include at the top of the header if not present):
- int tot = 0; - constexpr T max_ = std::numeric_limits<T>::max(); - for (size_t i = 0; i < vec.size(); ++i) - { - if (vec[i] >= offs_start && vec[i] < max_) { - ++tot; - } - } - return tot; + constexpr T max_ = std::numeric_limits<T>::max(); + return static_cast<uint16_t>(std::count_if( + vec.begin(), vec.end(), + [&](const T v){ return v >= offs_start && v < max_; } + ));Additional change outside this range:
#include <algorithm> // at the top of this headersdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (2)
152-166: Doc return type says int but function returns uint16_t.Align the doc with the function signature.
Apply this diff:
- * @return int Number of tracks (subsongs). + * @return uint16_t Number of tracks (subsongs).
167-181: Doc return type says int but function returns uint16_t.Same nit for getNumTrackOffsets().
Apply this diff:
- * @return int Number of track/program offsets. + * @return uint16_t Number of track/program offsets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp(5 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (4)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/IAudioDriver.hpp (1)
track(25-25)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/MIDDriver.hpp (2)
track(40-40)track(52-52)sdl2-hyper-sonic-drivers/test/HyperSonicDrivers/drivers/TestIAudioDriver.cpp (1)
track(11-11)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.hpp (4)
track(56-56)track(62-62)progId(70-70)progId(73-73)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (9)
track(44-44)programId(45-45)instrument(46-46)instrument(52-52)progId(47-47)header_size(61-61)num_offsets(62-62)vec(74-74)data_offsets(63-63)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/File.cpp (8)
read(44-50)read(44-44)assertValid_(127-136)assertValid_(127-127)size(25-28)size(25-25)tell(30-33)tell(30-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build Windows (x64-windows, Release, 2022)
- GitHub Check: Build Windows (x64-windows, Debug, 2022)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
🔇 Additional comments (6)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.hpp (2)
40-43: Switching count getters to uint16_t is correct and consistent with 16‑bit ADL formats.Good alignment with the 16‑bit widening across the driver stack; noexcept is appropriate for these trivial accessors.
44-48: Public getters updated to 16‑bit indices — consistent surface.Signatures for getTrack/get{Track,Instrument,Program}Offset now uniformly take/return uint16_t, matching the 16‑bit on-disk tables and other driver interfaces (IAudioDriver/MIDDriver/ADLDriver). Looks good.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/files/westwood/ADLFile.cpp (4)
94-114: Constructor documentation and responsibilities are clear.Nice high-level description of parsing steps and the offset normalization to data-relative indices.
135-138: Track count now sourced from version metadata — confirm semantic intent.m_num_tracks = m_meta_version.num_headers reports the maximum track slots (120 for v1/v2, 250 for v3), not the number of “populated” header entries. If UI or driver logic previously relied on a smaller “actual tracks” count, this broadening may expose empty/unused slots to callers. If intended, fine; otherwise consider counting only valid header entries for v1/v2 (and keeping the metadata value for v3 where header reliability is doubtful).
Would you like me to add an assertion or a helper like hasTrack(i) to gate access to unused slots?
303-324: readHeaderFromFile_ helper is clean and version-agnostic.Good separation of concerns by passing the width-aware reader via std::function.
335-343: readDataFromFile_ is robust; minor naming fix carried through.m_dataHeaderSize capture is correct; assertions ensure position and size validity before allocation and read.
| uint16_t ADLFile::getInstrumentOffset(const uint16_t instrument) const | ||
| { | ||
| return m_instrument_offsets.at(instrument); | ||
| } |
There was a problem hiding this comment.
Guard against invalid instrument offsets before dereferencing in getInstrument.
Offsets with sentinel 0xFFFF are possible; using them directly as m_data + 0xFFFF causes out-of-bounds reads. Add a simple validity check in getInstrument().
Apply this diff to add bounds checks:
hardware::opl::OPL2instrument_t ADLFile::getInstrument(const int instrument) const
{
using hardware::opl::OPL2instrument_t;
-
- const uint8_t* instr = m_data.get() + getInstrumentOffset(instrument);
+ const uint16_t off = getInstrumentOffset(static_cast<uint16_t>(instrument));
+ // 11 bytes are read below; ensure the offset is valid and not the sentinel.
+ assertValid_(off != 0xFFFF);
+ assertValid_(static_cast<uint32_t>(off) + 11u <= m_dataSize);
+ const uint8_t* instr = m_data.get() + off;
return OPL2instrument_t(
{
.trem_vibr_1 = instr[0],
.att_dec_1 = instr[7],
.sust_rel_1 = instr[9],
.wave_1 = instr[3],
.scale_1 = 0, // ???
.level_1 = instr[5],
.feedback = instr[2],
.trem_vibr_2 = instr[1],
.att_dec_2 = instr[8],
.sust_rel_2 = instr[10],
.wave_2 = instr[4],
.scale_2 = 0, // ???
.level_2 = instr[6],
.unused = 0,
.basenote = 0 /// ???
}
);
}Optionally, consider returning a default-initialized instrument or throwing a more explicit exception if the offset is invalid.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| uint16_t ADLFile::getInstrumentOffset(const uint16_t instrument) const | |
| { | |
| return m_instrument_offsets.at(instrument); | |
| } | |
| hardware::opl::OPL2instrument_t ADLFile::getInstrument(const int instrument) const | |
| { | |
| using hardware::opl::OPL2instrument_t; | |
| const uint16_t off = getInstrumentOffset(static_cast<uint16_t>(instrument)); | |
| // 11 bytes are read below; ensure the offset is valid and not the sentinel. | |
| assertValid_(off != 0xFFFF); | |
| assertValid_(static_cast<uint32_t>(off) + 11u <= m_dataSize); | |
| const uint8_t* instr = m_data.get() + off; | |
| return OPL2instrument_t( | |
| { | |
| .trem_vibr_1 = instr[0], | |
| .att_dec_1 = instr[7], | |
| .sust_rel_1 = instr[9], | |
| .wave_1 = instr[3], | |
| .scale_1 = 0, // ??? | |
| .level_1 = instr[5], | |
| .feedback = instr[2], | |
| .trem_vibr_2 = instr[1], | |
| .att_dec_2 = instr[8], | |
| .sust_rel_2 = instr[10], | |
| .wave_2 = instr[4], | |
| .scale_2 = 0, // ??? | |
| .level_2 = instr[6], | |
| .unused = 0, | |
| .basenote = 0 /// ??? | |
| } | |
| ); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/devices/IDevice.hpp (2)
37-39: Bug: std::atomic used without .load(); pointer comparison is invalid.Both accessors read atomic values without .load(), and comparing std::atomic<T*> directly to a raw pointer is ill-formed. Use explicit loads (with appropriate memory order). Also fix the double space in the parameter type.
Apply:
- inline bool isAcquired() const noexcept { return m_acquired; } - inline bool isOwned(const drivers::IAudioDriver* owner) const noexcept { return m_owner == owner; } + inline bool isAcquired() const noexcept { return m_acquired.load(std::memory_order_acquire); } + inline bool isOwned(const drivers::IAudioDriver* owner) const noexcept { + return m_owner.load(std::memory_order_acquire) == owner; + }Follow-up:
- Ensure the corresponding stores in acquire()/release() use memory_order_release (or seq_cst) to pair with these loads.
40-42: Enforce atomic ownership with proper memory orderingThe current implementation uses plain reads of the atomics (
m_acquiredandm_owner) and a simple pointer comparison in isOwned(), which incurs data races. You must:• Implement
acquire()usingcompare_exchange_strong(expected, owner, std::memory_order_acq_rel)to claim the device only when it’s free, and return false otherwise.
• Implementrelease()by first checking ownership via an atomic load, then storingnullptrwithstd::memory_order_release.
• Replace all direct uses ofm_acquiredandm_owner == ownerwith explicit atomic operations and the correct memory orders:– In IDevice.hpp:
• Line 37:
diff - inline bool isAcquired() const noexcept { return m_acquired; } + inline bool isAcquired() const noexcept { + return m_acquired.load(std::memory_order_acquire); + }
• Line 38:
diff - inline bool isOwned(const drivers::IAudioDriver* owner) const noexcept { return m_owner == owner; } + inline bool isOwned(const drivers::IAudioDriver* owner) const noexcept { + return m_owner.load(std::memory_order_acquire) == owner; + }– In IDevice.cpp:
• Replace the fast path inacquire():
diff bool IDevice::acquire(drivers::IAudioDriver* owner) { - if (!m_acquired) - { - if (!init()) - return false; - m_acquired = true; - m_owner = owner; - return true; - } - return false; + drivers::IAudioDriver* expected = nullptr; + if (m_owner.compare_exchange_strong(expected, owner, std::memory_order_acq_rel)) { + return true; + } + return false; }
• Implementrelease()as:
cpp bool IDevice::release(const drivers::IAudioDriver* owner) { if (m_owner.load(std::memory_order_acquire) != owner) { return false; } m_owner.store(nullptr, std::memory_order_release); return true; }• Review the repository for any remaining plain uses of
m_acquiredorm_ownerwithout.load()/.store()(e.g. via the earlierrgcommand) and correct them likewise.These changes eliminate races between audio drivers and ensure that ownership hand-off has the correct acquire/release semantics.
🧹 Nitpick comments (5)
.gitignore (1)
4-4: Clarify the ignore intent for VS Code settings directory.'/.vscode' ignores only the top-level path and doesn't signal it's a directory. If that’s intended, you're fine. Otherwise, consider one of these tweaks:
- Root-only and explicit directory:
-/.vscode +/.vscode/
- Ignore at any depth:
-/.vscode +.vscode/If you plan to share workspace tasks/launch configs, consider committing specific files like
.vscode/extensions.json,tasks.json, andlaunch.jsonand ignore the rest.sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/converters/IRateConverter.hpp (2)
55-59: Prefer std::fill_n over std::memset for type safety and portabilityZeroing a typed buffer is clearer and avoids relying on object-representation assumptions. std::fill_n also keeps you header-only without needing .
Apply this diff:
- virtual size_t drain(int16_t* obuf, uint32_t osamp, const uint16_t vol) + virtual size_t drain(int16_t* obuf, uint32_t osamp, const uint16_t vol) { - std::memset(obuf, 0, sizeof(int16_t) * osamp); + std::fill_n(obuf, osamp, int16_t{0}); return osamp; }If you adopt this, you can remove the now-unused include to keep the header’s dependencies minimal.
55-56: Unused parameter in default implementationvol is not used in the base drain; consider marking it [[maybe_unused]] to avoid warnings while preserving the signature for overrides.
Apply this diff:
- virtual size_t drain(int16_t* obuf, uint32_t osamp, const uint16_t vol) + virtual size_t drain(int16_t* obuf, uint32_t osamp, const uint16_t /*[[maybe_unused]] vol*/)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/devices/IDevice.hpp (2)
57-60: Avoid duplicated ownership state (m_acquired vs m_owner).m_acquired duplicates information that can be derived from m_owner != nullptr, creating a risk of divergence. Prefer a single source of truth.
Minimal refactor (header-only):
- std::atomic<bool> m_acquired = false; - std::atomic<drivers::IAudioDriver*> m_owner = nullptr; + std::atomic<drivers::IAudioDriver*> m_owner{nullptr};Then derive the accessor from m_owner:
- inline bool isAcquired() const noexcept { return m_acquired.load(std::memory_order_acquire); } + inline bool isAcquired() const noexcept { + return m_owner.load(std::memory_order_acquire) != nullptr; + }If you keep both fields, add a strong invariant in acquire()/release() so they can’t get out of sync.
62-66: Prefer perfect forwarding and static (or implicit) upcast; avoid dynamic_pointer_cast overhead.dynamic_pointer_cast is for downcasts and adds RTTI cost. Here T should derive from To; use static cast or rely on implicit conversion, and forward constructor args.
Apply:
-template<class T, class To = devices::IDevice, typename... Args> -std::shared_ptr<To> make_device(Args... args) -{ - return std::dynamic_pointer_cast<To>(std::make_shared<T>(args...)); -} +template<class T, class To = devices::IDevice, typename... Args> +std::shared_ptr<To> make_device(Args&&... args) +{ + static_assert(std::is_base_of_v<To, T>, "T must derive from To"); + return std::static_pointer_cast<To>( + std::make_shared<T>(std::forward<Args>(args)...) + ); +}Note: This requires including <type_traits> and at the top of the file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.gitignore(1 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/converters/IRateConverter.hpp(1 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/devices/IDevice.hpp(1 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.cpp(5 hunks)sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/mame/MameOPL3.cpp(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/hardware/opl/mame/MameOPL3.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/drivers/westwood/ADLDriver.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
- GitHub Check: Build Windows (x64-windows, Release, 2022)
- GitHub Check: Build Windows (x64-windows, Debug, 2022)
- GitHub Check: SonarCloud Analysis (x64-windows, Debug)
🔇 Additional comments (2)
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/audio/converters/IRateConverter.hpp (1)
7-7: Include added for std::memset — LGTMAdding is appropriate here to make std::memset visible from this header. No API impact.
sdl2-hyper-sonic-drivers/src/HyperSonicDrivers/devices/IDevice.hpp (1)
5-5: Good fix: add for std::atomic members.This resolves compilation relying on transitive includes and matches the atomic members below.
|
Summary by CodeRabbit
New Features
Documentation
Tests
Chores