[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: [e-mail]
        ReportedBy: [e-mail]
                CC: [e-mail]
    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 
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)

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #1 from GyB  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

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #2 from Jörg Höhle  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

Jörg Höhle  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

Jörg Höhle  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |source

--- Comment #3 from Jörg Höhle  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&notify)
  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #4 from Jörg Höhle  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

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #5 from Andrew Eikum  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.
> - 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).

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #6 from Andrew Eikum  2013-03-05 08:00:44 CST ---
(In reply to comment #5)
> (In reply to comment #3)
> > - 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).

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #7 from Andrew Eikum  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #8 from Jörg Höhle  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #9 from Andrew Eikum  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #10 from Jörg Höhle  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).

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #11 from Andrew Eikum  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?

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #12 from Jörg Höhle  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?

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #13 from Jörg Höhle  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #14 from Andrew Eikum  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #15 from Jörg Höhle  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

Andrew Eikum  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #43852|0                           |1
        is obsolete|                            |

--- Comment #16 from Andrew Eikum  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #17 from Jörg Höhle  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #18 from Jörg Höhle  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.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

GyB  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |f74128f132df677999c26bbb409
                   |                            |bc16bbb1e5e98
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED

--- Comment #19 from GyB  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...

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #20 from Alexandre Julliard  2013-03-15 14:47:19 CDT ---
Closing bugs fixed in 1.5.26.

[Bug 33045] Bunch of Heroes freezing on start at the beginning of the intro video

http://bugs.winehq.org/show_bug.cgi?id=33045

--- Comment #21 from Jörg Höhle  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