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

Re: [PATCH 5/6] dsound: rework ugly mixer logic

by Maarten Lankhorst

Op 22-10-12 17:42, Andrew Eikum schreef: > On Sat, Oct 20, 2012 at 12:03:55AM +0200, Maarten Lankhorst wrote: >> Op 19-10-12 15:54, Andrew Eikum schreef: >>> Mostly good cleanup in this one. Some thoughts below... >>> >>> On Tue, Oct 16, 2012 at 02:06:29PM +0200, Maarten Lankhorst wrote: >>>> diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h >>>> index feef787..7817b88 100644 >>>> --- a/dlls/dsound/dsound_private.h >>>> +++ b/dlls/dsound/dsound_private.h >>>> @@ -73,10 +73,8 @@ struct DirectSoundDevice >>>> - DWORD writelead, buflen, state, playpos, mixpos; >>>> + DWORD writelead, buflen, aclen, fraglen, state, playpos, pad; >>> If you're introducing new variables, surely you can come up with >>> something more descriptive than "aclen." Something like >>> "ac_buffer_frames," maybe? I'm a big fan of units on my variable >>> names so mistakes like "pos_bytes = ac_frames;" are obvious. >>> >>> Similar for "pad," maybe something like "in_mmdev_bytes" (Is that >>> actually used any more after your patches? You could re-use it if >>> not.) >> However what I do makes sense here, since all of the units in dsound right >> now are bytes, unless that gets changed. So I stick to them. >> I'm all for changing it to frames, but it was out of the scope for this patch. >> > Fair enough, I hadn't thought of that. I still think "aclen" is a > crummy name :P > >> And.. get out of my bikeshed! >> > Well, it does lead to errors (bbbf72ddcb1202) and I don't see any harm > in being explicit. It'll future-proof it, in case future variables > have some other unit. > > In any case, not a patch-killer, of course. > >>>> + block = device->pwfx->nBlockAlign; >>> Bleh. Do we really need to alias that? >> It's my bikeshed to paint! :P >> > My objection is it makes the code harder to read, which is something > we really don't need in dsound. For an even worse example, see the > w/wi/wfe/wfe2/wfe3 web in your 3rd patch. Yeah got tired of that too and fixed it in the atomic reopendevice rework, it uses better descriptions instead, and returns a newly allocated WAVEFORMATEX which is more clear for use. In this patch wfe2 is the format returned from IsFormatSupported, wfe3 is the temporary float format I use for testing. In the atomic reopendevice I thought about killing an extra copy from that step, but it's not worth it. If the allocation fails it's handled correctly, and the reallocation of device->buffer would very likely fail, too. >> Followup patch I didn't send in yet on removed this entirely and just consolidated both cases. >> >>>> + if (maxq > device->fraglen * 3) >>>> + maxq = device->fraglen * 3; >>>> + >>> This seems to be replacing ds_snd_queue_max, right? Why remove the >>> configurability? Why 3 instead of the old default of 10? This should >>> be a separate patch at least. >> No this does not replace ds_snd_queue_max, I always fill up the buffer to full, >> but I'm supposed to get a event every period, so instead of always filling the >> buffer to full I spread it out over multiple periods so hopefully applications >> have some time to catch up. This gets rid of clipping at the start. >> > Okay, thanks for explaining. I was going to give the behavior changes > a closer look after we figure out patch 3 and I get a chance to run my > tests. > That's actually the easiest patch, I pretend primary format is 22050 / 8 / 2 like windows, until setformat is called and then it copies the actual wfx from the current audioclient if priority != writeprimary, else it attempts to set it through creating a new iaudioclient with the specified format. Switching to and from primary priolevel will also re-create the audioclient, and if not writeprimary the preferred format will be float since it saves us a step in the mixer. ~Maarten

Oct. 22, 2012

Re: [PATCH 3/6] dsound: create a primary_pwfx separately from pwfx

by Andrew Eikum

