[PATCH v2 0/6] MR61: winegstreamer: Dynamic transform stream format change support.
Rémi Bernon (@rbernon)
wine at gitlab.winehq.org
Tue May 17 13:47:02 CDT 2022
On Tue May 17 18:24:24 2022 +0000, **** wrote:
> Zebediah Figura replied on the mailing list:
> ```
> On 5/17/22 00:48, Rémi Bernon (@rbernon) wrote:
> > On Tue May 17 03:55:11 2022 +0000, **** wrote:
> >> On 5/13/22 04:48, Rémi Bernon wrote:
> >>> diff --git a/dlls/winegstreamer/wg_transform.c b/dlls/winegstreamer/wg_transform.c
> >>> index b6cc51aecb7..9edcd72754f 100644
> >>> --- a/dlls/winegstreamer/wg_transform.c
> >>> +++ b/dlls/winegstreamer/wg_transform.c
> >>> @@ -52,17 +52,27 @@ struct wg_transform
> >>> guint input_max_length;
> >>> GstAtomicQueue *output_queue;
> >>> GstSample *output_sample;
> >>> + bool output_caps_changed;
> >>> GstCaps *output_caps;
> >>> };
> >> To be clear, what bothered me about the last iteration is that the
> >> format-change logic feels awkwardly split in half between the frontend
> >> and backend. Hence my proposal to move it *entirely* to the
> >> frontend—including keeping track of the previous type there, comparing
> >> caps, and returning MF_E_TRANSFORM_STREAM_CHANGE from the front end. So
> >> the existence of this field still feels awkward to me. Did I forget a
> >> discussion again about why it's necessary?
> >> For that matter, I could also see an argument for moving the whole thing
> >> to the back end, and handling the "frontend needs to explicitly
> >> invalidate the format" problem by adding a new unixlib API for it. (Or
> >> maybe just destroying and recreating the wg_transform? That's not ideal
> >> if it happens frequently, but I don't know if that's the case.)
> >
> > This is imo only making everything more complicated than it needs to
> be. We would have to copy back the sample and all its attributes
> somewhere in the transform, and copy it again on the next call, with a
> special conditional case just for that.
> I'm not sure I understand. Isn't it just a matter of:
> hr = wg_transform_get_buffer(wg_transform, &format);
> if (hr != S_OK) break;
> if (!wg_format_is_equal(&format, &transform->stream_format))
> {
> transform->stream_format = format;
> // fill other flags and return an empty sample...
> return MF_E_TRANSFORM_STREAM_CHANGE;
> }
> wg_transform_copy_buffer(wg_transform, &wg_sample);
> ...
> Not committed to the function names, but it's no coincidence that's the
> pattern we already use for the parser.
> (It's also not immediately obvious to me that the two unixlib functions
> can't be merged together somehow, using a NULL as the wg_sample pointer.
> Even thinking ahead towards zero-copy it's not clear that's untenable.
> I'm sure it's not the API you had in mind, and maybe I'm forgetting some
> consideration, but...)
> >
> > Although I don't really see how the current proposal (or its previous
> versions) is so awkward, yes the frontend and backend are made to work
> together and share some logic. This is really just some internal API and
> I really think we should make it ad-hoc depending on our needs, for
> simplicity rather than purity. The wg_transform is only the unix side of
> MF transforms / DMO, there's really no intention to have it used
> anywhere else.
> I understand that viewpoint, and maybe this is a personal thing, but I
> find it distinctly easier to reason about (and hence maintain) an
> internal API when it's designed like a standalone "pure" API layer. For
> whatever that means. That's especially true in the case of
> winegstreamer, which is designed to be an backend to several different
> media APIs.
> (Note also that thanks to Anton's work, we already use the wg_transform
> as a backend for DirectShow filters, and I think it'd be quite
> reasonable to use it for other objects in the future.)
> ```
> I'm not sure I understand. Isn't it just a matter of:
>
> hr = wg_transform_get_buffer(wg_transform, &format);
> if (hr != S_OK) break;
>
> if (!wg_format_is_equal(&format, &transform->stream_format))
> {
> transform->stream_format = format;
> // fill other flags and return an empty sample...
> return MF_E_TRANSFORM_STREAM_CHANGE;
> }
>
> wg_transform_copy_buffer(wg_transform, &wg_sample);
> ...
>
> Not committed to the function names, but it's no coincidence that's the
> pattern we already use for the parser.
>
> (It's also not immediately obvious to me that the two unixlib functions
> can't be merged together somehow, using a NULL as the wg_sample pointer.
> Even thinking ahead towards zero-copy it's not clear that's untenable.
Well it's actually pretty clear that it's not going to be possible to
implement it unless you move most of wg_transform_read_data logic out
of the unix side, to the PE-side, exposing additional entry points for
each of the steps it goes through (which is what you are doing there).
I don't think it's a good idea, because:
1) The entry points can actually be reduced to push_data / read_data
operations, which intuitively maps to the MF transforms or the DMO
ProcessInput / ProcessOutput methods.
2) It complicates the unixlib API by multiplying entry points, although
they are only going to work when called in a specific sequence. To
get zero-copy you'll have to add even more entry points, to provide
the output buffer beforehand, and release it after the processing.
3) This specific call sequence (although some steps such as format
change checks are not always useful, but will probably still be
needed every time) will have to be duplicated in each transform
when it can be factored in a single place.
> I understand that viewpoint, and maybe this is a personal thing, but I
> find it distinctly easier to reason about (and hence maintain) an
> internal API when it's designed like a standalone "pure" API layer. For
> whatever that means. That's especially true in the case of
> winegstreamer, which is designed to be an backend to several different
> media APIs.
>
> (Note also that thanks to Anton's work, we already use the wg_transform
> as a backend for DirectShow filters, and I think it'd be quite
> reasonable to use it for other objects in the future.)
Sure, and I think that the two entry point model where you push a input
sample, then try to fill an output sample, is both very simple to use
and versatile.
--
https://gitlab.winehq.org/wine/wine/-/merge_requests/61#note_951
More information about the wine-devel
mailing list