descramble refactored, verify descrambling on dump, disable cache rea…#354
Merged
descramble refactored, verify descrambling on dump, disable cache rea…#354
Conversation
…d on omnidrive, seek speed workaround
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a dvd::Scrambler and DataFrame-centric descrambling with per-frame Nintendo-key handling; refactors DataFrame to include a public nested DataFrame::ID; threads the Scrambler through read/dump/split/xbox flows; adds SCSI SEEK support; and further gates Mediatek leadout processing on OmniDrive firmware. Changes
Sequence DiagramsequenceDiagram
participant Dumper as DumpRoutine
participant Reader as read_dvd_sectors
participant SPTD as SPTD
participant Scrambler as dvd::Scrambler
Dumper->>Reader: call(lba_base, sectors_count, nintendo_key?, scrambler)
Reader->>SPTD: perform raw reads for LBA range
SPTD-->>Reader: return raw DataFrame(s)
loop per-frame
Reader->>Scrambler: descramble(df, nintendo_key?)
Scrambler->>Scrambler: validate DataFrame::ID
Scrambler->>Scrambler: compute table_offset / frame key
Scrambler->>Scrambler: process(df.main_data) xor _table
Scrambler->>Scrambler: verify EDC
alt EDC valid
Scrambler-->>Reader: success (may derive/update nintendo_key)
else EDC invalid
Scrambler-->>Reader: failure (mark invalid / optionally reapply)
end
end
Reader-->>Dumper: return combined status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 2
🧹 Nitpick comments (1)
dvd/xbox.ixx (1)
355-373: Consider hoisting theScramblerconstruction outside the retry loop.
dvd::Scrambler()at line 365 constructs the full 32 KB XOR table on each retry iteration. Moving it before the loop avoids redundant initialization.♻️ Suggested change
+ dvd::Scrambler scrambler; DataFrame df; bool ss_found = false; for(uint8_t ss_retries = 0; ss_retries < 4; ++ss_retries) { ... - if(dvd::Scrambler().descramble(df, std::nullopt)) + if(scrambler.descramble(df, std::nullopt))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/xbox.ixx` around lines 355 - 373, The Scrambler instance is constructed inside the retry loop causing the 32KB XOR table to be rebuilt on each iteration; move the construction of dvd::Scrambler() to before the for loop (reuse the instance when calling descramble) so descramble(df, std::nullopt) uses the same Scrambler across retries; update references in the loop to call the preconstructed scrambler and remove the per-iteration construction to avoid redundant initialization.
🤖 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_dump.ixx`:
- Line 1021: The log range uses an inclusive end in one place (LOG_R with lba +
(int32_t)sectors_to_read - 1) but an exclusive end elsewhere; make them
consistent by using the inclusive convention everywhere. Find the other LOG_R
call that prints the range using lba and sectors_to_read (the one currently
using lba + sectors_to_read) and change it to lba + (int32_t)sectors_to_read - 1
so both messages use the same inclusive end format; ensure any nearby comments
or tests expecting the exclusive form are updated to match.
- Around line 527-554: The loop rebuilds dvd::Scrambler() for every sector which
reconstructs a large 32KB table; move the Scrambler construction out of the
per-sector loop in read_dvd_sectors: create a single instance before the
for(uint32_t i = 0; i < sectors_count; ++i) loop (or make a function-local
static dvd::Scrambler scrambler) and use that instance to call descramble(df,
key), leaving the rest of the loop (key handling, nintendo_key update, setting
recording_frame via DataFrame_to_RecordingFrame) unchanged so the heavy table is
built once and reused.
---
Nitpick comments:
In `@dvd/xbox.ixx`:
- Around line 355-373: The Scrambler instance is constructed inside the retry
loop causing the 32KB XOR table to be rebuilt on each iteration; move the
construction of dvd::Scrambler() to before the for loop (reuse the instance when
calling descramble) so descramble(df, std::nullopt) uses the same Scrambler
across retries; update references in the loop to call the preconstructed
scrambler and remove the per-iteration construction to avoid redundant
initialization.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dvd/dvd_dump.ixx (1)
941-947: FIXME: temporary OmniDrive seek workaround should be tracked.The
throw_lineon failure is appropriate (a bad-state OmniDrive drive would corrupt the dump). TheFIXMEcomment flags this for removal once the firmware is fixed.Would you like me to open a tracking issue to remove this workaround once the OmniDrive firmware is patched?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` around lines 941 - 947, The temporary OmniDrive seek workaround needs a tracked TODO so it can be removed when firmware is fixed: update the FIXME comment near the omnidrive_firmware block (the if(omnidrive_firmware) { ... cmd_seek(*ctx.sptd, 0); throw_line(...) } code) to include a persistent tracking identifier (e.g., an issue/bug/ task number or URL) and a short rationale, e.g., "TODO: remove after OmniDrive firmware fix — tracked in ISSUE-XXXX"; keep the existing throw_line behavior unchanged so failures still abort, and ensure the comment format is consistent with other tracked TODOs in the codebase.
🤖 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_dump.ixx`:
- Around line 534-543: REFINE mode can skip re-reading LBA 0 so nintendo_key
stays 0 and descramble(df, key) fails for lba >= ECC_FRAMES; modify the logic in
read_dvd_sectors (or the caller before the main loop) to, when not in DUMP mode
and on a Nintendo disc, load the stored LBA 0 from the existing .iso (or cached
file_state entry) and call scrambler.descramble(df_of_lba0, 0) to compute and
set *nintendo_key using the existing derivation path (sum >> 4)+sum & 0xF before
processing other LBAs, ensuring nintendo_key is populated so descramble calls
for lba >= ECC_FRAMES use the correct key and avoid permanent ERROR_SKIP states.
---
Nitpick comments:
In `@dvd/dvd_dump.ixx`:
- Around line 941-947: The temporary OmniDrive seek workaround needs a tracked
TODO so it can be removed when firmware is fixed: update the FIXME comment near
the omnidrive_firmware block (the if(omnidrive_firmware) { ...
cmd_seek(*ctx.sptd, 0); throw_line(...) } code) to include a persistent tracking
identifier (e.g., an issue/bug/ task number or URL) and a short rationale, e.g.,
"TODO: remove after OmniDrive firmware fix — tracked in ISSUE-XXXX"; keep the
existing throw_line behavior unchanged so failures still abort, and ensure the
comment format is consistent with other tracked TODOs in the codebase.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
dvd/dvd_dump.ixx (2)
541-542: Nintendo key derivation logic is duplicated at line 914-915 — consider extracting a helper.The identical two-liner appears in both
read_dvd_sectors(lines 541–542) and the REFINE pre-loop (lines 914–915). A small local lambda or inline helper would remove the duplication.♻️ Proposed refactor
+ auto derive_nintendo_key = [](const DataFrame &df) -> uint8_t { + auto sum = std::accumulate(df.cpr_mai, df.cpr_mai + 8, 0); + return ((sum >> 4) + sum) & 0xF; + };Then replace both call sites with
*nintendo_key = derive_nintendo_key(df);.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` around lines 541 - 542, Duplicate Nintendo key derivation exists where the two-line calculation using df.cpr_mai is repeated in read_dvd_sectors and the REFINE pre-loop; extract that logic into a small helper (e.g., inline function or local lambda named derive_nintendo_key) that accepts the df (or a pointer/reference to df.cpr_mai) and returns the computed 4-bit key, then replace both occurrences with an assignment like *nintendo_key = derive_nintendo_key(df) (or equivalent), keeping the original computation ((sum >> 4) + sum) & 0xF but centralized in the new derive_nintendo_key function.
962-968: FIXME seek runs in all DumpModes and hard-aborts on failure.The pre-loop seek to LBA 0 fires for DUMP, REFINE, and VERIFY when
omnidrive_firmwareis true. If the seek fails,throw_lineaborts the entire operation in every mode. For a temporary firmware workaround (FIXME), a non-fatalLOG+ continue for REFINE/VERIFY might be preferable — a seek error is unlikely to prevent a successful re-read loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` around lines 962 - 968, The pre-loop seek guarded by omnidrive_firmware currently calls cmd_seek(*ctx.sptd, 0) and unconditionally aborts via throw_line on failure; change it so only the destructive DUMP mode still throws on seek failure while REFINE and VERIFY log and continue: after calling cmd_seek (using existing omnidrive_firmware, cmd_seek and ctx.sptd), check the current dump mode (e.g., DumpMode or the variable that holds the mode) and if status.status_code != 0 then if mode == DumpMode::DUMP call throw_line as before, otherwise emit a non-fatal log (use the project LOG or existing logger) with SPTD::StatusMessage(status) and continue without throwing.
🤖 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_dump.ixx`:
- Around line 899-918: The REFINE block can read only cfg.sector_size (2048)
into a RecordingFrame then access its full contents (UB) and also silently leave
nintendo_key zero if descramble fails; fix by requiring raw when attempting to
read/derive the Nintendo key (change the if to also check raw or ensure rf is
fully initialized) so RecordingFrame is fully populated before calling
RecordingFrame_to_DataFrame, and add a diagnostic log/error when
scrambler.descramble(df, *nintendo_key) returns false (include lba_index and
current key attempt) so callers know stored LBA 0 failed to yield a valid key;
use the existing RecordingFrame, read_entry, cfg.sector_size, raw,
RecordingFrame_to_DataFrame, scrambler.descramble, nintendo_key and
State::ERROR_SKIP symbols to locate and implement these changes.
---
Nitpick comments:
In `@dvd/dvd_dump.ixx`:
- Around line 541-542: Duplicate Nintendo key derivation exists where the
two-line calculation using df.cpr_mai is repeated in read_dvd_sectors and the
REFINE pre-loop; extract that logic into a small helper (e.g., inline function
or local lambda named derive_nintendo_key) that accepts the df (or a
pointer/reference to df.cpr_mai) and returns the computed 4-bit key, then
replace both occurrences with an assignment like *nintendo_key =
derive_nintendo_key(df) (or equivalent), keeping the original computation ((sum
>> 4) + sum) & 0xF but centralized in the new derive_nintendo_key function.
- Around line 962-968: The pre-loop seek guarded by omnidrive_firmware currently
calls cmd_seek(*ctx.sptd, 0) and unconditionally aborts via throw_line on
failure; change it so only the destructive DUMP mode still throws on seek
failure while REFINE and VERIFY log and continue: after calling cmd_seek (using
existing omnidrive_firmware, cmd_seek and ctx.sptd), check the current dump mode
(e.g., DumpMode or the variable that holds the mode) and if status.status_code
!= 0 then if mode == DumpMode::DUMP call throw_line as before, otherwise emit a
non-fatal log (use the project LOG or existing logger) with
SPTD::StatusMessage(status) and continue without throwing.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
dvd/dvd_dump.ixx (1)
854-856: No user warning when a Nintendo disc is detected without OmniDrive firmware.When
nintendo_keyis set butomnidrive_firmwareisfalse,rawbecomesfalseand the dump silently falls back to non-raw mode. The existing warning on line 856 only fires for explicitoptions.dvd_raw. Consider adding a similar diagnostic so users know Nintendo-specific handling is unavailable.💡 Suggested improvement
bool raw = (options.dvd_raw || nintendo_key) && omnidrive_firmware; if(options.dvd_raw && !omnidrive_firmware) LOG("warning: drive not compatible with raw DVD dumping"); + if(nintendo_key && !omnidrive_firmware) + LOG("warning: Nintendo disc detected but drive not compatible with raw DVD dumping");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` around lines 854 - 856, The code silently disables raw dumping when nintendo_key is set but omnidrive_firmware is false; add a diagnostic for that case so users know Nintendo-specific handling is unavailable. Modify the logic around the raw calculation (symbols: raw, options.dvd_raw, nintendo_key, omnidrive_firmware) to emit a LOG warning (same style as the existing LOG call) when nintendo_key && !omnidrive_firmware, e.g. a clear message stating Nintendo disc detected but OmniDrive firmware is missing so raw dumping/Nintendo handling will be skipped; keep the existing warning for options.dvd_raw as-is.
🤖 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_dump.ixx`:
- Line 541: The line performing a double assignment to *nintendo_key is a typo:
replace the duplicated assignment with a single assignment so that the result of
nintendo::derive_key(std::span(df.cpr_mai, df.cpr_mai + 8)) is stored once into
*nintendo_key (locate the occurrence around the use of nintendo_key,
nintendo::derive_key and df.cpr_mai and remove the extra "= *nintendo_key ="
duplication).
In `@dvd/nintendo.ixx`:
- Line 8: Remove the unused module import by deleting the "import dvd;" line at
the top of the file; the derive_key function only uses std::span and
std::accumulate so there are no dvd symbols referenced, removing this import
reduces coupling and avoids an unused-import warning.
---
Duplicate comments:
In `@dvd/dvd_dump.ixx`:
- Around line 897-913: The descramble failure path in the REFINE block (guarded
by nintendo_key, raw and dump_mode == DumpMode::REFINE) is silent; add an else
branch after the scrambler.descramble(...) call to emit a diagnostic log or
warning so failures are visible during debugging. Locate the block using symbols
RecordingFrame, State, read_entry, RecordingFrame_to_DataFrame and
scrambler.descramble, and log a concise message that includes the LBA index
(lba_index) and an indication that descrambling with the initial key failed; do
not change the existing successful-path logic that calls nintendo::derive_key.
---
Nitpick comments:
In `@dvd/dvd_dump.ixx`:
- Around line 854-856: The code silently disables raw dumping when nintendo_key
is set but omnidrive_firmware is false; add a diagnostic for that case so users
know Nintendo-specific handling is unavailable. Modify the logic around the raw
calculation (symbols: raw, options.dvd_raw, nintendo_key, omnidrive_firmware) to
emit a LOG warning (same style as the existing LOG call) when nintendo_key &&
!omnidrive_firmware, e.g. a clear message stating Nintendo disc detected but
OmniDrive firmware is missing so raw dumping/Nintendo handling will be skipped;
keep the existing warning for options.dvd_raw as-is.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
dvd/dvd_dump.ixx (1)
957-963: FIXME: Track OmniDrive firmware seek workaround.The unconditional
cmd_seek(*ctx.sptd, 0)for allomnidrive_firmwarepaths (including Blu-ray) looks intentional for now and the throw-on-failure is the right choice to avoid silent data corruption. Would you like me to open a tracking issue for removing this once the firmware is fixed?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dvd/dvd_dump.ixx` around lines 957 - 963, Unconditional seek call for omnidrive_firmware (cmd_seek(*ctx.sptd, 0)) is a temporary workaround and must be tracked: create a tracking issue in the project tracker describing the OmniDrive firmware bug and the current workaround (including that it runs for Blu-ray paths), then update the FIXME comment above the code to reference that issue (e.g., "FIXME: remove after OmniDrive firmware fix — see ISSUE-1234"), keeping the throw_on_failure behavior (throw_line("omnidrive: failed to seek, SCSI ({})", SPTD::StatusMessage(status))) unchanged so failures still abort; ensure the comment references the exact symbols omnidrive_firmware, cmd_seek, ctx.sptd, and throw_line to make future removal straightforward.
🤖 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_dump.ixx`:
- Around line 550-552: The code unconditionally overwrites any SCSI error in
status with a synthetic SPTD::Status{0x02,0x04,0x10} when descrambling fails
(valid == false), hiding real errors returned by cmd_read_omnidrive; change the
assignment so the synthetic "read error" status is only written when there is no
prior error (i.e., when status indicates success/zero), leaving an existing
non‑zero status from cmd_read_omnidrive untouched (referencing
cmd_read_omnidrive, valid, status and the SPTD::Status{0x02,0x04,0x10} literal
to locate the code).
---
Nitpick comments:
In `@dvd/dvd_dump.ixx`:
- Around line 957-963: Unconditional seek call for omnidrive_firmware
(cmd_seek(*ctx.sptd, 0)) is a temporary workaround and must be tracked: create a
tracking issue in the project tracker describing the OmniDrive firmware bug and
the current workaround (including that it runs for Blu-ray paths), then update
the FIXME comment above the code to reference that issue (e.g., "FIXME: remove
after OmniDrive firmware fix — see ISSUE-1234"), keeping the throw_on_failure
behavior (throw_line("omnidrive: failed to seek, SCSI ({})",
SPTD::StatusMessage(status))) unchanged so failures still abort; ensure the
comment references the exact symbols omnidrive_firmware, cmd_seek, ctx.sptd, and
throw_line to make future removal straightforward.
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
1 participant
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.
WIP
Summary by CodeRabbit
New Features
Refactor
Bug Fixes / Stability