Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port FNA over Media Foundation instead of Theorafile. #4

Merged
merged 3 commits into from
Aug 12, 2021

Conversation

rbernon
Copy link

@rbernon rbernon commented Jun 10, 2021

This will make it possible to run native XNA games which are still using WMV/WMA assets in Wine.

It requires FAudio changes for XNA Song WMA support, tracked in FNA-XNA/FAudio#253 as well as some Wine Mono build changes to build and install the new FNAMF native wrapper.

I'm not sure if it would be interesting to submit upstream, at least probably they won't be interested in this form as it's not configurable.

@rbernon
Copy link
Author

rbernon commented Jun 10, 2021

Removed last commit as it will only make sense when FAudio is built as PE and used for Wine too.

@flibitijibibo
Copy link

Huh, I'm surprised the Theorafile API managed to (mostly) work with MediaFoundation like that.

Something I thought about a while ago was refactoring Theorafile and XNA_Song into an XNAMedia DLL where we handle Song and Video data in an opaque decoding API that can support multiple formats, including Vorbis/Theora/WMA/WMV/mp3/etc. I kinda threw the idea out in FNA-XNA/FAudio#17 but maybe there's enough reason to make a real library to keep upstream FNA and wine-mono FNA sync'd. It'd also be good for supporting some of the less open stuff like Bink or hardware-specific decoders available on consoles, so there are definitely a lot of positives to making the Media namespace its own native library.

That's a whole other commitment though, for now this is fine for getting around my spec violation regarding XNA Media formats.

@madewokherd
Copy link
Owner

Good news: because of this pull request, I think I finally understand my criteria for accepting a diff from upstream. It's definitely not because I binged The Good Place last night instead of sleeping (which, to be fair, was likely to go poorly anyway because the heat was making it very difficult, and I may have gotten a similar quality of sleep this morning to what I'd have gotten otherwise but at a different time than usual) and my mind is in a very weird place right now. Keep in mind that all of this is said without having read your actual code.

It needs to be maintainable, by me, for the foreseeable future. Given my knowledge of the component in question is quite limited in this case, that ideally shouldn't require me to rewrite a bunch of stuff when upstream fixes this properly. Ultimately, we can jettison an upstream if we have to (we've already done that with winforms and wpf because the goals of the upstream project were so incompatible with ours that the problems introduced by the upstream changes themselves outweighed the benefits), and given how good FNA is already I'm at peace with this, but let's please still try to avoid that if we can.

I also need to know that we're not screwing over the upstream project while also making things more difficult for us in the long run, by not giving them a change they'd like to have. From #4 (comment) I take it upstream does not want this change in this form, and that more careful design work would be needed to integrate this properly.

Anyway, that (along with, is this good code that I think should be accepted into the project?) is the criteria I will use to judge this PR, along with all other PRs that diverge from upstream projects in the future.

debug.o: $(SRCDIR)/../../wine/debug.c
$(MINGW)-gcc -o $@ -D_WIN32_WINNT=0x0602 -D__WINESRC__ --std=c99 -c -Wall -Wno-format -Werror -I$(SRCDIR)/../.. -I$(SRCDIR)/../../wine $<

FNAMF.dll: $(C_SRCS:%.c=%.o) debug.o $(H_SRCS:%=$(SRCDIR)/%)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't make sense for this rule to depend on the headers, since they'd be read when compiling the .o files, not linking them, so if the headers changed we'd just rebuild the same final dll.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Makefile could perhaps be simpler, but I just moved the header dependencies to the source rules.

lib/FNAMF/main.c Outdated Show resolved Hide resolved
lib/FNAMF/main.c Show resolved Hide resolved
@rbernon
Copy link
Author

rbernon commented Jun 10, 2021

Sure, I understand and I tried to keep things as separate as possible to avoid conflict with upstream in this kind of scenario. I'd be happy to port that work to an eventual separate library but I think it'd be nice to have this in the meantime, as a temporary solution.

And in any case, it needs the FAudio changes to be upstreamed first.

@madewokherd
Copy link
Owner

Are the functions in main.c the extent of the API we expect to need here, based on the XNA API? If so, this is probably one of those cases where I can muddle through working with it if I need to, even though I don't understand how something as seemingly straightforward as mf_hasaudio works, at all.

Automated tests would still be really nice though, even if they're tests that require Windows and MS tools to actually build.

I think we need to find a way to do this without copying the VideoPlayer.cs code, because as is, I'm not going to be able to merge upstream changes into it, and it's quite difficult to review the patch.

Why do we need Wine's dwrite.h?

@madewokherd
Copy link
Owner

I think we need to find a way to do this without copying the VideoPlayer.cs code, because as is, I'm not going to be able to merge upstream changes into it, and it's quite difficult to review the patch.

If we have to, moving all of the Xiph VideoPlayer.cs into a new file so I get conflicts if it changes may be an acceptable solution.

@rbernon
Copy link
Author

rbernon commented Jun 10, 2021

Are the functions in main.c the extent of the API we expect to need here, based on the XNA API? If so, this is probably one of those cases where I can muddle through working with it if I need to, even though I don't understand how something as seemingly straightforward as mf_hasaudio works, at all.

