[PATCH v4 0/2] MR5255: winegstreamer: Append an optional parser before decoders in wg_transform.

[Rémi Bernon (＠rbernon)]

Supersedes https://gitlab.winehq.org/wine/wine/-/merge_requests/5090

--
  v4: winegstreamer: Append an optional parser before decoders.

https://gitlab.winehq.org/wine/wine/-/merge_requests/5255

[PATCH v4 1/2] winegstreamer: Release sink caps in the error path.

[Rémi Bernon]

From: Rémi Bernon

---
 dlls/winegstreamer/wg_transform.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c
index 516b28e82e2..c252142d13d 100644
--- a/dlls/winegstreamer/wg_transform.c
+++ b/dlls/winegstreamer/wg_transform.c
@@ -338,7 +338,7 @@ NTSTATUS wg_transform_create(void *args)
     struct wg_format output_format = *params->output_format;
     struct wg_format input_format = *params->input_format;
     GstElement *first = NULL, *last = NULL, *element;
-    GstCaps *raw_caps = NULL, *src_caps = NULL;
+    GstCaps *sink_caps = NULL, *src_caps = NULL;
     NTSTATUS status = STATUS_UNSUCCESSFUL;
     GstPadTemplate *template = NULL;
     struct wg_transform *transform;
@@ -395,7 +395,7 @@ NTSTATUS wg_transform_create(void *args)
      * raw output media type should be enough.
      */
     media_type = gst_structure_get_name(gst_caps_get_structure(transform->output_caps, 0));
-    if (!(raw_caps = gst_caps_new_empty_simple(media_type)))
+    if (!(sink_caps = gst_caps_new_empty_simple(media_type)))
         goto out;

switch (input_format.major_type)
@@ -408,12 +408,9 @@ NTSTATUS wg_transform_create(void *args)
         case WG_MAJOR_TYPE_VIDEO_INDEO:
         case WG_MAJOR_TYPE_VIDEO_WMV:
         case WG_MAJOR_TYPE_VIDEO_MPEG1:
-            if (!(element = find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, src_caps, raw_caps))
+            if (!(element = find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, src_caps, sink_caps))
                     || !append_element(transform->container, element, &first, &last))
-            {
-                gst_caps_unref(raw_caps);
                 goto out;
-            }
             break;

case WG_MAJOR_TYPE_AUDIO:
@@ -421,12 +418,9 @@ NTSTATUS wg_transform_create(void *args)
             break;
         case WG_MAJOR_TYPE_UNKNOWN:
             GST_FIXME("Format %u not implemented!", input_format.major_type);
-            gst_caps_unref(raw_caps);
             goto out;
     }

