Re: [PATCH] dsound: rework ugly mixer logic

[Andrew Eikum]

On Sat, Dec 29, 2012 at 01:24:03AM +0100, Maarten Lankhorst wrote:
> - if(!maxq){
> -
> /* nothing to do! */
> -
> LeaveCriticalSection(&device->mixlock);
> -
> return;
> }

This was removed in 8ba4090fc304993. It breaks starting the device in
some situations (write primary mode iirc).
> -
> if (native) {
> -
> 	void *buffer = NULL;
> -
> -
> 	hr = IAudioRenderClient_GetBuffer(device->render, maxq / block, (void*)&buffer);
> -
> 	if(FAILED(hr)){
> -
> 		WARN("GetBuffer failed: %08x\n", hr);
> -
> 		LeaveCriticalSection(&device->mixlock);
> -
> 		return;
> -
> 	}

I think this (mixing directly to RenderClient and skipping WaveQueue)
could be split into a separate patch.
>  hres = IAudioClient_Initialize(device->client,
>          AUDCLNT_SHAREMODE_SHARED, AUDCLNT_STREAMFLAGS_NOPERSIST |
> -
>        AUDCLNT_STREAMFLAGS_EVENTCALLBACK, prebuf_rt, 0, device->pwfx, NULL);
> -
>        AUDCLNT_STREAMFLAGS_EVENTCALLBACK, 800000, 0, device->pwfx, NULL);

...
> -
>    frames = (UINT64)device->pwfx->nSamplesPerSec * 800000 / 10000000;

Could you #define the 800000?

Anyway, I gave it a test on ALSA+Pulse and didn't find any issues.
I'll test the other backends when you resubmit.

Andrew

[PATCH] dsound: rework ugly mixer logic

[Maarten Lankhorst]

Op 31-12-12 17:59, Andrew Eikum schreef:
> On Sat, Dec 29, 2012 at 01:24:03AM +0100, Maarten Lankhorst wrote:
> > - if(!maxq){
> > -
> > /* nothing to do! */
> > -
> > LeaveCriticalSection(&device->mixlock);
> > -
> > return;
> > }
>
> This was removed in 8ba4090fc304993. It breaks starting the device in
> some situations (write primary mode iirc).

Writeprimary dsound tests still worked for me,  I don't particulary care though, if you want I'll zap it.
After rework fail it should no longer matter..
> > -
> > if (native) {
> > -
> > 	void *buffer = NULL;
> > -
> > -
> > 	hr = IAudioRenderClient_GetBuffer(device->render, maxq / block, (void*)&buffer);
> > -
> > 	if(FAILED(hr)){
> > -
> > 		WARN("GetBuffer failed: %08x\n", hr);
> > -
> > 		LeaveCriticalSection(&device->mixlock);
> > -
> > 		return;
> > -
> > 	}
>
> I think this (mixing directly to RenderClient and skipping WaveQueue)
> could be split into a separate patch.

Hate to sound like a broken record here, but the whole mixer logic breaks if you touch that code. :/
> >  hres = IAudioClient_Initialize(device->client,
> >          AUDCLNT_SHAREMODE_SHARED, AUDCLNT_STREAMFLAGS_NOPERSIST |
> > -
> >        AUDCLNT_STREAMFLAGS_EVENTCALLBACK, prebuf_rt, 0, device->pwfx, NULL);
> > -
> >        AUDCLNT_STREAMFLAGS_EVENTCALLBACK, 800000, 0, device->pwfx, NULL);
>
> ...
> > -
> >    frames = (UINT64)device->pwfx->nSamplesPerSec * 800000 / 10000000;
>
> Could you #define the 800000?

This is a very temporary solution as that call should not fail, and the branch will be killed off in the rework fail patch,
where it'll be treated like any other error.

As such the only place to get that variable would be IAudioClient::Initialize, which I think would be overkill to #define somewhere,
the rest will just use buffer length as returned by the driver, instead of making a temporary guess.
> Anyway, I gave it a test on ALSA+Pulse and didn't find any issues.
> I'll test the other backends when you resubmit.
>
> Andrew

[PATCH] dsound: rework ugly mixer logic

[Andrew Eikum]

On Mon, Dec 31, 2012 at 07:03:31PM +0100, Maarten Lankhorst wrote:
> Op 31-12-12 17:59, Andrew Eikum schreef:
> > On Sat, Dec 29, 2012 at 01:24:03AM +0100, Maarten Lankhorst wrote:
> > > - if(!maxq){
> > > -
> > > /* nothing to do! */
> > > -
> > > LeaveCriticalSection(&device->mixlock);
> > > -
> > > return;
> > > }
> >
> > This was removed in 8ba4090fc304993. It breaks starting the device in
> > some situations (write primary mode iirc).
>
> Writeprimary dsound tests still worked for me,  I don't particulary care though, if you want I'll zap it.
> After rework fail it should no longer matter..

