[PATCH v3 3/6] winegstreamer: Implement H264 SetOutputType by reconfiguring the pipeline.

[Zebediah Figura]

On 6/23/22 12:12, Rémi Bernon wrote:
> @@ -291,6 +297,7 @@ enum unix_funcs
>   
>       unix_wg_transform_create,
>       unix_wg_transform_destroy,
> +    unix_wg_transform_set_format,
>   
>       unix_wg_transform_push_data,
>       unix_wg_transform_read_data,

Perhaps set_output_format? Not that we're likely to need a 
set_input_format counterpart...

> +NTSTATUS wg_transform_set_format(void *args)
> +{
> +    struct wg_transform_set_format_params *params = args;
> +    struct wg_transform *transform = params->transform;
> +    GstSample *sample;
> +    GstEvent *event;
> +    GstCaps *caps;
> +    gchar *str;
> +
> +    if (!(caps = wg_format_to_caps(params->format)))
> +    {
> +        GST_ERROR("Failed to convert format to caps.");
> +        return STATUS_UNSUCCESSFUL;
> +    }
> +
> +    if (gst_caps_is_always_compatible(transform->output_caps, caps))
> +    {
> +        gst_caps_unref(caps);
> +        return STATUS_SUCCESS;
> +    }
> +
> +    gst_caps_unref(transform->output_caps);
> +    transform->output_caps = caps;

This isn't thread-safe; a simultaneous GST_EVENT_CAPS can modify the 
output caps.

> +
> +    if (!gst_pad_set_caps(transform->my_sink, caps)

The only effect of gst_pad_set_caps() is to send a CAPS event to the 
sink, but you've already set transform->output_caps above.

Note also that the event won't be serialized, which begs the 
question—should it be? I suspect the answer is, "well, we actually need 
buffers already sent to be re-decoded in the new format, which GStreamer 
doesn't support". Which would be a helpful thing to write in a comment 
if so.

Since we do flush output samples below, I suspect we should explicitly 
try to flush out samples which haven't been received yet before doing 
anything else in this function, which neatly solves all of the thread 
safety problems as well. I think the correct way to do that is with 
flush-start + flush-stop; a DRAIN query might work but I'm not sure if 
it's guaranteed to force decoders to stop waiting for more data before 
sending what they have.

> +            || !(event = gst_event_new_reconfigure())
> +            || !gst_pad_push_event(transform->my_sink, event))
> +    {
> +        GST_ERROR("Failed to reconfigure transform.");
> +        return STATUS_UNSUCCESSFUL;
> +    }
> +
> +    str = gst_caps_to_string(caps);
> +    GST_INFO("Configured new caps %s.", str);
> +    g_free(str);
> +
> +    if (transform->output_sample)
> +        gst_sample_unref(transform->output_sample);
> +    while ((sample = gst_atomic_queue_pop(transform->output_queue)))
> +        gst_sample_unref(sample);
> +    transform->output_sample = NULL;
> +
> +    return STATUS_SUCCESS;
> +}
> +
>   static void wg_sample_free_notify(void *arg)
>   {
>       struct wg_sample *sample = arg;