-    gst_caps_unref(raw_caps);
-
     switch (output_format.major_type)
     {
         case WG_MAJOR_TYPE_AUDIO:
@@ -507,6 +501,7 @@ NTSTATUS wg_transform_create(void *args)
         goto out;

gst_caps_unref(src_caps);
+    gst_caps_unref(sink_caps);

GST_INFO("Created winegstreamer transform %p.", transform);
     params->transform = (wg_transform_t)(ULONG_PTR)transform;
@@ -521,6 +516,8 @@ out:
         gst_object_unref(transform->my_src);
     if (src_caps)
         gst_caps_unref(src_caps);
+    if (sink_caps)
+        gst_caps_unref(sink_caps);
     if (transform->allocator)
         wg_allocator_destroy(transform->allocator);
     if (transform->drain_query)

[PATCH v4 2/2] winegstreamer: Append an optional parser before decoders.

[Rémi Bernon]

From: Rémi Bernon

---
 dlls/winegstreamer/quartz_transform.c |  3 ++-
 dlls/winegstreamer/unixlib.h          |  1 +
 dlls/winegstreamer/wg_transform.c     | 20 ++++++++++++++++++--
 3 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/dlls/winegstreamer/quartz_transform.c b/dlls/winegstreamer/quartz_transform.c
index 5189c0b22d3..760b46950bc 100644
--- a/dlls/winegstreamer/quartz_transform.c
+++ b/dlls/winegstreamer/quartz_transform.c
@@ -99,7 +99,8 @@ static HRESULT transform_init_stream(struct strmbase_filter *iface)
 {
     struct transform *filter = impl_from_strmbase_filter(iface);
     struct wg_format input_format, output_format;
-    struct wg_transform_attrs attrs = {0};
+    /* FIXME: GStreamer mpegvideoparse causes some test failures when inserted */
+    struct wg_transform_attrs attrs = {.no_parser = TRUE};
     HRESULT hr;

if (filter->source.pin.peer)
diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h
index 4ec9fce515e..b1975bae182 100644
--- a/dlls/winegstreamer/unixlib.h
+++ b/dlls/winegstreamer/unixlib.h
@@ -338,6 +338,7 @@ struct wg_transform_attrs
     UINT32 input_queue_length;
     BOOL allow_size_change;
     BOOL low_latency;
+    BOOL no_parser;
 };

struct wg_transform_create_params
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c
index c252142d13d..c7ef3235dd2 100644
--- a/dlls/winegstreamer/wg_transform.c
+++ b/dlls/winegstreamer/wg_transform.c
@@ -338,7 +338,7 @@ NTSTATUS wg_transform_create(void *args)
     struct wg_format output_format = *params->output_format;
     struct wg_format input_format = *params->input_format;
     GstElement *first = NULL, *last = NULL, *element;
-    GstCaps *sink_caps = NULL, *src_caps = NULL;
+    GstCaps *sink_caps = NULL, *src_caps = NULL, *parsed_caps = NULL;
     NTSTATUS status = STATUS_UNSUCCESSFUL;
     GstPadTemplate *template = NULL;
     struct wg_transform *transform;
@@ -390,6 +390,10 @@ NTSTATUS wg_transform_create(void *args)
     gst_pad_set_query_function(transform->my_sink, transform_sink_query_cb);
     gst_pad_set_chain_function(transform->my_sink, transform_sink_chain_cb);

+    media_type = gst_structure_get_name(gst_caps_get_structure(src_caps, 0));
+    if (!(parsed_caps = gst_caps_new_empty_simple(media_type)))
+        goto out;
+
     /* Since we append conversion elements, we don't want to filter decoders
      * based on the actual output caps now. Matching decoders with the
      * raw output media type should be enough.
@@ -408,7 +412,16 @@ NTSTATUS wg_transform_create(void *args)
         case WG_MAJOR_TYPE_VIDEO_INDEO:
         case WG_MAJOR_TYPE_VIDEO_WMV:
         case WG_MAJOR_TYPE_VIDEO_MPEG1:
-            if (!(element = find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, src_caps, sink_caps))
+            if (!transform->attrs.no_parser && (element = find_element(GST_ELEMENT_FACTORY_TYPE_PARSER, src_caps, parsed_caps))
+                    && !append_element(transform->container, element, &first, &last))
+                goto out;
+            else
+            {
+                gst_caps_unref(parsed_caps);
+                parsed_caps = gst_caps_ref(src_caps);
+            }
+
+            if (!(element = find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, parsed_caps, sink_caps))
                     || !append_element(transform->container, element, &first, &last))
                 goto out;
             break;
@@ -500,6 +513,7 @@ NTSTATUS wg_transform_create(void *args)
             || !push_event(transform->my_src, event))
         goto out;

+    gst_caps_unref(parsed_caps);
     gst_caps_unref(src_caps);
     gst_caps_unref(sink_caps);

@@ -516,6 +530,8 @@ out:
         gst_object_unref(transform->my_src);
     if (src_caps)
         gst_caps_unref(src_caps);
+    if (parsed_caps)
+        gst_caps_unref(parsed_caps);
     if (sink_caps)
         gst_caps_unref(sink_caps);
     if (transform->allocator)

[Zebediah Figura (＠zfigura)]

> > Working around the mpegaudioparse problem does not seem unreasonable to me, but I don't think doing it from the frontend makes sense at all. I'd also really rather not have specific workarounds in the code until the actual problem is understood.
>
> The problem resides in mpegvideoparse and the quartz tests, so having the workaround in the quartz mpeg decoder sounds resaonable. A bit more specifically, mpegvideoparse holds on some buffer we pass from the tests because it decides that it needs more data to tell whether they really are on frame boundaries or not, when it gets more and passes more data to ffmpeg the decoder in turn gets confused because it does not find the boundaries it expects. Splitting the frame data differently in the test makes the test pass. I think this is a non-issue anyway, the test comment suggests that native outputs buffer when EOS is seen anyway.

In that case I think we should just relax the quartz tests so that they accept whatever mpegvideoparse outputs.

Also:
> And, like I said in 5255, I don't see how restricting the caps is necessary. What parser are you imagining would parse to the wrong format?