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

DIRECTOR: handle macbinary audio streams #3497

Merged
merged 1 commit into from Nov 7, 2021

Conversation

@mistydemeo
Copy link
Contributor

@mistydemeo mistydemeo commented Nov 7, 2021

Certain audio streams can (and often do) contain resource forks, so the dumper companion will write them as macbinary. Right now, the audio code will choke on them since it tries to read from the macbinary header assuming it's audio. This adds a check for whether the file is macbinary and handles it appropriately.

In the future, I might run a pass over the Director code to support native Mac files on HFS+/APFS macOS, but there's a lot I'd need to update for that.

To test this, you can try the Pippin/Mac version of Jungle Park run through the dumper companion. Without this patch, it'll fail to recognize JUNGLE/JINGLE.AIFF; it won't play the intro audio and will spam the console with errors about it. With this patch. it works fine.

@sev-
Copy link
Member

@sev- sev- commented Nov 7, 2021

In fact, MacResMan is handling all the flavours of Mac resource forks, including the native. See MacResManager::open(). So, even this code could be simplified. I would write it something like this:

SeekableReadStream *stream;

if (_vm->getPlatform() != Common::kPlatformWindows) {
  _macresman = new Common::MacResManager(); // put the variable into class
  _macresman->open(Common::Path(pathMakeRelative(_path), g_director->_dirSeparator));
  stream = _macresman->getDataFork();
} else {
  stream = new File();
  stream->open(....
}

This is a logically clean solution. However, less logical but perfectly working would be to just use MacResManager::open(). It has a fallback mechanism that if the file is not binary, it is treating is as a plain data fork. And Data Fork is what you need.

Thus, in this particular case you could write just the following, and if you would need a resource fork, then the more complicated code from above:

 _macresman = new Common::MacResManager(); // put the variable into class
 _macresman->open(Common::Path(pathMakeRelative(_path), g_director->_dirSeparator));
 file = _macresman->getDataFork();

Loading

@mistydemeo mistydemeo force-pushed the director_handle_macbinary_audio branch from 26ea900 to b651646 Nov 7, 2021
@mistydemeo
Copy link
Contributor Author

@mistydemeo mistydemeo commented Nov 7, 2021

Went for the second solution. No idea what I did wrong last time, but when I tried it before it seemed as though it didn't work - but it seems just fine now!

Loading

@mistydemeo mistydemeo force-pushed the director_handle_macbinary_audio branch from b651646 to 9a53943 Nov 7, 2021
@mistydemeo mistydemeo force-pushed the director_handle_macbinary_audio branch from 9a53943 to 803ae19 Nov 7, 2021
@sev-
Copy link
Member

@sev- sev- commented Nov 7, 2021

Thank you!

Loading

@sev- sev- merged commit 753c1c2 into scummvm:master Nov 7, 2021
5 of 8 checks passed
Loading
@mistydemeo mistydemeo deleted the director_handle_macbinary_audio branch Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants