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

[Chris Robinson]

Hi.

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.
> +        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)?
> +    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.
> +    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).

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