Re: [PATCH v2 2/4] winegstreamer: Implement ::Set(Input/Output)Type for audio conversion transform.

[Zebediah Figura (she/her)]

On 11/18/20 2:52 PM, Derek Lesho wrote:
> Signed-off-by: Derek Lesho
> v2: Fix intermediate compile errors.
>
> dlls/winegstreamer/audioconvert.c | 190 +++++++++++++++++++++++++++++-
>  1 file changed, 185 insertions(+), 5 deletions(-)

As similar as the implementations of these two functions are, splitting
the patch probably wouldn't be a bad thing.
> diff --git a/dlls/winegstreamer/audioconvert.c b/dlls/winegstreamer/audioconvert.c
> index 91fa556cb88..04227d168ae 100644
> --- a/dlls/winegstreamer/audioconvert.c
> +++ b/dlls/winegstreamer/audioconvert.c
> @@ -1,4 +1,5 @@
>  #include "config.h"
> +#include <gst/gst.h>
>
> #include "gst_private.h"
>
> @@ -20,6 +21,10 @@ struct audio_converter
>  {
>      IMFTransform IMFTransform_iface;
>      LONG refcount;
> - IMFMediaType *input_type;
> - IMFMediaType *output_type;
> - CRITICAL_SECTION cs;
> - BOOL valid_state;

This field is unused in this patch, but is there a reason its uses can't
be replaced with "converter->input_type && converter->output_type"?
> };
>
> static struct audio_converter *impl_audio_converter_from_IMFTransform(IMFTransform *iface)
> @@ -63,6 +68,7 @@ static ULONG WINAPI audio_converter_Release(IMFTransform *iface)
>  if (!refcount)
>  {
> -
>    DeleteCriticalSection(&transform->cs);
>    heap_free(transform);
> }
>
> @@ -252,18 +258,191 @@ static HRESULT WINAPI audio_converter_GetOutputAvailableType(IMFTransform *iface
>      return hr;
>  }
>
> +static void audio_converter_update_pipeline_state(struct audio_converter *converter)
> +{
> - GstCaps *input_caps, *output_caps;
> -
> - if (!(converter->valid_state = converter->input_type && converter->output_type))
> -
>    return;
> -
> - if (!(input_caps = caps_from_mf_media_type(converter->input_type)))
> - {
> -
>    converter->valid_state = FALSE;
> -
>    return;
> - }
> - if (!(output_caps = caps_from_mf_media_type(converter->output_type)))
> - {
> -
>    converter->valid_state = FALSE;
> -
>    gst_caps_unref(input_caps);
> -
>    return;
> - }
> -
> - if (TRACE_ON(mfplat))
> - {
> -
>    gchar *input_caps_str, *output_caps_str;
> -
> -
>    input_caps_str = gst_caps_to_string(input_caps);
> -
>    output_caps_str = gst_caps_to_string(output_caps);
> -
> -
>    TRACE("Audio converter MFT configured to transform caps %s to caps %s\n",
> -
>        debugstr_a(input_caps_str), debugstr_a(output_caps_str));
> -
> -
>    g_free(input_caps_str);
> -
>    g_free(output_caps_str);
> - }
> -
> - gst_caps_unref(input_caps);
> - gst_caps_unref(output_caps);
> -
> - return;
>
> +}
> -

