Replace forbidden APIs and remove appcontainer conditions

[softworkz]

All C++/WinRT components must set these build variables, it's not only for UWP components.

They were probably disabled conditionally to make it compile with those user32.dll functions (EnumThreadWindows etc.), but - afaik - these are still not allowed to use from a packaged app. Not setting those variables may also have side effects on projection generation and consumption, so let's better avoid this in the first place.

[brabebhin]

Window::Current will fail on background threads, IIRC. That's why we went the hwnd path. See the conversations in the winui_build PR.

[softworkz]

Window::Current will fail on background threads, IIRC. That's why we went the hwnd path. See the conversations in the winui_build PR.

The "hwnd path" uses 'GetForCurrentThread', which means that it would also end up without getting the window.

All calls into the library are usually made from the UI thread (WinUI), that means the window could easily be determined before doing async stuff on other threads.

[softworkz]

Or the call can be made through the dispatcher, to be sure to get into the UI thread.

[brabebhin]

The various init methods are on background thread contexts because we made them async, so dispatcher won't work.

The problem at heart here is HDR support, more specifically automatic detection of HDR support on winUI. You can't use the winRT DisplayInformation easily due to changes in Windowing models in winui. So we either detect the hwnd automatically or pass the hwnd as parameter.

We are considering passing the hwnd as parameter. This will shoot winui and pure desktop in one fell swoop.

Our struggle here is to get rid of references to the UI frameworks and be fully winRT library again, which is the lowest common denominator for desktop, winui, uwp etc. I think this will also solve some of your struggles as well.

My idea was to get hdr support using directx but that doesn't seem to be working fine. Maybe there's another way, preferably supported for both uwp and winui that's not DisplayInformation.

In a nutshell, probably the best way to solve this problem is to not have the problem in the first place.

[softworkz]

The various init methods are on background thread contexts because we made them async, so dispatcher won't work.

The contrary: async is good, because then you can await for the dispatcher's invocation on the main thread to complete.

Dispatcher Explanation

The dispatcher's purpose is about to be able to queue some code execution to be performed on the main thread while being on a background thread. The result of the execution can also be awaited.

[softworkz]

I think this will also solve some of your struggles as well.

My struggles are all solved 😄

[brabebhin]

I am well aware what the dispatcher does. But we don't want a hard dependency to it. There's a winui_subtitles branch which removes the need for the dispatcher.

The reason we use the dispatcher is that subtitles flicker when we add them to the timedtexttrack without careful timing. The dispatcher was one way to fix that. But there's a possibility to do it without the dispatcher.

Our aim is to remove dependencies to UI frameworks as much as possible, not add more of them.

[softworkz]

I don't understand what you're talking about. Dispatcher/DispatcherQueue is being used all over the place in the code base...

[softworkz]

Dispatching is a core concept, so I don't think that it's even possible to go without it - and without a UI framework, there's no video display. So far there has been UWP and now there's WinUI which needs some adjustments. I don't really see any possibility nor sense in getting "independent". WinUI even requires different compilation which is unavoidable, so a dependency exists and won't go away by any means... 😆

[brabebhin]

But ffmpeginteropx is not a ui library. It just decodes/demux stuff and sends it to some other framework to do the UI stuff.

We can work without dispatching. We actually have for several years. The dispatching was introduced along with subtitle support, and it was essentially a hack. It still sort of is. Then it bubbled up all over the code. Back then, mediaplayer did not support demuxed subtitles, so we went along with TimedMetadataTrack.

Then when winui and its new dispatcher came up we were presented with some breaking changes.

The dispatcher requires a hard dependency to winui. Applications should be able to use ffmpeginteropx without forcing winui all over it.

Another problem in winui is that a dispatcher can have several windows, each on different monitors with different hdr support. We need to know the exact window that is used the ffmpeginteropx.

Us having that dispatcher is against any programming good practice. But it is a work around some limitations.

[softworkz]

But ffmpeginteropx is not a ui library. It just decodes/demux stuff and sends it to some other framework to do the UI stuff.

