Re: [PATCH 3/4] dsound: Implement capture buffer based on mmdevapi, try 2

[Andrew Eikum]

It would be nice if you used more descriptive variable names. For example:

+struct IDirectSoundCaptureBufferImpl
+{
+    DWORD buf_size;
+    DWORD pos;

What units are these sizes and positions in? Frames? Bytes? Time? What 
position is being represented here, read, write, or something else? 
Using more descriptive names helps when understanding and verifying code 
like this:

+    pos1 = This->pos;
+    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;
+    }
+    else
+        pos2 = pos1;
+    pos2 %= This->buf_size;

Are all of these positions in Bytes? I hope so... (Remember this has 
tripped things up in the past, see bbbf72ddcb). If everything is always 
in bytes, maybe make a note of that near the variable declarations, and 
then verify that it's true throughout the code. Also, why is pos1 a 
separate variable here?

These patches are all missing quite a lot of error handling, which 
really makes me hesitate. Especially on some important calls:

+        IAudioCaptureClient_GetBuffer(buf->cap_dev, &data, &read, 
&flags, &pos, 0);
+    IAudioClient_GetDevicePeriod(This->dev, &default_period, &min_period);
+        IAudioClient_Start(This->dev);

Knowing when these functions fail will help debugging if something goes 
wrong. Much worse to suddenly crash four function calls later due to 
invalid default_period value than to just error out appropriately when 
the error occurs.

This doesn't matter a whole lot, but I think it would be more clear to 
use a separate object here, instead of a separate refcount:

  struct IDirectSoundCaptureBufferImpl
  {
      IDirectSoundCaptureBuffer8 IDirectSoundCaptureBuffer8_iface;
-    LONG ref;
+    IDirectSoundNotify IDirectSoundNotify_iface;
+    LONG ref, notify_ref;

If not, at least a comment explaining why two refcounts are needed. 
Obviously a test would improve it even more, if one doesn't already exist.