[PATCH 3/5] winegstreamer: Implement WMA decoder ProcessInput.

[Zebediah Figura (she/her)]

Much of this patch doesn't make sense without 4/5. Could the two patches
be squashed together?



On 2/22/22 16:48, Rémi Bernon wrote:
> +    if (FAILED(hr = IMFSample_GetBufferCount(sample, &count)) || count != 1)
> +    {
> +        FIXME("Only samples with 1 buffer are supported!\n");
> +        return hr ? hr : E_FAIL;
> +    }

This made me do a double take. Since these conditions don't really share
their bodies, perhaps they should be separated?

> +void mf_destroy_wg_sample(struct wg_sample *sample)
> +{
> +    IMFMediaBuffer *media_buffer;
> +    HRESULT hr;
> +
> +    if (FAILED(hr = IMFSample_GetBufferByIndex(sample->private, 0, &media_buffer)))
> +        WARN("Failed to get first buffer, sample %p\n", sample->private);
> +    else
> +    {
> +        /* release lock and ref taken in mf_create_wg_sample */
> +        IMFMediaBuffer_Unlock(media_buffer);
> +        IMFMediaBuffer_SetCurrentLength(media_buffer, sample->size);
> +        IMFMediaBuffer_Release(media_buffer);

Why are we holding a reference to the media buffer? (And if we need to,
should we be storing that instead of the IMFSample?)

> +
> +        IMFMediaBuffer_Release(media_buffer);
> +    }
> +
> +    IMFSample_Release(sample->private);
> +    free(sample);
> +}
> diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h
> index 4adbb694766..59b15df7d08 100644
> --- a/dlls/winegstreamer/unixlib.h
> +++ b/dlls/winegstreamer/unixlib.h
> @@ -103,6 +103,14 @@ struct wg_format
>      } u;
>  };
>  
> +struct wg_sample
> +{
> +    UINT32 max_size;
> +    UINT32 size;
> +    BYTE *data;
> +    void *private;
> +};

"max_size" isn't relevant until patch 5/5.

"private" also doesn't particularly seem like it belongs here. In
particular, if you're going to wrap the wg_sample in another structure
in patch 5/5, I'd rather see the same approach taken for
frontend-specific data.

> +    if (FAILED(hr = IMFSample_ConvertToContiguousBuffer(sample, &media_buffer)))
> +        return hr;
> +    IMFMediaBuffer_Release(media_buffer);

This seems pointless (and hence, at least, doesn't belong in this
patch). Or does ConvertToContiguousBuffer() also modify the original sample?

> +    if (!(wg_sample->size = (wg_sample->size / info.cbSize) * info.cbSize))
> +    {
> +        mf_destroy_wg_sample(wg_sample);
> +        return S_OK;
> +    }

I can't tell if this is trying to align to info.cbSize or (as the
documentation specifies) treat it as a minimum, but either way I think
it's broken.

> +
> +    decoder->input_sample = wg_sample;

Do we have to store this for API compatibility? If so it would seem
helpful to specify that in a comment. It looks wrong as-is, since we
don't need the data after we copy it.

(And depending on how hard we need to preserve API compatibility in this
area, I'm concerned that patch 5/5 may be making assumptions about how
GStreamer uses buffers that aren't actually guaranteed.)
-------------- 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/20220222/d046b679/attachment.sig>