[PATCH v2 2/4] winegstreamer: Implement ::Set(Input/Output)Type for audio conversion transform.
Zebediah Figura (she/her)
zfigura at codeweavers.com
Tue Dec 1 14:10:57 CST 2020
On 11/18/20 2:52 PM, Derek Lesho wrote:
> Signed-off-by: Derek Lesho <dlesho at codeweavers.com>
> ---
> 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;
> }
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0x0D9D358A07A17840.asc
Type: application/pgp-keys
Size: 1769 bytes
Desc: not available
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20201201/6ce14c74/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20201201/6ce14c74/attachment.sig>
More information about the wine-devel
mailing list