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

[Andrew Eikum]

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.)
> @@ -209,7 +207,6 @@ HRESULT DSOUND_PrimaryCreate(DirectSoundDevice *device) DECLSPEC_HIDDEN;
>  HRESULT DSOUND_PrimaryDestroy(DirectSoundDevice *device) DECLSPEC_HIDDEN;
>  HRESULT DSOUND_PrimaryPlay(DirectSoundDevice *device) DECLSPEC_HIDDEN;
>  HRESULT DSOUND_PrimaryStop(DirectSoundDevice *device) DECLSPEC_HIDDEN;
> -HRESULT DSOUND_PrimaryGetPosition(DirectSoundDevice *device, LPDWORD playpos, LPDWORD writepos) DECLSPEC_HIDDEN;

Good riddance.
> @@ -682,147 +623,100 @@ static void DSOUND_PerformMix(DirectSoundDevice *device)
>    	LeaveCriticalSection(&device->mixlock);
>    	return;
>    }
> -
> - to_mix_frags = device->prebuf - (pad * device->pwfx->nBlockAlign + device->fraglen - 1) / device->fraglen;
> -
> - to_mix_bytes = to_mix_frags * device->fraglen;
> -
> - if(device->in_mmdev_bytes > 0){
> -
> DWORD delta_bytes = min(to_mix_bytes, device->in_mmdev_bytes);
> -
> device->in_mmdev_bytes -= delta_bytes;
> -
> device->playing_offs_bytes += delta_bytes;
> -
> device->playing_offs_bytes %= device->buflen;
> - block = device->pwfx->nBlockAlign;

Bleh. Do we really need to alias that?
> - 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.
> -
>
> writepos = (device->playpos + pad) % device->buflen;
>
> if (device->priolevel != DSSCL_WRITEPRIMARY) {
> -
> BOOL recover = FALSE, all_stopped = FALSE;
> -
> DWORD playpos, writepos, writelead, maxq, prebuff_max, prebuff_left, size1, size2;
> -
> LPVOID buf1, buf2;
> -
> BOOL all_stopped = FALSE;
> int nfiller;
> -
> BOOL native = device->normfunction == normfunctions[4];
> -
> DWORD bpp = device->pwfx->wBitsPerSample>>3;

Again, do we need to alias that?
> -static DWORD DSOUND_fraglen(DirectSoundDevice *device)
> -{
> - REFERENCE_TIME period;
> - HRESULT hr;
> - DWORD ret;
> -
> - hr = IAudioClient_GetDevicePeriod(device->client, &period, NULL);
> - if(FAILED(hr)){
> -
>    /* just guess at 10ms */
> -
>    WARN("GetDevicePeriod failed: %08x\n", hr);
> -
>    ret = MulDiv(device->pwfx->nBlockAlign, device->pwfx->nSamplesPerSec, 100);
> - }else
> -
>    ret = MulDiv(device->pwfx->nSamplesPerSec * device->pwfx->nBlockAlign, period, 10000000);
> -
> - ret -= ret % device->pwfx->nBlockAlign;
> - return ret;
>
> -}
> -

...
> - device->fraglen = MulDiv(device->pwfx->nSamplesPerSec, period, 10000000) * device->pwfx->nBlockAlign;

This should be a separate patch. I don't have an argument /against/
it, but why do you prefer 10ms over whatever the driver prefers?

Andrew

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

[Maarten Lankhorst]

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.

If you want to do it like that it's better to go all the way and introduce helpers
for all of wine's drivers and dsound/winmm to use instead. This would improve
readability a lot more instead of reinventing the wheel everywhere..

And.. get out of my bikeshed!
> > @@ -209,7 +207,6 @@ HRESULT DSOUND_PrimaryCreate(DirectSoundDevice *device) DECLSPEC_HIDDEN;
> >  HRESULT DSOUND_PrimaryDestroy(DirectSoundDevice *device) DECLSPEC_HIDDEN;
> >  HRESULT DSOUND_PrimaryPlay(DirectSoundDevice *device) DECLSPEC_HIDDEN;
> >  HRESULT DSOUND_PrimaryStop(DirectSoundDevice *device) DECLSPEC_HIDDEN;
> > -HRESULT DSOUND_PrimaryGetPosition(DirectSoundDevice *device, LPDWORD playpos, LPDWORD writepos) DECLSPEC_HIDDEN;
>
> Good riddance.
> > @@ -682,147 +623,100 @@ static void DSOUND_PerformMix(DirectSoundDevice *device)
> >    	LeaveCriticalSection(&device->mixlock);
> >    	return;
> >    }
> > -
> > - to_mix_frags = device->prebuf - (pad * device->pwfx->nBlockAlign + device->fraglen - 1) / device->fraglen;
> > -
> > - to_mix_bytes = to_mix_frags * device->fraglen;
> > -
> > - if(device->in_mmdev_bytes > 0){
> > -
> > DWORD delta_bytes = min(to_mix_bytes, device->in_mmdev_bytes);
> > -
> > device->in_mmdev_bytes -= delta_bytes;
> > -
> > device->playing_offs_bytes += delta_bytes;
> > -
> > device->playing_offs_bytes %= device->buflen;
> > - block = device->pwfx->nBlockAlign;
>
> Bleh. Do we really need to alias that?

It's my bikeshed to paint! :P

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.

See also below why..
> > -
> >
> > writepos = (device->playpos + pad) % device->buflen;
> >
> > if (device->priolevel != DSSCL_WRITEPRIMARY) {
> > -
> > BOOL recover = FALSE, all_stopped = FALSE;
> > -
> > DWORD playpos, writepos, writelead, maxq, prebuff_max, prebuff_left, size1, size2;
> > -
> > LPVOID buf1, buf2;
> > -
> > BOOL all_stopped = FALSE;
> > int nfiller;
> > -
> > BOOL native = device->normfunction == normfunctions[4];
> > -
> > DWORD bpp = device->pwfx->wBitsPerSample>>3;
>
> Again, do we need to alias that?

A followup patch removes it entirely and just mixes directly to the buffer for the native
case (most common, only oss4 and (maybe, just guessing?) coreaudio don't support
float, so I didn't want to touch that case even more.
> > -static DWORD DSOUND_fraglen(DirectSoundDevice *device)
> > -{
> > - REFERENCE_TIME period;
> > - HRESULT hr;
> > - DWORD ret;
> > -
> > - hr = IAudioClient_GetDevicePeriod(device->client, &period, NULL);
> > - if(FAILED(hr)){
> > -
> >    /* just guess at 10ms */
> > -
> >    WARN("GetDevicePeriod failed: %08x\n", hr);
> > -
> >    ret = MulDiv(device->pwfx->nBlockAlign, device->pwfx->nSamplesPerSec, 100);
> > - }else
> > -
> >    ret = MulDiv(device->pwfx->nSamplesPerSec * device->pwfx->nBlockAlign, period, 10000000);
> > -
> > - ret -= ret % device->pwfx->nBlockAlign;
> > - return ret;
> >
> > -}
> > -
>
> ...
> > - device->fraglen = MulDiv(device->pwfx->nSamplesPerSec, period, 10000000) * device->pwfx->nBlockAlign;
>
> This should be a separate patch. I don't have an argument /against/
> it, but why do you prefer 10ms over whatever the driver prefers?

I guess from the earlier comment you didn't notice I filled the complete buffer?

I also refer you to GetStreamLatency where I was getting the period from,
each time the event is fired you should write at least fraglen data.

I'm being generous here and write 3 periods in the mixer, since I expect
that wine will not always keep up. But this does not replace ds_snd_hel_frags
in any way and instead I just fill the whole buffer that's given to me.

I'm hoping that by limiting to only 3 periods the cpu usage will be smoothed out
more over time, because after all we would already be more than 3 periods behind
which leaves us in a serious chance of underrunning already..

Please take it from me: IT'S OK TO FAIL THOSE MMDEVAPI PERIOD TESTS,
BUT PLEASE DON'T LIE ABOUT THE HARDWARE CAPABILITIES REGARDING
TO MINIMUM PERIOD, DEFAULT PERIOD AND STREAM LATENCY.

Well.. default period can be 10 ms if you want, and minimum period is below 10 ms.
Helps pass some tests, but I degress..

For example Skyrim with a 36 ms stream latency will just buffer in more data to
compensate instead. But it can't do it if you report that it's fine to feed data every 5 ms.
There's no reason for dsound to be special, so I'd rather have it follow mmdevapi
behavior more closely here..

~Maarten

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

[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

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

[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