[PATCH v2 0/2] MR6696: mciseq: Don't seek to the end of the root chunk in RMID files.
Commit 9e1008f13735 added mmioSeek call that skips the whole data portion of the RMID chunk, breaking the handling of RMID (RIFF-based MIDI) files as it effectively skips the whole content of the RIFF file. Since mmioDescend expects the file position to be somewhere within the specified parent chunk, it's guaranteed to fail. $ WINEDEBUG=+mcimidi wine wintest.exe mcishell mci.c:178: Type your commands to the MCI, end with Ctrl-Z/^D open BOSSNOVA.RMI mci.c:188: command: open BOSSNOVA.RMI 0764:trace:mcimidi:MIDI_mciOpen (1, 00000200, 0051FB98) 0764:trace:mcimidi:MIDI_mciOpen wDevID=1 (lpParams->wDeviceID=1) 0764:trace:mcimidi:MIDI_mciOpen MCI_OPEN_ELEMENT L"bossnova.rmi"! 0764:trace:mcimidi:MIDI_mciOpen hFile=00000001 0764:trace:mcimidi:MIDI_mciOpen ParentChunk ckid=RIFF fccType=RMID cksize=000052DA mci.c:191: Test failed: mci open BOSSNOVA.RMI error: 296(40 MCIERR_INVALID_FILE) Its introduction seems unrelated to the rest of that commit and there's no reason given for placing it there, so let's remove the mmioSeek call to fix RMID file handling in MCI. Previous mmioDescend call will set the position to just after the chunk's form type, which is exactly where it should be when calling mmioDescend the second time to find the data chunk. Other parts of Wine code that deal with RIFF files don't seek in analogous situations either. Fixes: 9e1008f13735 ("Removed fixed size array to store specific data") -- v2: mciseq: Don't seek to the end of the root chunk in RMID files. midiseq: Reduce race window when closing sequencer. https://gitlab.winehq.org/wine/wine/-/merge_requests/6696
From: Eric Pouech <epouech(a)codeweavers.com> Signed-off-by: Eric Pouech <epouech(a)codeweavers.com> --- dlls/mciseq/mcimidi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dlls/mciseq/mcimidi.c b/dlls/mciseq/mcimidi.c index 4211ed5ca4a..02c212b8c70 100644 --- a/dlls/mciseq/mcimidi.c +++ b/dlls/mciseq/mcimidi.c @@ -133,8 +133,8 @@ static DWORD MIDI_drvClose(DWORD dwDevID) WINE_MCIMIDI* wmm = (WINE_MCIMIDI*)mciGetDriverData(dwDevID); if (wmm) { - HeapFree(GetProcessHeap(), 0, wmm); mciSetDriverData(dwDevID, 0); + HeapFree(GetProcessHeap(), 0, wmm); return 1; } return (dwDevID == 0xFFFFFFFF) ? 1 : 0; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6696
From: Sebastian Krzyszkowiak <dos(a)dosowisko.net> Commit 9e1008f13735 added mmioSeek call that skips the whole data portion of the RMID chunk, breaking the handling of RMID (RIFF-based MIDI) files as it effectively skips the whole content of the RIFF file. Since mmioDescend expects the file position to be somewhere within the specified parent chunk, it's guaranteed to fail. $ WINEDEBUG=+mcimidi wine wintest.exe mcishell mci.c:178: Type your commands to the MCI, end with Ctrl-Z/^D open BOSSNOVA.RMI mci.c:188: command: open BOSSNOVA.RMI 0764:trace:mcimidi:MIDI_mciOpen (1, 00000200, 0051FB98) 0764:trace:mcimidi:MIDI_mciOpen wDevID=1 (lpParams->wDeviceID=1) 0764:trace:mcimidi:MIDI_mciOpen MCI_OPEN_ELEMENT L"bossnova.rmi"! 0764:trace:mcimidi:MIDI_mciOpen hFile=00000001 0764:trace:mcimidi:MIDI_mciOpen ParentChunk ckid=RIFF fccType=RMID cksize=000052DA mci.c:191: Test failed: mci open BOSSNOVA.RMI error: 296(40 MCIERR_INVALID_FILE) Its introduction seems unrelated to the rest of that commit and there's no reason given for placing it there, so let's remove the mmioSeek call to fix RMID file handling in MCI. Previous mmioDescend call will set the position to just after the chunk's form type, which is exactly where it should be when calling mmioDescend the second time to find the data chunk. Other parts of Wine code that deal with RIFF files don't seek in analogous situations either. Fixes: 9e1008f13735 ("Removed fixed size array to store specific data") Signed-off-by: Sebastian Krzyszkowiak <dos(a)dosowisko.net> --- dlls/mciseq/mcimidi.c | 1 - 1 file changed, 1 deletion(-) diff --git a/dlls/mciseq/mcimidi.c b/dlls/mciseq/mcimidi.c index 02c212b8c70..f4c162ae7a5 100644 --- a/dlls/mciseq/mcimidi.c +++ b/dlls/mciseq/mcimidi.c @@ -717,7 +717,6 @@ static DWORD MIDI_mciOpen(WINE_MCIMIDI* wmm, DWORD dwFlags, LPMCI_OPEN_PARMSW lp if (ckMainRIFF.ckid == FOURCC_RIFF && ckMainRIFF.fccType == mmioFOURCC('R', 'M', 'I', 'D')) { mmckInfo.ckid = mmioFOURCC('d', 'a', 't', 'a'); - mmioSeek(wmm->hFile, ckMainRIFF.dwDataOffset + ((ckMainRIFF.cksize + 1) & ~1), SEEK_SET); if (mmioDescend(wmm->hFile, &mmckInfo, &ckMainRIFF, MMIO_FINDCHUNK) == 0) { TRACE("... is a 'RMID' file\n"); dwOffset = mmckInfo.dwDataOffset; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/6696
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=149127 Your paranoid android. === debian11b (64 bit WoW report) === user32: input.c:4305: Test succeeded inside todo block: button_down_hwnd_todo 1: got MSG_TEST_WIN hwnd 0000000001FD00FC, msg WM_LBUTTONDOWN, wparam 0x1, lparam 0x320032
Done. I can't tell what the test failures are about, test-linux-32 job seems flaky on pretty much all MRs around. Note that this patch only affects RMID files, which are probably the minority of MIDI files, and any exceptions/crashes/races/test failures would be already triggerable with equivalent SMF files too. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6696#note_85524
On Mon Oct 21 06:43:39 2024 +0000, Sebastian Krzyszkowiak wrote:
Done. I can't tell what the test failures are about, test-linux-32 job seems flaky on pretty much all MRs around. Note that this patch only affects RMID files, which are probably the minority of MIDI files, and any exceptions/crashes/races/test failures would be already triggerable with equivalent SMF files too. thanks at least the window related failures (eg user32) no longer show up in the test pipeline the remaining ones are likely not related to this MR
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6696#note_85552
This merge request was approved by eric pouech. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6696
participants (5)
-
Eric Pouech -
eric pouech (＠epo)
-
Marvin -
Sebastian Krzyszkowiak
-
Sebastian Krzyszkowiak (＠dos1)