On Sat, Oct 20, 2012 at 12:13:08AM +0200, Maarten Lankhorst wrote: > Hey, > > Op 19-10-12 15:29, Andrew Eikum schreef: > > Patches 1 and 2 in this series look fine. > > > > I have a series of patches similar to this one in my dsound > > multichannel branch. This patch seems to do too much at once. > > > > When I did this cleanup, I split it up into four patches: > > 1) Allocate the device format in the Device struct > > 2) Load the default format from the IAC, not hard-coded in dsound > > 3) Put SetFormat() calls into their own WFX that we don't really care > > about > > 4) Always use WAVE_FORMAT_EXTENSIBLE for the device format > 3 of 6 does essentially the step you call #4 and #2, other steps is just fixes I already have in my tree. > Tested quite extensively too > > The other steps I have already done in a followup series, except #1, it's unneeded. > It also breaks device->pwfx = NULL in the initial case, which is nice to see if something > depends on the new format before changing over is complete. This would otherwise > be very hardto detect. > > I'm surprised you call #2 a separate step since from what I gathered about > mmdevapi is that until win7 sp1 it only supported 1 rate, and we don't > implement the rate changing extension yet, so the whole popular format > testing was pointless to begin with. > Like the fraglen change in patch 5, It's a change in dsound's behavior that can be isolated from the rest of the patch. Being able to bisect these kinds of changes is really important, as you can run into some really unexpected bugs from how apps use dsound. Bug 29127 is an example of a regression that really surprised me. > Is there anything *technically* wrong with the patch series though? > I haven't dug into them that far yet. Patch 3 needs work, so I don't want to test the further ones until it's fixed. You don't have to do it the same way I did, but please at least split out the format initialization change from the SetFormat change. Andrew

Oct. 22, 2012

Re: [PATCH 5/6] dsound: rework ugly mixer logic

by Andrew Eikum

On Sat, Oct 20, 2012 at 12:03:55AM +0200, Maarten Lankhorst wrote: > Op 19-10-12 15:54, Andrew Eikum schreef: > > Mostly good cleanup in this one. Some thoughts below... > > > > On Tue, Oct 16, 2012 at 02:06:29PM +0200, Maarten Lankhorst wrote: > >> diff --git a/dlls/dsound/dsound_private.h b/dlls/dsound/dsound_private.h > >> index feef787..7817b88 100644 > >> --- a/dlls/dsound/dsound_private.h > >> +++ b/dlls/dsound/dsound_private.h > >> @@ -73,10 +73,8 @@ struct DirectSoundDevice > >> - DWORD writelead, buflen, state, playpos, mixpos; > >> + DWORD writelead, buflen, aclen, fraglen, state, playpos, pad; > > If you're introducing new variables, surely you can come up with > > something more descriptive than "aclen." Something like > > "ac_buffer_frames," maybe? I'm a big fan of units on my variable > > names so mistakes like "pos_bytes = ac_frames;" are obvious. > > > > Similar for "pad," maybe something like "in_mmdev_bytes" (Is that > > actually used any more after your patches? You could re-use it if > > not.) > However what I do makes sense here, since all of the units in dsound right > now are bytes, unless that gets changed. So I stick to them. > I'm all for changing it to frames, but it was out of the scope for this patch. > Fair enough, I hadn't thought of that. I still think "aclen" is a crummy name :P > And.. get out of my bikeshed! > Well, it does lead to errors (bbbf72ddcb1202) and I don't see any harm in being explicit. It'll future-proof it, in case future variables have some other unit. In any case, not a patch-killer, of course. > >> + block = device->pwfx->nBlockAlign; > > Bleh. Do we really need to alias that? > It's my bikeshed to paint! :P > My objection is it makes the code harder to read, which is something we really don't need in dsound. For an even worse example, see the w/wi/wfe/wfe2/wfe3 web in your 3rd patch. > Followup patch I didn't send in yet on removed this entirely and just consolidated both cases. > > > > >> + if (maxq > device->fraglen * 3) > >> + maxq = device->fraglen * 3; > >> + > > This seems to be replacing ds_snd_queue_max, right? Why remove the > > configurability? Why 3 instead of the old default of 10? This should > > be a separate patch at least. > No this does not replace ds_snd_queue_max, I always fill up the buffer to full, > but I'm supposed to get a event every period, so instead of always filling the > buffer to full I spread it out over multiple periods so hopefully applications > have some time to catch up. This gets rid of clipping at the start. > Okay, thanks for explaining. I was going to give the behavior changes a closer look after we figure out patch 3 and I get a chance to run my tests. Andrew

Oct. 22, 2012

Re: [PATCH 4/6] dsound: Use event based threads

by Andrew Eikum

On Fri, Oct 19, 2012 at 11:33:22PM +0200, Maarten Lankhorst wrote: > Op 19-10-12 15:40, Andrew Eikum schreef: > > I'm a big fan of error checking (or at least reporting), so these > > unchecked calls make me nervous. They'll probably never fail, but if > > they do, I think they should fail loudly. > > > Erm, dsound at this point will be worse off if you perform error reporting here than fail silently. > It leaves a bigger mess since ReopenDevice is not atomic right now.. > What I meant by "reporting" was write a WARN or something. Then we know something wrong happened even if we don't have the correct driver channel enabled. Andrew

