[PATCH v2 0/6] MR61: winegstreamer: Dynamic transform stream format change support.
Compared to last attempt, I moved the format pointer to a dedicated `wg_transform_read_data` parameter instead of a property of the sample. Most of the transforms use `NULL` there as they don't expect or support format change. H264 decoder passes a transform format member where it tracks the current stream format. -- v2: winegstreamer: Expose output media type attributes from the stream format. winegstreamer: Support wg_transform output format change events. winegstreamer: Track caps changes and keep them with the output buffers. winegstreamer: Only filter video caps attributes when format explicitely uses 0. winegstreamer: Set wg_transform output sample size on gst_buffer_map failure. winegstreamer: Append a videoconvert element to wg_transform video output. https://gitlab.winehq.org/wine/wine/-/merge_requests/61
From: Rémi Bernon <rbernon(a)codeweavers.com> This is only temporary, to make the tests results more consistent with both vaapi and libav plugins. 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(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 21392a82509..ae6e948318a 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -268,6 +268,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; + /* Let GStreamer choose a default number of threads. */ + gst_util_set_object_arg(G_OBJECT(element), "n-threads", "0"); break; case WG_MAJOR_TYPE_MPEG1_AUDIO: -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/61
On 5/13/22 04:48, Rémi Bernon wrote:
From: Rémi Bernon <rbernon(a)codeweavers.com>
This is only temporary, to make the tests results more consistent with both vaapi and libav plugins.
This is probably reasonable, but somewhat out of curiosity, what's the underlying issue?
From: Rémi Bernon <rbernon(a)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(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index ae6e948318a..9699d7e0a31 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -392,6 +392,7 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, struct wg_sample * if (!gst_buffer_map(buffer, &info, GST_MAP_READ)) { GST_ERROR("Failed to map buffer %p", buffer); + sample->size = 0; return STATUS_UNSUCCESSFUL; } @@ -464,10 +465,7 @@ NTSTATUS wg_transform_read_data(void *args) } if ((status = read_transform_output_data(transform->output_buffer, sample))) - { - sample->size = 0; return status; - } if (!(sample->flags & WG_SAMPLE_FLAG_INCOMPLETE)) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/61
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
From: Rémi Bernon <rbernon(a)codeweavers.com> In wg_transform we only want to remove width/height/framerate if the transform supports format change, and we want to keep the caps fixed otherwise so we can use gst_caps_is_always_compatible to monitor caps changes. 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(a)codeweavers.com> --- dlls/mf/tests/mf.c | 2 +- dlls/winegstreamer/h264_decoder.c | 8 ++++++++ dlls/winegstreamer/wg_format.c | 9 ++++++--- dlls/winegstreamer/wg_parser.c | 6 ++++++ 4 files changed, 21 insertions(+), 4 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 8f2c050e3c6..7170bad85a0 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -7160,7 +7160,7 @@ static void test_h264_decoder(void) "got status %#lx\n", status); hr = IMFSample_GetTotalLength(output.pSample, &length); ok(hr == S_OK, "GetTotalLength returned %#lx\n", hr); - todo_wine_if(length == 1920 * 1080 * 3 / 2) + todo_wine ok(length == 0, "got length %lu\n", length); ret = IMFSample_Release(output.pSample); ok(ret == 0, "Release returned %lu\n", ret); diff --git a/dlls/winegstreamer/h264_decoder.c b/dlls/winegstreamer/h264_decoder.c index de823741ba9..912a5bdf0b1 100644 --- a/dlls/winegstreamer/h264_decoder.c +++ b/dlls/winegstreamer/h264_decoder.c @@ -75,6 +75,14 @@ static HRESULT try_create_wg_transform(struct h264_decoder *decoder) if (output_format.major_type == WG_MAJOR_TYPE_UNKNOWN) return MF_E_INVALIDMEDIATYPE; + /* Don't force any specific size, H264 streams already have the metadata for it + * and will generate a MF_E_TRANSFORM_STREAM_CHANGE result later. + */ + output_format.u.video.width = 0; + output_format.u.video.height = 0; + output_format.u.video.fps_d = 0; + output_format.u.video.fps_n = 0; + if (!(decoder->wg_transform = wg_transform_create(&input_format, &output_format))) return E_FAIL; diff --git a/dlls/winegstreamer/wg_format.c b/dlls/winegstreamer/wg_format.c index 5b3a5617ff1..4cebeac9182 100644 --- a/dlls/winegstreamer/wg_format.c +++ b/dlls/winegstreamer/wg_format.c @@ -394,11 +394,14 @@ static GstCaps *wg_format_to_caps_video(const struct wg_format *format) gst_video_info_set_format(&info, video_format, format->u.video.width, abs(format->u.video.height)); if ((caps = gst_video_info_to_caps(&info))) { - /* Clear some fields that shouldn't prevent us from connecting. */ for (i = 0; i < gst_caps_get_size(caps); ++i) { - gst_structure_remove_fields(gst_caps_get_structure(caps, i), - "framerate", "pixel-aspect-ratio", "colorimetry", "chroma-site", NULL); + if (!format->u.video.width) + gst_structure_remove_fields(gst_caps_get_structure(caps, i), "width", NULL); + if (!format->u.video.height) + gst_structure_remove_fields(gst_caps_get_structure(caps, i), "height", NULL); + if (!format->u.video.fps_d && !format->u.video.fps_n) + gst_structure_remove_fields(gst_caps_get_structure(caps, i), "framerate", NULL); } } return caps; diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 48b88a4b11c..7d55897aa0a 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -598,6 +598,7 @@ static gboolean sink_query_cb(GstPad *pad, GstObject *parent, GstQuery *query) { GstCaps *caps, *filter, *temp; gchar *str; + gsize i; gst_query_parse_caps(query, &filter); @@ -608,6 +609,11 @@ static gboolean sink_query_cb(GstPad *pad, GstObject *parent, GstQuery *query) if (!caps) return FALSE; + /* Clear some fields that shouldn't prevent us from connecting. */ + for (i = 0; i < gst_caps_get_size(caps); ++i) + gst_structure_remove_fields(gst_caps_get_structure(caps, i), + "framerate", "pixel-aspect-ratio", "colorimetry", "chroma-site", NULL); + str = gst_caps_to_string(caps); GST_LOG("Stream caps are \"%s\".", str); g_free(str); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/61
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
From: Rémi Bernon <rbernon(a)codeweavers.com> Using a GstSample queue instead of GstBuffer queue to combine both. 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(a)codeweavers.com> --- dlls/winegstreamer/wg_transform.c | 76 +++++++++++++++++++++++-------- 1 file changed, 58 insertions(+), 18 deletions(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 9699d7e0a31..b6cc51aecb7 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -51,40 +51,77 @@ struct wg_transform GstBufferList *input; guint input_max_length; GstAtomicQueue *output_queue; - GstBuffer *output_buffer; + GstSample *output_sample; + GstCaps *output_caps; }; 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->output_caps, NULL, NULL))) + { + GST_ERROR("Failed to allocate transform %p output sample.", transform); + gst_buffer_unref(buffer); + return GST_FLOW_ERROR; + } + gst_atomic_queue_push(transform->output_queue, sample); + gst_buffer_unref(buffer); return GST_FLOW_OK; } +static gboolean transform_sink_event_cb(GstPad *pad, GstObject *parent, GstEvent *event) +{ + struct wg_transform *transform = gst_pad_get_element_private(pad); + + GST_LOG("transform %p, type \"%s\".", transform, GST_EVENT_TYPE_NAME(event)); + + switch (event->type) + { + case GST_EVENT_CAPS: + { + GstCaps *caps; + + gst_event_parse_caps(event, &caps); + + gst_caps_unref(transform->output_caps); + transform->output_caps = gst_caps_ref(caps); + break; + } + default: + GST_WARNING("Ignoring \"%s\" event.", GST_EVENT_TYPE_NAME(event)); + break; + } + + gst_event_unref(event); + return TRUE; +} + NTSTATUS wg_transform_destroy(void *args) { struct wg_transform *transform = args; - GstBuffer *buffer; + GstSample *sample; if (transform->input) gst_buffer_list_unref(transform->input); gst_element_set_state(transform->container, GST_STATE_NULL); - if (transform->output_buffer) - gst_buffer_unref(transform->output_buffer); - while ((buffer = gst_atomic_queue_pop(transform->output_queue))) - gst_buffer_unref(buffer); + if (transform->output_sample) + gst_sample_unref(transform->output_sample); + while ((sample = gst_atomic_queue_pop(transform->output_queue))) + gst_sample_unref(sample); g_object_unref(transform->their_sink); g_object_unref(transform->their_src); g_object_unref(transform->container); g_object_unref(transform->my_sink); g_object_unref(transform->my_src); + gst_caps_unref(transform->output_caps); gst_atomic_queue_unref(transform->output_queue); free(transform); @@ -162,10 +199,10 @@ static bool transform_append_element(struct wg_transform *transform, GstElement NTSTATUS wg_transform_create(void *args) { struct wg_transform_create_params *params = args; - GstCaps *raw_caps = NULL, *src_caps = NULL, *sink_caps = NULL; 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; NTSTATUS status = STATUS_UNSUCCESSFUL; GstPadTemplate *template = NULL; struct wg_transform *transform; @@ -194,9 +231,9 @@ NTSTATUS wg_transform_create(void *args) if (!transform->my_src) goto out; - if (!(sink_caps = wg_format_to_caps(&output_format))) + if (!(transform->output_caps = wg_format_to_caps(&output_format))) goto out; - if (!(template = gst_pad_template_new("sink", GST_PAD_SINK, GST_PAD_ALWAYS, sink_caps))) + if (!(template = gst_pad_template_new("sink", GST_PAD_SINK, GST_PAD_ALWAYS, transform->output_caps))) goto out; transform->my_sink = gst_pad_new_from_template(template, "sink"); g_object_unref(template); @@ -204,13 +241,14 @@ NTSTATUS wg_transform_create(void *args) goto out; gst_pad_set_element_private(transform->my_sink, transform); + gst_pad_set_event_function(transform->my_sink, transform_sink_event_cb); gst_pad_set_chain_function(transform->my_sink, transform_sink_chain_cb); /* 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. */ - media_type = gst_structure_get_name(gst_caps_get_structure(sink_caps, 0)); + media_type = gst_structure_get_name(gst_caps_get_structure(transform->output_caps, 0)); if (!(raw_caps = gst_caps_new_empty_simple(media_type))) goto out; @@ -316,7 +354,6 @@ NTSTATUS wg_transform_create(void *args) || !gst_pad_push_event(transform->my_src, event)) goto out; - gst_caps_unref(sink_caps); gst_caps_unref(src_caps); GST_INFO("Created winegstreamer transform %p.", transform); @@ -330,8 +367,8 @@ out: gst_object_unref(transform->their_src); if (transform->my_sink) gst_object_unref(transform->my_sink); - if (sink_caps) - gst_caps_unref(sink_caps); + if (transform->output_caps) + gst_caps_unref(transform->output_caps); if (transform->my_src) gst_object_unref(transform->my_src); if (src_caps) @@ -440,6 +477,7 @@ NTSTATUS wg_transform_read_data(void *args) struct wg_transform *transform = params->transform; struct wg_sample *sample = params->sample; GstBufferList *input = transform->input; + GstBuffer *output_buffer; GstFlowReturn ret; NTSTATUS status; @@ -456,7 +494,7 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_UNSUCCESSFUL; } - if (!transform->output_buffer && !(transform->output_buffer = gst_atomic_queue_pop(transform->output_queue))) + if (!transform->output_sample && !(transform->output_sample = gst_atomic_queue_pop(transform->output_queue))) { sample->size = 0; params->result = MF_E_TRANSFORM_NEED_MORE_INPUT; @@ -464,13 +502,15 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; } - if ((status = read_transform_output_data(transform->output_buffer, sample))) + output_buffer = gst_sample_get_buffer(transform->output_sample); + + if ((status = read_transform_output_data(output_buffer, sample))) return status; 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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/61
Signed-off-by: Zebediah Figura <zfigura(a)codeweavers.com>
From: Rémi Bernon <rbernon(a)codeweavers.com> Adding an info structure to the sample when the caps have changed, removing if once it has been reported to the caller. 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(a)codeweavers.com> --- dlls/mf/tests/mf.c | 4 ---- dlls/winegstreamer/h264_decoder.c | 7 +++++++ dlls/winegstreamer/quartz_transform.c | 2 ++ dlls/winegstreamer/wg_transform.c | 23 ++++++++++++++++++++++- dlls/winegstreamer/wma_decoder.c | 8 ++++++++ 5 files changed, 39 insertions(+), 5 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 7170bad85a0..291c9d0a175 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -7147,20 +7147,16 @@ static void test_h264_decoder(void) ok(i == 2, "got %lu iterations\n", i); todo_wine ok(h264_encoded_data_len == 1180, "got h264_encoded_data_len %lu\n", h264_encoded_data_len); - todo_wine ok(hr == MF_E_TRANSFORM_STREAM_CHANGE, "ProcessOutput returned %#lx\n", hr); ok(output.dwStreamID == 0, "got dwStreamID %lu\n", output.dwStreamID); ok(!!output.pSample, "got pSample %p\n", output.pSample); - todo_wine ok(output.dwStatus == MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE, "got dwStatus %#lx\n", output.dwStatus); ok(!output.pEvents, "got pEvents %p\n", output.pEvents); - todo_wine ok(status == MFT_PROCESS_OUTPUT_STATUS_NEW_STREAMS, "got status %#lx\n", status); hr = IMFSample_GetTotalLength(output.pSample, &length); ok(hr == S_OK, "GetTotalLength returned %#lx\n", hr); - todo_wine ok(length == 0, "got length %lu\n", length); ret = IMFSample_Release(output.pSample); ok(ret == 0, "Release returned %lu\n", ret); diff --git a/dlls/winegstreamer/h264_decoder.c b/dlls/winegstreamer/h264_decoder.c index 912a5bdf0b1..ea80956b038 100644 --- a/dlls/winegstreamer/h264_decoder.c +++ b/dlls/winegstreamer/h264_decoder.c @@ -569,6 +569,13 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, hr = wg_transform_read_data(decoder->wg_transform, wg_sample); mf_destroy_wg_sample(wg_sample); + + if (hr == MF_E_TRANSFORM_STREAM_CHANGE) + { + samples[0].dwStatus |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE; + *status |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE; + } + return hr; } diff --git a/dlls/winegstreamer/quartz_transform.c b/dlls/winegstreamer/quartz_transform.c index 447f331d474..44f4cd44ff4 100644 --- a/dlls/winegstreamer/quartz_transform.c +++ b/dlls/winegstreamer/quartz_transform.c @@ -350,6 +350,8 @@ static HRESULT WINAPI transform_sink_receive(struct strmbase_sink *pin, IMediaSa } if (FAILED(hr)) { + if (hr == MF_E_TRANSFORM_STREAM_CHANGE) + FIXME("Unexpected stream format change!\n"); IMediaSample_Release(output_sample); return hr; } diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index b6cc51aecb7..9edcd72754f 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -52,17 +52,27 @@ struct wg_transform guint input_max_length; GstAtomicQueue *output_queue; GstSample *output_sample; + bool output_caps_changed; GstCaps *output_caps; }; static GstFlowReturn transform_sink_chain_cb(GstPad *pad, GstObject *parent, GstBuffer *buffer) { struct wg_transform *transform = gst_pad_get_element_private(pad); + GstStructure *info = NULL; GstSample *sample; GST_LOG("transform %p, buffer %p.", transform, buffer); - if (!(sample = gst_sample_new(buffer, transform->output_caps, NULL, NULL))) + if (transform->output_caps_changed && !(info = gst_structure_new_empty("format-changed"))) + { + GST_ERROR("Failed to allocate transform %p output sample info.", transform); + gst_buffer_unref(buffer); + return GST_FLOW_ERROR; + } + transform->output_caps_changed = false; + + if (!(sample = gst_sample_new(buffer, transform->output_caps, NULL, info))) { GST_ERROR("Failed to allocate transform %p output sample.", transform); gst_buffer_unref(buffer); @@ -88,6 +98,9 @@ static gboolean transform_sink_event_cb(GstPad *pad, GstObject *parent, GstEvent gst_event_parse_caps(event, &caps); + transform->output_caps_changed = transform->output_caps_changed + || !gst_caps_is_always_compatible(transform->output_caps, caps); + gst_caps_unref(transform->output_caps); transform->output_caps = gst_caps_ref(caps); break; @@ -504,6 +517,14 @@ NTSTATUS wg_transform_read_data(void *args) output_buffer = gst_sample_get_buffer(transform->output_sample); + if (gst_sample_get_info(transform->output_sample)) + { + gst_sample_set_info(transform->output_sample, NULL); + params->result = MF_E_TRANSFORM_STREAM_CHANGE; + GST_INFO("Format changed detected, returning no output"); + return STATUS_SUCCESS; + } + if ((status = read_transform_output_data(output_buffer, sample))) return status; diff --git a/dlls/winegstreamer/wma_decoder.c b/dlls/winegstreamer/wma_decoder.c index 71369add244..57931a700eb 100644 --- a/dlls/winegstreamer/wma_decoder.c +++ b/dlls/winegstreamer/wma_decoder.c @@ -587,6 +587,14 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, } mf_destroy_wg_sample(wg_sample); + + if (hr == MF_E_TRANSFORM_STREAM_CHANGE) + { + FIXME("Unexpected stream format change!\n"); + samples[0].dwStatus |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE; + *status |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE; + } + return hr; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/61
On 5/13/22 04:48, Rémi Bernon wrote:
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index b6cc51aecb7..9edcd72754f 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -52,17 +52,27 @@ struct wg_transform guint input_max_length; GstAtomicQueue *output_queue; GstSample *output_sample; + bool output_caps_changed; GstCaps *output_caps; };
To be clear, what bothered me about the last iteration is that the format-change logic feels awkwardly split in half between the frontend and backend. Hence my proposal to move it *entirely* to the frontend—including keeping track of the previous type there, comparing caps, and returning MF_E_TRANSFORM_STREAM_CHANGE from the front end. So the existence of this field still feels awkward to me. Did I forget a discussion again about why it's necessary? For that matter, I could also see an argument for moving the whole thing to the back end, and handling the "frontend needs to explicitly invalidate the format" problem by adding a new unixlib API for it. (Or maybe just destroying and recreating the wg_transform? That's not ideal if it happens frequently, but I don't know if that's the case.)
From: Rémi Bernon <rbernon(a)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(a)codeweavers.com> --- dlls/mf/tests/mf.c | 17 ++++------ dlls/winegstreamer/gst_private.h | 1 + dlls/winegstreamer/h264_decoder.c | 53 +++++++++++++++++-------------- dlls/winegstreamer/main.c | 13 ++++++++ dlls/winegstreamer/unix_private.h | 1 + dlls/winegstreamer/unixlib.h | 7 ++++ dlls/winegstreamer/wg_parser.c | 1 + dlls/winegstreamer/wg_transform.c | 16 ++++++++++ 8 files changed, 75 insertions(+), 34 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 291c9d0a175..9a9e201bf87 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6801,7 +6801,7 @@ static void test_h264_decoder(void) ATTR_GUID(MF_MT_MAJOR_TYPE, MFMediaType_Video), ATTR_GUID(MF_MT_SUBTYPE, MFVideoFormat_NV12), ATTR_RATIO(MF_MT_PIXEL_ASPECT_RATIO, 1, 1), - ATTR_RATIO(MF_MT_FRAME_RATE, 60000, 1000, .todo_value = TRUE), + ATTR_RATIO(MF_MT_FRAME_RATE, 60000, 1000), ATTR_RATIO(MF_MT_FRAME_SIZE, actual_width, actual_height, .todo_value = TRUE), ATTR_UINT32(MF_MT_SAMPLE_SIZE, actual_width * actual_height * 3 / 2, .todo_value = TRUE), ATTR_UINT32(MF_MT_DEFAULT_STRIDE, actual_width, .todo_value = TRUE), @@ -6815,7 +6815,7 @@ static void test_h264_decoder(void) ATTR_GUID(MF_MT_MAJOR_TYPE, MFMediaType_Video), ATTR_GUID(MF_MT_SUBTYPE, MFVideoFormat_YV12), ATTR_RATIO(MF_MT_PIXEL_ASPECT_RATIO, 1, 1), - ATTR_RATIO(MF_MT_FRAME_RATE, 60000, 1000, .todo_value = TRUE), + ATTR_RATIO(MF_MT_FRAME_RATE, 60000, 1000), ATTR_RATIO(MF_MT_FRAME_SIZE, actual_width, actual_height, .todo_value = TRUE), ATTR_UINT32(MF_MT_SAMPLE_SIZE, actual_width * actual_height * 3 / 2, .todo_value = TRUE), ATTR_UINT32(MF_MT_DEFAULT_STRIDE, actual_width, .todo_value = TRUE), @@ -6829,7 +6829,7 @@ static void test_h264_decoder(void) ATTR_GUID(MF_MT_MAJOR_TYPE, MFMediaType_Video), ATTR_GUID(MF_MT_SUBTYPE, MFVideoFormat_IYUV), ATTR_RATIO(MF_MT_PIXEL_ASPECT_RATIO, 1, 1), - ATTR_RATIO(MF_MT_FRAME_RATE, 60000, 1000, .todo_value = TRUE), + ATTR_RATIO(MF_MT_FRAME_RATE, 60000, 1000), ATTR_RATIO(MF_MT_FRAME_SIZE, actual_width, actual_height, .todo_value = TRUE), ATTR_UINT32(MF_MT_SAMPLE_SIZE, actual_width * actual_height * 3 / 2, .todo_value = TRUE), ATTR_UINT32(MF_MT_DEFAULT_STRIDE, actual_width, .todo_value = TRUE), @@ -6843,7 +6843,7 @@ static void test_h264_decoder(void) ATTR_GUID(MF_MT_MAJOR_TYPE, MFMediaType_Video), ATTR_GUID(MF_MT_SUBTYPE, MFVideoFormat_I420), ATTR_RATIO(MF_MT_PIXEL_ASPECT_RATIO, 1, 1), - ATTR_RATIO(MF_MT_FRAME_RATE, 60000, 1000, .todo_value = TRUE), + ATTR_RATIO(MF_MT_FRAME_RATE, 60000, 1000), ATTR_RATIO(MF_MT_FRAME_SIZE, actual_width, actual_height, .todo_value = TRUE), ATTR_UINT32(MF_MT_SAMPLE_SIZE, actual_width * actual_height * 3 / 2, .todo_value = TRUE), ATTR_UINT32(MF_MT_DEFAULT_STRIDE, actual_width, .todo_value = TRUE), @@ -6857,7 +6857,7 @@ static void test_h264_decoder(void) ATTR_GUID(MF_MT_MAJOR_TYPE, MFMediaType_Video), ATTR_GUID(MF_MT_SUBTYPE, MFVideoFormat_YUY2), ATTR_RATIO(MF_MT_PIXEL_ASPECT_RATIO, 1, 1), - ATTR_RATIO(MF_MT_FRAME_RATE, 60000, 1000, .todo_value = TRUE), + ATTR_RATIO(MF_MT_FRAME_RATE, 60000, 1000), ATTR_RATIO(MF_MT_FRAME_SIZE, actual_width, actual_height, .todo_value = TRUE), ATTR_UINT32(MF_MT_SAMPLE_SIZE, actual_width * actual_height * 2, .todo_value = TRUE), ATTR_UINT32(MF_MT_DEFAULT_STRIDE, actual_width * 2, .todo_value = TRUE), @@ -7166,7 +7166,6 @@ static void test_h264_decoder(void) hr = IMFTransform_GetOutputStreamInfo(transform, 0, &output_info); ok(hr == S_OK, "GetOutputStreamInfo returned %#lx\n", hr); ok(output_info.dwFlags == flags, "got dwFlags %#lx\n", output_info.dwFlags); - todo_wine ok(output_info.cbSize == actual_width * actual_height * 2, "got cbSize %#lx\n", output_info.cbSize); ok(output_info.cbAlignment == 0, "got cbAlignment %#lx\n", output_info.cbAlignment); @@ -7208,14 +7207,12 @@ static void test_h264_decoder(void) memset(&output, 0, sizeof(output)); output.pSample = create_sample(NULL, actual_width * actual_height * 2); hr = IMFTransform_ProcessOutput(transform, 0, 1, &output, &status); - todo_wine ok(hr == S_OK, "ProcessOutput returned %#lx\n", hr); ok(output.dwStreamID == 0, "got dwStreamID %lu\n", output.dwStreamID); ok(!!output.pSample, "got pSample %p\n", output.pSample); ok(output.dwStatus == 0, "got dwStatus %#lx\n", output.dwStatus); ok(!output.pEvents, "got pEvents %p\n", output.pEvents); ok(status == 0, "got status %#lx\n", status); - if (hr != S_OK) goto skip_nv12_tests; hr = IMFSample_GetUINT32(sample, &MFSampleExtension_CleanPoint, &value); ok(hr == MF_E_ATTRIBUTENOTFOUND, "GetUINT32 MFSampleExtension_CleanPoint returned %#lx\n", hr); @@ -7232,9 +7229,7 @@ static void test_h264_decoder(void) time = 0xdeadbeef; hr = IMFSample_GetSampleTime(output.pSample, &time); - todo_wine ok(hr == S_OK, "GetSampleTime returned %#lx\n", hr); - todo_wine ok(time == 0, "got time %I64d\n", time); /* doesn't matter what frame rate we've selected, duration is defined by the stream */ @@ -7249,7 +7244,9 @@ static void test_h264_decoder(void) ok(hr == S_OK, "ConvertToContiguousBuffer returned %#lx\n", hr); hr = IMFMediaBuffer_Lock(media_buffer, &data, NULL, &length); ok(hr == S_OK, "Lock returned %#lx\n", hr); + todo_wine ok(length == nv12_frame_len, "got length %lu\n", length); + if (length != nv12_frame_len) goto skip_nv12_tests; for (i = 0; i < actual_aperture.Area.cy; ++i) { diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index 2a4b16079c1..55b2adc7e08 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -98,6 +98,7 @@ struct wg_transform *wg_transform_create(const struct wg_format *input_format, void wg_transform_destroy(struct wg_transform *transform); HRESULT wg_transform_push_data(struct wg_transform *transform, struct wg_sample *sample); HRESULT wg_transform_read_data(struct wg_transform *transform, struct wg_sample *sample); +void wg_transform_get_format(struct wg_transform *transform, struct wg_format *format); unsigned int wg_format_get_max_size(const struct wg_format *format); diff --git a/dlls/winegstreamer/h264_decoder.c b/dlls/winegstreamer/h264_decoder.c index ea80956b038..1b0061f4b4e 100644 --- a/dlls/winegstreamer/h264_decoder.c +++ b/dlls/winegstreamer/h264_decoder.c @@ -50,6 +50,7 @@ struct h264_decoder IMFMediaType *input_type; IMFMediaType *output_type; + struct wg_format format; struct wg_transform *wg_transform; }; @@ -89,8 +90,10 @@ static HRESULT try_create_wg_transform(struct h264_decoder *decoder) return S_OK; } -static HRESULT fill_output_media_type(IMFMediaType *media_type, IMFMediaType *default_type) +static HRESULT fill_output_media_type(struct h264_decoder *decoder, IMFMediaType *media_type) { + IMFMediaType *default_type = decoder->output_type; + struct wg_format *format = &decoder->format; UINT32 value, width, height; UINT64 ratio; GUID subtype; @@ -101,8 +104,7 @@ static HRESULT fill_output_media_type(IMFMediaType *media_type, IMFMediaType *de if (FAILED(hr = IMFMediaType_GetUINT64(media_type, &MF_MT_FRAME_SIZE, &ratio))) { - if (!default_type || FAILED(hr = IMFMediaType_GetUINT64(default_type, &MF_MT_FRAME_SIZE, &ratio))) - ratio = (UINT64)1920 << 32 | 1080; + ratio = (UINT64)format->u.video.width << 32 | format->u.video.height; if (FAILED(hr = IMFMediaType_SetUINT64(media_type, &MF_MT_FRAME_SIZE, ratio))) return hr; } @@ -111,24 +113,21 @@ static HRESULT fill_output_media_type(IMFMediaType *media_type, IMFMediaType *de if (FAILED(hr = IMFMediaType_GetItem(media_type, &MF_MT_FRAME_RATE, NULL))) { - if (!default_type || FAILED(hr = IMFMediaType_GetUINT64(default_type, &MF_MT_FRAME_RATE, &ratio))) - ratio = (UINT64)30000 << 32 | 1001; + ratio = (UINT64)format->u.video.fps_n << 32 | format->u.video.fps_d; if (FAILED(hr = IMFMediaType_SetUINT64(media_type, &MF_MT_FRAME_RATE, ratio))) return hr; } if (FAILED(hr = IMFMediaType_GetItem(media_type, &MF_MT_PIXEL_ASPECT_RATIO, NULL))) { - if (!default_type || FAILED(hr = IMFMediaType_GetUINT64(default_type, &MF_MT_PIXEL_ASPECT_RATIO, &ratio))) - ratio = (UINT64)1 << 32 | 1; + ratio = (UINT64)1 << 32 | 1; /* FIXME: read it from format */ if (FAILED(hr = IMFMediaType_SetUINT64(media_type, &MF_MT_PIXEL_ASPECT_RATIO, ratio))) return hr; } if (FAILED(hr = IMFMediaType_GetItem(media_type, &MF_MT_SAMPLE_SIZE, NULL))) { - if ((!default_type || FAILED(hr = IMFMediaType_GetUINT32(default_type, &MF_MT_SAMPLE_SIZE, &value))) && - FAILED(hr = MFCalculateImageSize(&subtype, width, height, &value))) + if (FAILED(hr = MFCalculateImageSize(&subtype, width, height, &value))) return hr; if (FAILED(hr = IMFMediaType_SetUINT32(media_type, &MF_MT_SAMPLE_SIZE, value))) return hr; @@ -136,8 +135,7 @@ static HRESULT fill_output_media_type(IMFMediaType *media_type, IMFMediaType *de if (FAILED(hr = IMFMediaType_GetItem(media_type, &MF_MT_DEFAULT_STRIDE, NULL))) { - if ((!default_type || FAILED(hr = IMFMediaType_GetUINT32(default_type, &MF_MT_DEFAULT_STRIDE, &value))) && - FAILED(hr = MFGetStrideForBitmapInfoHeader(subtype.Data1, width, (LONG *)&value))) + if (FAILED(hr = MFGetStrideForBitmapInfoHeader(subtype.Data1, width, (LONG *)&value))) return hr; if (FAILED(hr = IMFMediaType_SetUINT32(media_type, &MF_MT_DEFAULT_STRIDE, value))) return hr; @@ -272,23 +270,15 @@ static HRESULT WINAPI transform_GetInputStreamInfo(IMFTransform *iface, DWORD id static HRESULT WINAPI transform_GetOutputStreamInfo(IMFTransform *iface, DWORD id, MFT_OUTPUT_STREAM_INFO *info) { struct h264_decoder *decoder = impl_from_IMFTransform(iface); - UINT32 sample_size; - UINT64 frame_size; + UINT32 actual_width, actual_height; TRACE("iface %p, id %#lx, info %p.\n", iface, id, info); - if (!decoder->output_type) - sample_size = 1920 * 1088 * 2; - else if (FAILED(IMFMediaType_GetUINT32(decoder->output_type, &MF_MT_SAMPLE_SIZE, &sample_size))) - { - if (FAILED(IMFMediaType_GetUINT64(decoder->output_type, &MF_MT_FRAME_SIZE, &frame_size))) - sample_size = 1920 * 1088 * 2; - else - sample_size = (frame_size >> 32) * (UINT32)frame_size * 2; - } + actual_width = (decoder->format.u.video.width + 15) & ~15; + actual_height = (decoder->format.u.video.height + 15) & ~15; info->dwFlags = MFT_OUTPUT_STREAM_WHOLE_SAMPLES | MFT_OUTPUT_STREAM_SINGLE_SAMPLE_PER_BUFFER | MFT_OUTPUT_STREAM_FIXED_SAMPLE_SIZE; - info->cbSize = sample_size; + info->cbSize = actual_width * actual_height * 2; info->cbAlignment = 0; return S_OK; @@ -378,7 +368,7 @@ static HRESULT WINAPI transform_GetOutputAvailableType(IMFTransform *iface, DWOR if (FAILED(hr = IMFMediaType_SetGUID(media_type, &MF_MT_SUBTYPE, output_type))) goto done; - hr = fill_output_media_type(media_type, NULL); + hr = fill_output_media_type(decoder, media_type); done: if (SUCCEEDED(hr)) @@ -543,6 +533,7 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, struct h264_decoder *decoder = impl_from_IMFTransform(iface); MFT_OUTPUT_STREAM_INFO info; struct wg_sample *wg_sample; + UINT64 frame_rate; HRESULT hr; TRACE("iface %p, flags %#lx, count %lu, samples %p, status %p.\n", iface, flags, count, samples, status); @@ -572,6 +563,15 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (hr == MF_E_TRANSFORM_STREAM_CHANGE) { + wg_transform_get_format(decoder->wg_transform, &decoder->format); + + /* keep the frame rate that was requested, GStreamer doesn't provide any */ + if (SUCCEEDED(IMFMediaType_GetUINT64(decoder->output_type, &MF_MT_FRAME_RATE, &frame_rate))) + { + decoder->format.u.video.fps_n = frame_rate >> 32; + decoder->format.u.video.fps_d = (UINT32)frame_rate; + } + samples[0].dwStatus |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE; *status |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE; } @@ -639,6 +639,11 @@ HRESULT h264_decoder_create(REFIID riid, void **ret) decoder->IMFTransform_iface.lpVtbl = &transform_vtbl; decoder->refcount = 1; + decoder->format.u.video.format = WG_VIDEO_FORMAT_UNKNOWN; + decoder->format.u.video.width = 1920; + decoder->format.u.video.height = 1080; + decoder->format.u.video.fps_n = 30000; + decoder->format.u.video.fps_d = 1001; *ret = &decoder->IMFTransform_iface; TRACE("Created decoder %p\n", *ret); diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index c3adbb82d61..3afc279a86f 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -346,6 +346,19 @@ HRESULT wg_transform_read_data(struct wg_transform *transform, struct wg_sample return params.result; } +void wg_transform_get_format(struct wg_transform *transform, struct wg_format *format) +{ + struct wg_transform_get_format_params params = + { + .transform = transform, + .format = format, + }; + + TRACE("transform %p, format %p.\n", transform, format); + + __wine_unix_call(unix_handle, unix_wg_transform_get_format, ¶ms); +} + BOOL WINAPI DllMain(HINSTANCE instance, DWORD reason, void *reserved) { if (reason == DLL_PROCESS_ATTACH) diff --git a/dlls/winegstreamer/unix_private.h b/dlls/winegstreamer/unix_private.h index 7bce8263aaf..eeeda671ad8 100644 --- a/dlls/winegstreamer/unix_private.h +++ b/dlls/winegstreamer/unix_private.h @@ -36,5 +36,6 @@ extern NTSTATUS wg_transform_create(void *args) DECLSPEC_HIDDEN; extern NTSTATUS wg_transform_destroy(void *args) DECLSPEC_HIDDEN; extern NTSTATUS wg_transform_push_data(void *args) DECLSPEC_HIDDEN; extern NTSTATUS wg_transform_read_data(void *args) DECLSPEC_HIDDEN; +extern NTSTATUS wg_transform_get_format(void *args) DECLSPEC_HIDDEN; #endif /* __WINE_WINEGSTREAMER_UNIX_PRIVATE_H */ diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 5911278530d..029d6992342 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -260,6 +260,12 @@ struct wg_transform_read_data_params HRESULT result; }; +struct wg_transform_get_format_params +{ + struct wg_transform *transform; + struct wg_format *format; +}; + enum unix_funcs { unix_wg_parser_create, @@ -291,6 +297,7 @@ enum unix_funcs unix_wg_transform_push_data, unix_wg_transform_read_data, + unix_wg_transform_get_format, }; #endif /* __WINE_WINEGSTREAMER_UNIXLIB_H */ diff --git a/dlls/winegstreamer/wg_parser.c b/dlls/winegstreamer/wg_parser.c index 7d55897aa0a..c6382adcb65 100644 --- a/dlls/winegstreamer/wg_parser.c +++ b/dlls/winegstreamer/wg_parser.c @@ -1628,4 +1628,5 @@ const unixlib_entry_t __wine_unix_call_funcs[] = X(wg_transform_push_data), X(wg_transform_read_data), + X(wg_transform_get_format), }; diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 9edcd72754f..ff8de777232 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -537,3 +537,19 @@ NTSTATUS wg_transform_read_data(void *args) params->result = S_OK; return STATUS_SUCCESS; } + +NTSTATUS wg_transform_get_format(void *args) +{ + struct wg_transform_get_format_params *params = args; + struct wg_transform *transform = params->transform; + GstCaps *output_caps; + + if (!transform->output_sample) + return STATUS_UNSUCCESSFUL; + + output_caps = gst_sample_get_caps(transform->output_sample); + + wg_format_from_caps(params->format, output_caps); + + return STATUS_SUCCESS; +} -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/61
On Tue May 17 03:55:09 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 5/13/22 04:48, Rémi Bernon wrote:
From: Rémi Bernon <rbernon(a)codeweavers.com>
This is only temporary, to make the tests results more consistent with both vaapi and libav plugins. This is probably reasonable, but somewhat out of curiosity, what's the underlying issue?
As far as the test goes, you can in theory change the output format, and as long as there's been enough input buffer previously pushed, get new output sample in the new format without pushing anything more. This wouldn't be possible with a pipelined videoconvert as all the decoder output buffers would go through it immediately.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/61#note_925
On Tue May 17 03:55:11 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 5/13/22 04:48, Rémi Bernon wrote:
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index b6cc51aecb7..9edcd72754f 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -52,17 +52,27 @@ struct wg_transform guint input_max_length; GstAtomicQueue *output_queue; GstSample *output_sample; + bool output_caps_changed; GstCaps *output_caps; }; To be clear, what bothered me about the last iteration is that the format-change logic feels awkwardly split in half between the frontend and backend. Hence my proposal to move it *entirely* to the frontend—including keeping track of the previous type there, comparing caps, and returning MF_E_TRANSFORM_STREAM_CHANGE from the front end. So the existence of this field still feels awkward to me. Did I forget a discussion again about why it's necessary? For that matter, I could also see an argument for moving the whole thing to the back end, and handling the "frontend needs to explicitly invalidate the format" problem by adding a new unixlib API for it. (Or maybe just destroying and recreating the wg_transform? That's not ideal if it happens frequently, but I don't know if that's the case.)
To be clear, what bothered me about the last iteration is that the
format-change logic feels awkwardly split in half between the frontend
and backend. Hence my proposal to move it *entirely* to the
frontend—including keeping track of the previous type there, comparing
caps, and returning MF_E_TRANSFORM_STREAM_CHANGE from the front end. So
the existence of this field still feels awkward to me. Did I forget a
discussion again about why it's necessary?
This is imo only making everything more complicated than it needs to be. We would have to copy back the sample and all its attributes somewhere in the transform, and copy it again on the next call, with a special conditional case just for that. Although I don't really see how the current proposal (or its previous versions) is so awkward, yes the frontend and backend are made to work together and share some logic. This is really just some internal API and I really think we should make it ad-hoc depending on our needs, for simplicity rather than purity. The wg_transform is only the unix side of MF transforms / DMO, there's really no intention to have it used anywhere else. ``` For that matter, I could also see an argument for moving the whole thing to the back end, and handling the "frontend needs to explicitly invalidate the format" problem by adding a new unixlib API for it. (Or maybe just destroying and recreating the wg_transform? That's not ideal if it happens frequently, but I don't know if that's the case.) ``` I this this would be even worse, and we will need to pass the format to the transform anyway. We cannot destroy the transform, we would lose the decoder state. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/61#note_926
On 5/17/22 00:48, Rémi Bernon (@rbernon) wrote:
On Tue May 17 03:55:11 2022 +0000, **** wrote:
On 5/13/22 04:48, Rémi Bernon wrote:
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index b6cc51aecb7..9edcd72754f 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -52,17 +52,27 @@ struct wg_transform guint input_max_length; GstAtomicQueue *output_queue; GstSample *output_sample; + bool output_caps_changed; GstCaps *output_caps; }; To be clear, what bothered me about the last iteration is that the format-change logic feels awkwardly split in half between the frontend and backend. Hence my proposal to move it *entirely* to the frontend—including keeping track of the previous type there, comparing caps, and returning MF_E_TRANSFORM_STREAM_CHANGE from the front end. So the existence of this field still feels awkward to me. Did I forget a discussion again about why it's necessary? For that matter, I could also see an argument for moving the whole thing to the back end, and handling the "frontend needs to explicitly invalidate the format" problem by adding a new unixlib API for it. (Or maybe just destroying and recreating the wg_transform? That's not ideal if it happens frequently, but I don't know if that's the case.)
This is imo only making everything more complicated than it needs to be. We would have to copy back the sample and all its attributes somewhere in the transform, and copy it again on the next call, with a special conditional case just for that.
I'm not sure I understand. Isn't it just a matter of: hr = wg_transform_get_buffer(wg_transform, &format); if (hr != S_OK) break; if (!wg_format_is_equal(&format, &transform->stream_format)) { transform->stream_format = format; // fill other flags and return an empty sample... return MF_E_TRANSFORM_STREAM_CHANGE; } wg_transform_copy_buffer(wg_transform, &wg_sample); ... Not committed to the function names, but it's no coincidence that's the pattern we already use for the parser. (It's also not immediately obvious to me that the two unixlib functions can't be merged together somehow, using a NULL as the wg_sample pointer. Even thinking ahead towards zero-copy it's not clear that's untenable. I'm sure it's not the API you had in mind, and maybe I'm forgetting some consideration, but...)
Although I don't really see how the current proposal (or its previous versions) is so awkward, yes the frontend and backend are made to work together and share some logic. This is really just some internal API and I really think we should make it ad-hoc depending on our needs, for simplicity rather than purity. The wg_transform is only the unix side of MF transforms / DMO, there's really no intention to have it used anywhere else.
I understand that viewpoint, and maybe this is a personal thing, but I find it distinctly easier to reason about (and hence maintain) an internal API when it's designed like a standalone "pure" API layer. For whatever that means. That's especially true in the case of winegstreamer, which is designed to be an backend to several different media APIs. (Note also that thanks to Anton's work, we already use the wg_transform as a backend for DirectShow filters, and I think it'd be quite reasonable to use it for other objects in the future.)
On Tue May 17 05:48:42 2022 +0000, Rémi Bernon wrote:
``` To be clear, what bothered me about the last iteration is that the format-change logic feels awkwardly split in half between the frontend and backend. Hence my proposal to move it *entirely* to the frontend—including keeping track of the previous type there, comparing caps, and returning MF_E_TRANSFORM_STREAM_CHANGE from the front end. So the existence of this field still feels awkward to me. Did I forget a discussion again about why it's necessary? ``` This is imo only making everything more complicated than it needs to be. We would have to copy back the sample and all its attributes somewhere in the transform, and copy it again on the next call, with a special conditional case just for that. Although I don't really see how the current proposal (or its previous versions) is so awkward, yes the frontend and backend are made to work together and share some logic. This is really just some internal API and I really think we should make it ad-hoc depending on our needs, for simplicity rather than purity. The wg_transform is only the unix side of MF transforms / DMO, there's really no intention to have it used anywhere else. ``` For that matter, I could also see an argument for moving the whole thing to the back end, and handling the "frontend needs to explicitly invalidate the format" problem by adding a new unixlib API for it. (Or maybe just destroying and recreating the wg_transform? That's not ideal if it happens frequently, but I don't know if that's the case.) ``` I this this would be even worse, and we will need to pass the format to the transform anyway. We cannot destroy the transform, we would lose the decoder state.
``` For that matter, I could also see an argument for moving the whole thing to the back end ``` Moreover, and especially for the patch introducing the format change detection, I don't see how it doesn't fit that definition now. There's no information provided from the frontend anymore, and change is detected on the backend only. I'm not sure it'll stand for all the special cases where format change should be returned, but as far as I could check it seems to work okay that way with the few games I tried it with. The frontend still needs to query the new format in the next change but that's all and for me things aren't "split" anymore. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/61#note_927
On Tue May 17 18:24:24 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list: ``` On 5/17/22 00:48, Rémi Bernon (@rbernon) wrote:
On Tue May 17 03:55:11 2022 +0000, **** wrote:
On 5/13/22 04:48, Rémi Bernon wrote:
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index b6cc51aecb7..9edcd72754f 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -52,17 +52,27 @@ struct wg_transform guint input_max_length; GstAtomicQueue *output_queue; GstSample *output_sample; + bool output_caps_changed; GstCaps *output_caps; }; To be clear, what bothered me about the last iteration is that the format-change logic feels awkwardly split in half between the frontend and backend. Hence my proposal to move it *entirely* to the frontend—including keeping track of the previous type there, comparing caps, and returning MF_E_TRANSFORM_STREAM_CHANGE from the front end. So the existence of this field still feels awkward to me. Did I forget a discussion again about why it's necessary? For that matter, I could also see an argument for moving the whole thing to the back end, and handling the "frontend needs to explicitly invalidate the format" problem by adding a new unixlib API for it. (Or maybe just destroying and recreating the wg_transform? That's not ideal if it happens frequently, but I don't know if that's the case.)
This is imo only making everything more complicated than it needs to be. We would have to copy back the sample and all its attributes somewhere in the transform, and copy it again on the next call, with a special conditional case just for that. I'm not sure I understand. Isn't it just a matter of: hr = wg_transform_get_buffer(wg_transform, &format); if (hr != S_OK) break; if (!wg_format_is_equal(&format, &transform->stream_format)) { transform->stream_format = format; // fill other flags and return an empty sample... return MF_E_TRANSFORM_STREAM_CHANGE; } wg_transform_copy_buffer(wg_transform, &wg_sample); ... Not committed to the function names, but it's no coincidence that's the pattern we already use for the parser. (It's also not immediately obvious to me that the two unixlib functions can't be merged together somehow, using a NULL as the wg_sample pointer. Even thinking ahead towards zero-copy it's not clear that's untenable. I'm sure it's not the API you had in mind, and maybe I'm forgetting some consideration, but...)
Although I don't really see how the current proposal (or its previous versions) is so awkward, yes the frontend and backend are made to work together and share some logic. This is really just some internal API and I really think we should make it ad-hoc depending on our needs, for simplicity rather than purity. The wg_transform is only the unix side of MF transforms / DMO, there's really no intention to have it used anywhere else. I understand that viewpoint, and maybe this is a personal thing, but I find it distinctly easier to reason about (and hence maintain) an internal API when it's designed like a standalone "pure" API layer. For whatever that means. That's especially true in the case of winegstreamer, which is designed to be an backend to several different media APIs. (Note also that thanks to Anton's work, we already use the wg_transform as a backend for DirectShow filters, and I think it'd be quite reasonable to use it for other objects in the future.)
I'm not sure I understand. Isn't it just a matter of: hr = wg_transform_get_buffer(wg_transform, &format); if (hr != S_OK) break; if (!wg_format_is_equal(&format, &transform->stream_format)) { transform->stream_format = format; // fill other flags and return an empty sample... return MF_E_TRANSFORM_STREAM_CHANGE; } wg_transform_copy_buffer(wg_transform, &wg_sample); ... Not committed to the function names, but it's no coincidence that's the pattern we already use for the parser. (It's also not immediately obvious to me that the two unixlib functions can't be merged together somehow, using a NULL as the wg_sample pointer. Even thinking ahead towards zero-copy it's not clear that's untenable.
Well it's actually pretty clear that it's not going to be possible to implement it unless you move most of wg_transform_read_data logic out of the unix side, to the PE-side, exposing additional entry points for each of the steps it goes through (which is what you are doing there). I don't think it's a good idea, because: 1) The entry points can actually be reduced to push_data / read_data operations, which intuitively maps to the MF transforms or the DMO ProcessInput / ProcessOutput methods. 2) It complicates the unixlib API by multiplying entry points, although they are only going to work when called in a specific sequence. To get zero-copy you'll have to add even more entry points, to provide the output buffer beforehand, and release it after the processing. 3) This specific call sequence (although some steps such as format change checks are not always useful, but will probably still be needed every time) will have to be duplicated in each transform when it can be factored in a single place.
I understand that viewpoint, and maybe this is a personal thing, but I find it distinctly easier to reason about (and hence maintain) an internal API when it's designed like a standalone "pure" API layer. For whatever that means. That's especially true in the case of winegstreamer, which is designed to be an backend to several different media APIs.
(Note also that thanks to Anton's work, we already use the wg_transform as a backend for DirectShow filters, and I think it'd be quite reasonable to use it for other objects in the future.)
Sure, and I think that the two entry point model where you push a input sample, then try to fill an output sample, is both very simple to use and versatile. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/61#note_951
On 5/17/22 13:47, Rémi Bernon (@rbernon) wrote:
Well it's actually pretty clear that it's not going to be possible to implement it unless you move most of wg_transform_read_data logic out of the unix side, to the PE-side, exposing additional entry points for each of the steps it goes through (which is what you are doing there).
I don't think it's a good idea, because:
1) The entry points can actually be reduced to push_data / read_data operations, which intuitively maps to the MF transforms or the DMO ProcessInput / ProcessOutput methods.
But it doesn't map nicely to DirectShow, and it only works because ProcessOutput() is kind of badly designed. To be clear, I don't object in principle to having an internal API that closely matches a badly designed external one, but that's not quite what we have here. Some aspects of the Media Foundation API are implemented on the PE side, and the way in which they're split up feels awkward and less than fully intuitive to me.
2) It complicates the unixlib API by multiplying entry points, although they are only going to work when called in a specific sequence.
My proposal as given (and wg_parser as it exists, for that matter) is not great because get_buffer() kind of implies that it consumes a buffer, which it doesn't. But if you rename the first API to something like "wg_transform_get_output_format()" it becomes clearer. The point is that it doesn't actually affect the state and hence is well-defined at any point.
To get zero-copy you'll have to add even more entry points, to provide the output buffer beforehand, and release it after the processing.
I'm confused about this, because this doesn't match my (somewhat dim) awareness of what would be necessary for zero-copy. So maybe it would help to hash that out now completely. My understanding is: * in some cases we can and should transform data in-place (perhaps because the PE side expects it, not that I'm aware of any such cases). In that case we should feed a writable GstBuffer into the pipeline, and if the output buffer was not the same as the input buffer, blit the latter into the former. * if we cannot transform data in-place (either because GStreamer doesn't support it or because it'd confuse the PE side, not that I'm aware of any such cases there either), we should try to let GStreamer elements fill buffers allocated by the PE side. In order to achieve this we should set up a GstBufferPool ahead of time containing a fixed number of buffers. (I think we can guarantee we won't run out?) If the output buffer comes from this pool, we can retrieve the corresponding input buffer (the original intent of struct wg_sample) and return that pointer to user space. If not, we should call gst_buffer_pool_acquire_buffer() ourselves and blit the output buffer into it. In both cases, though, it doesn't make sense to force a specific output buffer unless it's the same as the input buffer, which means there's no need for an extra API. Is there some extra consideration I'm missing here? The other consequence that falls out from this is that the act of pushing data will inherently cause data to be written; i.e. there is no concept of "peek" quite like pipes. On the other hand, we can still "peek" at the last output buffer (and, more importantly, its associated metadata) without actually dequeuing it (and hence without changing the wg_transform state at all), which is kind of what I was trying to propose in the first place. It's not obvious to me why this is problematic; am I missing something here as well?
3) This specific call sequence (although some steps such as format change checks are not always useful, but will probably still be needed every time) will have to be duplicated in each transform when it can be factored in a single place.
Sure. On the other hand, we already have (somewhat misleadingly named) helpers that do most of the sample post-processing already. Maybe I'm missing a reason that they couldn't be extended to cover the entirety of ProcessOutput()...?
participants (2)
-
Rémi Bernon -
Zebediah Figura