MDS: track the read position instead of inferring it from the file pointer - #234
Closed
iTechMedic wants to merge 1 commit into
Closed
MDS: track the read position instead of inferring it from the file pointer#234iTechMedic wants to merge 1 commit into
iTechMedic wants to merge 1 commit into
Conversation
…inter
Seek() decided it was already in position by comparing Tell() against the
offset it was asked for. Those are not the same kind of number: Tell() is a
byte offset into the MDF, the argument is an address on the disc. On an image
with 2448-byte sectors they run at different rates, and on one with an
unstored pregap the disc address moves where the file offset does not.
When they did coincide the function returned early, and the early return
skipped recording the LBA. Read() takes both its gap detection and its
subchannel stride from that LBA, so it went on serving whichever frame the
reader was last on - and reported success doing it.
Two ways in, both ordinary:
* a contiguous 2352-byte image makes the two offsets equal at every frame,
so reading a track to its end and then reading on lands in the pregap
with a stale LBA, the hole goes undetected, and the next track's bytes
come back where zeros belong. This is the Video CD case: on the real
SVIDEOCD image, LBA 526 returned track 2's volume descriptor.
* a 2448-byte image makes them equal every 49th frame, since 49 * 2448 is
51 * 2352. Ending a read at frame 48 and then reading frame 51 served
frame 49.
Compare against the file offset actually computed, and set the LBA before any
early exit can skip it. The plain read path now advances the position like the
other two already did, so the class keeps one definition of where it is.
Two smaller holes closed alongside, both reachable through the same gap:
a read shorter than one frame skipped the gap check on its size alone, and
ReadSubchannel() failed outright on a frame Seek() and Read() were both
willing to answer with zeros.
Gap detection was also O(frames x tracks) on every read - a full track-table
walk per frame - and most images have no hole at all. Init() now settles that
once by comparing the summed track lengths against the disc length, which
makes the common path cheaper than it was before this change.
Four tests, one per fix, each verified by putting the bug back and watching
that test go red. Checked against real Alcohol images too (DESCENT_II,
SVIDEOCD, NFSSEBBC): the sparse ones take the walk path, the single-track one
does not, and the pre-fix reader fails the pregap read on SVIDEOCD itself.
Collaborator
Author
|
Superseded by #238, which combines this with the rest of the batch into a single branch. Same commits, no changes dropped. Closing this one. |
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.
One commit, with three new tests, all confirmed red before the fix.
Seek()decided it was already in position by comparingTell()against the offset it was asked for. Those are not the same kind of number.Tell()is a byte offset into the MDF, the argument is an address on the disc. On an image with 2448 byte sectors they run at different rates, and on one with an unstored pregap the disc address moves where the file offset does not.When they did coincide the function returned early, and the early return skipped recording the LBA.
Read()takes both its gap detection and its subchannel stride from that LBA, so it went on serving whichever frame the reader was last on, and reported success doing it.Two ways in, both ordinary:
The fix compares against the file offset actually computed, and sets the LBA before any early exit can skip it. The plain read path now advances the position like the other two already did, so the class keeps one definition of where it is.
The fixtures use the exact geometry of real Alcohol images rather than invented numbers. Parsing three of my own discs confirmed the convention the reader assumes:
lengthexcludes the pregap, andstart_offsetaccounts only for stored frames.Hardware tested on a Pi Zero 2 W: Descent II, audio track 13 played end to end and the game installed, on Win98 and XP.
Host suite 150/150.