Re: [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.