December 2012 - Wine-Devel - list.winehq.org

Re: [PATCH 3/3] usp10: Add support for format 2 pair adjustments.

by Dmitry Timoshkov

Huw Davies <huw(a)codeweavers.com> wrote: > > > typedef struct { > > > + WORD PosFormat; > > > + WORD Coverage; > > > + WORD ValueFormat1; > > > + WORD ValueFormat2; > > > + WORD ClassDef1; > > > + WORD ClassDef2; > > > + WORD Class1Count; > > > + WORD Class2Count; > > > + WORD Class1Record[1]; > > > +} GPOS_PairPosFormat2; > > > > Shouldn't this structure be 2 bytes aligned? > > I don't see why. They're all WORDs. There are 9 elements of 2 bytes in size each, 18 bytes in total, the compiler will align this structure to 4 bytes, making its size 20 bytes. -- Dmitry.

Dec. 20, 2012

Re: [PATCH 3/3] usp10: Add support for format 2 pair adjustments.

by Huw Davies

On Thu, Dec 20, 2012 at 11:26:36PM +0800, Dmitry Timoshkov wrote: > Huw Davies <huw(a)codeweavers.com> wrote: > > The size is irrelevant - it's variable sized anyway. We just care > > about the offsets of the elements. > > Anyway, in order to avoid any confusion or bugs in future with allocating > an array of such structures or using sizeof() on them I'd suggest to > explicitely specify alignment. Using an array of these structures or using sizeof() is wrong anyway as they are variable-sized...

Dec. 20, 2012

Re: winepulse test data review

by Arun Raghavan