It's been written to follow Theorafile API, so that porting the VideoPlayer class is not too hard, but it differs a bit. I think the Microsoft.Xna.Framework.Media namespace is from XNA and I would say that its API is more likely to be stable compared to Theorafile, but I don't expect Theorafile API to change or grow a lot either.

Automated tests would still be really nice though, even if they're tests that require Windows and MS tools to actually build.

That doesn't sound easy to be honest, unless FNA already has some test facility, which I don't see.

I think we need to find a way to do this without copying the VideoPlayer.cs code, because as is, I'm not going to be able to merge upstream changes into it, and it's quite difficult to review the patch.

Sure, I can try doing it this way. Not completely sure if we can perfectly fit Theorafile API though.

Why do we need Wine's dwrite.h?

No reason, I copied things around from monoDX, didn't think twice about it, I should filtered a bit. But as these Wine source extracts start to pile up, maybe it would be better to have them all in a single place in wine-mono instead?

To be completely clear, I only tested a few games with this, and it worked okay, but it would probably deserve better testing before considering merging it. I just wanted to gather early feedback so that I would not waste time doing something that's not viable.

@madewokherd
Copy link
Owner

Automated tests would still be really nice though, even if they're tests that require Windows and MS tools to actually build.

That doesn't sound easy to be honest, unless FNA already has some test facility, which I don't see.

All we need is a .exe that returns 0 on success and non-zero on failure. We don't even have to be able to build the exe without Windows/VS.

I think we need to find a way to do this without copying the VideoPlayer.cs code, because as is, I'm not going to be able to merge upstream changes into it, and it's quite difficult to review the patch.

Sure, I can try doing it this way. Not completely sure if we can perfectly fit Theorafile API though.

Actually, I know it's counter-intuitive and strays further away from what upstream would want, but can we replace the Xiph files entirely, in their current location? Given that we're not using them anyway, and we're not preserving the ability to use our fork on Linux or with theorafile, I'm not sure there's a reason to keep the originals around, and copying or renaming them complicates things for me.

Why do we need Wine's dwrite.h?

No reason, I copied things around from monoDX, didn't think twice about it, I should filtered a bit. But as these Wine source extracts start to pile up, maybe it would be better to have them all in a single place in wine-mono instead?

Hm, it probably was copied from wmwpfdwhelper and shouldn't be there either. I clearly didn't review the monoDX stuff as closely as I should have.

To be completely clear, I only tested a few games with this, and it worked okay, but it would probably deserve better testing before considering merging it. I just wanted to gather early feedback so that I would not waste time doing something that's not viable.

OK, when to merge/release is something for me to think about later. I'm not even sure if I want to do any more updates off the develop branch before wine 7.

@rbernon rbernon force-pushed the media-foundation branch 3 times, most recently from b453995 to 49cc358 Compare June 15, 2021 15:11
@rbernon
Copy link
Author

rbernon commented Jun 15, 2021

I rewrote the FNAMF source, so implement a direct Theorafile replacement. The FNA C# sources are now mostly untouched and FNAMF instead follows Theorafile behavior.

@rbernon
Copy link
Author

rbernon commented Jun 15, 2021

I also removed most of Wine source, only kept debug utility because it's handy for logging. I think MinGW has everything that's needed to build FNAMF otherwise.

@madewokherd
Copy link
Owner

Looks good to me other than the unused variable. I'm OK with not having tests.

Is there a game I can use to manually test this for future FNA merges?

@rbernon
Copy link
Author

rbernon commented Jun 16, 2021

I've been trying it on https://store.steampowered.com/app/213030/Penny_Arcades_On_the_RainSlick_Precipice_of_Darkness_3 as well as https://store.steampowered.com/app/107300/Breath_of_Death_VII

I can probably add a test but I'm not sure what we are actually interested in testing, either this Theorafile interface, or the XNA level API?

@rbernon rbernon force-pushed the media-foundation branch 2 times, most recently from fe6ce2d to f68d135 Compare June 18, 2021 08:46
@madewokherd
Copy link
Owner

Can this be merged now that we've updated FNA to 21.07?

@rbernon
Copy link
Author

rbernon commented Aug 2, 2021

I believe so but let me double check first. I think this also requires to use the new Win32 FAudio platform to work properly (otherwise XNA Song implementation would be on top of FAudio stb_vorbis). The wine-mono MR is currently missing these changes.

Note that this will also remove the SDL2 dependency.

@madewokherd
Copy link
Owner

I assume you mean it would remove the SDL2 dependency for FAudio only, and we'd still need it for FNA3D and FNA.

rbernon added 3 commits August 3, 2021 10:09
Proton video converter replaces videos with a 320x240 test pattern if no
transcoded video is available, this should be harmless.
@rbernon
Copy link
Author

rbernon commented Aug 3, 2021

Yes, I didn't have the whole picture. I changed the FAudio build only to use the Win32 platform remove its SDL2 dependency but I kept SDL2 otherwise.

@madewokherd madewokherd merged commit 6225d7e into madewokherd:main Aug 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants