C++/winRT migration

[brabebhin]

Hi all,

This is the PR for migrating the library to C++/winRT. The migration currently exists as a separate project, with a separate namespace, but this is just nicities for later.

What happened here, in a nuttshell:

1. All classes that were not needed to be winRT classes have been moved to vanila C++ (mostly sample providers and other low level stuff that integrates with FFMPEG).
2. There is still some work to be done on the hardware accelerated video effect (haven't migrated that but it should be mostly mechanical work).
3. Need to fix all bugs and test this well.

Some additional breaking changes:

1. Metadata tags are now returned as Map<string, Vector> instead of Vector<KeyValuePair<string, string>>. Mostly a small difference. What do you think, @lukasf @JunielKatarn ?

[brabebhin]

Am I dumb or is it really hard to implement the IBuufferByteAccess in c++/winrt?

[brabebhin]

I am quite close to getting it 100% working. There's a problem with the texture and buffer pools, objects returned in the pool can't be recycled apparently.

[brabebhin]

So directx and software decoding seems to work for video now. I will need to fix passthroughs.

Most bugs seem to originate from uninitialized variables.

[lukasf]

Wow @brabebhin, really nice work! This is really amazing. Kudos for all the heavy lifting.

I did not yet have time for a deep review. But some general remarks:

• When the migration is complete, this should be a drop-in replacement. So the namespace should be the same and also no API changes, if possible (the metadata API change is accepted). I guess you used a separate namespace for development and testing, which is fine. But finally, we should use the old namespace. From the consumer perspective, it should not make a difference if our Windows Runtime Component was developed using C++/CX or C++/WinRT.

• The files are in a new folder, which will make git lose the history. If you are more or less finished with migration, the new files should be put back into the old Source folder (any leftover files deleted there). Then I can review old against new implementation, which will be much better thea just looking at the new sources.

• There are a lot of changesets in here. I think we should make this a squash commit, to get a clean history. We need to be extra careful when merging this PR.

• We also need to think about when we will merge this. Since the changes are so massive, we probably should not hurry this one out. Maybe we should first push out the original 1.0 release PR (I planned to do this before my long journey, but didn't have the time in the end). Then we can thoroughly test this and make it a 1.1, once we are confident there are no leaks or bugs. We can include the metadata API change into the 1.0 release already, so we won't have API changes between 1.0 and 1.1.

[brabebhin]

@lukasf agreed. We need serious testing on this.

I originally wanted to do drop in migration, but the project file got all mingled up and decided starting fresh is just easier, and once everything works, I just rename the namespace and move the files back in.

I will need your help with the nuget package releases.

I will push the breaking changes to v1.0 as well if it's ok with you.

[lukasf]

Sure, if you like, you can push the API changes to 1.0 branch as well.

I can do the NuGet stuff once we are ready for a release.

[brabebhin]

@JunielKatarn

In your experience, does it make sense to initialize hstring instances as empty strings? like this? hstring bla = L"";?

[JunielKatarn]

@JunielKatarn

In your experience, does it make sense to initialize hstring instances as empty strings? like this? hstring bla = L"";?

I believe this is the "preferred modern C++" initialization style: winrt::hstring myHstring{};.

[lukasf]

So I merged the v1.0 PR. This one will need a rebase.

@brabebhin Do you have an account on nuget.org, by the way? I'd like to make you second owner for the libs, as a backup.

[brabebhin]

Hi,

Just created one now, same username, "Brabebhin".

[lukasf]

Okay, done. Updating the NuGet libs is pretty simple. Just click on „Upload“ on top, drop the created .nupkg, verify that the displayed metadata looks fine (version number, etc), and then proceed. It may take a few minutes until the libs go live.

I'll take care of this normally, but it's probably good if you can do it as well, just in case.

[brabebhin]

Got the invites, thanks. I hope you're doing well btw :)

[lukasf]

Yes, the vacation was pretty amazing, thanks! :)

[lukasf]

On a short test, the port seems to run fine on my PC. Basic playback working and no major memory leaks after opening a bunch of files. Fixed some NuGet issues and aligned toolset versions, trying to get AppVeyor build working...

[brabebhin]

I will need to port the BasicVideoEffect these coming days.

[brabebhin]

So apparently using win2D is quite the hustle. We need to download a specific version of win2D, use cppwinrt.exe to generate some header files and pack the dll along with the winmd. This will needs to be done manually...

I'm not sure if this is even worth it. That effect is buggy as hell anyway.

[lukasf]

The effect is not buggy as hell, I just broke it with the 1.0 namespace change.

On StackOverflow, I read that with more recent Win2D versions (as of mid 2018), you just need to reference the NuGet package and then the headers will be auto-generated. Did you try that?

Edit: I tried it and it works. I could include the headers like that, after adding the Win2D.uwp NuGet package:

#include <Generated Files/winrt/Microsoft.Graphics.Canvas.h>

Also Microsoft is moving Win2D into the WindowsAppSdk, and they have a separate NuGet package for WinUI 3 apps (which is not 100% finished as I read). But if we use that, it probably won't work in UWP apps. So if we cannot get the normal Win2D package working, we might have to move the effect out to a separate addon library, specific for WinUI 3. Maybe it would be a good idea anyways, to have a separate lib for stuff like the video effect and custom subtitle rendering, which are both more or less independend of the FFmpegInteropX core library.

[brabebhin]

I wasn't referring to the activation when i said it's buggy. For some reason, simply installing the nuget doesn't work for me. But if it works does you, then it means there's something wrong with me setup.

[brabebhin]

Thanks for the hint @lukasf

After installing some new updates to visual studio, it does generate the header files for me automatically. Which is great, I pushed the changeset.

All i need to do now is understand how the activatable class id for the effect works in C++/winRT. Apparently it is not as easy as in C++/CX. I'll leave this for later.

[brabebhin]

Turns out the problem was unrelated to the class ID, which is the same as in C++/CX, but rather was another idl/ cppwinrt gotcha:

The IDL of the class must contain a parameterless constructor. The [default_interface] attribute does not suffice, the constructor must be declared explicitly.

@lukasf I think the migration is now more or less complete. Should I start the process of moving the files back into the original folders with namespace rename or do you want to wait out a bit more so that we have more time to test with both sources present?

[lukasf]

I just pushed a big changeset to address all those compiler warnings. Some of them actually pointed to problems and even bugs. The basic funtionality seems to be working now. Maybe it is a good time to move the files back, so we can start a proper review of all file changes.

Of course, this will need intense testing of all the special cases and functionalities, with various files and formats.

[brabebhin]

I actually think we should merge the other outstanding PR before migrating the namespace and folder.

[lukasf]

True. But I think the stream buffer PR still needs some time. There are a few things I am not 100% happy with, and some cleanup needed.

Also, by introduction of the .editorconfig file, in all changes, tabs are replaced with spaces. This makes reviews difficult. Maybe we should make a PR to format the complete codebase correctly with spaces. This will make reviews a lot easier.

[brabebhin]

Hi @lukasf

I merged the branch fix-leaks. Can you check and see if the merge was ok?

Thanks.

[lukasf]

Looks good. I done some minor changes there.

We should also consider making use of com_ptr class in WinRT for all the D3D stuff. It really simplifies the ref handling, making it more solid at the same time. And you can just use .as<T> instead of QueryInterface, which is also a lot nicer.

[brabebhin]

Yes. That is a good idea. I also thought of it, but i initially decided to port the code as is and do stuff like this later.

On the other hand, since there's no rush, i could do it before merge.

[lukasf]

We can do it later as well. The merging will probably be easier, if we do not do too much of refactoring upfront.

[lukasf]

By the way, in the WinRT samples, I often see assignments through blocks like this:

int main()
{
    winrt::Windows::Foundation::IStringable s{ winrt::make<MyStringable>() };
    auto result{ s.ToString() };
}

Do you have any insights on why they do it like that? Can't we just write:

int main()
{
    winrt::Windows::Foundation::IStringable s = winrt::make<MyStringable>();
    auto result = s.ToString();
}

Their samples are full of that assignment style, but they never explain why it is done like that. I wonder if it has any benefits.

[brabebhin]

As per my understanding, it depends on what you initialize. For a class with constructor, it basically calls the constructor. For classes without constructors, it initializes the members of the class in order. This is quite the gotcha in some directx12 structures. I lost days debugging something like that. In your examples i think it doesn't make any difference between the two examples. It would just call the copy constructor. It basically allows you to RAII classes that don't have a constructor.

In some situations it also helps the compiler disambiguate situations in which you have an variable with the same name as a function and your try to call the constructor of that variable. Also in some exotic overuse of the auto keyword. Honestly i prefer the legacy way, it is much more readable.

[lukasf]

Yeah I only know this style from initialization from structs. And it comes with the disadvantage that you don't know which parameter goes into which field, if you don't know the exact struct layout.

But if you have a constructor, then why not call it directly? Why not store the result of a method with =? To me it feels like abusing a language construct that was ment for something different. And without any obvious benefit, at least I don't see it. I really wonder who came up with that idea, and why they decided to use it so heavily.

[lukasf]

I just read some interesting things about strong and weak references in C++/WinRT. In the old implementation, we had this requirement that we need to store the FFmpegMediaSource reference in the app, to prevent it beging collected during playback. With C++/WinRT, we can add get_strong() to the MSS event registration code. Then the MSS would hold a strong reference to the FFmpegMediaSource and prevent any early collection. No more need to store it in app code.

https://docs.microsoft.com/en-us/windows/uwp/cpp-and-winrt-apis/weak-references#safely-accessing-the-this-pointer-with-an-event-handling-delegate

Also in other event handlers, we should think about lifetime issues, and consider using strong or weak references explicitly, where appropriate.

[brabebhin]

Interesting find. I think that would basically solve the problem if we register events in the mss (like the starting event) with our ffmpeg media source as a strong reference.

I think we need to decide if we want to do those refactorings now. It could be useful to do all possible dangerous changes in one go.

Perhaps something along the lines of:

Merge stream buffer Put files back in folder Review changes Do dangerous stuff(com ptr, references etc) Review again Merge

[lukasf]

I agree that it is a good idea to test all this together. But I would really like to keep the history clean, and not mix too many things into one PR. So I would propose to bring all these to master, but as separate PRs (winrt, stream buffer, refactorings). Each PR should be roughly tested and reviewed. But final testing will be done on master branch once all things are there, before we make it an official release.

I think that it will also make reviews better, if you can see a clean diff of e.g. only the pure winrt migration without refactoring, only stream buffer, only com_ptr refactoring,...

What do you think?

[brabebhin]

This is also a good idea. But we need to decide if we want to merge this before stream buffer or after.

Once this is merged I will also import it in my own app and i will be able to do some more advanced testing on it.

[lukasf]

To me it does not really matter now which one gets merged first. Whatever feels finished first gets merged first. There will be some manual merge effort when these two get in touch, but I think it does not really matter in which order it arrives.

I am still not 100% happy with the stream buffer implementation. It works, but it has some design weakness that I don't like. I had to use workarounds to avoid circular references. Maybe I will come up with a better design to avoid these workarounds and have cleaner code. So that branch might also need some more time.

We should probably do a 1.0.1 release with the fix for the BasicVideoEffect, before anything else gets merged to master.

[brabebhin]

Ok, then i will put the files back in this week. Probably today or tomorrow, if things go well.

Imo a circular reference isn't bad per say, MF is full of those. We just need to be extra careful when we break it.

[brabebhin]

There is a slight chance that it may not be possible to move the files back. On the other hand it is possible to move the history.

[lukasf]

If you move the files back, we will be able to see 1:1 old file vs new file when reviewing the PR. And if we then squash merge the PR, we will have clean history with old to new.

[brabebhin]

OK, I've moved the files back, but the build isn't working. There seems to be a problem with the precompiled header which I can't figure out, tried several approached and all ended in the same problem. I pushed this so maybe you can also take a look and see what I'm missing. I will keep trying.

[lukasf]

I fixed most of the build but two errors remain, regarding WinRT actication factory. Probably need to compare project files more.

[brabebhin]

Cool. I will focus on that now. Nice job with the pch, I tried various combinations and none worked :( (I even thought its contents were to blame somehow, hence commenting it out)

[brabebhin]

The build seems fixed on my PC now. I will proceed to update the samples.

Edit: I think we broke something lol, I can't reference either the FFmpegInteropXWinUI project nor the FFmpegInteropX project from samples.

[lukasf]

The C# sample works fine, but I found two issues with the CPP sample, surprisingly:

1. The binding of CharacterEncodings does not work. We can get the value find, but an ArgumentException comes when assigning it to Combobox.ItemsSource.
2. When opening a file, somehow the app continues on a wrong thread, causing an exception when accessing the splitter (close pane). co_await the file open returns normally on UI thread, but co_await on our MediaStreamSource somehow returns on a background thread. That should not happen of course. Pretty weird. Playback still works, but there is something wrong with the async stuff we return in our CreateFromXX methods.

We need to find out what is wrong here.

[lukasf]

I fixed the second issue by manually switching back to UI thread. To me, this does not seem right. Normally, the co_await on the UI page should switch us back to UI thread, no matter what the lib does. But it seems that the WinRT resume on background switches the context in a way that UI won't switch back. This is very different from what I know async/await. I have kind of a bad feeling about this.

I tried a few things about the CharacterEncoding but failed. The old class has that Bindable attribute. But the attribute is missing in the winrt headers. And if I include the legacy header, I get loads of compile errors. I have also seen [bindable] in the idl file in some docs, so I added that but it didn't help as well. I am not even sure if that is the problem.

[brabebhin]

The problem seems to be the VectorView that we return rather than the CharacterEncoding class.

Edit: lol yes, it seems to work if we use winrt::single_threaded_observable_vector instead of single_threaded_vector. I gotta go to work now, but it might be worth to check if there's other places where we do this.

Honestly this looks like a bug in C++/winRT, I created cpp-winrt-strange-binding-problem so we can report it to the cpp/winrt repo.

[lukasf]

It's strange that the observable vector works, but the normal vector does not work. The normal vector is supposed to work as well, and it does in C++/CX. It's just that it won't report updates, but we are not updating this one anyways.

I needed to add the [bindable] attribute in the IDL file, otherwise binding to properties of CharacterEncoding did not work for the CPP sample. We should consider adding this to any class that might be bound in a UI, such as the StreamInfo classes.

And I just found two problems in the subtitle providers:

Looks like style parsing does not work anymore on the C++/WinRT branch. sscanf_s always returns 0 parameters parsed, although the input data looks perfectly fine. On master, the same input data returns 23 parameters parsed.

And also the code to parse the actual subtitle lines seems to be one index off, but that occurs on master as well?? I only tested with one file, need to test more. It might be a change in FFmpeg. The ssa text is not in the standard format anymore, looks like the first parameter is missing. I wonder how long this is broken. Subtitles are still displayed, but styles are not applied anymore.

[brabebhin]

I reported the vector stuff and got a response

https://github.com/microsoft/cppwinrt/issues/1164

I also remember reading somewhere that cppwinrt bindings only work with x:bind

[lukasf]

CppWinRT classes do support classic Binding, but only if the classes are annotated with the [bindable] attribute. Without the attribute, only x:Bind is supported. This is the same behavior as with C++/CX.

I fixed the problems with subtitles on both branches. WinRT branch additionally had some of these initialization issues.

[lukasf]

I tested this in an actual app and found some more issues, especially subtitle related. There was also a crash on cleanup, and some other things. Now it is looking pretty good, still more testing needed.

I tried a few hacks to get XML docs working, but it only works for methods, not properties, events or enums. It's a shame that C++/WinRT does not even support XML docs. Such a basic thing not working, every other language has that.

I noticed that AttachedFile is part of the IDL. It is only used internally, we should keep that as a pure C class. The current AttachedFile would break when our main class is destroyed. If we'd want to expose AttachedFiles, we'd need to think about refernce handling.

[brabebhin]

AttachedFile is public in the current c++/cx version.

I think docs are visual studio realm, even the idl file itself had poor tooling support, no auto format, no syntax highlighting.