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

[Zebediah Figura]

On 4/5/22 07:26, Rémi Bernon wrote:
> 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
>
> (This need to come first, before 231660-231665 as the spurious todo_wine
> is caused by the H264 format being synchronously rejected when libav
> plugins are used)
>
> In this patch I assumed that sink callbacks are always called from the
> same thread, and then do not require any locking around sink_caps
> accesses.

That's not guaranteed by the API. I'd rather just be explicitly thread-safe.
> @@ -264,6 +298,11 @@ NTSTATUS wg_transform_create(void *args)
>               break;
>       case WG_MAJOR_TYPE_VIDEO:
> -
>        if (!(element = create_element("videoconvert", "base"))
> -
>                || !transform_append_element(transform, element, &first, &last))
> -
>            goto out;

This is one of those things that probably should have been split anyway, 
but as before, I'm left wondering why we need this, and in particular 
why it's related to the rest of this patch. Why would H.264 dynamic 
format changes imply that the video format changes, and why would we 
need to convert it?
> -
>        /* Let GStreamer choose a default number of threads. */
> -
>        gst_util_set_object_arg(G_OBJECT(element), "n-threads", "0");

I'd be curious to learn whether this has a measurable positive impact, 
especially with different video sizes.
> @@ -425,7 +465,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)))

This bothers me. Patch ordering problems aside, the way this reads 
implies that the winegstreamer API consumer is free to ignore sample 
format changes, which doesn't seem right.
> - {
> -
>    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;
> -
>    }
> - }
> -
> - if ((status = read_transform_output_data(gst_sample_get_buffer(transform->output_sample), sample)))
>  {
>      sample->size = 0;
>      return status;