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!