[PATCH v2 0/6] MR61: winegstreamer: Dynamic transform stream format change support.

[Zebediah Figura]

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.)