Oct. 22, 2012

Re: [PATCH 5/5] services: Added tests for service thread termination without setting service state to stopped

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=22392 Your paranoid android. === WVISTAADM (32 bit service) === service.c:397: Test failed: status.dwCurrentState = 3 service.c:411: Test failed: status.dwCurrentState = 3 === W2K8SE (32 bit service) === service.c:397: Test failed: status.dwCurrentState = 3 service.c:411: Test failed: status.dwCurrentState = 3

Oct. 22, 2012

Re: ntdll feature detection patch set

by André Hentschel

Am 21.10.2012 02:51, schrieb James Eder: > I've been kicking this patch set around in my local git tree for a > while and I want to get it out there for comments and maybe even > inclusion in Wine. Seeing some comments in another thread about > wineboot made me think I should get these patches out rather than > holding onto them much longer. > > I feel fairly good about the first 6 patches. They're small, > bite-sized, and all x86/x86_64 so I can test them easily. However, > the last two in the series are some what large and do things with ARM > and PowerPC code. While it looks good to me, I don't have hardware or > a build environment for them. I may have some ARM+Linux hardware in > the coming months but I doubt I'll ever see a PowerPC come my way. > > If you have a build environment for ARM or PowerPC I would very much > appreciate some feedback. > > Also I did some greping and found two other locations in Wine with > similar do_cpuid type functions: dlls/wbemprox/builtin.c and > dlls/dbghelp/minidump.c. Would it be worthwhile to have the function > in a common header somewhere? > Sometimes it's best when you test yourself. You don't need hardware, qemu can emulate ARM, PPC and most other things. e.g. Debian has images for much platforms. I only have ARM hardware, but need to find some time to test your patchset. Further i think patch 7 and 8 should be squashed together. -- Best Regards, André Hentschel

Oct. 22, 2012

Re: [PATCH 4/4] mshtml: Added more events tests

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=22386 Your paranoid android. === W2KPROSP4 (32 bit) === No test summary line found === WXPPROSP3 (32 bit) === No test summary line found === W2K3R2SESP2 (32 bit) === No test summary line found === WVISTAADM (32 bit) === No test summary line found === W2K8SE (32 bit) === No test summary line found === W7PRO (32 bit) === No test summary line found === W7PROX64 (32 bit) === No test summary line found === TEST64_W7SP1 (32 bit) === No test summary line found === W7PROX64 (64 bit) === No test summary line found === TEST64_W7SP1 (64 bit) === No test summary line found

Oct. 22, 2012

Re: [PATCH 2/2] d3dxof: Use sizeof(GUID) instead of hardcoding the value.

by Alexandre Julliard

Christian Costa <titan.costa(a)gmail.com> writes: > @@ -599,7 +599,7 @@ static HRESULT WINAPI IDirectXFileDataImpl_GetId(IDirectXFileData* iface, LPGUID > if (!pGuid) > return DXFILEERR_BADVALUE; > > - memcpy(pGuid, &This->pobj->class_id, 16); > + memcpy(pGuid, &This->pobj->class_id, sizeof(GUID)); Using normal assignments would be even better. -- Alexandre Julliard julliard(a)winehq.org

Oct. 22, 2012

Re: [PATCH 1/4] mshtml: Fixed attribute object caching logic

by Marvin

Hi, While running your changed tests on Windows, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at http://testbot.winehq.org/JobDetails.pl?Key=22384 Your paranoid android. === W2KPROSP4 (32 bit) === No test summary line found === WXPPROSP3 (32 bit) === No test summary line found === W2K3R2SESP2 (32 bit) === No test summary line found === WVISTAADM (32 bit) === No test summary line found === W2K8SE (32 bit) === No test summary line found === W7PRO (32 bit) === No test summary line found === W7PROX64 (32 bit) === No test summary line found === TEST64_W7SP1 (32 bit) === No test summary line found === W7PROX64 (64 bit) === No test summary line found === TEST64_W7SP1 (64 bit) === No test summary line found

Oct. 22, 2012

Re: d3dx9_36: Add tests for D3DXSHRotateZ

by Alexandre Julliard

Michael Stefaniuc <mstefani(a)redhat.com> writes: > I'm removing CONST in the lines I touch so +1 on a concerted effort to > eradicate it. Though at 1.2k lines Alexandre might consider it too spammy. Yes, that's too much for a large scale cleanup. -- Alexandre Julliard julliard(a)winehq.org

Oct. 22, 2012