Re: [PATCH 4/6] dsound: Use event based threads
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
Re: [PATCH 4/6] dsound: Use event based threads
Op 19-10-12 15:40, Andrew Eikum schreef:
> I like it. Before I can give a sign-off, I need to run it through my
> usual battery of tests on all the platforms. I'll do that after patch
> 3 gets figured out.
>
> Some thoughts below...
>
> On Tue, Oct 16, 2012 at 02:06:28PM +0200, Maarten Lankhorst wrote:
>> +/* ALSA is retarded, add a timeout.. */
>> +ret = WaitForSingleObject(dev->sleepev, dev->sleeptime);
> No arguments here :) but I'm curious what you mean. Presumably
> winealsa isn't sending events when it ought to? I know Jorg has
> discovered that failure, I wonder if it's worth testing and fixing
> across all the drivers.
Well I was abusing the fact that native drivers continue firing after being
stopped, I have
a followup patch which leaves IAudioClient always running unless you enter
primary mode
instead.
>> diff --git a/dlls/dsound/primary.c b/dlls/dsound/primary.c
>> index 94bdf9c..9d9fa27 100644
>> --- a/dlls/dsound/primary.c
>> +++ b/dlls/dsound/primary.c
>> @@ -170,6 +172,7 @@ HRESULT DSOUND_ReopenDevice(DirectSoundDevice *device,
>> BOOL forcewave)
>> device->pwfx->wFormatTag = WAVE_FORMAT_PCM;
>> device->pwfx->cbSize = 0;
>> }
>> +IAudioClient_SetEventHandle(device->client, device->sleepev);
>>
>> hres = IAudioClient_GetService(device->client, &IID_IAudioRenderClient,
>> (void**)&device->render);
>> @@ -203,6 +206,19 @@ HRESULT DSOUND_ReopenDevice(DirectSoundDevice *device,
>> BOOL forcewave)
>> WARN("GetService failed: %08x\n", hres);
>> return hres;
>> }
>> +/* Now kick off the timer so the event fires periodically */
>> +IAudioClient_Start(device->client);
>> +
>> +IAudioClient_GetStreamLatency(device->client, &period);
> 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..
I do have a followup patch for this too though but I was going to see if this
would go through
first before checking if it is worth spending more time fixing things up.
~Maarten
Re: [PATCH 4/6] dsound: Use event based threads
I like it. Before I can give a sign-off, I need to run it through my
usual battery of tests on all the platforms. I'll do that after patch
3 gets figured out.
Some thoughts below...
On Tue, Oct 16, 2012 at 02:06:28PM +0200, Maarten Lankhorst wrote:
> + /* ALSA is retarded, add a timeout.. */
> + ret = WaitForSingleObject(dev->sleepev, dev->sleeptime);
No arguments here :) but I'm curious what you mean. Presumably
winealsa isn't sending events when it ought to? I know Jorg has
discovered that failure, I wonder if it's worth testing and fixing
across all the drivers.
> diff --git a/dlls/dsound/primary.c b/dlls/dsound/primary.c
> index 94bdf9c..9d9fa27 100644
> --- a/dlls/dsound/primary.c
> +++ b/dlls/dsound/primary.c
> @@ -170,6 +172,7 @@ HRESULT DSOUND_ReopenDevice(DirectSoundDevice *device,
> BOOL forcewave)
> device->pwfx->wFormatTag = WAVE_FORMAT_PCM;
> device->pwfx->cbSize = 0;
> }
> +IAudioClient_SetEventHandle(device->client, device->sleepev);
>
> hres = IAudioClient_GetService(device->client, &IID_IAudioRenderClient,
> (void**)&device->render);
> @@ -203,6 +206,19 @@ HRESULT DSOUND_ReopenDevice(DirectSoundDevice *device,
> BOOL forcewave)
> WARN("GetService failed: %08x\n", hres);
> return hres;
> }
> +/* Now kick off the timer so the event fires periodically */
> +IAudioClient_Start(device->client);
> +
> +IAudioClient_GetStreamLatency(device->client, &period);
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.
Andrew
