[PATCH 0/5] MR22: winegstreamer: Implement more sample attributes and timestamps.
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, 3 insertions(+), 1 deletion(-) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 49c7bfaa927..58eb1286401 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -389,7 +389,9 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, struct wg_sample * memcpy(sample->data, info.data, sample->size); gst_buffer_unmap(buffer, &info); - gst_buffer_resize(buffer, sample->size, -1); + + if (sample->flags & WG_SAMPLE_FLAG_INCOMPLETE) + gst_buffer_resize(buffer, sample->size, -1); GST_INFO("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); return STATUS_SUCCESS; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/22
On 5/2/22 16:24, Rémi Bernon wrote:
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, 3 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 49c7bfaa927..58eb1286401 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -389,7 +389,9 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, struct wg_sample *
memcpy(sample->data, info.data, sample->size); gst_buffer_unmap(buffer, &info); - gst_buffer_resize(buffer, sample->size, -1); + + if (sample->flags & WG_SAMPLE_FLAG_INCOMPLETE) + gst_buffer_resize(buffer, sample->size, -1);
GST_INFO("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); return STATUS_SUCCESS;
I guess this is fine, although it seems unnecessary? Is there a particular reason to do this?
From: Rémi Bernon <rbernon(a)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(a)codeweavers.com> --- dlls/mf/tests/mf.c | 2 +- dlls/winegstreamer/h264_decoder.c | 6 ++++++ dlls/winegstreamer/wg_format.c | 4 ++++ 3 files changed, 11 insertions(+), 1 deletion(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 75c873d5dc0..4d7ca4d825c 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6979,7 +6979,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 b98f137eb82..52d0fbaef27 100644 --- a/dlls/winegstreamer/h264_decoder.c +++ b/dlls/winegstreamer/h264_decoder.c @@ -74,6 +74,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 ff9238a6a69..7f3b2b0a7dd 100644 --- a/dlls/winegstreamer/wg_format.c +++ b/dlls/winegstreamer/wg_format.c @@ -390,6 +390,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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/22
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=113952 Your paranoid android. === debian11 (32 bit report) === mf: mf.c:6944: Test succeeded inside todo block: got length 0 === debian11 (32 bit Arabic:Morocco report) === mf: mf.c:6944: Test succeeded inside todo block: got length 0 === debian11 (32 bit German report) === mf: mf.c:6944: Test succeeded inside todo block: got length 0 === debian11 (32 bit French report) === mf: mf.c:6944: Test succeeded inside todo block: got length 0 === debian11 (32 bit Hebrew:Israel report) === mf: mf.c:6944: Test succeeded inside todo block: got length 0 === debian11 (32 bit Hindi:India report) === mf: mf.c:6944: Test succeeded inside todo block: got length 0 === debian11 (32 bit Japanese:Japan report) === mf: mf.c:6944: Test succeeded inside todo block: got length 0 === debian11 (32 bit Chinese:China report) === mf: mf.c:6944: Test succeeded inside todo block: got length 0 === debian11 (32 bit WoW report) === mf: mf.c:6944: Test succeeded inside todo block: got length 0 === debian11 (64 bit WoW report) === mf: mf.c:6944: Test succeeded inside todo block: got length 0
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 | 4 -- dlls/winegstreamer/h264_decoder.c | 8 +++ dlls/winegstreamer/unixlib.h | 1 + dlls/winegstreamer/wg_transform.c | 84 ++++++++++++++++++++++++------- 4 files changed, 74 insertions(+), 23 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 4d7ca4d825c..1104f164225 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6966,20 +6966,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 52d0fbaef27..bd55bb2f504 100644 --- a/dlls/winegstreamer/h264_decoder.c +++ b/dlls/winegstreamer/h264_decoder.c @@ -49,6 +49,7 @@ struct h264_decoder IMFMediaType *input_type; IMFMediaType *output_type; + struct wg_format stream_format; struct wg_transform *wg_transform; }; @@ -560,11 +561,18 @@ static HRESULT WINAPI transform_ProcessOutput(IMFTransform *iface, DWORD flags, if (FAILED(hr = mf_create_wg_sample(samples[0].pSample, &wg_sample))) return hr; + wg_sample->format = &decoder->stream_format; if (wg_sample->max_size < info.cbSize) hr = MF_E_BUFFERTOOSMALL; else hr = wg_transform_read_data(decoder->wg_transform, 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; + } + mf_destroy_wg_sample(wg_sample); return hr; } diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index f4e2ea4966b..7a31020fb87 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -122,6 +122,7 @@ struct wg_sample UINT32 max_size; UINT32 size; BYTE *data; + struct wg_format *format; }; struct wg_parser_buffer diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 58eb1286401..e0b0ce77a44 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -51,40 +51,73 @@ struct wg_transform GstBufferList *input; guint input_max_length; GstAtomicQueue *output_queue; - GstBuffer *output_buffer; + GstSample *output_sample; + GstCaps *sink_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->sink_caps, NULL, NULL))) + gst_atomic_queue_push(transform->output_queue, sample); - return GST_FLOW_OK; + gst_buffer_unref(buffer); + + return sample ? GST_FLOW_OK : GST_FLOW_ERROR; +} + +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->sink_caps); + transform->sink_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->sink_caps); gst_atomic_queue_unref(transform->output_queue); free(transform); @@ -162,10 +195,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 +227,9 @@ NTSTATUS wg_transform_create(void *args) if (!transform->my_src) goto out; - if (!(sink_caps = wg_format_to_caps(&output_format))) + if (!(transform->sink_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->sink_caps))) goto out; transform->my_sink = gst_pad_new_from_template(template, "sink"); g_object_unref(template); @@ -204,13 +237,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->sink_caps, 0)); if (!(raw_caps = gst_caps_new_empty_simple(media_type))) goto out; @@ -306,7 +340,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); @@ -320,8 +353,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->sink_caps) + gst_caps_unref(transform->sink_caps); if (transform->my_src) gst_object_unref(transform->my_src); if (src_caps) @@ -403,8 +436,10 @@ NTSTATUS wg_transform_read_data(void *args) struct wg_transform *transform = params->transform; struct wg_sample *sample = params->sample; GstBufferList *input = transform->input; + struct wg_format format; GstFlowReturn ret; NTSTATUS status; + GstCaps *caps; if (!gst_buffer_list_length(transform->input)) GST_DEBUG("Not input buffer queued"); @@ -419,7 +454,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; @@ -427,7 +462,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; } - if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample))) + { + wg_format_from_caps(&format, caps); + if (!wg_format_compare(&format, sample->format)) + { + *sample->format = format; + params->result = MF_E_TRANSFORM_STREAM_CHANGE; + return STATUS_SUCCESS; + } + } + + if ((status = read_transform_output_data(gst_sample_get_buffer(transform->output_sample), sample))) { sample->size = 0; return status; @@ -435,8 +481,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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/22
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=113953 Your paranoid android. === build (build log) === error: patch failed: dlls/mf/tests/mf.c:6966 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mf/tests/mf.c:6966 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mf/tests/mf.c:6966 Task: Patch failed to apply
On 5/2/22 16:24, Rémi Bernon wrote:
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 | 4 -- dlls/winegstreamer/h264_decoder.c | 8 +++ dlls/winegstreamer/unixlib.h | 1 + dlls/winegstreamer/wg_transform.c | 84 ++++++++++++++++++++++++------- 4 files changed, 74 insertions(+), 23 deletions(-)
This commit is difficult to review, because it does several things at once. At least it'd be nice to split it up, along the lines of: (1) queue GstSample objects instead of GstBuffer (2) compare format pointers and report when the format changes
diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index f4e2ea4966b..7a31020fb87 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -122,6 +122,7 @@ struct wg_sample UINT32 max_size; UINT32 size; BYTE *data; + struct wg_format *format; };
struct wg_parser_buffer
I was doubtful about introducing this abstraction in the first place, before we've actually implemented zero-copy, and indeed I'm finding it difficult to reason around... Anyway, it's probably not worth restructuring things at this rate, but I have a hard time feeling like "format" belongs here. It's an in-out parameter, and as an "out" parameter it is a property of the buffer, but as an "in" parameter it isn't, really. Actually, for that matter, why do we need to store the wg_format on the PE side? Can't we just store it on the unix side? (Maybe even check it in the chain function instead of in read_data, and store it in a flag on the object with gst_mini_object_set_qdata(), and then that'd remove the need to allocate GstSample objects. I don't remember if there was another impetus for using those?)
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 58eb1286401..e0b0ce77a44 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -51,40 +51,73 @@ struct wg_transform GstBufferList *input; guint input_max_length; GstAtomicQueue *output_queue; - GstBuffer *output_buffer; + GstSample *output_sample; + GstCaps *sink_caps;
I find "sink_caps" misleading; I expect "sink" to refer to the transform as a whole, or (somewhat by extension) the GStreamer bin it's wrapping. Hence "source" seems more correct, or (to match with the other members) "output".
};
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->sink_caps, NULL, NULL))) + gst_atomic_queue_push(transform->output_queue, sample);
- return GST_FLOW_OK; + gst_buffer_unref(buffer); + + return sample ? GST_FLOW_OK : GST_FLOW_ERROR;
Something of a nitpick, but I'm never a fan of checking the same condition in two subsequent conditionals. I'd rather just early-return GST_FLOW_ERROR here.
@@ -427,7 +462,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; }
- if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample))) + { + wg_format_from_caps(&format, caps); + if (!wg_format_compare(&format, sample->format)) + { + *sample->format = format; + params->result = MF_E_TRANSFORM_STREAM_CHANGE; + return STATUS_SUCCESS; + } + }
This looks wrong; aren't we dropping the sample data on the floor?
+ + if ((status = read_transform_output_data(gst_sample_get_buffer(transform->output_sample), sample))) { sample->size = 0; return status; @@ -435,8 +481,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(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 | 6 ------ dlls/winegstreamer/mfplat.c | 17 +++++++++++++++++ dlls/winegstreamer/unixlib.h | 5 +++++ dlls/winegstreamer/wg_transform.c | 25 +++++++++++++++++++++++++ 4 files changed, 47 insertions(+), 6 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 1104f164225..0f38f93ff0e 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6321,19 +6321,15 @@ static void test_wma_decoder(void) ok(flags == 0, "got flags %#lx\n", flags); time = 0xdeadbeef; hr = IMFSample_GetSampleTime(sample, &time); - todo_wine ok(hr == S_OK, "GetSampleTime returned %#lx\n", hr); - todo_wine ok(time == i * 928798, "got time %I64d\n", time); duration = 0xdeadbeef; hr = IMFSample_GetSampleDuration(sample, &duration); - todo_wine ok(hr == S_OK, "GetSampleDuration returned %#lx\n", hr); if (output.dwStatus == MFT_OUTPUT_DATA_BUFFER_INCOMPLETE || broken(output.dwStatus == (MFT_OUTPUT_DATA_BUFFER_INCOMPLETE|7))) { ok(length == wmadec_block_size, "got length %lu\n", length); - todo_wine ok(duration == 928798, "got duration %I64d\n", duration); check_sample_pcm16(sample, wmadec_data, output_file, TRUE); wmadec_data += wmadec_block_size; @@ -7059,9 +7055,7 @@ static void test_h264_decoder(void) /* doesn't matter what frame rate we've selected, duration is defined by the stream */ duration = 0xdeadbeef; hr = IMFSample_GetSampleDuration(output.pSample, &duration); - todo_wine ok(hr == S_OK, "GetSampleDuration returned %#lx\n", hr); - todo_wine ok(duration - 333666 <= 2, "got duration %I64d\n", duration); /* Win8 and before pad the data with garbage instead of original diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index 97e27bb7301..9dcfc558963 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -903,6 +903,7 @@ HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) { DWORD current_length, max_length; struct mf_sample *mf_sample; + LONGLONG time, duration; BYTE *buffer; HRESULT hr; @@ -913,6 +914,17 @@ HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) if (FAILED(hr = IMFMediaBuffer_Lock(mf_sample->media_buffer, &buffer, &max_length, ¤t_length))) goto out; + if (SUCCEEDED(IMFSample_GetSampleTime(sample, &time))) + { + mf_sample->wg_sample.flags |= WG_SAMPLE_FLAG_HAS_PTS; + mf_sample->wg_sample.pts = time; + } + if (SUCCEEDED(IMFSample_GetSampleDuration(sample, &duration))) + { + mf_sample->wg_sample.flags |= WG_SAMPLE_FLAG_HAS_DURATION; + mf_sample->wg_sample.duration = duration; + } + IMFSample_AddRef((mf_sample->sample = sample)); mf_sample->wg_sample.data = buffer; mf_sample->wg_sample.size = current_length; @@ -937,6 +949,11 @@ void mf_destroy_wg_sample(struct wg_sample *wg_sample) IMFMediaBuffer_SetCurrentLength(mf_sample->media_buffer, wg_sample->size); IMFMediaBuffer_Release(mf_sample->media_buffer); + if (wg_sample->flags & WG_SAMPLE_FLAG_HAS_PTS) + IMFSample_SetSampleTime(mf_sample->sample, wg_sample->pts); + if (wg_sample->flags & WG_SAMPLE_FLAG_HAS_DURATION) + IMFSample_SetSampleDuration(mf_sample->sample, wg_sample->duration); + IMFSample_Release(mf_sample->sample); free(mf_sample); } diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 7a31020fb87..1efc4fc439b 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -114,10 +114,15 @@ struct wg_format enum wg_sample_flag { WG_SAMPLE_FLAG_INCOMPLETE = 1, + WG_SAMPLE_FLAG_HAS_PTS = 2, + WG_SAMPLE_FLAG_HAS_DURATION = 4, }; struct wg_sample { + /* timestamp and duration are in 100-nanosecond units. */ + UINT64 pts; + UINT64 duration; UINT32 flags; UINT32 max_size; UINT32 size; diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index e0b0ce77a44..f394704b4fb 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -257,6 +257,9 @@ NTSTATUS wg_transform_create(void *args) * to match its expectations. */ transform->input_max_length = 16; + if (!(element = create_element("h264parse", "base")) + || !transform_append_element(transform, element, &first, &last)) + goto out; /* fallthrough */ case WG_MAJOR_TYPE_WMA: if (!(element = transform_find_element(GST_ELEMENT_FACTORY_TYPE_DECODER, src_caps, raw_caps)) @@ -395,6 +398,10 @@ NTSTATUS wg_transform_push_data(void *args) return STATUS_NO_MEMORY; } gst_buffer_fill(buffer, 0, sample->data, sample->size); + if (sample->flags & WG_SAMPLE_FLAG_HAS_PTS) + GST_BUFFER_PTS(buffer) = sample->pts * 100; + if (sample->flags & WG_SAMPLE_FLAG_HAS_DURATION) + GST_BUFFER_DURATION(buffer) = sample->duration * 100; gst_buffer_list_insert(transform->input, -1, buffer); GST_INFO("Copied %u bytes from sample %p to input buffer list", sample->size, sample); @@ -426,6 +433,24 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, struct wg_sample * if (sample->flags & WG_SAMPLE_FLAG_INCOMPLETE) gst_buffer_resize(buffer, sample->size, -1); + if (GST_BUFFER_PTS_IS_VALID(buffer)) + { + sample->flags |= WG_SAMPLE_FLAG_HAS_PTS; + sample->pts = GST_BUFFER_PTS(buffer) / 100; + } + if (GST_BUFFER_DURATION_IS_VALID(buffer)) + { + GstClockTime duration = GST_BUFFER_DURATION(buffer) / 100; + + duration = (duration * sample->size) / info.size; + GST_BUFFER_DURATION(buffer) -= duration * 100; + if (GST_BUFFER_PTS_IS_VALID(buffer)) + GST_BUFFER_PTS(buffer) += duration * 100; + + sample->flags |= WG_SAMPLE_FLAG_HAS_DURATION; + sample->duration = duration; + } + GST_INFO("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); return STATUS_SUCCESS; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/22
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=113954 Your paranoid android. === build (build log) === error: patch failed: dlls/mf/tests/mf.c:6966 error: patch failed: dlls/mf/tests/mf.c:6321 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mf/tests/mf.c:6966 error: patch failed: dlls/mf/tests/mf.c:6321 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mf/tests/mf.c:6966 error: patch failed: dlls/mf/tests/mf.c:6321 Task: Patch failed to apply
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 | 2 -- dlls/winegstreamer/mfplat.c | 5 +++++ dlls/winegstreamer/unixlib.h | 1 + dlls/winegstreamer/wg_transform.c | 4 ++++ 4 files changed, 10 insertions(+), 2 deletions(-) diff --git a/dlls/mf/tests/mf.c b/dlls/mf/tests/mf.c index 0f38f93ff0e..b98b3dfcf66 100644 --- a/dlls/mf/tests/mf.c +++ b/dlls/mf/tests/mf.c @@ -6309,9 +6309,7 @@ static void test_wma_decoder(void) ok(status == 0, "got status %#lx\n", status); value = 0xdeadbeef; hr = IMFSample_GetUINT32(sample, &MFSampleExtension_CleanPoint, &value); - todo_wine ok(hr == S_OK, "GetUINT32 MFSampleExtension_CleanPoint returned %#lx\n", hr); - todo_wine ok(value == 1, "got MFSampleExtension_CleanPoint %u\n", value); hr = IMFSample_GetTotalLength(sample, &length); ok(hr == S_OK, "GetTotalLength returned %#lx\n", hr); diff --git a/dlls/winegstreamer/mfplat.c b/dlls/winegstreamer/mfplat.c index 9dcfc558963..ee3a1f5e024 100644 --- a/dlls/winegstreamer/mfplat.c +++ b/dlls/winegstreamer/mfplat.c @@ -904,6 +904,7 @@ HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) DWORD current_length, max_length; struct mf_sample *mf_sample; LONGLONG time, duration; + UINT32 value; BYTE *buffer; HRESULT hr; @@ -924,6 +925,8 @@ HRESULT mf_create_wg_sample(IMFSample *sample, struct wg_sample **out) mf_sample->wg_sample.flags |= WG_SAMPLE_FLAG_HAS_DURATION; mf_sample->wg_sample.duration = duration; } + if (SUCCEEDED(IMFSample_GetUINT32(sample, &MFSampleExtension_CleanPoint, &value)) && value) + mf_sample->wg_sample.flags |= WG_SAMPLE_FLAG_SYNC_POINT; IMFSample_AddRef((mf_sample->sample = sample)); mf_sample->wg_sample.data = buffer; @@ -953,6 +956,8 @@ void mf_destroy_wg_sample(struct wg_sample *wg_sample) IMFSample_SetSampleTime(mf_sample->sample, wg_sample->pts); if (wg_sample->flags & WG_SAMPLE_FLAG_HAS_DURATION) IMFSample_SetSampleDuration(mf_sample->sample, wg_sample->duration); + if (wg_sample->flags & WG_SAMPLE_FLAG_SYNC_POINT) + IMFSample_SetUINT32(mf_sample->sample, &MFSampleExtension_CleanPoint, 1); IMFSample_Release(mf_sample->sample); free(mf_sample); diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index 1efc4fc439b..0a0e5b592da 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -116,6 +116,7 @@ enum wg_sample_flag WG_SAMPLE_FLAG_INCOMPLETE = 1, WG_SAMPLE_FLAG_HAS_PTS = 2, WG_SAMPLE_FLAG_HAS_DURATION = 4, + WG_SAMPLE_FLAG_SYNC_POINT = 8, }; struct wg_sample diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index f394704b4fb..ce88b91455b 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -402,6 +402,8 @@ NTSTATUS wg_transform_push_data(void *args) GST_BUFFER_PTS(buffer) = sample->pts * 100; if (sample->flags & WG_SAMPLE_FLAG_HAS_DURATION) GST_BUFFER_DURATION(buffer) = sample->duration * 100; + if (!(sample->flags & WG_SAMPLE_FLAG_SYNC_POINT)) + GST_BUFFER_FLAG_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT); gst_buffer_list_insert(transform->input, -1, buffer); GST_INFO("Copied %u bytes from sample %p to input buffer list", sample->size, sample); @@ -450,6 +452,8 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, struct wg_sample * sample->flags |= WG_SAMPLE_FLAG_HAS_DURATION; sample->duration = duration; } + if (!GST_BUFFER_FLAG_IS_SET(buffer, GST_BUFFER_FLAG_DELTA_UNIT)) + sample->flags |= WG_SAMPLE_FLAG_SYNC_POINT; GST_INFO("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); return STATUS_SUCCESS; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/22
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=113955 Your paranoid android. === build (build log) === error: patch failed: dlls/mf/tests/mf.c:6966 error: patch failed: dlls/mf/tests/mf.c:6321 error: patch failed: dlls/mf/tests/mf.c:6309 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mf/tests/mf.c:6966 error: patch failed: dlls/mf/tests/mf.c:6321 error: patch failed: dlls/mf/tests/mf.c:6309 Task: Patch failed to apply === debian11 (build log) === error: patch failed: dlls/mf/tests/mf.c:6966 error: patch failed: dlls/mf/tests/mf.c:6321 error: patch failed: dlls/mf/tests/mf.c:6309 Task: Patch failed to apply
On Thu May 5 00:35:50 2022 +0000, **** wrote:
Zebediah Figura (she/her) replied on the mailing list: ``` On 5/2/22 16:24, Rémi Bernon wrote:
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, 3 insertions(+), 1 deletion(-)
diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 49c7bfaa927..58eb1286401 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -389,7 +389,9 @@ static NTSTATUS read_transform_output_data(GstBuffer *buffer, struct wg_sample *
memcpy(sample->data, info.data, sample->size); gst_buffer_unmap(buffer, &info); - gst_buffer_resize(buffer, sample->size, -1); + + if (sample->flags & WG_SAMPLE_FLAG_INCOMPLETE) + gst_buffer_resize(buffer, sample->size, -1);
GST_INFO("Copied %u bytes, sample %p, flags %#x", sample->size, sample, sample->flags); return STATUS_SUCCESS; I guess this is fine, although it seems unnecessary? Is there a particular reason to do this?
Possibly avoiding whatever resize involves if it's not necessary, but I think some video buffers didn't expect to be resized.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/22#note_620
On Thu May 5 00:52:55 2022 +0000, **** wrote:
Zebediah Figura (she/her) replied on the mailing list: ``` On 5/2/22 16:24, Rémi Bernon wrote:
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 | 4 -- dlls/winegstreamer/h264_decoder.c | 8 +++ dlls/winegstreamer/unixlib.h | 1 + dlls/winegstreamer/wg_transform.c | 84 ++++++++++++++++++++++++------- 4 files changed, 74 insertions(+), 23 deletions(-)
This commit is difficult to review, because it does several things at once. At least it'd be nice to split it up, along the lines of: (1) queue GstSample objects instead of GstBuffer (2) compare format pointers and report when the format changes
diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index f4e2ea4966b..7a31020fb87 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -122,6 +122,7 @@ struct wg_sample UINT32 max_size; UINT32 size; BYTE *data; + struct wg_format *format; };
struct wg_parser_buffer I was doubtful about introducing this abstraction in the first place, before we've actually implemented zero-copy, and indeed I'm finding it difficult to reason around... Anyway, it's probably not worth restructuring things at this rate, but I have a hard time feeling like "format" belongs here. It's an in-out parameter, and as an "out" parameter it is a property of the buffer, but as an "in" parameter it isn't, really. Actually, for that matter, why do we need to store the wg_format on the PE side? Can't we just store it on the unix side? (Maybe even check it in the chain function instead of in read_data, and store it in a flag on the object with gst_mini_object_set_qdata(), and then that'd remove the need to allocate GstSample objects. I don't remember if there was another impetus for using those?) diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c index 58eb1286401..e0b0ce77a44 100644 --- a/dlls/winegstreamer/wg_transform.c +++ b/dlls/winegstreamer/wg_transform.c @@ -51,40 +51,73 @@ struct wg_transform GstBufferList *input; guint input_max_length; GstAtomicQueue *output_queue; - GstBuffer *output_buffer; + GstSample *output_sample; + GstCaps *sink_caps; I find "sink_caps" misleading; I expect "sink" to refer to the transform as a whole, or (somewhat by extension) the GStreamer bin it's wrapping. Hence "source" seems more correct, or (to match with the other members) "output". };
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->sink_caps, NULL, NULL))) + gst_atomic_queue_push(transform->output_queue, sample);
- return GST_FLOW_OK; + gst_buffer_unref(buffer); + + return sample ? GST_FLOW_OK : GST_FLOW_ERROR; Something of a nitpick, but I'm never a fan of checking the same condition in two subsequent conditionals. I'd rather just early-return GST_FLOW_ERROR here. @@ -427,7 +462,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; }
- if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample))) + { + wg_format_from_caps(&format, caps); + if (!wg_format_compare(&format, sample->format)) + { + *sample->format = format; + params->result = MF_E_TRANSFORM_STREAM_CHANGE; + return STATUS_SUCCESS; + } + } This looks wrong; aren't we dropping the sample data on the floor?
Zebediah Figura (she/her) replied on the mailing list: > diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h > index f4e2ea4966b..7a31020fb87 100644 > --- a/dlls/winegstreamer/unixlib.h > +++ b/dlls/winegstreamer/unixlib.h > @@ -122,6 +122,7 @@ struct wg_sample > UINT32 max_size; > UINT32 size; > BYTE *data; > + struct wg_format *format; > }; > > struct wg_parser_buffer I was doubtful about introducing this abstraction in the first place, before we've actually implemented zero-copy, and indeed I'm finding it difficult to reason around... Anyway, it's probably not worth restructuring things at this rate, but I have a hard time feeling like "format" belongs here. It's an in-out parameter, and as an "out" parameter it is a property of the buffer, but as an "in" parameter it isn't, really.
I think it is, in both ways. The format tells what the client believes the sample format is (even if it is uninitialized at the time it gives it to wg_transform), but it can describe what it expects in term of data size and layout for instance. If it doesn't match what we're going to give it, we return a format change notification and the new format.
Actually, for that matter, why do we need to store the wg_format on the PE side? Can't we just store it on the unix side?
(Maybe even check it in the chain function instead of in read_data, and store it in a flag on the object with gst_mini_object_set_qdata(), and then that'd remove the need to allocate GstSample objects. I don't remember if there was another impetus for using those?)
I don't see what benefit it would have, we need to return the information about the new format to the client anyway so that it can update the properties it exposes to the MF caller. GstSample seems a much nicer high level API to me, and that it is better to use it than than something low level like gst_mini_object_set_qdata. In either case you will need to allocate something, either the GstSample or the wg_format you keep on the buffers, because the format can change and because buffers can be queued. GstSample and caps is much cleaner imho. In any case I'd rather avoid another complete rewrite at this point, it's been months already since I started upstreaming these patches and I would prefer to keep things like this if it's not completely backwards.
@@ -427,7 +462,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; }
- if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample))) + { + wg_format_from_caps(&format, caps); + if (!wg_format_compare(&format, sample->format)) + { + *sample->format = format; + params->result = MF_E_TRANSFORM_STREAM_CHANGE; + return STATUS_SUCCESS; + } + }
This looks wrong; aren't we dropping the sample data on the floor?
No? We're not releasing output_sample there. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/22#note_621
On 5/5/22 02:20, Rémi Bernon (@rbernon) wrote:
diff --git a/dlls/winegstreamer/unixlib.h b/dlls/winegstreamer/unixlib.h index f4e2ea4966b..7a31020fb87 100644 --- a/dlls/winegstreamer/unixlib.h +++ b/dlls/winegstreamer/unixlib.h @@ -122,6 +122,7 @@ struct wg_sample UINT32 max_size; UINT32 size; BYTE *data; + struct wg_format *format; };
struct wg_parser_buffer
I was doubtful about introducing this abstraction in the first place, before we've actually implemented zero-copy, and indeed I'm finding it difficult to reason around...
Anyway, it's probably not worth restructuring things at this rate, but I have a hard time feeling like "format" belongs here. It's an in-out parameter, and as an "out" parameter it is a property of the buffer, but as an "in" parameter it isn't, really.
I think it is, in both ways. The format tells what the client believes the sample format is (even if it is uninitialized at the time it gives it to wg_transform), but it can describe what it expects in term of data size and layout for instance. If it doesn't match what we're going to give it, we return a format change notification and the new format.
Maybe. It's not an immediately clear design, though, which is why I'm inclined to separate it somehow. (But see below anyway...)
Actually, for that matter, why do we need to store the wg_format on the PE side? Can't we just store it on the unix side?
(Maybe even check it in the chain function instead of in read_data, and store it in a flag on the object with gst_mini_object_set_qdata(), and then that'd remove the need to allocate GstSample objects. I don't remember if there was another impetus for using those?)
I don't see what benefit it would have, we need to return the information about the new format to the client anyway so that it can update the properties it exposes to the MF caller.
Er, right, I forgot we still need to store the format anyway, so ignore that part :-) Still, the first part seems reasonable, unless I'm missing something? Or, frankly, we could make it output-only, and set the "format changed" flag entirely on the client side. The way things currently are, the logic is kind of split in two, and it feels awkward.
GstSample seems a much nicer high level API to me, and that it is better to use it than than something low level like gst_mini_object_set_qdata.
In either case you will need to allocate something, either the GstSample or the wg_format you keep on the buffers, because the format can change and because buffers can be queued. GstSample and caps is much cleaner imho.
In any case I'd rather avoid another complete rewrite at this point, it's been months already since I started upstreaming these patches and I would prefer to keep things like this if it's not completely backwards.
@@ -427,7 +462,18 @@ NTSTATUS wg_transform_read_data(void *args) return STATUS_SUCCESS; }
- if ((status = read_transform_output_data(transform->output_buffer, sample))) + if (sample->format && (caps = gst_sample_get_caps(transform->output_sample))) + { + wg_format_from_caps(&format, caps); + if (!wg_format_compare(&format, sample->format)) + { + *sample->format = format; + params->result = MF_E_TRANSFORM_STREAM_CHANGE; + return STATUS_SUCCESS; + } + }
This looks wrong; aren't we dropping the sample data on the floor?
No? We're not releasing output_sample there.
Sorry, not dropping it on the floor exactly, but we're also not filling the buffer, whereas the mfplat code (and the tests) looks like it expects the buffer to be filled.
participants (3)
-
Marvin -
Rémi Bernon -
Zebediah Figura (she/her)