[Bug 33045] New: Bunch of Heroes freezing on start at the beginning of the intro video
http://bugs.winehq.org/show_bug.cgi?id=33045 Bug #: 33045 Summary: Bunch of Heroes freezing on start at the beginning of the intro video Product: Wine Version: 1.5.24 Platform: x86 OS/Version: Linux Status: NEW Severity: normal Priority: P2 Component: winmm&mci AssignedTo: wine-bugs(a)winehq.org ReportedBy: gyebro69(a)gmail.com CC: hoehle(a)users.sourceforge.net Classification: Unclassified Created attachment 43679 --> http://bugs.winehq.org/attachment.cgi?id=43679 terminal output + winedbg backtrace The game is locking up when trying to play the first intro video. The sound from the video can be heard for a half a second then the game hangs. Shortly after the lock-up Wine prints
err:ntdll:RtlpWaitForCriticalSection section 0x7543d3c "waveform.c: WINMM_Device.lock" wait timed out in thread 0059, blocked by 005c, retrying (60 sec)
The issue is present since d4b2d48f249341208bf3c2f48303643308d86658 is the first bad commit commit d4b2d48f249341208bf3c2f48303643308d86658 Author: Jörg Höhle <hoehle(a)users.sourceforge.net> Date: Sun Jan 27 10:43:43 2013 +0100 winmm: More compatible waveIn/Out[Un]Prepare WHDR_* flag handling. Before that commit the videos were playing without freezing, but audio was missing (so maybe it's not a real regression). Reverting the commit fixes the lock-up (no audio during the intro videos though). The audio properties of the video files: Format: PCM (Little, signed) Bitrate: 1411.2 kbps Channels: 2 Sampling rate: 44.1 kHz Bit depths: 16 bits No demo has been released, let me know if you need further logs or tests. wine-1.5.24-119-g209b58c Fedora 18 Alsa 1.0.26 Pulseaudio is not running Audio device: NVIDIA Corporation MCP61 High Definition Audio (rev a2) -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #1 from GyB <gyebro69(a)gmail.com> 2013-02-22 23:30:22 CST --- Created attachment 43680 --> http://bugs.winehq.org/attachment.cgi?id=43680 +winmm,+dsound,+alsa,+driver,+mci,+mmdevapi debug log -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #2 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-02-28 06:39:44 CST --- >Before that commit [...] audio was missing That's not a good starting point, but let's see. There's nothing obviously wrong with the patch. Things to try: - Does warn+heap make a difference? - What does Valgrind say? - Use +relay once. Does any of the <20 waveOut* calls fail? - Try out the individual hunks of the offending patch, which one makes a difference? - Add TRACE to waveOutWrite+(Un)Prepare to reveal dwBufferLength and dwFlags. Is the number of bytes not a multiple of nBlockAlign (4 from 44100x16x2)? I've never checked whether the new winmm/waveform.c code handles that well. Something already gets weird after the second waveOutWrite/WOD_PushData. No call to AC_GetBuffer, as if 0 frames (or < 4 bytes) were supplied. What did the second header contain? After the third call, we see the GetBuffer call asking for 4095 frames, and that's the last trace from thread 55. Your backtrace indicates that it may be looping forever inside WOD_PushData. - Please instrument both loops in WOD_PushData with TRACE of device->first%p, device->playing, device->last, device->ofs_bytes%u, copy_bytes, queue_bytes and queue%p (the first loop before the if, the second best before memcpy). It's also weird that the app sends out a single initial header of 4095 frames, then waits for completion callback before sending the second header. That will stutter. Indeed: 0022:trace:alsa:alsa_write_data XRun state avail 3624, recovering Yet none of that explains the change in behaviour that the patch causes. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 Jörg Höhle <hoehle(a)users.sourceforge.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |aeikum(a)codeweavers.com -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 Jörg Höhle <hoehle(a)users.sourceforge.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |source --- Comment #3 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-03-01 03:56:22 CST ---
Is the number of bytes not a multiple of nBlockAlign? That's it! Following an intuition, I added -1 to the one line in our winmm/wave tests "make sure fragment length is a multiple of block size" and got a deadlock resembling yours.
There are 2 winmm tests that have been on my TODO list for long: 1. Tests with length not a multiple of block size ;-) 2. Tests involving WHDR_BEGIN&ENDLOOP. I'm convinced that native winmm operates at the level of bytes, not frames. That's what I derive from my overflow tests (see bug #23079, comment #4), performed on w95. Not having to consider block size also makes chaining ACM codecs somewhat easier: just operate on a stream of bytes. We need a fix to the MarkDoneHeaders/PushData pair. Things to consider: 1. Perform tests with length not a multiple of block size. 2. Investigate what native does with trailing bytes in case of underrun. - Throw away? - Do they prevent buffer notification, awaiting more bytes from the next header to complete a frame? That is a hairy situation! - Do they count in GetPosition? Alas, this mostly requires audible tests, not testbot. 3. Make MarkDoneHeaders optional, such that waveOutWrite -> BeginPlaying -> PushData(nomark) DevicesThreadProc -> PushData(mark¬ify) IOW, fix bug #TODO. Ensure that PushData does not depend on state variables changes performed in the mark phase. I've had this change in mind for some time, but it's precisely the complicated logic in those two functions that prevented it. I ended up never reviewing this part of winmm. 4. Document the invariants. When is device->loop_start invalid? When is device->last invalid? 5. Safe guard against rogue IAC_GetPosition in MarkDone. Currently it seems that a header can be put into the notification list, yet still be referenced from device->playing. 6. WHDR_ENDLOOP is queried in 3 places. Have the code of all those similar, such that it becomes obvious that they all implement the same behaviour. - What about WHDR_ENDLOOP without BEGINLOOP? - What about consecutive WHDR_BEGINLOOP? - What about BEGINLOOP|ENDLOOP in one header? - What about BEGIN ... BEGIN|END ... END? - Handle BEGIN ... END ... BEGIN ... END well. Be careful about loops. One of my notes is that I suspect MarkDoneHeaders incorrect with waveOutBreakLoop & several loops, e.g. one already played and one playing. There are two traversals of the header list. One based on GetPosition (clock_frames), the other based on Get/ReleaseBuffer (queue_frames). Clearly, both cannot share the same device->loop_counter. I like Andrew's idea of a MarkDoneHeaders distinct from PushData. The code just needs to step the "done" and "playing" pointers into the header list more carefully. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #4 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-03-01 04:05:13 CST ---
IOW, fix bug #TODO. That's bug #28464, comment #6. That would also prevent a possible dead-lock in win16 apps, because waveOut16 is not expected to use ReleaseThunkLock, unlike e.g. waveOutReset16 and all functions that can provoke a callback. http://source.winehq.org/source//dlls/mmsystem.dll16/mmsystem.c
-- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #5 from Andrew Eikum <aeikum(a)codeweavers.com> 2013-03-05 07:56:05 CST --- (In reply to comment #3)
Is the number of bytes not a multiple of nBlockAlign? That's it! Following an intuition, I added -1 to the one line in our winmm/wave tests "make sure fragment length is a multiple of block size" and got a deadlock resembling yours.
Yep, that's exactly it. Bunch of Heroes uses headers with 0x7FFF byte buffers, which isn't a multiple of 4.
2. Investigate what native does with trailing bytes in case of underrun. - Throw away? - Do they prevent buffer notification, awaiting more bytes from the next header to complete a frame? That is a hairy situation! - Do they count in GetPosition? Alas, this mostly requires audible tests, not testbot.
Looks to me like they get thrown away. I wrote a test with two headers, one header is one byte short and the other is one byte long. After the first header plays, waveOutGetPosition(TIME_SAMPLES) reports one sample short. After the second header, it still reports exactly one sample short. Using TIME_BYTES similarly reports (dwBufferSize - dwBufferSize % nBlockAlign). -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #6 from Andrew Eikum <aeikum(a)codeweavers.com> 2013-03-05 08:00:44 CST --- (In reply to comment #5)
(In reply to comment #3)
2. Investigate what native does with trailing bytes in case of underrun.
Looks to me like they get thrown away. I wrote a test with two headers, one header is one byte short and the other is one byte long. After the first header plays, waveOutGetPosition(TIME_SAMPLES) reports one sample short. After the second header, it still reports exactly one sample short. Using TIME_BYTES similarly reports (dwBufferSize - dwBufferSize % nBlockAlign).
I should clarify, this is true even without the underrun. Queuing the two buffers immediately still results in a TIME_SAMPLES that's one sample short (and similarly for TIME_BYTES). -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #7 from Andrew Eikum <aeikum(a)codeweavers.com> 2013-03-05 09:51:58 CST --- Created attachment 43819 --> http://bugs.winehq.org/attachment.cgi?id=43819 Truncate patch and tests I'm attaching a patchset here. The first patch truncates non-block-aligned headers. The second patch is a test which demonstrates that that's not completely correct. Swapping the header order seems to matter. The below is with no underruns. Header 1: 1/4s - 1 byte Header 2: 1/4s + 1 byte The callback for Header 2 is never called, position is (1/2s - 1 byte), and waveOutClose() gives STILLPLAYING. But if you switch them around: Header 1: 1/4s + 1 byte Header 2: 1/4s - 1 byte Both headers get callbacks, position is 1/2s, and waveOutClose() works normally. If you write either header alone, the callback occurs and waveOutClose() operates normally. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #8 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-03-05 10:09:19 CST --- Please investigate possible difference in behaviour between the last buffer in a queue and all other buffers before. E.g. 3 buffers -1/+1/0 and +1/-1/0. Why? Because the occasional GetPosition failures in our wave tests appear to solely occur when playing several headers and shortly before the buffer reaches the end (e.g. within the last 10ms). That lead me to believe that native does something special in that case. For instance, I've been wondering whether native may send out the last header's callback notification slightly ahead of time, to help some apps produce continuous sound.
But if you switch them around The results are ugly, as if native inconsistently applied rounding. It would be good if you had some of these tests on testbot, I could take it to w95 this week-end.
-- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #9 from Andrew Eikum <aeikum(a)codeweavers.com> 2013-03-05 12:29:08 CST --- (In reply to comment #8)
Please investigate possible difference in behaviour between the last buffer in a queue and all other buffers before. E.g. 3 buffers -1/+1/0 and +1/-1/0. Why? Because the occasional GetPosition failures in our wave tests appear to solely occur when playing several headers and shortly before the buffer reaches the end (e.g. within the last 10ms). That lead me to believe that native does something special in that case. For instance, I've been wondering whether native may send out the last header's callback notification slightly ahead of time, to help some apps produce continuous sound.
In the (-1,+1,0) case, it always seems to stall on the last callback. (+1,-1,0) performs all three callbacks.
But if you switch them around The results are ugly, as if native inconsistently applied rounding. It would be good if you had some of these tests on testbot, I could take it to w95 this week-end.
Sure thing: https://testbot.winehq.org/JobDetails.pl?Key=24619 This runs both (+1,-1) and (-1,+1). View the full logs to see the trace() statements. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #10 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-03-06 02:50:58 CST --- Is the data sufficient to conclude that: 1. We know nothing yet about w9x (more this week-end). 2. NT-XP machines round down. I can confirm testbot xp results on a real XP machine (though I'd like to see GetPos in bytes, given bug #23079). 3. mmdevapi-era machines (Vista+) handle that situation inconsistently. We lack an explanation for the different results of +1/-1 vs. -1/+1. (w8 results from newtestbot might be interesting too). I've expressed my opinion often enough. Wine ought to implement sensible behaviour of the APIs at the time they were mostly used. IOW w9x-wxp for winmm. Also, we shouldn't implement code that may hang (by not sending events). There are enough bug reports of apps that hang, and I find those to be among the most difficult to solve, simply because nobody knows what an app is waiting for. Given the XP results, I wouldn't try to complete frames with bytes from a previous buffer. MSDN insists on aligned buffers for waveInAddBuffer, so it's doubtful that one should try to complete a buffer-1 with one byte from the next buffer, causing misalignment of all consecutive data. Well, that's a point where audible tests would make a difference (I'd be pleased to throw an audible test at a w95 machine). So far we only handled PCM data. We don't know how the ACM behaves. Perhaps +1/-1 should work at the ACM level, but rounding down is a must after that has been converted to PCM? (Possibly nonsense, as I'm not sure the ACM codecs would ever return data that doesn't fill a full frame). -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #11 from Andrew Eikum <aeikum(a)codeweavers.com> 2013-03-06 08:54:23 CST --- I agree with all of that. I don't think ACM would have any kind of special handling. I'll wait for your 9x test results, but otherwise I think I can clean up the tests a bit and send off the truncate patch from attachment 43819. Sound good? -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #12 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-03-07 14:06:46 CST --- Unlike XP, w95 reports no GetPosition test failure -- if I move the mouse wildly! Otherwise position is slightly too low: wave.c:1604: Test failed: after position: 22034 or 22042 So truly, the event is received shortly before finishing playing. Given that w95 counts in bytes, we should produce a test with MMTIME_BYTES. Your patch is partly wrong. You cannot mix frame rounding ops from the ACM target (PCM) domain with byte counts from the ACM source domain, they don't have the same "dimension". That's a type error. Regarding the use of the ACM, interesting tests involve IMAADPCM (blocksize 256 or such) and the defunct winemp3 (blocksize 1). I believe we should let the ACM count src bytes as the ACM likes to do and solely round frames on the PCM target side. Perhaps we should then verify that the ACM gives us a multiple of frames. Which makes me think that the wave test may not be significant on w95, because WAVE_MAPPER was used, which may do its own things. Oops, looking at WID_PullACMData, I noticed that waveIN notification when the ACM is in use is still bogus. I think I mentioned that once but it got off my mind :-( My September 2011 commit 3ba00cf538cc07d8e36ae82d43a29247cb37a299 only handled PushData and PullData. Checking how the ACM handles buffers not matching source block alignment merits own tests. It too can get hairy. E.g. if cbSrcLengthUsed < dwBufferLength, what will GetPosition look like, when is WOM_DONE issued? -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #13 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-03-08 07:09:19 CST --- Another possibility for handling bytes not matching ACM or PCM block alignment is: after the header is consumed (ignoring remainder bytes), bump GetPosition to follow, i.e. pretend all bytes were played. IOW, GetPosition matches the sum of dwBufferLength. This is to work-around the weakness of the WINMM API that one receives no feedback about how many bytes were relevant in waveOutWrite. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #14 from Andrew Eikum <aeikum(a)codeweavers.com> 2013-03-08 11:13:29 CST --- Created attachment 43852 --> http://bugs.winehq.org/attachment.cgi?id=43852 Updated: winmm: Truncate buffers to align to frame size (In reply to comment #12)
Unlike XP, w95 reports no GetPosition test failure
So we have inconsistent behavior across Windows versions. I'm leaning towards the truncate patch, since it exists and is less invasive than tracking bytes.
Your patch is partly wrong. You cannot mix frame rounding ops from the ACM target (PCM) domain with byte counts from the ACM source domain, they don't have the same "dimension". That's a type error.
Yes, good catch.
Regarding the use of the ACM, interesting tests involve IMAADPCM (blocksize 256 or such) and the defunct winemp3 (blocksize 1).
In fact, winecfg's Test Sound button uses IMAADPCM with block size 2048.
I believe we should let the ACM count src bytes as the ACM likes to do and solely round frames on the PCM target side. Perhaps we should then verify that the ACM gives us a multiple of frames.
This is getting pretty far into theoretical territory. But it's not a bad idea, since non-block-aligned buffers make us deadlock. I've added that to the patch. (In reply to comment #13)
Another possibility for handling bytes not matching ACM or PCM block alignment is: after the header is consumed (ignoring remainder bytes), bump GetPosition to follow, i.e. pretend all bytes were played. IOW, GetPosition matches the sum of dwBufferLength. This is to work-around the weakness of the WINMM API that one receives no feedback about how many bytes were relevant in waveOutWrite.
I'm not opposed to this, but I think it's too invasive to justify, unless we have an application that needs it. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #15 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-03-08 12:04:46 CST ---
Yes, good catch. Perhaps you misunderstood me, because your second patch still contains several occurrences of said type error: ash->cbSrcLength = WINMM_FixedBufferLen(header->dwBufferLength, device); Unless your second patch was written prior to my comment #12.
I'm leaning towards the truncate patch Me too, for different reasons: that appears to be XP behaviour according to your testbot job, and XP has a good reputation of compatibility and stability. w95 doesn't. Sadly, I have no w98SE machine to test on, as that would be my preferred w9x test OS.
IMAADPCM with block size 2048 Hmm...What if an app wants to record that but supplies 1024 bytes buffers? I guess we should recheck those bug reports about sndrec32.
-- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 Andrew Eikum <aeikum(a)codeweavers.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #43852|0 |1 is obsolete| | --- Comment #16 from Andrew Eikum <aeikum(a)codeweavers.com> 2013-03-08 12:57:58 CST --- Created attachment 43853 --> http://bugs.winehq.org/attachment.cgi?id=43853 winmm: Truncate buffers to align to frame size (In reply to comment #15)
Yes, good catch. Perhaps you misunderstood me, because your second patch still contains several occurrences of said type error: ash->cbSrcLength = WINMM_FixedBufferLen(header->dwBufferLength, device); Unless your second patch was written prior to my comment #12.
Um, shoot, you're right. I must have fallen asleep when I was fixing it. Here's another version. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #17 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-03-08 15:14:55 CST --- Here are results from WAVE_FORMAT_IMA_ADPCM on a real w2k machine. The ADPCM parameters are as known from winmm/tests/wave.c. wave.c:1628: testing 4095 bytes then 4095 bytes wave.c:1667: Test failed: after position: 15150 samples wave.c:1673: Test failed: after position: 7679 bytes wave.c:1628: testing 4096 bytes then 4095 bytes (or 4095 then 4096) wave.c:1667: Test failed: after position: 15655 samples wave.c:1673: Test failed: after position: 7935 bytes wave.c:1628: testing 4096 bytes then 4096 bytes wave.c:1667: Test failed: after position: 16160 samples wave.c:1673: Test failed: after position: 8191 bytes The order of headers does not matter. Clipping to 256 bytes blocksize is obvious. I've not checked the exact values, but it looks like ACM's cbSrcLengthUsed is used from each header to limit GetPosition, dwHeaderLength being irrelevant. The test may not be trustworthy should ADPCM output length depend on the data, because I fed it null bytes, no valid IMA_ADPCM data blocks. I haven't checked whether one can conclude from such data whether w2k's winmm internally counts in bytes or in samples (cf. bug #23079, comment #4). The data is consistent with bytes = MulDiv(samples, nAvgBytesPerSec, nSamplesPerSec); For sure it makes WINMM_FramesToMMTime smell wrong. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 --- Comment #18 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-03-09 04:31:10 CST --- Created attachment 43862 --> http://bugs.winehq.org/attachment.cgi?id=43862 patch: Fix notification when recording using MSACM. The data is equally consistent with samples = bytes * nSamplesPerSec / nAvgBytesPerSec; // not MulDiv Your patch looks ok now, yet I still have a suggestion. In WOD_PushData, it's important that both the while and for loop compute the same number of frames. To ease the job of your poor reviewer, it would be nice to have both use the same function, e.g. WINMM_HeaderLenBytes (BTW, I don't like having three tiny functions do almost the same thing). Here's a patch to fix notification in WID_PullACMData atop your patch. I plan to rebase it against 1.5.25 and submit it on Monday, so that it does not come in the middle of your patch series. What still needs fixing? - GetPosition returns bogus numbers with ACM codecs, e.g. wave.c:1626: Test failed: after position: 15655 samples wave.c:1632: Test failed: after position: 4007680 bytes - not * 256 - Think about WHDR_ENDLOOP without preceding BEGIN. Wine will hang or crash. - MarkDoneHeaders is still bogus w.r.t. LOOP, see comment #3 - WID_PullACMData looks incomplete. It starts with checking device->acm_hdr as if it could persist from a prior invocation, however it frees it when returning normally. Thus it should remain local to the function, with device->acm_offs. In error situations, the current code may break, because it may see device->acm_hdr.cbDstLength != 0 on entry from a prior acmStreamConvert MMSYSERR_* return. Hmm, now that my patch eliminated the tail call, it could as well fix that. - WID_PullACMData needs better error handling. If acmStreamConvert returns an error, the IACaptureClient buffer remains locked. - In WID_PullACMData, the queue->lpNext condition looks bogus in the light of IMA_ADPCM's 256 bytes block size. More generally, PullACMData needs a redesign. What to do when srcLengthUsed < packet_frames ?!? I.e. when the mmdevapi packet length does not match the codec's blocksize, e.g. using 10ms packets while IMA_ADPCM likely needs multiples of 256 bytes. - We should not throw away recorded bytes given enough buffers. - mmdevapi does not accept retrieving less than one packet... Ouch, that's enough material for a separate bug report. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 GyB <gyebro69(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |f74128f132df677999c26bbb409 | |bc16bbb1e5e98 Status|NEW |RESOLVED Resolution| |FIXED --- Comment #19 from GyB <gyebro69(a)gmail.com> 2013-03-11 22:30:13 CDT --- Andrew, Jörg, thanks for both of your hard work on this bug. Fixed by http://source.winehq.org/git/wine.git/commitdiff/f74128f132df677999c26bbb409... -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #20 from Alexandre Julliard <julliard(a)winehq.org> 2013-03-15 14:47:19 CDT --- Closing bugs fixed in 1.5.26. -- Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email Do not reply to this email, post in Bugzilla using the above URL to reply. ------- You are receiving this mail because: ------- You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=33045
--- Comment #21 from Jörg Höhle <hoehle(a)users.sourceforge.net> 2013-03-15 19:50:22 CDT ---
>- GetPosition returns bogus numbers with ACM codecs
That one is fixed in wine-1.5.26 by commit
2722f2cbfcd5f45a3f53e5b818439fba1de2c14f
The app may work, however there are even more issues involving the ACM. Add to:
>What still needs fixing?
- When using the ACM resampler (dlls/msacm32/pcmconverter),
rounding in dlls/winmm/waveform.c:WOD_PushData:
if(device->orig_fmt->nSamplesPerSec != device->samples_per_sec)
device->played_frames += MulDiv(avail_frames,
device->orig_fmt->nSamplesPerSec, device->samples_per_sec);
causes GetPosition to report byte numbers larger than the sum of
WHDR->dwBufferLength!
I have no idea how many apps are prepared to handle that :-(
Rounding down may not be a solution, as that may cause the opposite problem:
unexpectedly never reach the sum.
I discovered this bug using Wine's PCM resampler. I expect this issue to not
only hit the resampler, but all codecs with variable bit rate, likely MP3.
The problem is that instead of
sum += acm.cbSrcBytesUsed
there's some chunking coming from not being able or wanting to write a full
block worth of samples to mmdevapi, so partial sums come into play. Add to that
the different frame rates and you've got a rounding issue, much like what
happens when adding floating point numbers. Here it would be interesting to
find out whether native counts in bytes, target frames or source frames
internally, so as to be compatible.
>wave.c:1628: testing 4096 bytes then 4096 bytes
>wave.c:1673: Test failed: after position: 8191 bytes
Apparently w2k does not count in bytes, as there's no explanation for 8191 from
4096+4096 ... except:
>The test may not be trustworthy should ADPCM output length depend on the data,
>because I fed it null bytes, no valid IMA_ADPCM data blocks.
>- WID_PullACMData
> Ouch, that's enough material for a separate bug report.
Created it. WID_PullACMData or broken ACM recording is bug #33155
--
Configure bugmail: http://bugs.winehq.org/userprefs.cgi?tab=email
Do not reply to this email, post in Bugzilla using the
above URL to reply.
------- You are receiving this mail because: -------
You are watching all bug changes.
participants (1)
-
wine-bugs＠winehq.org