[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 <rbernon at codeweavers.com>
> 
> 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 <rbernon at codeweavers.com>
> ---
>   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;