Track and restore the last audio position #387
Conversation
|
Seems OK, I'll do some tests. PR description is a very good explanation of reasons behind this change - add it to the commit message, please. |
| @@ -143,6 +143,7 @@ class CDROM_Interface_Image : public CDROM_Interface | |||
| uint32_t adjustOverRead(const uint32_t offset, | |||
| const uint32_t requested_bytes); | |||
| int length_redbook_bytes = -1; | |||
| uint32_t audio_pos = (std::numeric_limits<uint32_t>::max)(); // last position when playing audio | |||
dreamer
May 22, 2020
Member
There's no need for parenthesis around ::max any more :)
There's no need for parenthesis around ::max any more :)
kcgen
May 22, 2020
Author
Member
Excellent! I thought it was just the simpler min(a, b), max(a, b) functions; great to see us not pinned down by MSVC-custom stuff anymore.
Excellent! I thought it was just the simpler min(a, b), max(a, b) functions; great to see us not pinned down by MSVC-custom stuff anymore.
| @@ -176,6 +178,8 @@ class CDROM_Interface_Image : public CDROM_Interface | |||
| Bit32u getRate() { return 44100; } | |||
| Bit8u getChannels() { return 2; } | |||
| int getLength(); | |||
| void setAudioPosition(uint32_t pos) { audio_pos = pos; }; | |||
dreamer
May 22, 2020
Member
Remove unnecessary semicolon.
Remove unnecessary semicolon.
| void setAudioPosition(uint32_t pos) { (void)pos; }; | ||
| // This is a no-op because we track the audio position in all | ||
| // areas of this class. |
dreamer
May 22, 2020
Member
Suggested change
void setAudioPosition(uint32_t pos) { (void)pos; };
// This is a no-op because we track the audio position in all
// areas of this class.
// This is a no-op because we track the audio position in all
// areas of this class.
void setAudioPosition(uint32_t) {}
| void setAudioPosition(uint32_t pos) { (void)pos; }; | |
| // This is a no-op because we track the audio position in all | |
| // areas of this class. | |
| // This is a no-op because we track the audio position in all | |
| // areas of this class. | |
| void setAudioPosition(uint32_t) {} |
kcgen
May 22, 2020
Author
Member
Folded into the main commit.
Folded into the main commit.
| track_pos = requested_pos; | ||
| audio_pos = requested_pos; | ||
| else | ||
| audio_pos = {}; |
dreamer
May 22, 2020
Member
Why not audio_pos = 0;?
Why not audio_pos = 0;?
kcgen
May 22, 2020
•
Author
Member
I had read somewhere that {} resets class/struct members back to their original initialized values, so I had wanted this to be std::numeric_limits<uint32_t>::max().
But alas.. that's a myth and it simply sets it to zero, which makes your comment perfectly valid! (to answer why we don't want to save 0 in the failed-seek case: if this function'srequested_pos argument was also 0 but we failed to seek to 0, then saving 0 would cause subsequent streaming-decodes to assume the track is correctly positioned when it's actually not; and is probably in a broken state).
I've added this correction back into the main commit.
I had read somewhere that {} resets class/struct members back to their original initialized values, so I had wanted this to be std::numeric_limits<uint32_t>::max().
But alas.. that's a myth and it simply sets it to zero, which makes your comment perfectly valid! (to answer why we don't want to save 0 in the failed-seek case: if this function'srequested_pos argument was also 0 but we failed to seek to 0, then saving 0 would cause subsequent streaming-decodes to assume the track is correctly positioned when it's actually not; and is probably in a broken state).
I've added this correction back into the main commit.
|
I did some testing with games mounting via cue/ogg and iso and in all cases CD audio playback worked fine, so there are no obvious regressions. |
|
Thanks @dreamer for the extra testing; I can also confirm tests are passing on my side too. |
In the CDROM image code, we previously kept track of the 'read head' to avoid unnecessary seeking and allow sequential decode. This technique breaks down if the program mixes data reads with audio playback in the same track, such as in Secret of Monkey Island when using a single the BIN/CUE pair of its CDROM (ISO+tracks, or seprated BINs +CUE are not affected). This commit now keeps track of the last audio position in all audio events that move the file pointer. It also now confirms and re-positions the track on subsequent audio events, if needed.
One of the enhancements in the CDDA code is tracking the 'read head' position to avoid unnecessary seeking during sequential decodes. This technique breaks down if the program mixes data reads with audio playback in the same track, such as in Secret of Monkey Island when using a single the BIN/CUE pair of its CDROM (ISO+tracks, or seprated BINs +CUE are not affected).
This PR now keeps track of the last audio position in all audio events that move the file pointer. It also now confirms and re-positions the track on subsequent audio events, if needed.
Thank you to @flashmasta for catching this and writing up a detailed issue!
Fixes #385 (the crux of the issue anyway).