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.
From: Rémi Bernon rbernon@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@codeweavers.com --- dlls/winegstreamer/wg_transform.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 21392a82509..dcc334caf7a 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -114,7 +114,18 @@ static GstElement *transform_find_element(GstElementFactoryListType type, GstCap for (tmp = transforms; tmp != NULL && element == NULL; tmp = tmp->next) { name = gst_plugin_feature_get_name(GST_PLUGIN_FEATURE(tmp->data)); - if (!(element = gst_element_factory_create(GST_ELEMENT_FACTORY(tmp->data), NULL))) + + /* VA-API plugins are not supported at the moment, they do not support + * buffer pool negociation and will consistently ignore our pool, as + * it is not VA-API compatible, but also our plane alignment requirements + * from video meta. They also are asynchronous by design, which isn't + * going to let us implement zero-copy in the transforms reliably, and + * vaapidecodebin includes a videoconvert element which does more than + * we would like. + */ + if (!strncmp(name, "vaapi", 5)) + GST_FIXME("Ignoring unsupported %s plugin", name); + else if (!(element = gst_element_factory_create(GST_ELEMENT_FACTORY(tmp->data), NULL))) GST_WARNING("Failed to create %s element.", name); } gst_plugin_feature_list_free(transforms);
On 5/12/22 03:15, Rémi Bernon wrote:
From: Rémi Bernon rbernon@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@codeweavers.com
dlls/winegstreamer/wg_transform.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 21392a82509..dcc334caf7a 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -114,7 +114,18 @@ static GstElement *transform_find_element(GstElementFactoryListType type, GstCap for (tmp = transforms; tmp != NULL && element == NULL; tmp = tmp->next) { name = gst_plugin_feature_get_name(GST_PLUGIN_FEATURE(tmp->data));
if (!(element = gst_element_factory_create(GST_ELEMENT_FACTORY(tmp->data), NULL)))
/* VA-API plugins are not supported at the moment, they do not support* buffer pool negociation and will consistently ignore our pool, as* it is not VA-API compatible, but also our plane alignment requirements* from video meta. They also are asynchronous by design, which isn't* going to let us implement zero-copy in the transforms reliably, and* vaapidecodebin includes a videoconvert element which does more than* we would like.*/
I'll admit, none of these sound like good reasons to ignore vaapi.
Regarding allocation: the source is not actually required to use an allocator proposed by the sink in an ALLOCATION query. The documentation isn't really clear about this, but I asked for a clarification from the GStreamer developers. vaapi's behaviour here is not a bug.
Regarding asynchronicity: It's not obvious from the comment why asynchronicity implies that zero-copy can't be done. Regardless, though, there's no API guarantee that filters are synchronous, including decoders. We shouldn't hardcode vaapi on those grounds.
Regarding videoconvert: why is this a problem?
To summarize, it's not clear from the comment why any of these things are a problem, and, even assuming they are, none of them inhere to vaapi specifically.
While zero-copy is desirable, I don't think it should be an absolute requirement if we can't easily guarantee it, and the details regarding ALLOCATION queries suggests that we indeed can't.
It may be that hardware plugins provide consistently worse performance than anything else, and should be disabled on those grounds. (Perhaps even for the above reasons—although I'd find it surprising that a CPU copy is the bottleneck and not the download itself—and in any case the comment doesn't make that clear). It's not immediately clear that hardware plugins are *always* going to provide worse performance, regardless of CPU setup, but it also wouldn't surprise me if so.
Ideally this should be a decision made on a lower level than Wine, but I also recognize that it's going to be hard to do so, since it depends on a lot of context, and I think it's fine to put code in Wine for those reasons. If we do, though:
* I think we should match elements based on GST_ELEMENT_FACTORY_KLASS_HARDWARE than string-matching the element name;
* ideally we should disprefer hardware decoders rather than disabling them completely.
if (!strncmp(name, "vaapi", 5))GST_FIXME("Ignoring unsupported %s plugin", name);else if (!(element = gst_element_factory_create(GST_ELEMENT_FACTORY(tmp->data), NULL))) GST_WARNING("Failed to create %s element.", name); } gst_plugin_feature_list_free(transforms);
From: Rémi Bernon rbernon@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@codeweavers.com --- dlls/winegstreamer/wg_transform.c | 75 +++++++++++++++++++++++-------- 1 file changed, 57 insertions(+), 18 deletions(-)
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index dcc334caf7a..87baa80e840 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);
@@ -173,10 +210,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; @@ -205,9 +242,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); @@ -215,13 +252,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;
@@ -322,7 +360,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); @@ -336,8 +373,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) @@ -445,6 +482,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;
@@ -461,15 +499,16 @@ 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; GST_INFO("Cannot read %u bytes, no output available", sample->max_size); return STATUS_SUCCESS; } + output_buffer = gst_sample_get_buffer(transform->output_sample);
- if ((status = read_transform_output_data(transform->output_buffer, sample))) + if ((status = read_transform_output_data(output_buffer, sample))) { sample->size = 0; return status; @@ -477,8 +516,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;
From: Rémi Bernon rbernon@codeweavers.com
To support formats with no forced width/height in the H264 transform.
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@codeweavers.com --- dlls/mf/tests/mf.c | 1 - dlls/winegstreamer/h264_decoder.c | 6 ++++++ dlls/winegstreamer/wg_format.c | 4 ++++ 3 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 8f2c050e3c6..97c0c94159b 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -7160,7 +7160,6 @@ 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) 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..58960cd632a 100644 --- a/dlls/winegstreamer/h264_decoder.c +++ b/dlls/winegstreamer/h264_decoder.c @@ -75,6 +75,12 @@ 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; + 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..d313c285d73 100644 --- a/dlls/winegstreamer/wg_format.c +++ b/dlls/winegstreamer/wg_format.c @@ -399,6 +399,10 @@ static GstCaps *wg_format_to_caps_video(const struct wg_format *format) { 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); } } return caps;
From: Rémi Bernon rbernon@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@codeweavers.com --- dlls/winegstreamer/gst_private.h | 3 ++- dlls/winegstreamer/h264_decoder.c | 9 ++++++++- dlls/winegstreamer/main.c | 6 ++++-- dlls/winegstreamer/quartz_transform.c | 2 +- dlls/winegstreamer/unixlib.h | 1 + dlls/winegstreamer/wg_transform.c | 14 ++++++++++++++ dlls/winegstreamer/wma_decoder.c | 2 +- 7 files changed, 31 insertions(+), 6 deletions(-)
diff --git a/dlls/winegstreamer/gst_private.h b/dlls/winegstreamer/gst_private.h index 2a4b16079c1..159143d7e54 100644 --- a/dlls/winegstreamer/gst_private.h +++ b/dlls/winegstreamer/gst_private.h @@ -97,7 +97,8 @@ struct wg_transform *wg_transform_create(const struct wg_format *input_format, const struct wg_format *output_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); +HRESULT wg_transform_read_data(struct wg_transform *transform, struct wg_sample *sample, + 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 58960cd632a..68f4ba7562d 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; };
@@ -564,7 +565,13 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (wg_sample->max_size < info.cbSize) hr = MF_E_BUFFERTOOSMALL; else - hr = wg_transform_read_data(decoder->wg_transform, wg_sample); + hr = wg_transform_read_data(decoder->wg_transform, wg_sample, &decoder->format); + + if (hr == MF_E_TRANSFORM_STREAM_CHANGE) + { + samples[0].dwStatus |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE; + *status |= MFT_OUTPUT_DATA_BUFFER_FORMAT_CHANGE; + }
mf_destroy_wg_sample(wg_sample); return hr; diff --git a/dlls/winegstreamer/main.c b/dlls/winegstreamer/main.c index c3adbb82d61..5075b3118cd 100644 --- a/dlls/winegstreamer/main.c +++ b/dlls/winegstreamer/main.c @@ -329,16 +329,18 @@ HRESULT wg_transform_push_data(struct wg_transform *transform, struct wg_sample return params.result; }
-HRESULT wg_transform_read_data(struct wg_transform *transform, struct wg_sample *sample) +HRESULT wg_transform_read_data(struct wg_transform *transform, struct wg_sample *sample, + struct wg_format *format) { struct wg_transform_read_data_params params = { .transform = transform, .sample = sample, + .format = format, }; NTSTATUS status;
- TRACE("transform %p, sample %p.\n", transform, sample); + TRACE("transform %p, sample %p, format %p.\n", transform, sample, format);
if ((status = __wine_unix_call(unix_handle, unix_wg_transform_read_data, ¶ms))) return HRESULT_FROM_NT(status); diff --git a/dlls/winegstreamer/quartz_transform.c b/dlls/winegstreamer/quartz_transform.c index 447f331d474..3b43f182ecb 100644 --- a/dlls/winegstreamer/quartz_transform.c +++ b/dlls/winegstreamer/quartz_transform.c @@ -342,7 +342,7 @@ static HRESULT WINAPI transform_sink_receive(struct strmbase_sink *pin, IMediaSa return hr; }
- hr = wg_transform_read_data(filter->transform, &output_wg_sample); + hr = wg_transform_read_data(filter->transform, &output_wg_sample, NULL); if (hr == MF_E_TRANSFORM_NEED_MORE_INPUT) { IMediaSample_Release(output_sample); diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 5911278530d..e8cdfaf7217 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -257,6 +257,7 @@ struct wg_transform_read_data_params { struct wg_transform *transform; struct wg_sample *sample; + struct wg_format *format; HRESULT result; };
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 87baa80e840..66f8da0a940 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -483,8 +483,10 @@ NTSTATUS wg_transform_read_data(void *args) struct wg_sample *sample = params->sample; GstBufferList *input = transform->input; GstBuffer *output_buffer; + struct wg_format format; GstFlowReturn ret; NTSTATUS status; + GstCaps *caps;
if (!gst_buffer_list_length(transform->input)) GST_DEBUG("Not input buffer queued"); @@ -508,6 +510,18 @@ NTSTATUS wg_transform_read_data(void *args) } output_buffer = gst_sample_get_buffer(transform->output_sample);
+ if (params->format && (caps = gst_sample_get_caps(transform->output_sample))) + { + wg_format_from_caps(&format, caps); + if (!wg_format_compare(&format, params->format)) + { + *params->format = format; + 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))) { sample->size = 0; diff --git a/dlls/winegstreamer/wma_decoder.c b/dlls/winegstreamer/wma_decoder.c index 71369add244..2bad84df0de 100644 --- a/dlls/winegstreamer/wma_decoder.c +++ b/dlls/winegstreamer/wma_decoder.c @@ -580,7 +580,7 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, wg_sample->size = 0; if (wg_sample->max_size < info.cbSize) hr = MF_E_BUFFERTOOSMALL; - else if (SUCCEEDED(hr = wg_transform_read_data(decoder->wg_transform, wg_sample))) + else if (SUCCEEDED(hr = wg_transform_read_data(decoder->wg_transform, wg_sample, NULL))) { if (wg_sample->flags & WG_SAMPLE_FLAG_INCOMPLETE) samples[0].dwStatus |= MFT_OUTPUT_DATA_BUFFER_INCOMPLETE;
From: Rémi Bernon rbernon@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@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 66f8da0a940..56ef49a68e7 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -435,6 +435,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; }
@@ -523,10 +524,7 @@ NTSTATUS wg_transform_read_data(void *args) }
if ((status = read_transform_output_data(output_buffer, sample))) - { - sample->size = 0; return status; - }
if (!(sample->flags & WG_SAMPLE_FLAG_INCOMPLETE)) {
On 5/12/22 03:15, Rémi Bernon (@rbernon) wrote:
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.
I'm sorry, but I still don't see why the wg_transform object needs to store the previous format. It may not make the API any more complex to you, but conceptually I find it more complex, and I'd rather avoid maintaining that complexity.
What is it that makes "peeking" for a sample untenable, or buffering it on the client side?
On Thu May 12 16:42:57 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list:
On 5/12/22 03:15, Rémi Bernon (@rbernon) wrote: > 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. > I'm sorry, but I still don't see why the wg_transform object needs to store the previous format. It may not make the API any more complex to you, but conceptually I find it more complex, and I'd rather avoid maintaining that complexity. What is it that makes "peeking" for a sample untenable, or buffering it on the client side?I'm sorry, but I still don't see why the wg_transform object needs to store the previous format. It may not make the API any more complex to you, but conceptually I find it more complex, and I'd rather avoid maintaining that complexity.
The `wg_transform` doesn't store the current format, it keeps the format of each buffer it queues.
What is it that makes "peeking" for a sample untenable, or buffering it on the client side?
There's no really such thing as "peeking" when `read_data` also pushes the input buffers, and when output presense also depends on it. Unless of course you consider that peeking should do that first, in which case it's not really just peeking.
Then, like I previously described, to be able to implement zero copy, we will need to have a buffer ready for the decoder to write to, before we generate any output, and so before we push the input data.
The buffer we will want to write to is the buffer that the MF transform user has given us, and we shouldn't write to it if the buffer format changes. In that particular case, yes, we will have to buffer the data instead, but that's the only case where we want to do it.
If "peeking" does everything because it has to (prepares output buffer, pushes input data, check whether output has been produced or if format has changed), it's not really just peeking anymore and it's exactly and everything that `read_data` is going to do.
On 5/12/22 12:12, Rémi Bernon (@rbernon) wrote:
On Thu May 12 16:42:57 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list:
On 5/12/22 03:15, Rémi Bernon (@rbernon) wrote: > 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. > I'm sorry, but I still don't see why the wg_transform object needs to store the previous format. It may not make the API any more complex to you, but conceptually I find it more complex, and I'd rather avoid maintaining that complexity. What is it that makes "peeking" for a sample untenable, or buffering it on the client side?I'm sorry, but I still don't see why the wg_transform object needs to store the previous format. It may not make the API any more complex to you, but conceptually I find it more complex, and I'd rather avoid maintaining that complexity.
The `wg_transform` doesn't store the current format, it keeps the format of each buffer it queues.
I'm sorry, that was an error on my part. I should have said not "store the previous format" but "know the previous format".
What is it that makes "peeking" for a sample untenable, or buffering it on the client side?
There's no really such thing as "peeking" when `read_data` also pushes the input buffers, and when output presense also depends on it. Unless of course you consider that peeking should do that first, in which case it's not really just peeking.
Then, like I previously described, to be able to implement zero copy, we will need to have a buffer ready for the decoder to write to, before we generate any output, and so before we push the input data.
The buffer we will want to write to is the buffer that the MF transform user has given us, and we shouldn't write to it if the buffer format changes. In that particular case, yes, we will have to buffer the data instead, but that's the only case where we want to do it.
If "peeking" does everything because it has to (prepares output buffer, pushes input data, check whether output has been produced or if format has changed), it's not really just peeking anymore and it's exactly and everything that `read_data` is going to do.
I guess "peek" implies that it writes the output data without removing it, so maybe that's a failure of language on my part. I don't necessarily mean we need to write the output data. We just need a way to retrieve *only* the caps (possibly with other metadata), without dequeuing anything else.
Granted, you can't implement a "peek" operation with zero-copy, either, but you also don't get "read" with quite the same semantics. At the moment it's hard for me to say what the zero-copy API should look like.
If nothing else it can be made into a separate syscall, like "get_format". Yes, having a separate syscall means more glue, but like I said, I'd rather have more syscalls with a clear and idiomatic organization, than less syscalls with a monolithic one.
On Thu May 12 18:24:35 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list:
On 5/12/22 03:15, Rémi Bernon wrote: > From: Rémi Bernon <rbernon@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@codeweavers.com> > --- > dlls/winegstreamer/wg_transform.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c > index 21392a82509..dcc334caf7a 100644 > --- a/dlls/winegstreamer/wg_transform.c > +++ b/dlls/winegstreamer/wg_transform.c > @@ -114,7 +114,18 @@ static GstElement *transform_find_element(GstElementFactoryListType type, GstCap > for (tmp = transforms; tmp != NULL && element == NULL; tmp = tmp->next) > { > name = gst_plugin_feature_get_name(GST_PLUGIN_FEATURE(tmp->data)); > - if (!(element = gst_element_factory_create(GST_ELEMENT_FACTORY(tmp->data), NULL))) > + > + /* VA-API plugins are not supported at the moment, they do not support > + * buffer pool negociation and will consistently ignore our pool, as > + * it is not VA-API compatible, but also our plane alignment requirements > + * from video meta. They also are asynchronous by design, which isn't > + * going to let us implement zero-copy in the transforms reliably, and > + * vaapidecodebin includes a videoconvert element which does more than > + * we would like. > + */ I'll admit, none of these sound like good reasons to ignore vaapi. Regarding allocation: the source is not actually required to use an allocator proposed by the sink in an ALLOCATION query. The documentation isn't really clear about this, but I asked for a clarification from the GStreamer developers. vaapi's behaviour here is not a bug. Regarding asynchronicity: It's not obvious from the comment why asynchronicity implies that zero-copy can't be done. Regardless, though, there's no API guarantee that filters are synchronous, including decoders. We shouldn't hardcode vaapi on those grounds. Regarding videoconvert: why is this a problem? To summarize, it's not clear from the comment why any of these things are a problem, and, even assuming they are, none of them inhere to vaapi specifically. While zero-copy is desirable, I don't think it should be an absolute requirement if we can't easily guarantee it, and the details regarding ALLOCATION queries suggests that we indeed can't. It may be that hardware plugins provide consistently worse performance than anything else, and should be disabled on those grounds. (Perhaps even for the above reasons—although I'd find it surprising that a CPU copy is the bottleneck and not the download itself—and in any case the comment doesn't make that clear). It's not immediately clear that hardware plugins are *always* going to provide worse performance, regardless of CPU setup, but it also wouldn't surprise me if so. Ideally this should be a decision made on a lower level than Wine, but I also recognize that it's going to be hard to do so, since it depends on a lot of context, and I think it's fine to put code in Wine for those reasons. If we do, though: * I think we should match elements based on GST_ELEMENT_FACTORY_KLASS_HARDWARE than string-matching the element name; * ideally we should disprefer hardware decoders rather than disabling them completely.
VA-API gets in the way of a simple implementation. We can workaround all of its defects, but it will make everything more complicated than it needs to be.
Yes, we can support their low quality plugins which ignore the buffer requirements that downstream explicitly requests (I'm not even talking about using an allocator we would provide), which means that we would have instead to 1) detect that they didn't, 2) do the plane alignment and copy ourselves, with all the additional code that it involves to do it properly for any of the various video formats that could be used.
Or, we can instead disable these plugins for now, as we are not interested in them, and as they are only ever going to be useful in the future when we can pass GPU images through to the presenter.
Tbh I even think we should instead hardcode the plugins we want to use, because this is delegating too much control to GStreamer plugin matching algorithm when the transforms, and the games using them, actually require a very specific behavior from the decoder.
Same for the implicit videoconvert, we don't want it because it makes the transform behavior different from how native transform behaves. We want to be able to change video format any time, even after some buffers have already been decoded and are queued for output.
On 5/12/22 14:00, Rémi Bernon (@rbernon) wrote:
On Thu May 12 18:24:35 2022 +0000, **** wrote:
Zebediah Figura replied on the mailing list:
On 5/12/22 03:15, Rémi Bernon wrote: > From: Rémi Bernon <rbernon@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@codeweavers.com> > --- > dlls/winegstreamer/wg_transform.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c > index 21392a82509..dcc334caf7a 100644 > --- a/dlls/winegstreamer/wg_transform.c > +++ b/dlls/winegstreamer/wg_transform.c > @@ -114,7 +114,18 @@ static GstElement *transform_find_element(GstElementFactoryListType type, GstCap > for (tmp = transforms; tmp != NULL && element == NULL; tmp = tmp->next) > { > name = gst_plugin_feature_get_name(GST_PLUGIN_FEATURE(tmp->data)); > - if (!(element = gst_element_factory_create(GST_ELEMENT_FACTORY(tmp->data), NULL))) > + > + /* VA-API plugins are not supported at the moment, they do not support > + * buffer pool negociation and will consistently ignore our pool, as > + * it is not VA-API compatible, but also our plane alignment requirements > + * from video meta. They also are asynchronous by design, which isn't > + * going to let us implement zero-copy in the transforms reliably, and > + * vaapidecodebin includes a videoconvert element which does more than > + * we would like. > + */ I'll admit, none of these sound like good reasons to ignore vaapi. Regarding allocation: the source is not actually required to use an allocator proposed by the sink in an ALLOCATION query. The documentation isn't really clear about this, but I asked for a clarification from the GStreamer developers. vaapi's behaviour here is not a bug. Regarding asynchronicity: It's not obvious from the comment why asynchronicity implies that zero-copy can't be done. Regardless, though, there's no API guarantee that filters are synchronous, including decoders. We shouldn't hardcode vaapi on those grounds. Regarding videoconvert: why is this a problem? To summarize, it's not clear from the comment why any of these things are a problem, and, even assuming they are, none of them inhere to vaapi specifically. While zero-copy is desirable, I don't think it should be an absolute requirement if we can't easily guarantee it, and the details regarding ALLOCATION queries suggests that we indeed can't. It may be that hardware plugins provide consistently worse performance than anything else, and should be disabled on those grounds. (Perhaps even for the above reasons—although I'd find it surprising that a CPU copy is the bottleneck and not the download itself—and in any case the comment doesn't make that clear). It's not immediately clear that hardware plugins are *always* going to provide worse performance, regardless of CPU setup, but it also wouldn't surprise me if so. Ideally this should be a decision made on a lower level than Wine, but I also recognize that it's going to be hard to do so, since it depends on a lot of context, and I think it's fine to put code in Wine for those reasons. If we do, though: * I think we should match elements based on GST_ELEMENT_FACTORY_KLASS_HARDWARE than string-matching the element name; * ideally we should disprefer hardware decoders rather than disabling them completely.VA-API gets in the way of a simple implementation. We can workaround all of its defects, but it will make everything more complicated than it needs to be.
Yes, we can support their low quality plugins which ignore the buffer requirements that downstream explicitly requests (I'm not even talking about using an allocator we would provide), which means that we would have instead to 1) detect that they didn't, 2) do the plane alignment and copy ourselves, with all the additional code that it involves to do it properly for any of the various video formats that could be used.
I don't think that these things are prohibitive.
We can detect that the upstream element isn't using our pool simply by checking the pool. We can copy, without manually implementing alignment logic, by using GstVideoFrame APIs.
I recognize that I'm asking for more work to be done than you were planning on doing; that can be frustrating; I'm frankly happy to do the work myself. I'd just rather it be done this way, since according to my understanding of the GStreamer API this is the right thing to do.
Or, we can instead disable these plugins for now, as we are not interested in them, and as they are only ever going to be useful in the future when we can pass GPU images through to the presenter.
Tbh I even think we should instead hardcode the plugins we want to use, because this is delegating too much control to GStreamer plugin matching algorithm when the transforms, and the games using them, actually require a very specific behavior from the decoder.
Same for the implicit videoconvert, we don't want it because it makes the transform behavior different from how native transform behaves. We want to be able to change video format any time, even after some buffers have already been decoded and are queued for output.
I really don't think we should be hardcoding plugins, especially plugins that aren't part of core, base, or good. (That we already hardcode some plugins for wg_parser is not ideal and should probably be changed.) Nor do I think we should depend on implementation details if we can at all avoid it.
-
Rémi Bernon -
Rémi Bernon (＠rbernon)
-
Zebediah Figura