Replies: 17 comments 81 replies
-
This looks fine. Yes, I think there's no practical difference within FFmpeg between winUI and desktop. Only our own library needs a winUI dependency, although maybe we should try to remove that as well? The only reason we need UI framework reference is to handle the subtitle flicker, but IIRC we should now be able to pull subtitle samples the same way we pull audio and video samples with some adjustments? I think this is worth pursuing, it will greatly simplify our interface and will also be needed for transcoding anyway.
This is fine to be in a different package. Given that winUI/Desktop is unlikely to fix this any time soon, we should take it out of the lib.
Do we really need difference packages for transcoding only? I mean they will take out decoders which results in a smaller build footprint, but as far as I know, any transcoding tool is also capable of playback. |
Beta Was this translation helpful? Give feedback.
-
|
We need the Window to check for HDR capabilities on WinUI. Theoretically, we could still hide the WinUI dependency: We can add a WindowHandle parameter (uint64) to the desktop build. We can easily check if WinUI is available, like I did in the first version of the winui branch. If it is, we get the WindowId from the WindowHandle (there's an API for that) and then do HDR detection. That way, the same lib can be used from desktop and winui, without strong dependency on the WinAppSDK. For the transcoding, sure we could omit the "Transcoding Only" package. I also said we can go with all-in-one package only, plus the playback package. It probably does not make sense to blow up the packages like that, just to save a few MB. But we don't need to take that decision now. |
Beta Was this translation helpful? Give feedback.
-
|
That is assuming we rework the way we do subtitles. Which might not be an easy task. |
Beta Was this translation helpful? Give feedback.
-
|
Should we also tag the Playback packages with "LGPL"? I don't really plan to do a GPL build for the playback part. It is mostly the encoders where GPL matters. On the other hand, we did not have SSL support for quite some time, because openssl had a weird license and other SSL libs were GPL. Thinking about it, IIRC there are some effects which are GPL only. |
Beta Was this translation helpful? Give feedback.
-
|
Yea, we probably should do so. |
Beta Was this translation helpful? Give feedback.
-
|
I remembered that we can replace referenced NuGet packages like this: We could just refernce the LGPL ffmpeg build by default from our lib. If people want to use a GPL build instead, they just exclude all assets from the LGPL ffmpeg build and add reference to GPL build. That way, we don't need separate LGPL and GPL versions of our lib, only to support LGPL and GPL ffmpeg builds. |
Beta Was this translation helpful? Give feedback.
-
|
I had one more idea when I was working on the config class: It became really huge and confusing. Mix of decoder stuff, subtitle stuff, buffering, etc,... Should we use a 2.0 release for a rework, like breaking up the single class into multiple sub-classes? That would make it easier for devs to find the relevant settings for a specific area. |
Beta Was this translation helpful? Give feedback.
-
|
Agreed. I'll do it. |
Beta Was this translation helpful? Give feedback.
-
|
Okay great. What sub classes shall we use?
Not sure if the last two are really needed. But if we add the, we could keep the main config class clean and distribute all settings to sub classes. Maybe DecoderSettings is not really needed, MaxThreads could go to Audio/Video and SkipErrors to PlaybackSettings. I'd propose to keep the property name on MediaSourceConfig short. So have a MediaSourceConfig.Audio property of type AudioSettings. Ofc, drop all deprecated settings. |
Beta Was this translation helpful? Give feedback.
-
|
Oh and let's rename PassthroughAudioMP3/AAC to SystemDecoderMP3/AAC. Passthrough has a special meaning in the audio domain (pass audio untouched to receiver). We should not create confusion here. In the video area, we already switched to the "SystemDecoder" term, which makes things much more clear. |
Beta Was this translation helpful? Give feedback.
-
|
I would merge the last 3 actually. Looks good otherwise. |
Beta Was this translation helpful? Give feedback.
-
|
That's also an option. Like some GeneralSettings for everything not A/V/S specific. |
Beta Was this translation helpful? Give feedback.
-
|
Currently, I think we should go with the following packages in the first step: Two lib packages:
One FFmpeg package. Possible names:
Considerations:
|
Beta Was this translation helpful? Give feedback.
-
|
What was the reason we need the desktop package? Path based API? |
Beta Was this translation helpful? Give feedback.
-
|
The Desktop package also has the WindowHandle parameter to allow HDR checking on WinUI. On UWP, I might want to add a different parameter (DispatcherQueue), so media sources can also be created on a background thread, while still having access to the dispatcher and HDR checking. We could probably add the path based API to the UWP package as well. But it would only work for app local files, so I don't know how much sense it makes. Right now it would still be possible to support both UWP and Desktop/WinUI from the same lib. But it is already a bit stretched. And the platforms might diverge even more in future. So I'd think it is better to do the split now, instead of having trouble with this later. |
Beta Was this translation helpful? Give feedback.
-
|
Back in time when my app was uwp, I actually wanted to have path based API for local files. It might be useful for playing locally downloaded files or app resources. You do have a point about diverging APIs in the future. Yeah, it's probably best to separate them now. It would have been cool to not have to separate them, but it is what it is. After all, Microsoft also separates packages. |
Beta Was this translation helpful? Give feedback.
-
|
Okay then we should add the path API to all platforms. We can still create a NuGet package which targets both UWP and Desktop. It would just reference the corresponding dedicated package. Then at least from the dev perspective, it's almost like a single lib. |
Beta Was this translation helpful? Give feedback.
-
|
I'm more concerned about overhead on our side. |
Beta Was this translation helpful? Give feedback.
-
|
On the topic of nuget packages, I discovered a new thing to be the central nexus of my hatred for the remaining of the year: the FFmpegInteropX.DotNet project. Its mare existence is an affront to everything good humanity stands for. I'm joking. We should discuss once and for all the problem of the output path for this project. In the current nuspec file, it says
However, the path is incorrect, the files are always generated in Release/ directory, not in the Release_WinUI directory. |
Beta Was this translation helpful? Give feedback.
-
|
Yeah I was trying different things with the build config. This needs to be cleaned up. |
Beta Was this translation helpful? Give feedback.
-
|
Given all the various conversations on the topic of packaged vs unpackged, desktop vs winUI, UWP vs everything else that have happened lately, I think it is a good idea to centralize our current status. As @softworkz discovered, we do not always work correctly. This refers strictly to the master branch.
|
Beta Was this translation helpful? Give feedback.
-
|
When you set the property IsVideoFrameServerModeEnabled, you will get the events VideoFrameAvailable and SubtitleFrameChanged in MediaPlayer, when there's a video frame or a subtitle frame available in the timeline (this is important, MediaPlayer still handles audio-video-subtitle synchronization). Additionally, MediaPlayer will not render video to a MPE if attached to one, but will render subtitles. You can then use the methods CopyFrameToVideoSurface and RenderSubtitlesToSurface to copy the video frame or render the subtitle frame to a DirectX surface of your choosing. This happens on the rendering side of the MediaPlayer. The decoding/demuxing side can do whatever it pleases, so in the case of ffmpeginteropx, it will not matter if the video decoder mode is DirectX, Passthrough or ffmpeg software decoder. Frame server mode was used by people to create custom MediaPlayerElements for winUI before it was officially available. There'a an example here There are some other disadvantages/limitations:
|
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the information. I have come across that sample already, but it wasn't clear to me that you can take care of displaying the video frames yourself.
You could create your own MPE control from the source code... |
Beta Was this translation helpful? Give feedback.
-
|
I think there's copying in some situations, but inside graphics memory which should be fine. Performance seems to be fine with frame server mode. Good idea about the source code, haven't thought of that actually. |
Beta Was this translation helpful? Give feedback.
-
It does (somewhat) matter for us: When somebody is watching with a laptop on batteries and then gets back and tells that with a competitor's player he can watch the double or triple amount of time before batteries are exhausted, then we have a bit of a problem. 😢
WinUI3 source hasn't been been available so far, but they've sneaked it out now, secretly and silently. It's not even announced yet. Probably they'll waiting to do this on .NET Conf next week. |
Beta Was this translation helpful? Give feedback.
-
|
I think copying is involved when doing multi gpu situations. Like a gaming laptop connected to an external display. But this is the responsibility of the app. |
Beta Was this translation helpful? Give feedback.
-
|
I don't like the way how discussions are always hiding the latest (direct) responses, so I'm reposting as a comment:
It does (somewhat) matter for us: When somebody is watching with a laptop on batteries and then gets back and tells that with a competitor's player he can watch the double or triple amount of time before batteries are exhausted, then we have a bit of a problem. 😢
WinUI3 source hasn't been been available so far, but they've sneaked it out now, secretly and silently. It's not even announced yet. Probably they'll waiting to do this on .NET Conf next week. |
Beta Was this translation helpful? Give feedback.
-
|
Interesting. That repo also contains a "phone" folder/namespace. |
Beta Was this translation helpful? Give feedback.
-
|
Maybe they realized that some of the controls intended for Windows phones were also used by non-phone UWP apps and they determined that it would be blocking users from migrating to WinUI3 if it wouldn't have corresponding versions of those controls. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks @softworkz, I was looking into the controls/dev folder. Looks like in dxaml they have the older controls, which are not yet migrated to C++/WinRT. Interesting. |
Beta Was this translation helpful? Give feedback.
-
|
Yes, and the source appears to be incomplete. MediaPlayerPresenter works roughly the way I imagined it would, with a swapchain for video rendering, but the code that returns the swap chain from the MediaPlayer is missing. Still, it does offer some insight into how things works for reference purposes. MPE source code has nothing useful about subtitles. |
Beta Was this translation helpful? Give feedback.
-
|
No, that's the actual code. They would have been stupid to develop WinUI2 and 3 simultaneously in a copy-code manner. The controls "become" WinUI3 controls in the Microsoft namespace by code generation. I did a lot of tweaking to the projects in the solution to get the important ones even loaded in VS. But now, when I do a search for ListViewItem for example, you get a good picture of the structure:
When I open one of the files in the winrtgeneratedclasses folder, you can see this:
|
Beta Was this translation helpful? Give feedback.
-
|
Now that the evidence is tightening, I wanted to note that there's one line in your table which is not correct:
I know it "works", but it's not intentional that it works. You could see that it doesn't work in a project with integrated packaging, only when packaging with a WAP project. This works, because the WAP project is "dumb". It treats "your" package (pre50) with the library dll as a UWP component - because that's what it is and how it is compiled like. It doesn't perform any checks whether it matches together with the other libraries being packaged. So, "currtently working" is actually a "no" - but it will be working soon of course... |
Beta Was this translation helpful? Give feedback.
-
|
You mean random crashes? |
Beta Was this translation helpful? Give feedback.
-
|
Yes, possibly. Not necessarily, but something like: it's possible that it's always working fine in many cases, but then you add another component which does similar API calls (but was compiled in the intended way which might not be compatible with UWP), then there can be conflicts, same like I've seen and same like the other guy reported on Hololens. Let's put it like this: For sure it is not a scenario which the architects at MS have had in mind or even thought about. There are enough "holes" already in the plot they have worked out, so it's not safe to rely on something which they didn't even have on the table. |
Beta Was this translation helpful? Give feedback.
-
|
What i can tell for sure is that the app has been rock solid stable for months. But on the other hand i don't use custom ffmpeg builds, and ffmpeginteropx is referenced directly through project references. |
Beta Was this translation helpful? Give feedback.
-
|
I have no doubt that it's running fine, and when it did so far, then it will surely continue to do so. I mean, it's like a little bomb. Maybe it will never detonate, but you never know. And in case when you would be distributing your app to a larger audience, it's a totally different game either: No matter if the user count has four, five, six or more zeros - those users will encounter issues that you wouldn't have seen yourself in 1000 years. But users will encounter everything that can possibly be encountered. Always... Once I had seen that crash on retrieving the title from the ffmpeg dictionary, I did something incredibly stupid: I commented out the offending line, tested, and concluded that everything else is fine because it is all working and I've been so focused on the other crash issues that I thought it's just peripheral and unrelated. A beginner's mistake actually. |
Beta Was this translation helpful? Give feedback.
-
|
I don't know what you're doing with your player, so I can only say: if there aren't any serious stakes behind it, then you can let it go and see what happens. If it would be a commercial product, though, I'd say that under no circumstance you could you release it that way. |
Beta Was this translation helpful? Give feedback.
-
|
I've cleaned up this thread a bit. Unfortunately I had to delete some conversations between me and @softworkz related to subtitles and directx which were a tad offtopic. I am shocked there is no "move" option on this forum. If you need confirmation on the directx parts we can discuss them on a different thread/topic. |
Beta Was this translation helpful? Give feedback.
-
That's fine, I don't mind. |
Beta Was this translation helpful? Give feedback.
-
|
PS: Feel free to clean up other chit-chat/gossip talk as well :-) |
Beta Was this translation helpful? Give feedback.
-
|
I have updated the top post with the new lib layout, as I imagine it for v2.0. |
Beta Was this translation helpful? Give feedback.
-
Minor version bumps in ffmpeg may add APIs but not change existing ones and also not change any struct layouts and sizes (public part of structs, sometimes there's a private part which may change iirc) |
Beta Was this translation helpful? Give feedback.
-
|
Yes, I also think that sticking to same major version should be sufficient. Fields can be added in minor versions. But as long as we use the official APIs to create struct instances, using a higher minor version should be fine. Using a lower minor version can lead to problems. |
Beta Was this translation helpful? Give feedback.
-
|
So if we're separating the desktop from uwp build this should also deal with the wrong dll linkage. I actually had some more tests about this and it is indeed very difficult to support a single build for both uwp and desktop. |
Beta Was this translation helpful? Give feedback.
-
|
Not difficult when you use my PR 😄 |
Beta Was this translation helpful? Give feedback.
-
|
Your PR was just way too huge and hard to get. Also, I did not like that it just deleted work I had done e.g. on encoders support, instead of making things configurable. Most of the functionality of that PR is now already included, and the latest one adds desktop ffmpeg build, but in a much more flexible way. Once that is merged, we should be more or less on par with your PR (at least I hope so, I need to verify). |
Beta Was this translation helpful? Give feedback.
-
That's why I submitted parts separately.
These were just a few lines and they were of no value at all. These wouldn't have been useful for adding encoding support, which involves a massive amount of work which wouldn't benefit from those lines
No hard feelings from my side. You can close all those PRs. I hope they were useful. I merely wanted point out that it's no longer "very difficult" because I've done all the work and research already. 😉 |
Beta Was this translation helpful? Give feedback.
-
|
Why not use a single top-level package? There's no need to have separate ones and it works without blowing up download sizes:
|
Beta Was this translation helpful? Give feedback.
-
|
Yes, top level package should be ok. I wonder why Microsoft doesn't do it with win2D for example. They seem to create different packages for uwp and winui. |
Beta Was this translation helpful? Give feedback.
-
|
Funny, the exact same question came to my mind just half an hour ago when I reread the top post. |
Beta Was this translation helpful? Give feedback.
-
|
It shouldn't matter. The available types should be visible to the right target runtime. If they can do this across aspnet core versions, surely they can do it on win2D. |
Beta Was this translation helpful? Give feedback.
-
|
Sure, but they probably knew that there's no future for UWP (and no funds allowed to dedicate on such work). |
Beta Was this translation helpful? Give feedback.
-
|
They have also renamed the package for the WinUI. The old one did not follow any of their current naming conventions. The common top leven thing also only really works for smaller lib structures. If you have a larger lib "landscape" where devs add only some of the available libs, the approach does not work. |
Beta Was this translation helpful? Give feedback.
-
VersioningThe previous/current way of versioning isn't quite straightforward, as you don't know which lib version corresponds to which FFmpeg version. For example: 5.1.123 and 6.0.123 would be at the same implementation level of FFmpegInteropX, just one compiled with FFmpeg 5.1 and the other with FFmpeg 6.0 |
Beta Was this translation helpful? Give feedback.
-
|
A single package makes sense and avoids the crashes users have encountered frequently (like mentioned further down) |
Beta Was this translation helpful? Give feedback.
-
|
The ffmpeg package is really just a build of the ffmpeg lib. It would be weird to give it anything else but the ffmpeg version number. It can also be helpful for people to know which ffmpeg version is actually used (and complain if it is too old ^^). Having a separate version number for the lib is helpful as well I think. If devs update the nuget package and see only a build version bump, they know there's probably not much new. But if they see a minor or even major version bump, they know that it might make sense to check details for new functionality in the lib. It would be weird to introduce major new features and only increment the build number. I know that it can be confusing at first sight. But I still think it makes sense. Any different kind of version scheme would have other major drawbacks. |
Beta Was this translation helpful? Give feedback.
-
|
Strictly speaking, that kind of nuget versioning is not valid to use anyway, because it relies on consistent continuous upward compatibility - which is not the case here. A 5.1 lib (<= see how useful it would be when the lib as versioned like this) won't be compatible with a 6.0 ffmpeg - which nuget logic takes for granted. |
Beta Was this translation helpful? Give feedback.
-
|
BTW: I think *.Decoding.LGPL is a bit too much. Any future packages can add suffixes, but the normal ones don't need such bloated naming. |
Beta Was this translation helpful? Give feedback.
-
|
Maybe you are right. Will think about that. |
Beta Was this translation helpful? Give feedback.
-
Framework PackagesAnother topic to consider might be to publish the FFmpeg packages as "Framework Packages". You have surely seen those in the project tree in Visual Studio. Just recently I came to a better understanding about how these are working and are intended for. In short: These are largely nuget packages like all others with a little bit of extra magic: When you compile an exe project, then MSBuild will copy those libraries to the output folder same like for other nugets. But the big benefit of that is that any updates to your application will not include the framework files and when the user updates your app, it will only download your app's MSIX because it already has the framework package. |
Beta Was this translation helpful? Give feedback.
-
|
Interesting. But I am not sure if I want to open that box right now. Could open up a host of new issues 😄 |
Beta Was this translation helpful? Give feedback.
-
|
Interesting to see how this will work with dotnet native compiler, assuming it will ever support winrt. |
Beta Was this translation helpful? Give feedback.
-
I'll let you know how it worked out. My primary use case is shipping the WebView2 runtime which is by far bigger.
This is not about compilation, it's just nuget packages where the output doesn't get packaged as there's a separate dependency MSIX package.. |
Beta Was this translation helpful? Give feedback.
-
Plot HoleI'm lifting this out of the "Versioning" thread because I think it's separate and important. You (@lukas) wrote:
At another occasion I had asked whether it wouldn't make sense to ship the lib in a single package together with FFmpeg (so everything is compiled consistently). You had responded that you want to keep the FFmpeg package separate to allow others to bring your own ffmpeg (BYOF 😉). These two things are pretty contradictive IMO. If somebody has a customized ffmpeg, it's not realistic to expect that they would adapt to the same major FFmpeg version that FFmpegInteropX is expecting for a certain release. But when you say that you don't want to do this and you want to deliberately change the FFmpeg version at whenever you like, it is no longer useful for many, because you cannot expect them to follow the same timeline of FFmpeg versions updates. Either of the two makes sense to me, the current way not that much. |
Beta Was this translation helpful? Give feedback.
-
Yes, it wasn't set up correctly, and it's true that I spent the most time on this. But the crashes I had on UWP those were caused by incompatible ffmpegs. |
Beta Was this translation helpful? Give feedback.
-
|
The WinUI builds weren't like far off. It was well researched and well done in many points except a few which had only become clear after substantial effort and deep research. |
Beta Was this translation helpful? Give feedback.
-
Put a big warning somewhere like: It is recommended to compile the lib together with a custom ffmpeg This is what would have turned me in the right direction from the start. |
Beta Was this translation helpful? Give feedback.
-
|
We can also hard link the dependency in the nuspec file so it only accepts a specific version. |
Beta Was this translation helpful? Give feedback.
-
|
Actually, we could compile the lib against an older ffmpeg build and still combine it with newer ffmpeg builds, as long as there is no major version update. We have done so for quite a while: We still compiled against ffmpeg 5.0, when we already shipped with ffmpeg 5.1. I only noticed this by coincidence. Then I updated this, so the lib compiles against 5.1 as well (which probably broke your setup). But we could have easily stayed on 5.0 as well without issues. Only when we want to use new API (which is rarely the case, except due to deprecations), we need to link a later lib. But since the frequency of major ffmpeg updates has gone up considerably, it's not a real long term solution for you. |
Beta Was this translation helpful? Give feedback.
-
|
Considering it is possible to alter media stream descriptors once the media source has been created, as we previously learned from the Microsoft fork, how about we slightly rethink the approach to HDR? At the moment, we take an optional intptr/windowId in the factory methods. This has the shortcoming that if the user moves the MPE from a window that supports HDR to a window that doesn't, or the other way around, this will result in the incorrect HDR metadata being set. Users have to recreate the ffmpeg media source with the correct window id, which is not ideal. The following 2 suggestions both relay on having support for dynamic stream descriptor changes.
|
Beta Was this translation helpful? Give feedback.
-
|
When we get the DisplayConfig using the WindowId, we can attach an event handler and get notified, if the Windows is moved to a different display or the display settings have changed. So I'd see this as the preferred way. It does not require any manual notifications from the app dev, and it does not require new APIs. |
Beta Was this translation helpful? Give feedback.
-
I have been thinking about how to structure the NuGet Packages for a 2.0 release. It should be simple to consume, but also flexible and extensible. If we introduce encoding/transcoding APIs, I would like the NuGet structure to work well with that. Where possible, we should follow established conventions.
We will have two main targets: UWP and Desktop (.NET/Win32 with or without WinUI)
The main focus of our lib is currently playback scenarios. Our ffmpeg build only includes decoders and effects. Even if we'd introduce transcoding through ffmpeg, the majority of our users will only use it for decoding. So we should not start delivering full ffmpeg builds including encoders to devs who only want to use playback. We should be prepared now to deliver different versions of FFmpeg in the future.
Proposed NuGet Structure
Current Scenario: Decoding/Playback
FFmpegInteropX.Decoding.LGPL
The top level package does not contain files, it only references the lib and the ffmpeg package, as UWP or Desktop version, depending on target platform. It is the convenience package that most devs will use. Alternatively, devs can use the Lib package, if they deliver their own custom ffmpeg build. The Lib package does not contain a ffmpeg reference. The custom build must ofc be binary compatible (at least same major version, maybe also same minor version, not sure about the latter).
For decoding, we probably do not really need a GPL package. I think it only adds a few effects. GPL only really makes sense if we deliver a full package with encoding/transcoding. Still, it's probably good to include LGPL in the lib name, so we could release a GPL version later, if we see a need for that.
Note on the FFmpeg versions:
A separate Desktop build seems to be required, as laid out by @softworkz. Altough an UWP build works fine in most scenarios, there seems to be some corner cases where UWP and Desktop dependencies collide.
BasicVideoEffect
The BasicVideoEffect should be removed from the main lib, because it introduces a dependency to Win2d, which might not be desirable for all users. It currently only works on UWP anyways. I'd still like to keep it in, since it's quite convenient (unless MediaPlaybackList is used). I would put it in a separate package:
FFmpegInteropX.UWP.Decoding.VideoAdjustmentsEffect
Potential future scenario:
"Transcoding" (remux, demux, transcode, also contains decode ofc)
FFmpegInteropX.Full.LGPL
FFmpegInteropX.Desktop.Full.GPL
Beta Was this translation helpful? Give feedback.
All reactions