Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Play into subsequent track(s) if requested length requires it #51

Merged
merged 1 commit into from Nov 24, 2019

Conversation

kcgen
Copy link
Member

@kcgen kcgen commented Nov 22, 2019

Fixes #50

In debugging and performing this fix, many debug messages were improved as well as making some small small code adjustments to improve readability, such as using iterators for a given track, track->attribute instead of subtracting into the array vector tracks[track -1].attribute

@kcgen kcgen requested a review from dreamer Nov 22, 2019
@kcgen kcgen self-assigned this Nov 22, 2019
@kcgen kcgen added bug cleanup labels Nov 22, 2019
Copy link
Member

@dreamer dreamer left a comment

Overall these changes look really good :)

src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
src/dos/cdrom_image.cpp Show resolved Hide resolved
src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
src/dos/cdrom_image.cpp Show resolved Hide resolved
src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
src/misc/support.cpp Outdated Show resolved Hide resolved
@kcgen kcgen force-pushed the krcroft/fix-getaudiosub-segv branch from 37ff619 to fb18d4e Compare Nov 23, 2019
src/dos/cdrom.h Outdated Show resolved Hide resolved
src/dos/cdrom.h Outdated Show resolved Hide resolved
include/support.h Outdated Show resolved Hide resolved
src/dos/cdrom.h Outdated Show resolved Hide resolved
src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
src/dos/cdrom_image.cpp Outdated Show resolved Hide resolved
@kcgen kcgen force-pushed the krcroft/fix-getaudiosub-segv branch 2 times, most recently from e83c5cd to 9174f2b Compare Nov 24, 2019
…ent track

Issue reported by Dagar and Pr3tty F1y, and confirmed as a bug by ripsaw8080.
Thank you!

This fixes the GoG release of Betrayal at Krondor which (either due to CD mastering
issues or a faulty rip), requests playback of a given track at the tail end
of the prior track.

In debugging and performing this fix, many debug messages were improved as well
as making some small small code adjustments, such as using iterators to point to
individual tracks (track->attribute) instead of using the tracks array
(tracks[track -1].attribute).
@kcgen kcgen force-pushed the krcroft/fix-getaudiosub-segv branch from f271e98 to ce9fe84 Compare Nov 24, 2019
@kcgen
Copy link
Member Author

@kcgen kcgen commented Nov 24, 2019

Reverted to casting instead of using %zu and building clean; all set I think.
Thank you for the thorough review and feedback @dreamer .

@kcgen
Copy link
Member Author

@kcgen kcgen commented Nov 24, 2019

Something unexpected! Destruction Derby now plays its CDDA, without any special MSF time handling either. Somehow this PR's fixed whatever was causing issues with Destruction Derby to not play.
2019-11-23_21-20

Update: Destruction Derby has protection that checks the track MSF times, and we see this PR calculates the correct MSFs (when using compressed audio tracks) such that they match the BIN/CUE values exactly:
2019-11-23_22-01

(In the console debug output, the duplicate track info queries are performed by Destruction Derby. I'm not sure why, but maybe the dev team's hardware they needed a second call for the result to be trusted).

@kcgen
Copy link
Member Author

@kcgen kcgen commented Nov 24, 2019

Test commands:

tar zx < destruction_derby-split-*
cd "Destruction Derby"
/usr/src/dosbox-staging/src/dosbox start.bat

That loads the Opus+ISO cue CD, for which you should hear CD-DA at the main menu as well during play game.

The in-game options menu also lets you play each track.

Launching vorbis.bat instead of start.bat will load the more typical Ogg-Vorbis+ISO cue CD instead.

Try that with this branch (optionally enable the #define DEBUG 1 in cdrom_image.cpp to see more verbose CD messages), versus svn/trunk.

Note: the following content under test is a DOS game that requires its original and obsoleted media be present and running on obsoleted and unsupported hardware within the obsoleted and unsupported DOS operating system. This meets the criteria for exemption as specified: "Computer programs and video games distributed in formats that have become obsolete and which require the original media or hardware as a condition of access.", here: https://www.copyright.gov/1201/docs/librarian_statement_01.html

Content under test:

@kcgen kcgen requested a review from dreamer Nov 24, 2019
Copy link
Member

@dreamer dreamer left a comment

Looks great, glad to see it improve game compatibility as well :)


track_num = track->number;
attr = track->attr;
index = 1;
Copy link
Member

@dreamer dreamer Nov 24, 2019

Choose a reason for hiding this comment

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

This is weird… GitHub colorized index as if it was a keyword or a function…

(seems not to be an issue for compiler, but it's curious)

@dreamer dreamer merged commit eaeb001 into master Nov 24, 2019
13 checks passed
@kcgen kcgen deleted the krcroft/fix-getaudiosub-segv branch Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants