DIRECTOR: add support for absolute timing CD audio playback via cuesheets #5428

mistydemeo · 2023-11-14T03:59:27Z

This adds support for playing CD audio via absolute timecodes instead of via track numbers. This is needed for several Director games. In order for this to work via CD audio emulation, we need access to a full table of contents for the disc. We can't just use the audio files for the CD tracks, because in almost every case, a data track comes before the first CD track - it's impossible to know what absolute sector an audio track begins and ends that without knowing the size of the data track.

To fix this, I've made use of cuesheets in order to have access to a full table of contents of the disc. In absolute playback mode, we check to see if the game directory has a file named disc.cue, and if so, we parse that and use it as the TOC. I'm using the new cuesheet parser that @sev- wrote. It's been updated with a few changes to accommodate this:

Several methods have been added to Common::CueSheet to allow retrieving tracks.
The parser has been reoriented so that the internal code keeps a flat array of tracks, instead of having tracks belong to files. This makes the track getter methods more convenient, and also eliminates a quirk where there were a few "fake" empty tracks inside file arrays.

The generic emulation-based CD audio player has been updated with a new playAbsolute method which takes an absolute timecode as its argument. At the moment, the macOS CD code doesn't implement playAbsolute; it's effectively a form of emulation already, which reads the AIFF files that macOS exposes instead of lower-level CDDA playback methods, and so we don't have access to the disc's TOC.

I've tested with Classical Cats for Mac. Results so far: it works great, and is playing back music as expected. There is a quirk where it's always playing from frame 0, but that's not the fault of the xlib - I haven't checked where the number is coming from, but it seems to be assigned in lingo outside the xlib.

Marking as draft because I haven't yet implemented true absolute playback support for the Windows or SDL CD-ROM devices. Also marking as a draft for feedback from @sev- on the changes to the cuesheet code.

engines/director/lingo/xlibs/applecdxobj.cpp

engines/director/lingo/xlibs/cdromxobj.cpp

bluegr · 2023-11-14T09:29:34Z

Marking as draft because I haven't yet implemented true absolute playback support for the Windows or SDL CD-ROM devices.

But you can just add a stub for this in backends/audiocd/win32/win32-audiocd.cpp, like you did for Linux and MacOS, right?

sev- · 2023-11-17T00:50:55Z

Just in one of the programs I looked at, there was an example with a multitrack image with multiple files in each of them.

I have no prediction if we meet such a beast in the wild.

mistydemeo · 2023-11-17T04:24:11Z

Just in one of the programs I looked at, there was an example with a multitrack image with multiple files in each of them.

As in, one cue with multiple files? That should be fine, though we'll want to make sure that the timing value we're parsing out is consistent between both kinds.

But you can just add a stub for this in backends/audiocd/win32/win32-audiocd.cpp, like you did for Linux and MacOS, right?

Yeah, definitely. I'll add that now while I wait to figure out a real implementation.

sev-

Thank you for your work.

I have several minor notes, but the most significant one, is that if I read the code correctly, on Linux and Windows, the cue sheets will not be used, because (a) they override the default method and (b) return false.

So, if I read the code correctly, I would drop those overrides in all backends and at this moment, have playAbsolute() work only with the CUE sheets, not talking to the real hardware, e.g. leave only the default implementation.

backends/audiocd/default/default-audiocd.cpp

backends/audiocd/linux/linux-audiocd.cpp

sev- · 2023-11-18T19:49:11Z

backends/audiocd/sdl/sdl-audiocd.cpp

@@ -123,6 +123,12 @@ bool SdlAudioCDManager::play(int track, int numLoops, int startFrame, int durati
 	return true;
 }

