Conversation
* Raw BD read * code review * fix * fix build * Fix bd scrambler * currently only works with dump size 1 * OmniDrive bluray data frame is 2070 bytes --------- Co-authored-by: Hennadiy Brych <gennadiy.brich@gmail.com>
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds Blu-ray raw-read and extraction support: new BD C++ modules ( Changes
Sequence DiagramsequenceDiagram
participant Caller as redumper/dvd_dump
participant SPTD as SPTD (device)
participant Frame as bd::DataFrame
participant Scrambler as bd::Scrambler
Caller->>SPTD: read raw sectors (LBA, count, disc_type=BD, raw)
SPTD-->>Caller: raw OmniDrive frame bytes
Caller->>Frame: reinterpret bytes as OmniDriveDataFrame / bd::DataFrame
Caller->>Scrambler: Scrambler::get().descramble(Frame.main_data, psn)
Scrambler->>Scrambler: init 16-bit LFSR from PSN
Scrambler->>Scrambler: XOR bytes with LFSR stream, update register
Scrambler->>Caller: return descrambled bytes / status (EDC check)
Caller->>Caller: validate EDC, log, write output or handle error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
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 (1)
dvd/dvd_dump.ixx (1)
522-543:⚠️ Potential issue | 🔴 CriticalGuard unsupported raw disc types; current flow can return an unset
SPTD::Status.When
rawis true for a non-DVD/non-BD type, no branch assignsstatusbefore return. That creates undefined/error-prone behavior depending onSPTD::Statusconstruction.🔧 Proposed fix
-SPTD::Status read_dvd_sectors(SPTD &sptd, uint8_t *sectors, uint32_t sector_size, int32_t lba, uint32_t sectors_count, bool force_unit_access, DiscType disc_type, bool raw) +SPTD::Status read_dvd_sectors(SPTD &sptd, uint8_t *sectors, uint32_t sector_size, int32_t lba, uint32_t sectors_count, bool force_unit_access, DiscType disc_type, bool raw) { - SPTD::Status status; + SPTD::Status status{}; @@ else if(disc_type == DiscType::BLURAY || disc_type == DiscType::BLURAY_R) { @@ } } + else + { + throw_line("raw read is unsupported for this disc type"); + } return status; } @@ - bool raw = (options.dvd_raw || options.bd_raw || nintendo_key) && omnidrive_firmware; + bool raw = omnidrive_firmware && + (nintendo_key + || (ctx.disc_type == DiscType::DVD && options.dvd_raw) + || ((ctx.disc_type == DiscType::BLURAY || ctx.disc_type == DiscType::BLURAY_R) && options.bd_raw));Run this read-only check to verify branch coverage and
SPTD::Statusconstruction semantics:#!/bin/bash set -euo pipefail echo "== Locate SPTD::Status declaration and fields ==" fd -a 'sptd\.(ixx|hh|hpp|h)$' | while read -r f; do echo "--- $f ---" rg -n -C3 'struct Status|class Status|status_code|sense_key|asc|ascq|Status\(' "$f" || true done echo echo "== Inspect raw-mode gating and branch coverage in dvd dump ==" rg -n -C4 'bool raw =|SPTD::Status read_dvd_sectors|if\(!raw\)|DiscType::DVD|DiscType::BLURAY_R|DiscType::HDDVD' dvd/dvd_dump.ixxExpected result: either explicit safe initialization is guaranteed for
SPTD::Status, or code should be hardened as shown above.Also applies to: 877-881
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` around lines 522 - 543, The branch where raw==true for unsupported disc types can leave the SPTD::Status variable (status) unset; update the read path in the function that calls cmd_read and cmd_read_omnidrive so that every branch assigns a valid SPTD::Status before use/return: add an explicit assignment of a safe/failed SPTD::Status (or return early) for the raw && disc_type not DVD/BLURAY/BLURAY_R case, or ensure status is default-initialized where declared; modify the branches around cmd_read, cmd_read_omnidrive, and the raw check (referencing variables/functions status, raw, DiscType, cmd_read, cmd_read_omnidrive, and SPTD::Status) so no execution path can return/consume an uninitialized status.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dvd/dvd_split.ixx`:
- Around line 267-276: When scrambler.descramble(df, lba - bd::LBA_START) fails,
do not write the original (still-scrambled) df to iso_fs; instead preserve the
existing descramble_errors logic (use descramble_errors.emplace_back/extend) but
then, if leave_unchanged is false, zero-fill the payload bytes in df (the
portion that would contain the descrambled data) before the iso_fs.write((char
*)&df, FORM1_DATA_SIZE) call; if leave_unchanged is true, leave df untouched and
write it as-is. Ensure the zero-fill targets the same frame payload region
handled by the DVD invalid-frame code path and keep the write and fail-check
flow unchanged.
---
Outside diff comments:
In `@dvd/dvd_dump.ixx`:
- Around line 522-543: The branch where raw==true for unsupported disc types can
leave the SPTD::Status variable (status) unset; update the read path in the
function that calls cmd_read and cmd_read_omnidrive so that every branch assigns
a valid SPTD::Status before use/return: add an explicit assignment of a
safe/failed SPTD::Status (or return early) for the raw && disc_type not
DVD/BLURAY/BLURAY_R case, or ensure status is default-initialized where
declared; modify the branches around cmd_read, cmd_read_omnidrive, and the raw
check (referencing variables/functions status, raw, DiscType, cmd_read,
cmd_read_omnidrive, and SPTD::Status) so no execution path can return/consume an
uninitialized status.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CMakeLists.txtcd/protection.ixxdvd/bd.ixxdvd/bd_scrambler.ixxdvd/dvd_dump.ixxdvd/dvd_split.ixxoptions.ixxredumper.ixx
superg
commented
Mar 1, 2026
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dvd/bd.ixx (1)
21-35: Add compile-time layout guards for BD wire structs.These structs represent on-disc/raw-frame layout; add
static_assertchecks to catch accidental ABI/layout drift at compile time.Suggested patch
export struct DataFrame { uint8_t main_data[FORM1_DATA_SIZE]; uint32_t edc; }; +static_assert(sizeof(DataFrame) == FORM1_DATA_SIZE + sizeof(uint32_t)); `#pragma` pack(push, 1) export struct OmniDriveDataFrame { DataFrame data_frame; uint8_t unknown[18]; }; `#pragma` pack(pop) +static_assert(sizeof(OmniDriveDataFrame) == sizeof(DataFrame) + 18); +static_assert(alignof(OmniDriveDataFrame) == 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/bd.ixx` around lines 21 - 35, Add compile-time layout guards for the BD wire structs by adding static_asserts that validate sizeof(DataFrame) == FORM1_DATA_SIZE + sizeof(uint32_t) and sizeof(OmniDriveDataFrame) equals sizeof(DataFrame) + 18, plus static_asserts on offsetof(DataFrame, edc) == FORM1_DATA_SIZE and offsetof(OmniDriveDataFrame, data_frame) == 0 to ensure packing; reference the types DataFrame and OmniDriveDataFrame and the constant FORM1_DATA_SIZE when writing these checks so any ABI/layout drift is caught at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dvd/bd.ixx`:
- Around line 21-35: Add compile-time layout guards for the BD wire structs by
adding static_asserts that validate sizeof(DataFrame) == FORM1_DATA_SIZE +
sizeof(uint32_t) and sizeof(OmniDriveDataFrame) equals sizeof(DataFrame) + 18,
plus static_asserts on offsetof(DataFrame, edc) == FORM1_DATA_SIZE and
offsetof(OmniDriveDataFrame, data_frame) == 0 to ensure packing; reference the
types DataFrame and OmniDriveDataFrame and the constant FORM1_DATA_SIZE when
writing these checks so any ABI/layout drift is caught at compile time.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dvd/bd_scrambler.ixx (1)
6-8: Remove unused includes.
<optional>and<vector>are included but not used in this module.🧹 Proposed fix
`#include` <climits> `#include` <cstddef> `#include` <cstdint> -#include <optional> `#include` <span> -#include <vector>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/bd_scrambler.ixx` around lines 6 - 8, The includes <optional> and <vector> in dvd/bd_scrambler.ixx are unused; remove those two include lines and keep only the necessary `#include` <span> (or any other actually used headers) so the module only imports headers it uses; locate the top-of-file include block in bd_scrambler.ixx and delete the <optional> and <vector> entries.dvd/dvd_dump.ixx (1)
887-892: Minor comment discrepancy.The comment mentions
7 * 2072butsizeof(bd::OmniDriveDataFrame)is actually 2070 bytes (2052 + 18). The calculation still fits within the 16384 byte limit (7 * 2070 = 14490), so this is just a documentation nit.📝 Suggested comment fix
else { - // bd: ensure default total transfer length is less than 16384 (7 * 2072) + // bd: ensure default total transfer length is less than 16384 (7 * 2070) LOG("warning: setting dump read size to 7 for raw BD dumping"); dump_read_size = 7; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` around lines 887 - 892, Update the inline comment that computes the BD dump frame size to use the correct sizeof(bd::OmniDriveDataFrame) value (2070) instead of 2072 in the explanatory text near the else block that sets dump_read_size = 7; i.e., change the comment "7 * 2072" to "7 * 2070" (keeping the LOG and dump_read_size = 7 logic unchanged) so the documentation matches the actual frame size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dvd/bd_scrambler.ixx`:
- Around line 6-8: The includes <optional> and <vector> in dvd/bd_scrambler.ixx
are unused; remove those two include lines and keep only the necessary `#include`
<span> (or any other actually used headers) so the module only imports headers
it uses; locate the top-of-file include block in bd_scrambler.ixx and delete the
<optional> and <vector> entries.
In `@dvd/dvd_dump.ixx`:
- Around line 887-892: Update the inline comment that computes the BD dump frame
size to use the correct sizeof(bd::OmniDriveDataFrame) value (2070) instead of
2072 in the explanatory text near the else block that sets dump_read_size = 7;
i.e., change the comment "7 * 2072" to "7 * 2070" (keeping the LOG and
dump_read_size = 7 logic unchanged) so the documentation matches the actual
frame size.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
dvd/bd_scrambler.ixx (1)
2-14: Several imports appear unused.Headers
<algorithm>and<cstddef>are included but not used in this file. Module importscd.cdrom,dvd,dvd.edc,utils.endian, andutils.miscalso don't appear to be referenced in the implementation.If these are transitive dependencies required by the module system, consider adding a brief comment to clarify their purpose.
♻️ Proposed cleanup if imports are not required
module; -#include <algorithm> `#include` <climits> -#include <cstddef> `#include` <cstdint> `#include` <span> export module bd.scrambler; -import cd.cdrom; -import dvd; -import dvd.edc; -import utils.endian; -import utils.misc;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/bd_scrambler.ixx` around lines 2 - 14, The includes <algorithm> and <cstddef> and the module imports cd.cdrom, dvd, dvd.edc, utils.endian and utils.misc in bd_scrambler.ixx appear unused; remove those unnecessary headers and module imports from the top of the file (around the export module bd.scrambler; block) to clean up dependencies, or if any are required only for transitive/module-interface reasons add a short comment next to each import explaining why it must remain.dvd/dvd_dump.ixx (1)
73-73: Clarify the NINTENDO book type assignment.Book type index 15 is being renamed from "RESERVED5" to "NINTENDO". This appears to be for GameCube/Wii disc identification where the physical structure's first byte is 0xFF (per line 660-661). Consider adding a brief comment explaining this Nintendo-specific book type usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` at line 73, The "NINTENDO" entry replacing "RESERVED5" lacks context; add a brief inline comment next to the book type array entry "NINTENDO" in dvd_dump.ixx explaining that index 15 is used for GameCube/Wii discs where the physical structure's first byte equals 0xFF (the check performed elsewhere when determining book type), clarifying this is a Nintendo-specific identifier and referencing the code path that checks the physical structure byte so future readers understand why RESERVED5 was repurposed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dvd/bd_scrambler.ixx`:
- Around line 2-14: The includes <algorithm> and <cstddef> and the module
imports cd.cdrom, dvd, dvd.edc, utils.endian and utils.misc in bd_scrambler.ixx
appear unused; remove those unnecessary headers and module imports from the top
of the file (around the export module bd.scrambler; block) to clean up
dependencies, or if any are required only for transitive/module-interface
reasons add a short comment next to each import explaining why it must remain.
In `@dvd/dvd_dump.ixx`:
- Line 73: The "NINTENDO" entry replacing "RESERVED5" lacks context; add a brief
inline comment next to the book type array entry "NINTENDO" in dvd_dump.ixx
explaining that index 15 is used for GameCube/Wii discs where the physical
structure's first byte equals 0xFF (the check performed elsewhere when
determining book type), clarifying this is a Nintendo-specific identifier and
referencing the code path that checks the physical structure byte so future
readers understand why RESERVED5 was repurposed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Chores