http://bugs.winehq.org/show_bug.cgi?id=33045
Jörg Höhle hoehle@users.sourceforge.net changed:
What |Removed |Added ---------------------------------------------------------------------------- Keywords| |source
--- Comment #3 from Jörg Höhle hoehle@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.