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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from
Nov 24, 2019
Merged

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

merged 1 commit into from
Nov 24, 2019

Conversation

kcgen
Copy link
Member

@kcgen kcgen commented Nov 22, 2019 •
edited
Loading

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 November 22, 2019 08:42
@kcgen kcgen self-assigned this Nov 22, 2019
@kcgen kcgen added bug Something isn't working cleanup Non-functional changes that simplify, improve maintainability, or squash warnings labels Nov 22, 2019
dreamer
dreamer requested changes Nov 23, 2019
View reviewed changes
Copy link
Member

@dreamer dreamer left a comment

Choose a reason for hiding this comment

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

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 November 23, 2019 22:33
kcgen
kcgen commented Nov 24, 2019
View reviewed changes
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 November 24, 2019 04:34
Play into subsequent track(s) if playback length exceeds the the curr…
ce9fe84 
…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 November 24, 2019 04:50
@kcgen
Copy link
Member Author

kcgen commented Nov 24, 2019 •
edited
Loading

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 commented Nov 24, 2019 •
edited
Loading

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 commented Nov 24, 2019 •
edited
Loading

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 November 24, 2019 05:54
dreamer
dreamer approved these changes Nov 24, 2019
View reviewed changes
Copy link
Member

@dreamer dreamer left a comment

Choose a reason for hiding this comment

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

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

src/dos/cdrom_image.cpp

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

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
@kcgen kcgen deleted the krcroft/fix-getaudiosub-segv branch November 26, 2019 01:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dreamer dreamer dreamer approved these changes

Assignees

@kcgen kcgen

Labels
bug Something isn't working cleanup Non-functional changes that simplify, improve maintainability, or squash warnings
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CDDA: does not continue into subsequent track(s) if playback length requires it
2 participants
@kcgen @dreamer