Andrew Eikum wrote:
This [lock-less] seems to be the main goal of the patch.
It'll not be bad to fix the capture bug independently.
and maybe we can get that into Wine after the next release?
That requires agreement on the lock-less protocol.
Basis: atomic 32bit updates. Do we want distinguished alignment declarations for e.g. This->held_frames?
Write: InterlockedExchange/Increment/etc. is clear
Read: Open for discussion
Basically, all I need is a snapshot of a 32bit entity, i.e. an atomic read. The code doesn't bother whether that value is stale or not. It works in either case. All that is required is that it's updated some time eventually (i.e. not running off local cache for hours).
Back in the old days, people used foo = (volatile X) bar; or even volatile X bar; ... foo=bar; Such code has been present e.g. in mciseq for the last 10 years.
Currently, this has my preference for Intel: foo = bar; // e.g. held = This->held_frames; __asm__ __volatile__("":::"memory"); /* compiler barrier is enough for x86 */
I wouldn't be opposed to held = (volatile DWORD) This->held_frames; Purists spit at volatile for threading, but that's what has been used for decades and continues to be used (just google some random threading app...)
I've also suggested InterlockedExchange(&held, This->held_frames) but that's just wasting a read of the obsolete value of held. So why use that form?
Do we need to care for anything else than Intel?
One may envision other approaches, e.g. a second critical section for the callback and Start/Stop only. But This->held_frames is updated within IAC_Get/ReleaseBuffer, so that doesn't solve the issue.
POSIX purists would say that a protocol is needed between the two communicating threads (the Get/Release one and the cb, not the Start/Stop thread). But I don't care about POSIX requirements imposed by weird 8/16/24/32/40/64 bit HW and strange non-UNIX systems when the target is Wine-Is-Not-an-Emulator.
Regards, Jörg Höhle