Re: [PATCH v2 3/4] winegstreamer: Implement ::Process(Input/Output) 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 | 202 +++++++++++++++++++++++++++++-
>  dlls/winegstreamer/gst_private.h  |   1 +
>  dlls/winegstreamer/mfplat.c       |  75 +++++++++++
>  3 files changed, 273 insertions(+), 5 deletions(-)
>
> diff --git a/dlls/winegstreamer/audioconvert.c b/dlls/winegstreamer/audioconvert.c
> index 04227d168ae..deb5ceb2f7c 100644
> --- a/dlls/winegstreamer/audioconvert.c
> +++ b/dlls/winegstreamer/audioconvert.c
> @@ -24,7 +24,8 @@ struct audio_converter
>      IMFMediaType *input_type;
>      IMFMediaType *output_type;
>      CRITICAL_SECTION cs;
> - BOOL valid_state;
> - BOOL valid_state, inflight;
> - GstElement *container, *appsrc, *audioconvert, *resampler, *appsink;
>
> };
>
> static struct audio_converter *impl_audio_converter_from_IMFTransform(IMFTransform *iface)
> @@ -69,6 +70,7 @@ static ULONG WINAPI audio_converter_Release(IMFTransform *iface)
>      if (!refcount)
>      {
>          DeleteCriticalSection(&transform->cs);
> -
>    gst_object_unref(transform->container);
>    heap_free(transform);
> }
>
> @@ -261,22 +263,54 @@ static HRESULT WINAPI audio_converter_GetOutputAvailableType(IMFTransform *iface
>  static void audio_converter_update_pipeline_state(struct audio_converter *converter)
>  {
>      GstCaps *input_caps, *output_caps;
> -
>
> GstStructure *input_structure, *output_structure;
> -
>
> guint64 channels_in, channels_out, channel_mask = 0;
>
> if (!(converter->valid_state = converter->input_type && converter->output_type))
> -
>
> {
> -
>    gst_element_set_state(converter->container, GST_STATE_READY);
>    return;
> -
>
> }
>
> if (!(input_caps = caps_from_mf_media_type(converter->input_type)))
> {
>     converter->valid_state = FALSE;
> -
>    gst_element_set_state(converter->container, GST_STATE_READY);
>    return;
>
> }
>    if (!(output_caps = caps_from_mf_media_type(converter->output_type)))
>    {
>        converter->valid_state = FALSE;
> -
>    gst_element_set_state(converter->container, GST_STATE_READY);
> -
>    gst_caps_unref(input_caps);
> -
>    return;
> -
>
> }
> -
> -
>
> /* audioconvert needs a valid channel-mask */

Well, that's not quite true, at least from my reading of the source, but
it does need a valid channel mask for configurations other than mono or
stereo.

The code below is confusing, though; I'm not sure why channel-mask would
be present in the caps at all.

Note that a simpler way to set the channel mask is to use
gst_audio_info_to_caps() in caps_from_mf_media_type().
> -
>
> input_structure = gst_caps_get_structure(input_caps, 0);
> -
>
> output_structure = gst_caps_get_structure(output_caps, 0);
> -
> -
>
> if (!gst_structure_get(input_structure, "channels", G_TYPE_INT, &channels_in, NULL) ||
> -
>    !gst_structure_get(output_structure, "channels", G_TYPE_INT, &channels_out, NULL))
> -
>
> {
> -
>    converter->valid_state = FALSE;
> -
>    gst_element_set_state(converter->container, GST_STATE_READY);
>    gst_caps_unref(input_caps);
> -
>    gst_caps_unref(output_caps);
>    return;
>
> }
> -
>
> gst_structure_get(input_structure, "channel-mask", GST_TYPE_BITMASK, &channel_mask, NULL);
> -
>
> if (channel_mask == 0)
> -
>    gst_caps_set_simple(input_caps, "channel-mask", GST_TYPE_BITMASK, (guint64) (1 << channels_in) - 1, NULL);
> -
> -
>
> gst_structure_get(output_structure, "channel-mask", GST_TYPE_BITMASK, &channel_mask, NULL);
> -
>
> if (channel_mask == 0)
> -
>    gst_caps_set_simple(output_caps, "channel-mask", GST_TYPE_BITMASK, (guint64) (1 << channels_out) - 1, NULL);
> -
> -
>
> g_object_set(converter->appsrc, "caps", input_caps, NULL);
> -
>
> g_object_set(converter->appsink, "caps", output_caps, NULL);
> -
>
> if (TRACE_ON(mfplat))
>    {
>        gchar *input_caps_str, *output_caps_str;
>
> @@ -293,6 +327,7 @@ static void audio_converter_update_pipeline_state(struct audio_converter *conver
>  gst_caps_unref(input_caps);
>  gst_caps_unref(output_caps);
> -
>
> gst_element_set_state(converter->container, GST_STATE_PLAYING);
>
> return;
>
> }
> @@ -496,17 +531,114 @@ static HRESULT WINAPI audio_converter_ProcessMessage(IMFTransform *iface, MFT_ME
>
> static HRESULT WINAPI audio_converter_ProcessInput(IMFTransform *iface, DWORD id, IMFSample *sample, DWORD flags)
>  {
> - FIXME("%p, %u, %p, %#x.\n", iface, id, sample, flags);
> - struct audio_converter *converter = impl_audio_converter_from_IMFTransform(iface);
> - GstBuffer *gst_buffer;
> - HRESULT hr = S_OK;
> - int ret;
> - return E_NOTIMPL;
> - TRACE("%p, %u, %p, %#x.\n", iface, id, sample, flags);
> -
> - if (flags)
> -
>    WARN("Unsupported flags %#x\n", flags);
> -
> - if (id != 0)
> -
>    return MF_E_INVALIDSTREAMNUMBER;
> -
> - EnterCriticalSection(&converter->cs);
> -
> - if (!converter->valid_state)
> - {
> -
>    hr = MF_E_TRANSFORM_TYPE_NOT_SET;
> -
>    goto done;
> - }
> -
> - if (converter->inflight)
> - {
> -
>    hr = MF_E_NOTACCEPTING;
> -
>    goto done;
> - }
> -
> - if (!(gst_buffer = gst_buffer_from_mf_sample(sample)))

These parentheses are redundant (and I don't think particularly
clarifying). The same applies to many other cases in this patch.
> - {
> -
>    hr = E_FAIL;
> -
>    goto done;
> - }
> -
> - g_signal_emit_by_name(converter->appsrc, "push-buffer", gst_buffer, &ret);
> - gst_buffer_unref(gst_buffer);
> - if (ret != GST_FLOW_OK)
> - {
> -
>    ERR("Couldn't push buffer ret = %d\n", ret);

gst_flow_get_name() may be a good idea here.
> -
>    hr = E_FAIL;
> -
>    goto done;
> - }

None of these gotos are actually saving you any lines.
> -
> - converter->inflight = TRUE;
> -
> - done:
> - LeaveCriticalSection(&converter->cs);
> -
> - return hr;
>
> }
>
> static HRESULT WINAPI audio_converter_ProcessOutput(IMFTransform *iface, DWORD flags, DWORD count,
>          MFT_OUTPUT_DATA_BUFFER *samples, DWORD *status)
>  {
> - FIXME("%p, %#x, %u, %p, %p.\n", iface, flags, count, samples, status);
> - struct audio_converter *converter = impl_audio_converter_from_IMFTransform(iface);
> - MFT_OUTPUT_DATA_BUFFER *relevant_buffer = NULL;
> - GstSample *sample;
> - HRESULT hr = S_OK;
> - unsigned int i;
> - return E_NOTIMPL;
> - TRACE("%p, %#x, %u, %p, %p.\n", iface, flags, count, samples, status);
> -
> - if (flags)
> -
>    WARN("Unsupported flags %#x\n", flags);
> -
> - for (i = 0; i < count; i++)
> - {
> -
>    MFT_OUTPUT_DATA_BUFFER *out_buffer = &samples[i];
> -
> -
>    if (out_buffer->dwStreamID != 0)
> -
>        return MF_E_INVALIDSTREAMNUMBER;
> -
> -
>    if (relevant_buffer)
> -
>        return MF_E_INVALIDSTREAMNUMBER;
> -
> -
>    relevant_buffer = out_buffer;
> - }
> -
> - if (!relevant_buffer)
> -
>    return S_OK;

This code is very strange. Isn't it easier just to validate "count ==
1"? And is it really correct to return MF_E_INVALIDSTREAMNUMBER if more
than one sample is passed?
> -
> - EnterCriticalSection(&converter->cs);
> -
> - if (!converter->valid_state)
> - {
> -
>    hr = MF_E_TRANSFORM_TYPE_NOT_SET;
> -
>    goto done;
> - }
> -
> - if (!converter->inflight)
> - {
> -
>    hr = MF_E_TRANSFORM_NEED_MORE_INPUT;
> -
>    goto done;
> - }

More gotos that don't save you anything.
> -
> - g_signal_emit_by_name(converter->appsink, "pull-sample", &sample);
> -
> - converter->inflight =  FALSE;
> -
> - relevant_buffer->pSample = mf_sample_from_gst_buffer(gst_sample_get_buffer(sample));
> - gst_sample_unref(sample);
> - relevant_buffer->dwStatus = S_OK;
> - relevant_buffer->pEvents = NULL;
> - *status = 0;
> -
> - done:
> - LeaveCriticalSection(&converter->cs);
> -
> - return hr;
>
> }
>
> static const IMFTransformVtbl audio_converter_vtbl =
> @@ -542,6 +674,7 @@ static const IMFTransformVtbl audio_converter_vtbl =
>  HRESULT audio_converter_create(REFIID riid, void **ret)
>  {
>      struct audio_converter *object;
> -
>
> HRESULT hr;
>
> TRACE("%s %p\n", debugstr_guid(riid), ret);
>
> @@ -553,6 +686,65 @@ HRESULT audio_converter_create(REFIID riid, void **ret)
>  InitializeCriticalSection(&object->cs);
>
> - object->container = gst_bin_new(NULL);
> -
> - if (!(object->appsrc = gst_element_factory_make("appsrc", NULL)))
> - {
> -
>    ERR("Failed to create appsrc");

I'd recommend including a message here about possibly missing
gst-plugins-base (and similarly below), since it's a very common mistake
for users to make.
> -
>    hr = E_FAIL;
> -
>    goto failed;
> - }
> - gst_bin_add(GST_BIN(object->container), object->appsrc);
> -
> - if (!(object->audioconvert = gst_element_factory_make("audioconvert", NULL)))
> - {
> -
>    ERR("Failed to create converter\n");
> -
>    hr = E_FAIL;
> -
>    goto failed;
> - }
> - gst_bin_add(GST_BIN(object->container), object->audioconvert);
> -
> - if (!(object->resampler = gst_element_factory_make("audioresample", NULL)))
> - {
> -
>    ERR("Failed to create resampler\n");
> -
>    hr = E_FAIL;
> -
>    goto failed;
> - }
> - gst_bin_add(GST_BIN(object->container), object->resampler);
> -
> - if (!(object->appsink = gst_element_factory_make("appsink", NULL)))
> - {
> -
>    ERR("Failed to create appsink\n");
> -
>    hr = E_FAIL;
> -
>    goto failed;
> - }
> - gst_bin_add(GST_BIN(object->container), object->appsink);
> -
> - if (!(gst_element_link(object->appsrc, object->audioconvert)))
> - {
> -
>    ERR("Failed to link appsrc to audioconvert\n");
> -
>    hr = E_FAIL;
> -
>    goto failed;
> - }
> -
> - if (!(gst_element_link(object->audioconvert, object->resampler)))
> - {
> -
>    ERR("Failed to link audioconvert to resampler\n");
> -
>    hr = E_FAIL;
> -
>    goto failed;
> - }
> -
> - if (!(gst_element_link(object->resampler, object->appsink)))
> - {
> -
>    ERR("Failed to link resampler to appsink\n");
> -
>    hr = E_FAIL;
> -
>    goto failed;
> - }
> -

More gotos that don't save you anything.
>  *ret = &object->IMFTransform_iface;
>  return S_OK;
>
> +failed:
> -
> - IMFTransform_Release(&object->IMFTransform_iface);
> - return hr;
>
> }
> diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h
> index 7889c996204..7f96c06dfaf 100644
> --- a/dlls/winegstreamer/gst_private.h
> +++ b/dlls/winegstreamer/gst_private.h
> @@ -81,6 +81,7 @@ HRESULT winegstreamer_stream_handler_create(REFIID riid, void **obj) DECLSPEC_HI
>  IMFMediaType *mf_media_type_from_caps(const GstCaps *caps) DECLSPEC_HIDDEN;
>  GstCaps *caps_from_mf_media_type(IMFMediaType *type) DECLSPEC_HIDDEN;
>  IMFSample *mf_sample_from_gst_buffer(GstBuffer *in) DECLSPEC_HIDDEN;
> +GstBuffer *gst_buffer_from_mf_sample(IMFSample *in) DECLSPEC_HIDDEN;
>
> HRESULT winegstreamer_stream_handler_create(REFIID riid, void **obj) DECLSPEC_HIDDEN;
>
> diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c
> index 909fb8b3572..7fdf722ec2e 100644
> --- a/dlls/winegstreamer/mfplat.c
> +++ b/dlls/winegstreamer/mfplat.c
> @@ -877,3 +877,78 @@ done:
>  return out;
>
> }
> -
>
> +GstBuffer* gst_buffer_from_mf_sample(IMFSample *mf_sample)
> +{
> - GstBuffer *out = gst_buffer_new();
> - IMFMediaBuffer *mf_buffer = NULL;
> - LONGLONG duration, time;
> - DWORD buffer_count;
> - unsigned int i;
> - HRESULT hr;
> -
> - if (FAILED(hr = IMFSample_GetSampleDuration(mf_sample, &duration)))
> -
>    goto fail;
> -
> - if (FAILED(hr = IMFSample_GetSampleTime(mf_sample, &time)))
> -
>    goto fail;
> -
> - GST_BUFFER_DURATION(out) = duration;
> - GST_BUFFER_PTS(out) = time * 100;
> -
> - if (FAILED(hr = IMFSample_GetBufferCount(mf_sample, &buffer_count)))
> -
>    goto fail;
> -
> - for (i = 0; i < buffer_count; i++)
> - {
> -
>    DWORD buffer_max_size, buffer_size;
> -
>    GstMapInfo map_info;
> -
>    GstMemory *memory;
> -
>    BYTE *buf_data;
> -
> -
>    if (FAILED(hr = IMFSample_GetBufferByIndex(mf_sample, i, &mf_buffer)))
> -
>        goto fail;
> -
> -
>    if (FAILED(hr = IMFMediaBuffer_GetMaxLength(mf_buffer, &buffer_max_size)))
> -
>        goto fail;

This value is unused.
> -
> -
>    if (FAILED(hr = IMFMediaBuffer_GetCurrentLength(mf_buffer, &buffer_size)))
> -
>        goto fail;
> -
> -
>    memory = gst_allocator_alloc(NULL, buffer_size, NULL);
> -
>    gst_memory_resize(memory, 0, buffer_size);
> -
> -
>    if (!(gst_memory_map(memory, &map_info, GST_MAP_WRITE)))
> -
>    {
> -
>        hr = E_FAIL;
> -
>        goto fail;
> -
>    }
> -
> -
>    if (FAILED(hr = IMFMediaBuffer_Lock(mf_buffer, &buf_data, NULL, NULL)))
> -
>        goto fail;
> -
> -
>    memcpy(map_info.data, buf_data, buffer_size);
> -
> -
>    if (FAILED(hr = IMFMediaBuffer_Unlock(mf_buffer)))
> -
>        goto fail;
> -
> -
>    if (FAILED(hr = IMFMediaBuffer_SetCurrentLength(mf_buffer, buffer_size)))
> -
>        goto fail;

This doesn't look like it belongs here.
> -
> -
>    gst_memory_unmap(memory, &map_info);
> -
> -
>    gst_buffer_append_memory(out, memory);
> -
> -
>    IMFMediaBuffer_Release(mf_buffer);
> -
>    mf_buffer = NULL;
> - }
> -
> - return out;
> -
>
> +fail:
> - ERR("Failed to copy IMFSample to GstBuffer, hr = %#x\n", hr);
> - if (mf_buffer)
> -
>    IMFMediaBuffer_Release(mf_buffer);
> - gst_buffer_unref(out);
> - return NULL;
>
> +}