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;