Yes, it's not a UI library but there's still a threading model which you need to obey to, even though you might not notice in most cases as the interop is taking care of this for you. When interacting directly with certain components, you need to handle this yourself and the Dispatcher/Dispatcher queue is (very) convenient way for doing so.

In C#, there's even a UI framework independent abstraction over CoreDispatcher and Dispatcher queue, which you leverage by using System.Threading.Tasks.SynchronizationContext instead of the dispatcher mechanisms.

Applications should be able to use ffmpeginteropx without forcing winui all over it.

For which purpose? What's the use case for that?

Another problem in winui is that a dispatcher can have several windows, each on different monitors with different hdr support. We need to know the exact window that is used the ffmpeginteropx.

I think the approach from @lukasf is right, to provide an overload which takes a WindowId. For the consumer of FFmpegInteropX it's easy to determine and supply that WindowId, so I don't see autodetection as a severe problem. Much more interesting (to me at least) would be the case when the window is dragged from one display to another. For example, the user has an HDR TV connected, initiates playback on the non-HDR monitor and then moves the window to the TV in fullscreen for viewing.

[softworkz]

ffmpeginteropx is not a ui library

We can work without dispatching

You could of course fence-out all tasks which involve accessing anything in Microsoft.UI.xxxx to the consumer of the lib and this way get rid of WinUI3 dependencies. When you don't access any of those API's you won't need the dispatcher. But I don't see the point of doing so, because even without dependencies on WinUI, you would still need to compile separate binaries and cannot use the same for UWP and Win SDK, so nothing would be won and the library would be less comfortable to use. That's what I meant to say above.

[brabebhin]

The only threading model we theoretically should care about is MediaStreamSource.

All of the problems we use the UI framework to fix can be solved without it.

Subtitles are now supported with Mediastreamsource, so we can implement that(time consuming but possible, we would also lose subtitke customization).

We can fully offload the HDR detection part to the user. Or at least ask the user for data that we need. I'm wondering if we should do this or at least support this as a use case for when apps use pure directx for rendering. The problem with this is that most users will replicate code. We could write some extensions lib for that, and cover winui and UWP for example.

Take a look at the microsoft fork, last time i checked they didn't care about the UI framework.

I'm pretty sure we can use the UWP build with windows app sdk. Most of the winRT components that are part of universal windows contracts also don't have winui specific builds. This is why cswinrt and vcrtforwarders exist.

I've seen people use it in pure desktop before winUi and winappsdk were a thing. It's cswinrt that broke things

[softworkz]

I've seen people use it in pure desktop before winUi and winappsdk were a thing.

Yes, before net5, you could consume them directly.

I'm pretty sure we can use the UWP build with windows app sdk.

Definitely not.

Most of the winRT components that are part of universal windows contracts also don't have winui specific builds

Of course not, because those libs were never using app-local runtimes. They are framework libs not packaged apps.

This is why cswinrt and vcrtforwarders exist.

CsWinRT requires that the component is compiled with DesktopCompatible being set.

vcrtforwarders are for using components non-packaged apps - only.

[brabebhin]

The docs say vcrtfowarders is needed if desktop compatible is not set and targeting UWP. Are you implying the doc is wrong (again)?

[softworkz]

The docs say vcrtfowarders is needed if desktop compatible is not set and targeting UWP. Are you implying the doc is wrong (again)?

Where are those docs? And why "again"?

Now for the subject:

1

All projection samples in the CsWinRT repo use DesktopCompatible: https://github.com/microsoft/CsWinRT/tree/master/src/Samples

2

At least at two places it was explicitly written that it is required to set DesktopCompatible. (need to find them again)

3

Even in this post from @lukasf, he stated that he has set DesktopCompatible. https://github.com/microsoft/CsWinRT/issues/1253#issuecomment-1289514095 He just didn't know that it was ineffective.

4

The readme of the forwarders lib doesn't even talk about WinRT:

5

Neither does their repo: https://github.com/microsoft/vcrt-forwarders

None of the sample is showing use of it in a packaged app: https://github.com/Microsoft/RegFree_WinRT It's all about using in a Desktop app.

