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

[Zebediah Figura (she/her)]

Ech, sorry, I managed to drop this thread on the floor...

On 2/14/22 04:07, Rémi Bernon wrote:
>> * 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.

Right, I know that some objects really do need to output a hardcoded
list of formats, that's what I was trying to say in my message. But I'm
wondering if we at any point want a generic decoder. Note that mfplat is
not the only concern here (we may need it as a DMO, as well as
DirectShow via DMOs.) I'm not sure enough that we need a generic decoder
that I think this is a priority, but I'm also not sure that we don't
need one.

Or, frankly, if games ask for a specific decoder but don't make
assumptions about the output format, we should take advantage of that if
at all possible. Format conversion is very much not cheap.

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

Probably. Note that part of the concern is elements without a fixed
number of pads.

Still, it's probably not worth worrying about.

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

I mean the format conversion, resampling, videoflip, etc. elements. It's
something that can be deduplicated later, though, and will be at this
rate if at all...

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

Well, like I said, I think we should make a point of printing error and
warning messages to console; they won't appear otherwise with default
log settings.

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

Thanks for taking care of that!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 495 bytes
Desc: OpenPGP digital signature
URL: <http://www.winehq.org/pipermail/wine-devel/attachments/20220222/478ba696/attachment.sig>