[PATCH 0/5] MR22: winegstreamer: Implement more sample attributes and timestamps.
Rémi Bernon (@rbernon)
wine at gitlab.winehq.org
Thu May 5 02:20:21 CDT 2022
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 at 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 at 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
More information about the wine-devel
mailing list