[PATCH v4 0/3] MR22: winegstreamer: Implement more sample attributes and timestamps.
Rémi Bernon (@rbernon)
wine at gitlab.winehq.org
Fri May 6 02:01:38 CDT 2022
On Thu May 5 22:55:39 2022 +0000, **** wrote:
> Zebediah Figura (she/her) replied on the mailing list:
> ```
> 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.
> ```
Zebediah Figura (she/her) replied on the mailing list:
>>> 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.
There are cases, like when the application calls ProcessMessage with
BEGIN_STREAMING, where we want to reset the format and receive format
change notification, again. Having the stream format stored on the MF
transform side lets us do that by reseting it to its default, and
trigger the format change again.
Call of Duty: Black Ops 3 depends on these notifications.
I'll try something, but that game, and the others using the H264 have
been really sensitive to tiny behavior changes.
>>>> @@ -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.
The tests check that the returned sample, on format change, is empty,
which should be the case, and mf_destroy_wg_sample will convert the wg
sample properties back to the MF side.
After a stream change you need, and can, call ProcessOutput once more,
immediately, to get the sample data.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/22#note_716
More information about the wine-devel
mailing list