[PATCH 1/3] winegstreamer: Introduce new wg_transform struct.

[Zebediah Figura]

On 2/11/22 17:46, Rémi Bernon wrote:
> On 2/12/22 00:19, Zebediah Figura wrote:
>> On 2/11/22 03:36, Rémi Bernon wrote:
>>> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=51931
>>> Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52391
>>> Signed-off-by: Rémi Bernon <rbernon at codeweavers.com>
>>> ---
>>>
>>> This would serve a simpler purpose than the full-fledged wg_parser, and
>>> be used by synchronous transforms only (at least there's no thread on
>>> the Wine side), which simply work on the push then pull loop model.
>>
>> Do you by chance have a complete branch somewhere that I can look at or
>> mess with? I'd in particular like to see just how much code is
>> identical, or explicitly different, between the parser and transform
>> objects.
>>
>>> The wg_transform name is chosen to be generic and possibly cover both
>>> decoder and encoders (and possibly converters), but maybe it's better
>>> to keep it specific for now and only design it for decoders. This would
>>> simplify the format question in the next patch.
>>
>> I suspect it makes more sense for it to be a generic transform object.
>>   From what I've seen GStreamer doesn't really discriminate between
>> encoders, decoders, and other transforms, and I don't think that
>> winegstreamer should either.
>>
> 
> I have three branches on top of each other, wip/wmadev/v1,
> wip/h264dec/v1, and wip/aacdec/v1 in https://github.com/rbernon/wine,
> although the last one is not polished at all and full of wip patches for
> tests.
> 
> Probably the most interesting things are in the wmadec and h264dec
> patches. As far as I could see (although I didn't test it much) the
> aacdec didn't really require anything except the format mappings.

Thanks, that's about what I was looking for.

I only ended up looking at the WMA branch, but here are my thoughts:

* There's surprisingly even less code in there than I thought, so I'm 
inclined to think that a separate wg_transform is a good idea. That said:

* wg_transform is built without an equivalent of 
wg_parser_get_preferred_format() and all of the relevant support code. 
That gives me some pause. It doesn't give me a *lot* of pause, because 
the main point of wg_transform is to handle those applications that try 
to create *specific* decoders, and we probably want those specific 
decoders to report the same exact formats as they do on Windows. There 
are a few examples I can think of of applications that would be able to 
make use of a generic decoder, but if we have enough specific decoders 
already there may be no point anyway.

* wg_transform is not designed to handle one-to-many elements and cuts 
out all of the relevant code. I think this is fine, ultimately; the only 
real example of that I can think of is the case of a demuxer that 
supports push mode, and if we ever need to support that (which we 
probably won't) it *probably* makes more sense to use wg_parser anyway.

* The post-processing logic is kind of duplicated. Would it make sense 
to separate that into a helper function and then use it for both 
wg_parser and wg_transform?

* sink_chain_cb is kind of redone. To be fair, we don't care about EOS 
or segment events. On the other hand, I'm not thrilled about the way we 
pull outputs in wg_parser, and we probably want to make it look more 
like what you have for wg_transform. [In specific I suspect we want to 
generate our own segment events for DirectShow rather than using 
GStreamer's, and once that's done we should make EOS a return value, and 
preëmptively grab sample buffers...] I can probably live with having two 
different implementations for a while, but ultimately I think we want to 
avoid that, at least in the unixlib API.

* I think we want to handle GST_MESSAGE_ERROR and GST_MESSAGE_WARNING. 
We can't use bus_handler_cb() directly (since we don't want to handle 
GST_MESSAGE_DURATION_CHANGED) but we could probably add a helper for 
those two at least.

* We might want QoS logic. That's probably not that impactful in terms 
of "is wg_transform worth it", though, and it doesn't need to be done 
immediately anyway...

* More of a case of "if you don't want to do it then I will sooner or 
later", but if we're going to have two different objects I'd prefer to 
move the common code (which ends up mostly being all of the wg_format 
conversion) to a third file, just for maintainability's sake.