+bool SdlAudioCDManager::playAbsolute(int startFrame, int numLoops, int duration, bool onlyEmulate,
+		Audio::Mixer::SoundType soundType) {
+	// TODO not implemented


I am confused with these. To me, it seems that they will override the default method, e.g. emulation, and will be no-op. Am I reading the code correctly?

I'd gotten confused by the inheritance in a previous iteration and didn't realize I could safely omit playAbsolute implementations. I've deleted these now.

I checked the SDL docs and see how I think I could implement this method for real, but I also don't have access to a Linux machine to test.

sev- · 2023-11-18T19:49:55Z

common/formats/cue.cpp

@@ -249,17 +249,19 @@ void CueSheet::parseFilesContext(const char *line) {
 		int trackNum = atoi(nexttok(s, &s).c_str());
 		String trackType = nexttok(s, &s);

-		if (trackNum < 0 || (_currentTrack > 0 && _currentTrack + 1 != trackNum)) {
+		if (trackNum < 0 || (_currentTrack > 0 && _currentTrack + 2 != trackNum)) {


What is + 2? Please add a comment above

Added a comment explaining it.

common/formats/cue.cpp

sev- · 2023-11-18T19:52:47Z

common/formats/cue.cpp

+
+	// Not found within any tracks, but could be in the final track
+	if (frame > _tracks.back().indices.back()) {
+		debug(5, "CueSheet::getTrackAtFrame: Returning final track");


I would turn it into a warning(), e.g. the user code is requesting us to do something odd, beyond the CD limits.

The request isn't as odd as it sounds - I'll update the comment to explain why.

The cuesheet gives us the starting indices of each track, but not track length or ending indices. In other words, we don't know how long the song is - we actually don't have an indication of whether the index the caller passed is in the disc or not, we just know that it comes after the start of this track.

The only actual way to 100% for sure know the number of sectors in the track is to actually check the file on disk and see how large it is. (Or load it as an audio file and check the duration.) That's a bit too complex to handle here; I'd rather defer complexity around loading and examining audio files to the CD audio player.

common/formats/cue.cpp

sev- · 2023-11-18T19:56:09Z

engines/director/lingo/xlibs/applecdxobj.h

@@ -22,11 +22,20 @@
 #ifndef DIRECTOR_LINGO_XLIBS_APPLECDXOBJ_H
 #define DIRECTOR_LINGO_XLIBS_APPLECDXOBJ_H

+#include "common/formats/cue.h"


I'd recommend forward declaration for Common::CueSheet instead of the whole #include it in the header.

sev-

Great! There is only a tiny nitpick in regards of code formatting

backends/audiocd/default/default-audiocd.h

backends/audiocd/default/default-audiocd.cpp

sev- · 2023-11-30T00:41:07Z

Thank you!

mistydemeo requested a review from sev- November 14, 2023 03:59

mistydemeo force-pushed the director_cue_sheets branch from 3fddb3b to 4b49942 Compare November 14, 2023 04:16

bluegr reviewed Nov 14, 2023

View reviewed changes

engines/director/lingo/xlibs/applecdxobj.cpp Show resolved Hide resolved

bluegr reviewed Nov 14, 2023

View reviewed changes

engines/director/lingo/xlibs/cdromxobj.cpp Outdated Show resolved Hide resolved

sev- requested changes Nov 18, 2023

View reviewed changes

mistydemeo added 2 commits November 22, 2023 22:07

COMMON: reorient cue around tracks

b734d0a

COMMON: avoid inserting null tracks

81785e8

mistydemeo marked this pull request as ready for review November 23, 2023 06:10

mistydemeo force-pushed the director_cue_sheets branch 3 times, most recently from 64cabae to a311459 Compare November 23, 2023 06:19

sev- reviewed Nov 25, 2023

View reviewed changes

backends/audiocd/default/default-audiocd.h Outdated Show resolved Hide resolved

backends/audiocd/default/default-audiocd.cpp Outdated Show resolved Hide resolved

backends/audiocd/default/default-audiocd.cpp Show resolved Hide resolved

mistydemeo force-pushed the director_cue_sheets branch from a311459 to a9da715 Compare November 25, 2023 01:39

mistydemeo added 2 commits November 24, 2023 23:01

BACKENDS: allow audio CDs to play absolute position

96991db

DIRECTOR: CD xlibs support absolute positioning

65d66ed

mistydemeo force-pushed the director_cue_sheets branch from a9da715 to 65d66ed Compare November 25, 2023 07:01

sev- merged commit bf233f0 into scummvm:master Nov 30, 2023
8 checks passed

mistydemeo mentioned this pull request Dec 1, 2023

DIRECTOR: more improvements to AppleCD #5486

Merged

DIRECTOR: add support for absolute timing CD audio playback via cuesheets #5428

DIRECTOR: add support for absolute timing CD audio playback via cuesheets #5428

mistydemeo commented Nov 14, 2023

bluegr commented Nov 14, 2023

sev- commented Nov 17, 2023

mistydemeo commented Nov 17, 2023

sev- left a comment

sev- Nov 18, 2023

mistydemeo Nov 23, 2023

sev- Nov 18, 2023

mistydemeo Nov 23, 2023

sev- Nov 18, 2023

mistydemeo Nov 20, 2023

sev- Nov 18, 2023

mistydemeo Nov 23, 2023

sev- left a comment

sev- commented Nov 30, 2023

DIRECTOR: add support for absolute timing CD audio playback via cuesheets #5428

DIRECTOR: add support for absolute timing CD audio playback via cuesheets #5428

Conversation

mistydemeo commented Nov 14, 2023

bluegr commented Nov 14, 2023

sev- commented Nov 17, 2023

mistydemeo commented Nov 17, 2023

sev- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sev- left a comment

Choose a reason for hiding this comment

sev- commented Nov 30, 2023