[PATCH 4/5] dsound: Add support for OpenAL IDirectSoundCaptureBuffer

[Maarten Lankhorst]

Hi Chris,

Chris Robinson schreef:
> On Monday 30 November 2009 1:56:20 pm Maarten Lankhorst wrote:
>   
>> +    if (This->playing)
>> +    {
>> +        pos2 = This->format->nSamplesPerSec / 100;
>> +        pos2 *= This->format->nBlockAlign;
>> +        pos2 += pos1;
>> +        if (!This->looping && pos2 > This->buf_size)
>> +            pos2 = 0;
>> +        else
>> +            pos2 %= This->buf_size;
>> +    }
>>     
>
> Does non-looping capture let the write position get to This->buf_size? It 
> looks like a position of This->buf_size should set the position to 0 too, if 
> This->buf_size+1 would.
>   
It's handled properly with the modulus.
>> +        if (format->wFormatTag == WAVE_FORMAT_EXTENSIBLE)
>> +        {
>> +            WAVEFORMATEXTENSIBLE *wfe = (WAVEFORMATEXTENSIBLE*)format;
>> +            if (!IsEqualGUID(&wfe->SubFormat,
>> &KSDATAFORMAT_SUBTYPE_IEEE_FLOAT))
>> +                return DSERR_BADFORMAT;
>> +            else if (format->cbSize < sizeof(WAVEFORMATEXTENSIBLE)-
>> sizeof(WAVEFORMATEX))
>> +                return DSERR_INVALIDPARAM;
>> +            else if (format->cbSize > sizeof(WAVEFORMATEXTENSIBLE)-
>> sizeof(WAVEFORMATEX)
>> +                && format->cbSize != sizeof(WAVEFORMATEXTENSIBLE))
>> +                return DSERR_CONTROLUNAVAIL;
>> +        }
>>     
>
> You should probably check the cbSize before accessing the extended 
> information, otherwise it may indicate that there isn't enough space for the 
> extended information. Also, is it necessary to reject formats with a higher 
> cbSize than needed (IIRC, Wine's tests showed some Windows versions allowed 
> it)?
>   
Woops, mild oversight there. cbSize is allowed to be 
sizeof(WAVEFORMATEXTENSIBLE)-sizeof(WAVEFORMATEX), or 
sizeof(WAVEFORMATEXTENSIBLE) (sloppy programming), but the error 
checking is a lot stricter compared to WAVEFORMATEX, so this is correct.
>> +    buf_format = alGetEnumValue(fmt_str);
>>     
>
> Unfortunately, you can't call this function when a context is not set (it'll 
> probably work on some systems, but technically shouldn't). And setting a 
> context makes no sense for capture. Setting buf_format directly to the enum in 
> the switches should be all you need to do.. just make sure to initialize it to 
> 0 so the check won't use it uninitialized.
>   
Theoretically you're right. I'll send an updated version.
>> +    This->dev = alcCaptureOpenDevice(This->parent->device,
>>  This->format->nSamplesPerSec, buf_format, This->buf_size);
>>     
>
> Note that the buffer size given to OpenAL is in sample frames, not bytes. It's 
> not a direct problem here, per se, but it may be an issue if a large size is 
> requested (ie, where n samples would work, but n*2 or n*4 won't)
For bigger buffers we should probably just request up to a 500 ms buffer 
top, but I suppose there's no harm in wasting some extra space, as long 
as it's not excessive. I'll call it with buf_size / nBlockAlign for now, 
if an application requests a huge buffer and the code breaks, I will 
revisit this again. :)

Cheers,
Maarten.