6

Besides this, that repo is dead. No release since three years. This is surely not the key intended technology for this purpose.

7

Also, read this: https://github.com/microsoft/vcrt-forwarders/issues/12

(I've read so many more things during the past days, I can't revisit each of them)

[brabebhin]

These docs https://learn.microsoft.com/en-us/windows/apps/develop/platform/csharp-winrt/

WinRT type activation C#/WinRT supports activation of WinRT types hosted by the operating system, as well as third-party components such as Win2D. Support for third-party component activation in a desktop application is enabled with registration free WinRT activation (see Enhancing Non-packaged Desktop Apps using Windows Runtime Components), available in Windows 10, version 1903 and later. Native C++ components should set the Windows Desktop Compatible property to True either via the project properties or the .vcxproj file, in order to reference and forward the Microsoft.VCLibs.Desktop binaries to consuming apps. Otherwise, the VCRT Forwarders package will be required by consuming apps if the component targets UWP apps only.

[softworkz]

This is exactly what I've experienced and why it is wrong to use this with packaged apps:

[brabebhin]

Ok so either docs are wrong or there's a bug somewhere.

[softworkz]

These docs learn.microsoft.com/en-us/windows/apps/develop/platform/csharp-winrt

WinRT type activation C#/WinRT supports activation of WinRT types hosted by the operating system, as well as third-party components such as Win2D. Support for third-party component activation in a desktop application is enabled with registration free WinRT activation (see Enhancing Non-packaged Desktop Apps using Windows Runtime Components), available in Windows 10, version 1903 and later. Native C++ components should set the Windows Desktop Compatible property to True either via the project properties or the .vcxproj file, in order to reference and forward the Microsoft.VCLibs.Desktop binaries to consuming apps. Otherwise, the VCRT Forwarders package will be required by consuming apps if the component targets UWP apps only.

This proves everything I'm saying:

• Components should set DesktopCompatible to true INSTEAD of using VcrtForwarders
• This is only for NON-PACKAGED APPS

[softworkz]

You need to read more carefully... 😆

[brabebhin]

I think we can agree this PR isn't needed.

[brabebhin]

Additionally i think we need a table with all the various app models that are used, and the dependencies they require in order to work properly, so we're on the same page on what a winui 3 app is and isn't. I'll do this later today.

[softworkz]

This PR is exactly what's needed to get everything right.

I'll push another update which includes all other PRs, and then it's up to you whether you want to take it or how long you will take to realize that it's right.

[softworkz]

Additionally i think we need a table with all the various app models that are used, and the dependencies they require in order to work properly, so we're on the same page on what a winui 3 app is and isn't. I'll do this later today.

You need to get clear about the difference between a "packaged app" and a "non-packaged app". This is the important distinction, even though these (unhandy) terms seem to be peripheral.

You can see the DesktopCompatible switch as a kind of successor to VcrtForwarders. It is preferred because it is compatible with both - packaged and non-packaged apps, while VcrtForwarders cannot be safely used with packaged apps (even though it works sometimes, or for some time - until it causes crashes). DesktopCompatible works for all cases and without issues. That's also why VcrtForwarders is no longer updated.

[lukasf]

Hey guys, I noticed that the tone has become a bit rough here. Please be nice and friendly. Let's be constructive. I think it's great that @softworkz wants to help improve the lib, and he brought up some valid points here (and in other discussions). Still I want to be sure that any proposed changes are indeed required and effective. There are a lot of different things proposed here and in other PRs. We should find out what is really needed and helpful, and bring those changes into the lib.

In this PR there are two seemingly unrelated things. One is about removing APIs, the other has to do with project settings, which does not really seem to be related. And then there is the discussion about DesktopCompatible, which again has nothing to do with either of these. I'd prefer to have separate PRs for things that are totally unrelated. It also makes discussions a lot easier, if there is only a single topic in one PR. And sometimes it makes more sense to create and issue first, explain and discuss it there, and only create a PR when a clear idea exists how to solve the issue. The PR discussion is then focused on the solution details, not about explaining and discussing problems.

