ffmpeginteropx / FFmpegInteropX

FFmpeg decoding library for Windows 10 UWP and WinUI 3 Apps
Apache License 2.0
211 stars 53 forks source link

Native aot support in cswinrt #438

Open brabebhin opened 2 weeks ago

brabebhin commented 2 weeks ago

I've noticed cswinrt now supports native aot. We should investigate what we need to change, if anything, in our projection library to be compatible with this great achievement.

softworkz commented 2 weeks ago

I don't like it. Making all classes partial - are they sick? But the real showstopper is that they are sacrificing reflection - that doesn't fly for our code base. It's been workinng before in .net native, so it's not impossible. Oh, and did you read/watch their presentations to the end? In both, the WinUI3 community call and in the WASDK 1.6 release notes, it's ending with a statement that there's a huge bug and essentially you cannot use it yet...

brabebhin commented 2 weeks ago

Tbh i stopped watching the presentation at the "make every class partial" business and then continued reading on some documents.

As you know we will be taking our sweet time supporting things, i just opened this issue so we don't forget about it xD

I have no idea why they don't support reflection. I know reflection is tricky to use with trimming, but i remember being able to use reflection on a class if it is also used somewhere else in code already, and for those that aren't, there used to be a file where you could exclude it from trimming. I would if this would include serialization as well. That would be spectacularly bad lol, to the point it makes the feature useless.

Who knows, maybe they will support it later on, and the whole "make every class partial" is the best they could do. I have a feeling there's only 1 human working on cswinrt anyway.

brabebhin commented 2 weeks ago

Tbh, not supporting data binding out of the box and having to manually exclude properties from trimming is hilarious. What were they thinking? This sounds like a preview of pre-alpha version.

softworkz commented 2 weeks ago

Yup, exactly! Probably they got some pressure to get this out...

lukasf commented 2 weeks ago

To me, this also sounds more like a beta version right now. Still, I am happy that they are working on it, the improvements in startup speed are impressive (though we already know that from uwp .net native). At first I did not like their approach, but it also has its upsides, like code is supposedly 100% safe to run properly with native aot, if no more aot compiler warnings are shown (and bugs in CsWinRT are solved).

Reflection is supported, but only when methods which do use reflection have been annotated with the corresponding attributes, stating exactly which kind of reflection info is required for a type used. Then, CsWinRT will generate the required reflection code (only for the required parts) ahead of time. Also, remember that reflection is not even needed in many cases, e.g. by using x:Bind instead of Binding, then type safe binding code is generated without any reflection calls. So you don't even need to make your ViewModel classes partial, if you use x:Bind exclusively (which is what I always try do for performance reasons). Or when you use code generation for json serializers, you also don't need the partial modifier or any other reflection stuff, since reflection is just not used.

Anyways, I don't think that we need changes in our lib. The whole projection library is generated by CsWinRT, so all we can do is update CsWinRT to the latest version from time to time, while they are hopefully working on the major bugs still existing in native aot.

brabebhin commented 1 week ago

X:bind is another good idea that only got 40% implemented. Since it doesn't support binding outside the data template, any data template that has a context menu, for example, will either have to keep using traditional binding, use events instead of binding commands to menu items or find a (ugly) way to bundle commands and data together which is going to be problematic in any kind of non-trivial application, since data and behaviour is no longer easily separated. The only relatively non-ugly way to do this is to implement some sort of tuple to bundle commands and data yet still keep them separated. Ugly :)

lukasf commented 1 week ago

That's true. But on the other hand, it was never a good idea to declare context menus in data templates. Having that whole control tree getting created and data bound for every list item during scrolling is a real performance killer. That's why I always used workarounds to generate the context menus on-the-fly on right tap (using some generic mechanism), instead of declaring them in data templates - even before x:Bind was introduced. It's not nice, but it works and it is much more efficient.

The only place where I currently still use traditional binding is when using the WCT DataGrid, where by design, Binding is the only way to get data in. For the rest, it is 100% x:Bind.