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@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/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.