Zaxxon (see Bug 30836) shows the problem on some, but not all,
systems.  Anyway, PerformMix() does more than just mix and write data,
so returning early only because we have no frames to write is not
always correct.
> > > -
> > > if (native) {
> > > -
> > > 	void *buffer = NULL;
> > > -
> > > -
> > > 	hr = IAudioRenderClient_GetBuffer(device->render, maxq / block, (void*)&buffer);
> > > -
> > > 	if(FAILED(hr)){
> > > -
> > > 		WARN("GetBuffer failed: %08x\n", hr);
> > > -
> > > 		LeaveCriticalSection(&device->mixlock);
> > > -
> > > 		return;
> > > -
> > > 	}
> >
> > I think this (mixing directly to RenderClient and skipping WaveQueue)
> > could be split into a separate patch.
>
> Hate to sound like a broken record here, but the whole mixer logic breaks if you touch that code. :/

Really? It looks like an optimization to me: mix directly into the
RenderClient buffer instead of to the intermediary buffer if the
target format is float. What breaks if we use the intermediary buffer
in every case?
> > >  hres = IAudioClient_Initialize(device->client,
> > >          AUDCLNT_SHAREMODE_SHARED, AUDCLNT_STREAMFLAGS_NOPERSIST |
> > > -
> > >        AUDCLNT_STREAMFLAGS_EVENTCALLBACK, prebuf_rt, 0, device->pwfx, NULL);
> > > -
> > >        AUDCLNT_STREAMFLAGS_EVENTCALLBACK, 800000, 0, device->pwfx, NULL);
> >
> > ...
> > > -
> > >    frames = (UINT64)device->pwfx->nSamplesPerSec * 800000 / 10000000;
> >
> > Could you #define the 800000?
>
> This is a very temporary solution as that call should not fail, and the branch will be killed off in the rework fail patch,
> where it'll be treated like any other error.
>
> As such the only place to get that variable would be IAudioClient::Initialize, which I think would be overkill to #define somewhere,
> the rest will just use buffer length as returned by the driver, instead of making a temporary guess.

Okay, fair enough.

Andrew

[PATCH] dsound: rework ugly mixer logic

[Maarten Lankhorst]

Op 31-12-12 20:01, Andrew Eikum schreef:
> On Mon, Dec 31, 2012 at 07:03:31PM +0100, Maarten Lankhorst wrote:
> > Op 31-12-12 17:59, Andrew Eikum schreef:
> > > On Sat, Dec 29, 2012 at 01:24:03AM +0100, Maarten Lankhorst wrote:
> > > > - if(!maxq){
> > > > -
> > > > /* nothing to do! */
> > > > -
> > > > LeaveCriticalSection(&device->mixlock);
> > > > -
> > > > return;
> > > > }
> > >
> > > This was removed in 8ba4090fc304993. It breaks starting the device in
> > > some situations (write primary mode iirc).
> >
> > Writeprimary dsound tests still worked for me,  I don't particulary care though, if you want I'll zap it.
> > After rework fail it should no longer matter..
>
> Zaxxon (see Bug 30836) shows the problem on some, but not all,
> systems.  Anyway, PerformMix() does more than just mix and write data,
> so returning early only because we have no frames to write is not
> always correct.
> > > > -
> > > > if (native) {
> > > > -
> > > > 	void *buffer = NULL;
> > > > -
> > > > -
> > > > 	hr = IAudioRenderClient_GetBuffer(device->render, maxq / block, (void*)&buffer);
> > > > -
> > > > 	if(FAILED(hr)){
> > > > -
> > > > 		WARN("GetBuffer failed: %08x\n", hr);
> > > > -
> > > > 		LeaveCriticalSection(&device->mixlock);
> > > > -
> > > > 		return;
> > > > -
> > > > 	}
> > >
> > > I think this (mixing directly to RenderClient and skipping WaveQueue)
> > > could be split into a separate patch.
> >
> > Hate to sound like a broken record here, but the whole mixer logic breaks if you touch that code. :/
>
> Really? It looks like an optimization to me: mix directly into the
> RenderClient buffer instead of to the intermediary buffer if the
> target format is float. What breaks if we use the intermediary buffer
> in every case?

I suppose the if native branch could be taken out, but that's not nearly as interesting as the other
changes I was making in that function, and not nearly as likely to mess up anything.

~Maarten