About the APIs: I was not aware that the API is not allowed in Windows Store apps. In the past, we only targeted UWP, where all the APIs you see in VS can actually be used. That is not true anymore for Desktop/WinUI apps, so we need to be more careful when usind Win32 APIs. However, the proposed change does not work: Window.Current alway returns null for WinUI apps. It is only used in WinUI2/UWP. We should check if there are other ways to get hold of the main window.

I am not sure about your intention with the second changeset. Are these properties needed for Desktop or WinUI apps? I removed them because I thought they are only required for UWP apps.

Oh and by the way: I am currently super busy, so it is difficult for me to even follow all the discussions here. Sorry about that. Trying to catch up a bit.

[softworkz]

I've update this PR and commented out the part for which an alternative is yet to be found,

[softworkz]

Hey guys, I noticed that the tone has become a bit rough here. Please be nice and friendly. Let's be constructive.

To be honest, I've gotten a little frustrated, because it's the opposite of constructive when everything I'm stating - and which is resulted from in-depth research on the subjects - is getting doubted out of a position with sparse knowledge on the individual subjects. I won't continue to justify everything twice and thrice, but I will of course reply kindly if being asked how I come to certain conclusions.

In this PR there are two seemingly unrelated things. One is about removing APIs, the other has to do with project settings, which does not really seem to be related. And then there is the discussion about DesktopCompatible, which again has nothing to do with either of these.