On Thu, 2012-12-20 at 13:34 +0100, Maarten Lankhorst wrote: > Op 04-12-12 14:28, Joerg-Cyril.Hoehle(a)t-systems.com schreef: > > Hi, > > > > Maarten Lankhorst wrote: > >> Actually in winepulse, sleep / 12000 (8 ms I guess?) ms would work, no sleep at all works too, > > Yes, I wanted to go no further than 8ms below the 10ms period limit. > > > >> it was just the mixing of various levels of sleep that was failing. Not sure why though, > >> might be something about the scheduling throwing it off. > > There's a perfectly simple explanation of this that applies to both winepulse and winecoreaudio. > > It goes as follows. > > > > IOW, there's a bug in winecoreaudio. It may explain why the test stutters on my MacOS 10.5.8 machine, > > regardless of how accurate or "improved" GetPosition is. > > > > winecoreaudio's IAC_ReleaseBuffer sends packets off as soon as it receives data, unlike winealsa/wineoss. > > > > Consider the sleep sequence 0 5 8 0 5 ms... > > > > + The transition 8 -> 0ms sleep is fine, as data is sent 2ms after the prior packet > > > > - The transition 0 -> 5 is most problematic, as it means that 2 ReleaseBuffer > > will be 15ms apart, even though only 10ms of data was sent. > > > > I believe winepulse is affected by this too, like winecoreaudio. > > > > The stable sequences "always 0" or "always 8ms sleep" are paced regularly, so underruns > > are avoided as long as the OS schedules the threads soon enough. > > > > > > Winealsa and wineOSS are not affected, as they write data on the 10ms period boundaries, > > -- like native presumably does. > > > > > > winecoreaudio solution paths: > > - write packets on period tick, not at release time > > - or add silence lead-in > > - or delay first packet until period tick only (if small) > > - or ... > > > > I think it should be possible to produce reliable continuous sound in winepulse and winecoreaudio > > yet maintain their current "send packet" codebase instead of transitioning to a separate buffer > > that winealsa and wineoss use. > > > Actually, Arun Raghavan found this thread and pointed me towards > updating pulseaudio, it seems it was a bug in pulseaudio fixed in 3.0: > > Op 20-12-12 12:43, Arun Raghavan schreef: > > Hi Maarten, > > While following some random thread, I cam across this: > > > > http://www.winehq.org/pipermail/wine-devel/2012-December/098008.html > > > > I see you mention: > > > > """ > > I may also be hitting some pulseaudio bug though, if I play some music on the background when one > > of those tests start the sound during the test works ok, even if I pause it after that. > > """ > > > > We recently fixed a bug where pausing and resuming a stream could end up > > adding a hidden latency to the stream, and this would not happen if > > another stream was playing. The fix is in 3.0, so if this is what you > > were referring to, might be worth trying with newer PA. > > This does make my life a lot easier though, as finding a way to only queue on multiple > of periods was giving me a headache, results from 10 seconds now: That's good to hear. > render.c:2238: Should play 1000ms continuous tone with fragment size 441. > render.c:2290: Released 441441=1001x441 -881 frames at 44100 worth 9990ms in 9983ms > render.c:2238: Should play 1000ms continuous tone with fragment size 441. > render.c:2290: Released 441441=1001x441 -882 frames at 44100 worth 9990ms in 9990ms > render.c:2238: Should play 1000ms continuous tone with fragment size 441. > render.c:2290: Released 441441=1001x441 -882 frames at 44100 worth 9990ms in 9988ms > render.c:2238: Should play 1000ms continuous tone with fragment size 441. > render.c:2290: Released 441441=1001x441 -881 frames at 44100 worth 9990ms in 9987ms > render.c:2238: Should play 1000ms continuous tone with fragment size 441. > render.c:2290: Released 441441=1001x441 -682 frames at 44100 worth 9995ms in 9986ms > render.c:2238: Should play 1000ms continuous tone with fragment size 441. > render.c:2290: Released 441441=1001x441 -724 frames at 44100 worth 9994ms in 9987ms > render.c:2238: Should play 1000ms continuous tone with fragment size 441. > render.c:2290: Released 441441=1001x441 -881 frames at 44100 worth 9990ms in 9984ms > render.c:2238: Should play 1000ms continuous tone with fragment size 441. > render.c:2290: Released 441441=1001x441 -767 frames at 44100 worth 9993ms in 9989ms > render.c:2238: Should play 1000ms continuous tone with fragment size 441. > render.c:2290: Released 441441=1001x441 -441 frames at 44100 worth 10000ms in 9991ms > render.c:2238: Should play 1000ms continuous tone with fragment size 441. > render.c:2290: Released 441441=1001x441 -882 frames at 44100 worth 9990ms in 9988ms > render: 41019 tests executed (0 marked as todo, 0 failures), 0 skipped. > > I would be interested in the exact commit though, so I can get it backported to > precise/quantal, from ubuntuforums' winepulse thread it seems I'm not the only one > that might have been hitting this problem. The bug becomes really visible when your stream requests a high latency (and no other stream with a lower latency request is connected, like the music player or Flash). This is the commit of interest to you: http://cgit.freedesktop.org/pulseaudio/pulseaudio/commit/?id=29f064aa3d3a83… Cheers, Arun

Dec. 20, 2012

Re: [PATCH 3/3] usp10: Add support for format 2 pair adjustments.

by Dmitry Timoshkov

Huw Davies <huw(a)codeweavers.com> wrote: > typedef struct { > + WORD PosFormat; > + WORD Coverage; > + WORD ValueFormat1; > + WORD ValueFormat2; > + WORD ClassDef1; > + WORD ClassDef2; > + WORD Class1Count; > + WORD Class2Count; > + WORD Class1Record[1]; > +} GPOS_PairPosFormat2; Shouldn't this structure be 2 bytes aligned? -- Dmitry.

Dec. 20, 2012

Re: [PATCH 3/3] usp10: Add support for format 2 pair adjustments.

by Huw Davies

On Thu, Dec 20, 2012 at 11:05:48PM +0800, Dmitry Timoshkov wrote: > Huw Davies <huw(a)codeweavers.com> wrote: > > > > > typedef struct { > > > > + WORD PosFormat; > > > > + WORD Coverage; > > > > + WORD ValueFormat1; > > > > + WORD ValueFormat2; > > > > + WORD ClassDef1; > > > > + WORD ClassDef2; > > > > + WORD Class1Count; > > > > + WORD Class2Count; > > > > + WORD Class1Record[1]; > > > > +} GPOS_PairPosFormat2; > > > > > > Shouldn't this structure be 2 bytes aligned? > > > > I don't see why. They're all WORDs. > > There are 9 elements of 2 bytes in size each, 18 bytes in total, > the compiler will align this structure to 4 bytes, making its > size 20 bytes. The size is irrelevant - it's variable sized anyway. We just care about the offsets of the elements.

