Re: [PATCH 3/3] winecfg: add speaker config controls to audio tab

[Andrew Eikum]

I think you can squash the last two patches. No need to add symbols
before they're used.

Does it make sense to have a "system default" configuration as well?
For example if the user changes which device ALSA's "default" points
to from a 5.1 setup to a stereo setup, the "system default" setting
should report the configurations correctly (or, as correctly as the
backend allows, but you get the point).

This might require storing the user-selected speaker configuration
elsewhere in the registry, and updating the real registry entry on
mmdevapi load.

Couple notes below.

On Mon, Jan 26, 2015 at 10:28:26PM +0000, Mark Harmstone wrote:
> +    info->speaker_config = 999;
> +    if(SUCCEEDED(hr) && pv.vt == VT_UI4){
> +        i = 0;
> +        while (speaker_configs[i].text_id != 0 && info->speaker_config == 999) {
> +            if ((speaker_configs[i].speaker_mask & pv.u.ulVal) == speaker_configs[i].speaker_mask)
> +                info->speaker_config = i;
> +            i++;
> +        }
> +    }

This is kind of awkward. Could be a for-loop and break early.
> +
> +    /* fallback to stereo */
> +    if(info->speaker_config == 999)
> +        info->speaker_config = 2;
> +

Yeah, I don't like 999 either. The "system default" would provide a
reasonable fallback option here, too.
> +    IPropertyStore_Release(ps);
> +
>      return TRUE;
> +    for (i = 0; i < num_render_devs; i++) {
> +        ERR("%s -> %u\n", wine_dbgstr_w(render_devs[i].id), speaker_configs[render_devs[i].speaker_config].speaker_mask);

Looks like a debug line that should be removed or switched to TRACE.
> +        pv.u.ulVal = speaker_configs[render_devs[i].speaker_config].speaker_mask;
> +        WARN("speaker_config = %u\n", pv.u.ulVal);

Another debug line?
> +
> +        hr = IPropertyStore_SetValue(ps, &PKEY_AudioEndpoint_PhysicalSpeakers, &pv);
> +
> +        if (FAILED(hr))
> +            WARN("IPropertyStore_SetValue failed for %s: 0x%08x\n", wine_dbgstr_w(render_devs[i].id), hr);
> +
> +        IPropertyStore_Commit(ps);

This isn't actually implemented in mmdevapi, so we end up with noisy
"stub" messages on the terminal. Best thing to do would be to fix the
property store implementation, but in lieu of that, I'd just switch
the FIXME in mmdevapi with a TRACE.

Andrew