From: "Rémi Bernon" Subject: Re: [PATCH 4/5] winegstreamer: Register WMA decoder transform stub. Message-Id: Date: Fri, 21 Jan 2022 19:19:31 +0100 In-Reply-To: <3ec19c73-6caf-15b5-e1cb-544c9bfb3f4f@codeweavers.com> References: <20220120104152.2236851-1-rbernon@codeweavers.com> <20220120104152.2236851-4-rbernon@codeweavers.com> <2d892439-0926-6669-11b6-4c6c8aa1d105@codeweavers.com> <669bfe25-49ec-a509-b999-b6d23c9948e7@codeweavers.com> <600afee8-6b14-4bb0-5bda-8a70a0e20ec2@codeweavers.com> <3ec19c73-6caf-15b5-e1cb-544c9bfb3f4f@codeweavers.com> On 1/21/22 19:03, Zebediah Figura (she/her) wrote: > On 1/21/22 11:41, Rémi Bernon wrote: >> On 1/21/22 18:23, Zebediah Figura (she/her) wrote: >>> On 1/21/22 02:57, Giovanni Mascellani wrote: >>>> Hi, >>>> >>>> Il 21/01/22 02:44, Zebediah Figura (she/her) ha scritto: >>>>> Is there an application that requires this, other than FAudio? >>>> >>>> I don't know, but notice that Mono ships a copy of FAudio, so if we >>>> want >>>> to patch FAudio, we have to patch that one as well. >>>> >>>>> And if not, can we instead try to change FAudio to not require >>>>> specific decoders? >>>> >>>> Even if the current user is just FAudio, wouldn't it better to >>>> implement properly CWMADecMediaObject anyway? This way you >>>> automatically catch all future users of it. Why is >>>> CWMADecMediaObject something that we might not want to implement? >>> >>> We will need some way to decode arbitrary data via IMFTransform >>> objects, but it seems preferable to avoid having to implement more >>> objects than we need. E.g. trying to catch as many as possible via a >>> generic decoder seems like a good idea. In that case we'd want to use >>> MFTEnum() in FAudio instead of hardcoding the CLSID. >>> >> >>  From the few early tests I've made each transform seems to have its >> own behavior and some games heavily hardcode their logic around it. >> >> I think catching everything behind a generic decoder is not a good >> idea in that situation, and it makes the code harder to understand as >> the specific logic that needs to be implement to satisfy each game and >> each corner case is interleaved together. > > I'm not necessarily arguing for keeping *everything* behind a generic > decoder, but it'd be nice to catch everything that doesn't otherwise > care. Sure, some applications are going to depend on idiosyncratic > behaviour, but the hope is that most are more well-behaved and don't > care, especially if they're using MFTEnum() rather than hardcoding > CLSIDs in the first place. > All the games I've seen which required the MF transforms (H264 and AAC) are instantiating them by their class id. FAudio does it too for the WMA decoder, and although it could use MFTEnum I don't see how easier it would make anything else, we'll still need that WMA decoder class, or a class which can decode WMA. For now it's the only class there is so it's named wma_decoder, but could easily be renamed later if it matches some other codec. >> >> I don't really why it would be so bad to keep them split for now until >> we have a better view of how different they are. If there really is so >> many of them and if they really are similar, it'd be simpler to >> refactor things later when we now what we need to cover. But I don't >> think it's going to be the case. >> >> I think most of the transform code is going to be very simple anyway, >> and just a matter of validating media types and a bit of sequence >> logic, the meat of the decoding is delegated to Gstreamer. > > Simple, sure, but even as stubs it's over 300 lines. Seems like the kind > of thing it'd be nice to avoid if possible. > The fully working WMA decoder implementation (still has a lot of stubs, but native one has too) is 638 lines. H264 is 702 lines and an early rip-off of WMA for AAC is also ~600 lines, they all share a new common unix-side wg_transform object which is 721 lines. Doesn't really seem too bad to me. For comparison, "generic" decode transform that's been floating around is 1230 lines, and doesn't work with some of the games I've tried with (and is also broken in 7.0). I don't know how much specific code it needed on the wg_parser side, but there was some. -- Rémi Bernon