Dec. 20, 2012

Re: [PATCH 3/3] usp10: Add support for format 2 pair adjustments.

by Huw Davies

On Thu, Dec 20, 2012 at 10:12:38PM +0800, Dmitry Timoshkov wrote: > Huw Davies <huw(a)codeweavers.com> wrote: > > > typedef struct { > > + WORD PosFormat; > > + WORD Coverage; > > + WORD ValueFormat1; > > + WORD ValueFormat2; > > + WORD ClassDef1; > > + WORD ClassDef2; > > + WORD Class1Count; > > + WORD Class2Count; > > + WORD Class1Record[1]; > > +} GPOS_PairPosFormat2; > > Shouldn't this structure be 2 bytes aligned? I don't see why. They're all WORDs. Huw.

Dec. 20, 2012

Re: mmdevapi: Avoid lock contention after SetEvent.

by Andrew Eikum

On Thu, Dec 20, 2012 at 02:03:06PM +0100, Joerg-Cyril.Hoehle(a)t-systems.com wrote: > what's up with this patch? > > I wrote: > >SetEvent is one of those calls with a high probability that the > >Linux scheduler will switch to another thread, e.g. the one receiving > >the event. As the critical section is still hold, it'll prevent the > >receiver from invoking mmdevapi, e.g. GetCurrentPadding, without two > >more thread switches (forth and back). We don't need that. > > Now that This->event is constant between IAC_Start and IAC_Release, > thanks to my "SetEventHandle is allowed once only" patch, > it's safe to move SetEvent(This->event) outside of the CS. > > Unlike the rejected patch about a CreateTimerQueue race condition > internal to Wine (and only relevant in lock-less mode), > this one is between the driver and the client app that blocks when calling e.g. > GetCurrentPadding, what almost all apps will do, including our DSound > http://source.winehq.org/source/dlls/dsound/mixer.c#L679 > I don't have any problems with this patch, but I don't know our synchronization methods well enough to know if this is correct. Is it true that SetEvent() causes a context switch? I see it eventually resolves down to a write() in <server/thread.c:send_thread_wakeup()>, but I don't know what that implies. Julliard, any thoughts? Andrew

Dec. 20, 2012

mmdevapi: Avoid lock contention after SetEvent.

by Joerg-Cyril.Hoehle＠t-systems.com

Hi, what's up with this patch? I wrote: >SetEvent is one of those calls with a high probability that the >Linux scheduler will switch to another thread, e.g. the one receiving >the event. As the critical section is still hold, it'll prevent the >receiver from invoking mmdevapi, e.g. GetCurrentPadding, without two >more thread switches (forth and back). We don't need that. Now that This->event is constant between IAC_Start and IAC_Release, thanks to my "SetEventHandle is allowed once only" patch, it's safe to move SetEvent(This->event) outside of the CS. Unlike the rejected patch about a CreateTimerQueue race condition internal to Wine (and only relevant in lock-less mode), this one is between the driver and the client app that blocks when calling e.g. GetCurrentPadding, what almost all apps will do, including our DSound http://source.winehq.org/source/dlls/dsound/mixer.c#L679 Regards, Jörg Höhle

Dec. 20, 2012

Re: winepulse test data review

by Maarten Lankhorst

