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

[Rémi Bernon]

On 2/12/22 02:33, Zebediah Figura wrote:
> 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.
> 


I don't think this is required here, the way the transforms work they 
just provide a set of default supported formats, from which the 
application is supposed to chose, and, most of the time applications 
just select the first format, expecting it to be the same as on Windows.

Then, after the transform has started processing some buffers, the 
output format may change, for instance if the stream has some metadata, 
and there's a notification that the application is supposed to handle. 
They just then usually query the new format and use it, I'm not even 
sure they can change it again at this point.

For the H264 transform, this really depends on the same format as on 
Windows, the default output format is NV12 (*), and although some are 
looking for it and explicitly selecting it, some other games assume it 
is the first format. As they don't even check that it is, reporting some 
other format instead just makes the displayed frame incorrect.

H264 buffer also usually have metadata to describe the actual stream 
format, so games don't even bother setting the output format properties, 
they use the first available one, and they wait and expect the stream 
output format change event to arrive - after which some query the output 
format properties, and some don't but still expect the change event.

(*) And not standard NV12 but NV12 in the exact same way Microsoft H264 
decoder outputs, which means with aligned planes, which GStreamer 
doesn't seem to provide and which requires some tweaking. We need 
several H264 transform patches to implement that.


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


Yes, from the IMFTransform interface I think they are supposed to 
support multiple streams, and probably there's some demuxer transforms, 
but I hope we won't need them for a while. And if we do, maybe it could 
be a matter of adding a stream index to the push / pull data functions.


> * 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?
> 


What post-processing do you mean?


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


Currently there's no bus, I'm not sure we need one?


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


Sure, I'll do that.

Cheers,
-- 
Rémi Bernon <rbernon at codeweavers.com>