Detect when OPL4 playback needs YRW801 ROM - #152
Conversation
| { | ||
| case 0x66: // end of command data | ||
| _opl4YRW801Req = yrwUse & ~fileRom; | ||
| return; |
There was a problem hiding this comment.
I'd prefer if you just exit the while loop here instead of copying the "set variable + return" code.
| if (_CMD_INFO[curCmd].cmdLen == 0) | ||
| return; | ||
| if (filePos + _CMD_INFO[curCmd].cmdLen > _fileHdr.dataEnd) | ||
| return; |
There was a problem hiding this comment.
maybe just quit the while-loop here as well instead of doing a hard return
|
|
||
| if (chipDev->romWrite == NULL) | ||
| return; | ||
| emu_logf(&_logger, PLRLOG_INFO, "OPL4 requires external sample ROM %s.\n", romFile); |
There was a problem hiding this comment.
either remove or set to PLRLOG_DEBUG
I don't want this to spam the VGMPlay terminal window by default.
| { | ||
| if (_fileReqCbFunc == NULL) | ||
| { | ||
| emu_logf(&_logger, PLRLOG_WARN, "No file request callback available for %s.\n", romFile); |
| DATA_LOADER* romDLoad = _fileReqCbFunc(_fileReqCbParam, this, romFile); | ||
| if (romDLoad == NULL) | ||
| { | ||
| emu_logf(&_logger, PLRLOG_WARN, "Couldn't load %s.\n", romFile); |
There was a problem hiding this comment.
I'm not sure if this warning makes sense, because it would effectively duplicate the warning output that the callback itself is supposed to do.
(Right now I am unsure if the callback itself should issue the warning/error or the OPL4 ROM loader function here. I guess keeping the warning here makes it easier for the logging system to keep track of it.)
| fflush(stdout); | ||
|
|
||
| char filePath[0x400]; | ||
| if (fgets(filePath, sizeof(filePath), stdin) == NULL) |
There was a problem hiding this comment.
Sorry, but I am against user input here.
You are not supposed to block the audio render thread to wait for the user to input things. It should just fail.
Please remove this new RequestFileCallback function and just rename TryLoadExternalFile back to RequestFileCallback.
| fprintf(stderr, "Enter path to \"%s\" (empty to skip): ", fileName); | ||
| fflush(stderr); | ||
|
|
||
| if (fgets(filePath, sizeof(filePath), stdin) == NULL) { |
There was a problem hiding this comment.
same as in player.cpp - please don't wait for user input inside callbacks
Avoid requesting yrw801.rom for YMF278B/OPL4 VGMs that only use the FM part or rely solely on embedded custom wave data. Add a pre-scan of OPL4 commands that tracks wave-table usage, distinguishing custom sample tables from YRW/GM wave entries. Also make the sample tools report and request missing external ROM files more clearly.
8fbf77f to
867223e
Compare
|
Ok, a bunch of changes based on your reviewpoints, thanks for that btw! |
2 participants
Avoid requesting yrw801.rom for YMF278B/OPL4 VGMs that only use the FM part or rely solely on embedded custom wave data. Add a pre-scan of OPL4 commands that tracks wave-table usage, distinguishing custom sample tables from YRW/GM wave entries.
Also make the sample tools report and request missing external ROM files more clearly.