In fact all the changes are related by a chain of causalities, otherwise I would have kept things separate indeed. I agree that it's still more useful to be able to discuss individual changes separately and that's why I have created those partial PRs which are covering individual aspects of the 'all-in-one' PR (#379).

So here's the big picture on how everything is related to each other:

AppContainerApplication and ApplicationType must be set (this PR)

For a C++/WinRT component these need to be set (not only for UWP) => https://github.com/ffmpeginteropx/FFmpegInteropX/pull/381/commits/6d1e7eb81666db6d3c84d1472ac6b7b1e93e3ec1

You said you can no longer rely on VS indicating which APIs can be used, but in fact you still can rely on it: As soon as you set those properties, the project will no longer compile and VS Intellisense will show that you cannot use EnumThreadWindows etc.

As a consequence, this code needs to be replaced => https://github.com/ffmpeginteropx/FFmpegInteropX/pull/381/commits/0f7b3f01a97118400dedaf50bd0e17a267665a4b

I hope this clears up how the changes are related and why they are combined in this PR.

[!NOTE] Without this change, other changes I'm proposing might not work as they should, that's why this PR (#381) is included in the main PR (https://github.com/ffmpeginteropx/FFmpegInteropX/pull/379).

Now, let's spin this further down while we're at it, so you can get a full overview at glance:

Getting DLL Dependencies right by specifying DesktopCompatible

The DesktopCompatible setting is required to enable consumption of FFmpegInteropX in both, packaged and non-packaged WinUI or Desktop Apps. This must be set for all non-UWP builds It is the correct and officially supported and intended way for authoring C++/WinRT components.

[!NOTE] This change is crucial for everything to work, and the starting point for other changes, that's why I made no separate PR for it and it's included in the main PR only (#379)

Removing the Environment Parameters for Command Line Builds

I made this a separate PR (#380), but there's an implication: Setting DesktopCompatible has no effect with the current command line builds (due to the app_platform=UWP parameter).

[!NOTE] We would get communications problems when one of us is using VS for building and another one is using the command line, and I'd like to avoid any arguing due to different results. The situation is extremely unfortunate, because when you have built from the command line and then switch to VS and build there, you still might get wrong results when intermediates from the command line build are still present. As such, it is inevitable to include the PR (#380) also in the main PR (#379)

Desktop Build for FFmpeg is Required

The effect of the DesktopCompatible variable is that the outputs are compiled without dependencies on the app-local runtimes used for UWP components. As a matter of fact, the FFmpeg dlls that we're using need to be free from app-local dependencies as well.

For this reason, the separate FFmpeg build is needed. It doesn't make sense to PR it separately, it's only included in the main PR (#379). Of course it's separated by commits.

Replacing FFmpegInteropLibs.props

This is in a separate PR (#383) and normally it could remain separate. But there's a problem: Now that we have a separate FFmpeg variant for non-UWP builds, the FFmpegInteropLibs.props file would need to be adjusted - which would have been a pretty obscure change. That's why I have included the separate PR (#383) in the main PR (#379) and on top of it, I have made it conditional for using either FFmpeg-UWP or FFmpeg-Desktop build outputs.

Means it's once again a dependent change.

Use Proper Platform Targeting

Getting the targeting versions in the output and in the nuget packages right, is more or less the icing on the cake (PR #382). I have included this in the main PR (#379) to get a consistent and valid result.

Wrapping Up

I hope it's more clear now why I had to do the PRs in the way I did and how all parts are related together and depending on each other. Questioning submitted code is a good and valid procedure, but when it's not founded on substantial research, knowledge and understanding, then it's neither productive nor does it lead anywhere. That's why I found it inevitable to have one PR which demonstrates how all parts are being combined together to produce a correct and working result: #379

[lukasf]

Okay I understand now how the two commits are related.

But I have to say I agree with @brabebhin that we should fully understand what we are doing. And in the big PR, only at the very end it became clear where the problem really is. And to me it is not at all clear if the big PR is really needed like that. Especially the question if we need a separate ffmpeg build. The DesktopCompatible flag does not at all mean that the lib can only be used by desktop apps. Instead it means that the output of a UWP lib can also be used by Desktop apps (without forwarders). I think, if we manage to set the flag on the ffmpeg build, we can continue using just a single build, which would make most of the big PR obsolete.

[softworkz]

Okay I understand now how the two commits are related.

But I have to say I agree with @brabebhin that we should fully understand what we are doing. And in the big PR, only at the very end it became clear where the problem really is. And to me it is not at all clear if the big PR is really needed like that. Especially the question if we need a separate ffmpeg build. The DesktopCompatible flag does not at all mean that the lib can only be used by desktop apps. Instead it means that the output of a UWP lib can also be used by Desktop apps (without forwarders). I think, if we manage to set the flag on the ffmpeg build, we can continue using just a single build, which would make most of the big PR obsolete.

Sure thing, you can take all the time you need. I'm not dependent on this PR. I have my own version and I'm painfully switching between three source trees to manage my custom version, the official full PR and the individual partial PRs.

On my side, I'm good already. I'm submitting the PRs for once in order to give something in return for your hard work on this library and secondly it would of course be beneficial to have aligned sources for going forward in the future.

[brabebhin]

On the topic of window id, I'm personally happy with throwing an exception or simply ignoring the window id of 0 as if HDR is not supported. If a fool proof method that's supported in both uwp and winui comes up, we can afford to do a breaking change for that. Overloads without window id can be used for audio only scenarios.

[softworkz]

On the topic of window id, I'm personally happy with throwing an exception or simply ignoring the window id of 0 as if HDR is not supported. If a fool proof method that's supported in both uwp and winui comes up, we can afford to do a breaking change for that. Overloads without window id can be used for audio only scenarios.

I agree, I think that's just fine.

[brabebhin]

If we do a desktop specific build that targets apps outside of appcontainer, we can actually keep the forbidden API.

[softworkz]

If we do a desktop specific build that targets apps outside of appcontainer, we can actually keep the forbidden API.

The forbidden API was only used for WinUI, it is not needed otherwise, The target use case would be pretty small (only for WinUI3 and non-packaged).

And one thing we haven't talked about yet: As of now, a 3rd build for desktop won't even be necessary, as the WinUI build can cover this.

[brabebhin]

I'm talking about a build that's not even coded in yet. If we do a desktop build, that conditional compile will change from winui to desktop.

You are right that it's a pretty niche target. This is purely a hypothetical thing atm.

[softworkz]

I'm talking about a build that's not even coded in yet

It is..

Of course there's no specific implementation so far, that's true.