This function doesn't really do anything but trace; I'm not sure it
makes sense in this patch.
> static HRESULT WINAPI audio_converter_SetInputType(IMFTransform *iface, DWORD id, IMFMediaType *type, DWORD flags)
>  {
> - FIXME("%p, %u, %p, %#x.\n", iface, id, type, flags);
> - unsigned int i;
> - HRESULT hr;
> - return E_NOTIMPL;
> - struct audio_converter *converter = impl_audio_converter_from_IMFTransform(iface);
> -
> - TRACE("%p, %u, %p, %#x.\n", iface, id, type, flags);
> -
> - if (id != 0)
> -
>    return MF_E_INVALIDSTREAMNUMBER;
> -
> - if (type)
> - {
> -
>    GUID major_type, subtype;
> -
>    BOOL found = FALSE;
> -
>    DWORD unused;
> -
> -
>    if (FAILED(IMFMediaType_GetGUID(type, &MF_MT_MAJOR_TYPE, &major_type)))
> -
>        return MF_E_INVALIDTYPE;
> -
>    if (FAILED(IMFMediaType_GetGUID(type, &MF_MT_SUBTYPE, &subtype)))
> -
>        return MF_E_INVALIDTYPE;
> -
>    if (FAILED(IMFMediaType_GetUINT32(type, &MF_MT_AUDIO_SAMPLES_PER_SECOND, &unused)))
> -
>        return MF_E_INVALIDTYPE;
> -
>    if (FAILED(IMFMediaType_GetUINT32(type, &MF_MT_AUDIO_NUM_CHANNELS, &unused)))
> -
>        return MF_E_INVALIDTYPE;
> -
>    if (IsEqualGUID(&subtype, &MFAudioFormat_PCM) && FAILED(IMFMediaType_GetUINT32(type, &MF_MT_AUDIO_BITS_PER_SAMPLE, &unused)))
> -
>        return MF_E_INVALIDTYPE;
> -
> -
>    if (!(IsEqualGUID(&major_type, &MFMediaType_Audio)))
> -
>        return MF_E_INVALIDTYPE;
> -
> -
>    for (i = 0; i < ARRAY_SIZE(raw_types); i++)
> -
>    {
> -
>        if (IsEqualGUID(&subtype, raw_types[i]))
> -
>        {
> -
>            found = TRUE;
> -
>            break;
> -
>        }
> -
>    }

I guess I didn't notice this in patch 1/4, but raw_types seems a bit
overkill when only two types are used (and it's not clear to me that
more will be needed).
> -
> -
>    if (!found)
> -
>        return MF_E_INVALIDTYPE;
> - }
> -
> - if (flags & MFT_SET_TYPE_TEST_ONLY)
> -
>    return S_OK;

Wait, so what happens if MFT_SET_TYPE_TEST_ONLY is used with a NULL type?
> -
> - hr = S_OK;
> -
> - EnterCriticalSection(&converter->cs);
> -
> - if (type)
> - {
> -
>    if (!converter->input_type)
> -
>        if (FAILED(hr = MFCreateMediaType(&converter->input_type)))
> -
>            goto done;
> -
> -
>    if (FAILED(hr = IMFMediaType_CopyAllItems(type, (IMFAttributes *) converter->input_type)))
> -
>        goto done;

This "goto" is redundant. The former could potentially be made redundant
as well.
> - }
> - else if (converter->input_type)
> - {
> -
>    IMFMediaType_Release(converter->input_type);
> -
>    converter->input_type = NULL;
> - }
> -
> - done:
> - if (hr == S_OK)
> -
>    audio_converter_update_pipeline_state(converter);
> - LeaveCriticalSection(&converter->cs);
> -
> - return S_OK;
>
> }
>
> static HRESULT WINAPI audio_converter_SetOutputType(IMFTransform *iface, DWORD id, IMFMediaType *type, DWORD flags)
>  {
> - FIXME("%p, %u, %p, %#x.\n", iface, id, type, flags);
> - struct audio_converter *converter = impl_audio_converter_from_IMFTransform(iface);
> - GUID major_type, subtype;
> - unsigned int i;
> - DWORD unused;
> - HRESULT hr;
> - return E_NOTIMPL;
> - TRACE("%p, %u, %p, %#x.\n", iface, id, type, flags);
> -
> - if (id != 0)
> -
>    return MF_E_INVALIDSTREAMNUMBER;
> -
> - if (!converter->input_type)
> -
>    return MF_E_TRANSFORM_TYPE_NOT_SET;
> -
> - if (type)
> - {
> -
>    /* validate the type */
> -
> -
>    if (FAILED(IMFMediaType_GetGUID(type, &MF_MT_MAJOR_TYPE, &major_type)))
> -
>        return MF_E_INVALIDTYPE;
> -
>    if (FAILED(IMFMediaType_GetGUID(type, &MF_MT_SUBTYPE, &subtype)))
> -
>        return MF_E_INVALIDTYPE;
> -
>    if (FAILED(IMFMediaType_GetUINT32(type, &MF_MT_AUDIO_NUM_CHANNELS, &unused)))
> -
>        return MF_E_INVALIDTYPE;
> -
>    if (IsEqualGUID(&subtype, &MFAudioFormat_PCM) && FAILED(IMFMediaType_GetUINT32(type, &MF_MT_AUDIO_BITS_PER_SAMPLE, &unused)))
> -
>        return MF_E_INVALIDTYPE;
> -
>    if (FAILED(IMFMediaType_GetUINT32(type, &MF_MT_AUDIO_SAMPLES_PER_SECOND, &unused)))
> -
>        return MF_E_INVALIDTYPE;
> -
> -
>    if (!(IsEqualGUID(&major_type, &MFMediaType_Audio)))
> -
>        return MF_E_INVALIDTYPE;
> -
> -
>    for (i = 0; i < ARRAY_SIZE(raw_types); i++)
> -
>    {
> -
>        if (IsEqualGUID(&subtype, raw_types[i]))
> -
>            break;
> -
>        if (i == ARRAY_SIZE(raw_types))
> -
>            return MF_E_INVALIDTYPE;
> -
>    }
> - }
> -
> - if (flags & MFT_SET_TYPE_TEST_ONLY)
> -
>    return S_OK;
> -
> - EnterCriticalSection(&converter->cs);
> -
> - hr = S_OK;
> -
> - if (type)
> - {
> -
>    if (!converter->output_type)
> -
>        if (FAILED(hr = MFCreateMediaType(&converter->output_type)))
> -
>            goto done;
> -
> -
>    if (FAILED(hr = IMFMediaType_CopyAllItems(type, (IMFAttributes *) converter->output_type)))
> -
>        goto done;
> - }
> - else if (converter->output_type)
> - {
> -
>    IMFMediaType_Release(converter->output_type);
> -
>    converter->output_type = NULL;
> - }
> -
> - done:
> - if (hr == S_OK)
> -
>    audio_converter_update_pipeline_state(converter);
> - LeaveCriticalSection(&converter->cs);
> -
> - return hr;
>
> }

Most of the above comments apply here as well.
> static HRESULT WINAPI audio_converter_GetInputCurrentType(IMFTransform *iface, DWORD id, IMFMediaType **type)
> @@ -363,7 +542,6 @@ static const IMFTransformVtbl audio_converter_vtbl =
>  HRESULT audio_converter_create(REFIID riid, void **ret)
>  {
>      struct audio_converter *object;
> - HRESULT hr;

This should be part of the previous patch.
>  TRACE("%s %p\n", debugstr_guid(riid), ret);
>
> @@ -373,6 +551,8 @@ HRESULT audio_converter_create(REFIID riid, void **ret)
>      object->IMFTransform_iface.lpVtbl = &audio_converter_vtbl;
>      object->refcount = 1;
> - InitializeCriticalSection(&object->cs);
> -

Can you please add debug info to the CS?
>  *ret = &object->IMFTransform_iface;
>  return S_OK;
>
> }