Op 04-12-12 14:28, Joerg-Cyril.Hoehle(a)t-systems.com schreef: > Hi, > > Maarten Lankhorst wrote: >> Actually in winepulse, sleep / 12000 (8 ms I guess?) ms would work, no sleep at all works too, > Yes, I wanted to go no further than 8ms below the 10ms period limit. > >> it was just the mixing of various levels of sleep that was failing. Not sure why though, >> might be something about the scheduling throwing it off. > There's a perfectly simple explanation of this that applies to both winepulse and winecoreaudio. > It goes as follows. > > IOW, there's a bug in winecoreaudio. It may explain why the test stutters on my MacOS 10.5.8 machine, > regardless of how accurate or "improved" GetPosition is. > > winecoreaudio's IAC_ReleaseBuffer sends packets off as soon as it receives data, unlike winealsa/wineoss. > > Consider the sleep sequence 0 5 8 0 5 ms... > > + The transition 8 -> 0ms sleep is fine, as data is sent 2ms after the prior packet > > - The transition 0 -> 5 is most problematic, as it means that 2 ReleaseBuffer > will be 15ms apart, even though only 10ms of data was sent. > > I believe winepulse is affected by this too, like winecoreaudio. > > The stable sequences "always 0" or "always 8ms sleep" are paced regularly, so underruns > are avoided as long as the OS schedules the threads soon enough. > > > Winealsa and wineOSS are not affected, as they write data on the 10ms period boundaries, > -- like native presumably does. > > > winecoreaudio solution paths: > - write packets on period tick, not at release time > - or add silence lead-in > - or delay first packet until period tick only (if small) > - or ... > > I think it should be possible to produce reliable continuous sound in winepulse and winecoreaudio > yet maintain their current "send packet" codebase instead of transitioning to a separate buffer > that winealsa and wineoss use. > Actually, Arun Raghavan found this thread and pointed me towards updating pulseaudio, it seems it was a bug in pulseaudio fixed in 3.0: Op 20-12-12 12:43, Arun Raghavan schreef: > Hi Maarten, > While following some random thread, I cam across this: > > http://www.winehq.org/pipermail/wine-devel/2012-December/098008.html > > I see you mention: > > """ > I may also be hitting some pulseaudio bug though, if I play some music on the background when one > of those tests start the sound during the test works ok, even if I pause it after that. > """ > > We recently fixed a bug where pausing and resuming a stream could end up > adding a hidden latency to the stream, and this would not happen if > another stream was playing. The fix is in 3.0, so if this is what you > were referring to, might be worth trying with newer PA. This does make my life a lot easier though, as finding a way to only queue on multiple of periods was giving me a headache, results from 10 seconds now: render.c:2238: Should play 1000ms continuous tone with fragment size 441. render.c:2290: Released 441441=1001x441 -881 frames at 44100 worth 9990ms in 9983ms render.c:2238: Should play 1000ms continuous tone with fragment size 441. render.c:2290: Released 441441=1001x441 -882 frames at 44100 worth 9990ms in 9990ms render.c:2238: Should play 1000ms continuous tone with fragment size 441. render.c:2290: Released 441441=1001x441 -882 frames at 44100 worth 9990ms in 9988ms render.c:2238: Should play 1000ms continuous tone with fragment size 441. render.c:2290: Released 441441=1001x441 -881 frames at 44100 worth 9990ms in 9987ms render.c:2238: Should play 1000ms continuous tone with fragment size 441. render.c:2290: Released 441441=1001x441 -682 frames at 44100 worth 9995ms in 9986ms render.c:2238: Should play 1000ms continuous tone with fragment size 441. render.c:2290: Released 441441=1001x441 -724 frames at 44100 worth 9994ms in 9987ms render.c:2238: Should play 1000ms continuous tone with fragment size 441. render.c:2290: Released 441441=1001x441 -881 frames at 44100 worth 9990ms in 9984ms render.c:2238: Should play 1000ms continuous tone with fragment size 441. render.c:2290: Released 441441=1001x441 -767 frames at 44100 worth 9993ms in 9989ms render.c:2238: Should play 1000ms continuous tone with fragment size 441. render.c:2290: Released 441441=1001x441 -441 frames at 44100 worth 10000ms in 9991ms render.c:2238: Should play 1000ms continuous tone with fragment size 441. render.c:2290: Released 441441=1001x441 -882 frames at 44100 worth 9990ms in 9988ms render: 41019 tests executed (0 marked as todo, 0 failures), 0 skipped. I would be interested in the exact commit though, so I can get it backported to precise/quantal, from ubuntuforums' winepulse thread it seems I'm not the only one that might have been hitting this problem. ~Maarten

