Re: [PATCH 3/5] winegstreamer: Support dynamic wg_transform output format change.

[Zebediah Figura (she/her)]

On 5/2/22 16:24, Rémi Bernon wrote:
> From: Rémi Bernon
>
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=45988
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=47084
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=49715
> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52183
> Signed-off-by: Rémi Bernon
>
> dlls/mf/tests/mf.c                |  4 --
>   dlls/winegstreamer/h264_decoder.c |  8 +++
>   dlls/winegstreamer/unixlib.h      |  1 +
>   dlls/winegstreamer/wg_transform.c | 84 ++++++++++++++++++++++++-------
>   4 files changed, 74 insertions(+), 23 deletions(-)

This commit is difficult to review, because it does several things at 
once. At least it'd be nice to split it up, along the lines of:

(1) queue GstSample objects instead of GstBuffer

(2) compare format pointers and report when the format changes
> diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h
> index f4e2ea4966b..7a31020fb87 100644
> --- a/dlls/winegstreamer/unixlib.h
> +++ b/dlls/winegstreamer/unixlib.h
> @@ -122,6 +122,7 @@ struct wg_sample
>      UINT32 max_size;
>      UINT32 size;
>      BYTE *data;
> - struct wg_format *format;
>
> };
>
> struct wg_parser_buffer

I was doubtful about introducing this abstraction in the first place, 
before we've actually implemented zero-copy, and indeed I'm finding it 
difficult to reason around...

Anyway, it's probably not worth restructuring things at this rate, but I 
have a hard time feeling like "format" belongs here. It's an in-out 
parameter, and as an "out" parameter it is a property of the buffer, but 
as an "in" parameter it isn't, really.

Actually, for that matter, why do we need to store the wg_format on the 
PE side? Can't we just store it on the unix side?

(Maybe even check it in the chain function instead of in read_data, and 
store it in a flag on the object with gst_mini_object_set_qdata(), and 
then that'd remove the need to allocate GstSample objects. I don't 
remember if there was another impetus for using those?)
> diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c
> index 58eb1286401..e0b0ce77a44 100644
> --- a/dlls/winegstreamer/wg_transform.c
> +++ b/dlls/winegstreamer/wg_transform.c
> @@ -51,40 +51,73 @@ struct wg_transform
>       GstBufferList *input;
>       guint input_max_length;
>       GstAtomicQueue *output_queue;
> - GstBuffer *output_buffer;
> - GstSample *output_sample;
> - GstCaps *sink_caps;

I find "sink_caps" misleading; I expect "sink" to refer to the transform 
as a whole, or (somewhat by extension) the GStreamer bin it's wrapping. 
Hence "source" seems more correct, or (to match with the other members) 
"output".
> };
>
> static GstFlowReturn transform_sink_chain_cb(GstPad *pad, GstObject *parent, GstBuffer *buffer)
>   {
>       struct wg_transform *transform = gst_pad_get_element_private(pad);
> -
>
> GstSample *sample;
>
> GST_LOG("transform %p, buffer %p.", transform, buffer);
> - gst_atomic_queue_push(transform->output_queue, buffer);
> - if ((sample = gst_sample_new(buffer, transform->sink_caps, NULL, NULL)))
> -
>    gst_atomic_queue_push(transform->output_queue, sample);
>
> - return GST_FLOW_OK;
> - gst_buffer_unref(buffer);
> -
> - return sample ? GST_FLOW_OK : GST_FLOW_ERROR;

Something of a nitpick, but I'm never a fan of checking the same 
condition in two subsequent conditionals. I'd rather just early-return 
GST_FLOW_ERROR here.
> @@ -427,7 +462,18 @@ NTSTATUS wg_transform_read_data(void *args)
>           return STATUS_SUCCESS;
>       }
> - if ((status = read_transform_output_data(transform->output_buffer, sample)))
> - if (sample->format && (caps = gst_sample_get_caps(transform->output_sample)))
> - {
> -
>    wg_format_from_caps(&format, caps);
> -
>    if (!wg_format_compare(&format, sample->format))
> -
>    {
> -
>        *sample->format = format;
> -
>        params->result = MF_E_TRANSFORM_STREAM_CHANGE;
> -
>        return STATUS_SUCCESS;
> -
>    }
> - }

This looks wrong; aren't we dropping the sample data on the floor?
> -
> - if ((status = read_transform_output_data(gst_sample_get_buffer(transform->output_sample), sample)))
>  {
>      sample->size = 0;
>      return status;
>
> @@ -435,8 +481,8 @@ NTSTATUS wg_transform_read_data(void *args)
>   if (!(sample->flags & WG_SAMPLE_FLAG_INCOMPLETE))
>   {
> -
>    gst_buffer_unref(transform->output_buffer);
> -
>    transform->output_buffer = NULL;
> -
>    gst_sample_unref(transform->output_sample);
> -
>    transform->output_sample = NULL;
> }
>
> params->result = S_OK;