Dec. 20, 2012

Re: msxml3: Add custom handling for DISPID_VALUE for node map

by Jacek Caban

Hi Nikolay, On 12/20/12 11:29, Nikolay Sivov wrote: > On 12/19/2012 17:22, Jacek Caban wrote: >> On 12/19/12 08:24, Nikolay Sivov wrote: >>> Add custom handling for DISPID_VALUE for node map >> If this has to be done for all IDispatchEx objects with DISPID_VALUE, it >> may be worth considering changing dispex.c. Something like: >> >> if(id == DISPID_VALUE && wFlags == DISPATCH_METHOD) >> wFlags |= DISPATCH_PROPERTYGET; >> >> before ITypeInfo::Invoke call should do the trick and replace both this >> patch and already committed node list patch. > So I did more testing and it doesn't work like that for all > DISPID_VALUE methods, what a surprise: > > http://www.winehq.org/pipermail/wine-patches/2012-December/120928.html > > Do you think it's still better to centrally change it in dispex.c and > wrongly accept DISPATCH_METHOD when we shouldn't? I'm not really convinced by those tests, here is why: + + hr = IXMLDOMParseError_QueryInterface(error, &IID_IDispatchEx, (void**)&dispex); + ok(hr == S_OK, "got 0x%08x\n", hr); + + V_VT(&arg) = VT_I4; + V_I4(&arg) = 0; + dispparams.cArgs = 1; + dispparams.cNamedArgs = 0; + dispparams.rgdispidNamedArgs = NULL; + dispparams.rgvarg = &arg; + + V_VT(&ret) = VT_EMPTY; + V_DISPATCH(&ret) = (void*)0x1; + hr = IDispatchEx_Invoke(dispex, DISPID_VALUE, &IID_NULL, 0, DISPATCH_METHOD, &dispparams, &ret, NULL, NULL); + ok(hr == DISP_E_MEMBERNOTFOUND, "got 0x%08x\n", hr); + ok(V_VT(&ret) == VT_EMPTY, "got %d\n", V_VT(&ret)); + ok(V_DISPATCH(&ret) == (void*)0x1, "got %p\n", V_DISPATCH(&ret)); IXMLDOMParseError's DISPID_VALUE is errorCode property, which doesn't take arguments (in C world, it's no argument except retval). You're passing one argument, so the call should return error (I'd expect DISP_E_BADPARAMCOUNT, but well...). + hr = IXMLDOMSchemaCollection_QueryInterface(cache, &IID_IDispatchEx, (void**)&dispex); + ok(hr == S_OK, "got 0x%08x\n", hr); + + V_VT(&arg) = VT_I4; + V_I4(&arg) = 0; + dispparams.cArgs = 1; + dispparams.cNamedArgs = 0; + dispparams.rgdispidNamedArgs = NULL; + dispparams.rgvarg = &arg; + + V_VT(&ret) = VT_EMPTY; + V_DISPATCH(&ret) = (void*)0x1; + hr = IDispatchEx_Invoke(dispex, DISPID_VALUE, &IID_NULL, 0, DISPATCH_METHOD, &dispparams, &ret, NULL, NULL); + ok(hr == DISP_E_MEMBERNOTFOUND, "got 0x%08x\n", hr); + ok(V_VT(&ret) == VT_EMPTY, "got %d\n", V_VT(&ret)); + ok(V_DISPATCH(&ret) == (void*)0x1, "got %p\n", V_DISPATCH(&ret)); This one is indeed showing different behaviour, although the test uses an empty collection, so the call probably shouldn't work anyway. I'm not saying that there should be no custom implementations, but AFAICS majority of objects can be fixed by simple IDispatchEx implementation modification and if there are cases that still need fixing, then we can fall back to custom implementation. Cheers, Jacek

Dec. 20, 2012