WPF Control Should Not Require WinForm Reference

[thenadz]

The way the WPF control is implemented right now is lazy (no offence intended!). I get it. Adding a wrapper around the WinForm implementation was easier to get something out the door quickly.

But the control would be far more useful if it was correctly implemented natively as WFP, complete with dependency properties to be bound, etc.

[jeremyVignelles]

I have made a few search and found that this is probably harder than expected :
VLC for windows can draw on a HWND, thanks to the libvlc_media_player_set_hwnd (which is called by Vlc.DotNet ).
A quick way to do this is to have a WinForms control and get the HWND on it (as in current implementation), but WPF does not have a HWND per control. There is a method called HwndSource.FromVisual(), but it gets the WPF window's handle, not the UserControl's handle.

Source: https://social.msdn.microsoft.com/Forums/vstudio/en-US/cc6297db-6ed9-4d68-abe2-47769e06d93a/any-way-to-get-an-hwnd-of-a-usercontrol?forum=wpf

There is two requests in your question:

• Remove the dependency on WinForms. I agree on that, but I don't know how to do this...
• Implement proper data-binding on WPF library, which can be done with the current WinForms implementation

How do you see the implementation in a pure WPF control? Maybe a in-memory output and then render it in some way with some WPF component? Maybe getting at a lower level so that we can use the vout plugins for the WPF View?

[thenadz]

@jeremyVignelles,

Good work on digging into what would be required to remove dependency on WinForms!

Based on your work, I did a bit of digging myself and I believe deriving HwndHost for the VLC viewer control is the way to get the window pointer required to get libvlc working directly with WPF.

Regarding the data binding, I was only thinking initially that all of the properties that already exist in the WinForm control would be exposed in the WPF control and would be exposed as dependency properties to support binding rather than simple CLR properties. In addition to what's already exposed as properties, being able to bind the meida value currently only settable through SetMedia() would be great.

I may take a crack at getting the WinForm dependency removed this weekend. Having taken the time to poke at the problem a bit, I think it may not actually be particularly hard.

[jeremyVignelles]

@thenadz Did you manage to do what you wanted?
I'd be really happy to try it out for my project, though I don't think I would have much time to implement it myself.

[thenadz]

@jeremyVignelles I began work on this, but ran into a roadblock related to my relative lack of knowledge in how HMNDs work. I believe the general framework for the change is in place, but I think I need an expert in this type of thing to get me the rest of the way there.

[jeremyVignelles]

Maybe I can try to help. Do you have a fork where I can see what you have yet?

[thenadz]

@jeremyVignelles My current progress is now in this fork: https://github.com/thenadz/Vlc.DotNet

Right now I would expect if you include the WPF VLC control it will pop out a WPF window with the VLC content rather than embedding it into the intended location. Let me know if you run into issues even getting that far.

[jeremyVignelles]

@thenadz I looked at your code, made it compile (not clean solution, many things removed) but I was able to have a pop-out window as you said.

I tried using dotPeek to look at how WinFormHost was doing its integration. Most of the code relies on native interop through the use of System.Windows.Host.

I think we're doing it the wrong way by trying to reinvent the wheel and subclass hwndHost directly : WindowsFormsHost needs the inclusion of additional dependencies, but is able to handle hard tasks for us (resizing, event handling...)

What do you think about keeping the WindowsFormsIntegration dependency, but at least removing Vlc.DotNet.WinForms and reimplementing it in a clean MVVM style? However, doing this would require breaking compatibility with older releases...

[thenadz]

@jeremyVignelles I think whether that solution is adequate depends a lot on what your goals are. If your goal is just to address bindability (excuse the made up word...), then you could just leave the reference to the WinForm VlcControl and wrap interactions in dependency properties. If your goal is to remove reliance on an unrelated UI technology (WinForms) then all WinForm references (both parts of .NET and the WinForm VlcControl) need to be removed.

This issue really is two things, and your proposition addresses the binding support, though if that is the scope of the change you're interested in making I would be inclined to not bother removing dependency on WinForm VlcControl -- I don't think you gain much.

[jeremyVignelles]

I agree with you, but removing the WinForms dependency seems really hard, time consuming and bug-prone. I'm not saying it's a bad idea at all, but considering this project as a side-project driven by volunteers, does it worth it?

Moreover, one thing is really important to consider before doing anything : do not break the API. You may introduce new APIs in the next version, but the change should be progressive as much as possible.
When doing a breaking change, increment the major version number and document it. I will personally be using this library in production at work, and I don't want my application to be ruined after a package upgrade.

That being said, I heard that there was an official UWP version. I wonder how they do this...

[thenadz]

Gotcha! It sounds like we're on the same page then. Yes, we're also using this in production. Major rev would definitely be necessary given the scope of changes required if WinForms are ripped out.

[jeremyVignelles]

I found the repository there : https://code.videolan.org/videolan/vlc-winrt
After digging a little, they have a setHwnd method that is not used anywhere. Now, they have an Instance class (modules/libvlcppcx/InstanceCX.*pp files) in which they are initalizing a DirectXManager (modules/libVLCX/DirectXManager.cpp) and a SwapChainPanel (UWP control).

In a few words : DirectX and managed C++

I found this project : https://github.com/Microsoft/WPFDXInterop but you have to install DirectX sdk and Windows SDK before getting started. I suppose users must have DirectX installed too... I'm not convinced that it's the way to go.

I think we should do this in two steps :

1. Expose bindability, I have seen change wpf VlcControl using mvvm #246, don't know if it works or if it is the API we want. Keep the legacy API as-is.
2. Try to remove the WinForms reference.

What benefits do you expect from removing that dependency? I would only expect a cleaner architecture (conceptually, I agree that WPF apps should not rely on that legacy technology.)

[thenadz]

@jeremyVignelles thanks for doing the actionable research! Yes, I agree with everything you've said. Bringing in a dependency on DirectX sounds like we're robbing Peter to pay Paul. I also agree that it makes a lot of sense to address the two issues separately, and bind ability can be exposed without the need to break backward compatibility so makes senese to do so.

Regarding the last point, removing WinForms dependency in my mind is purely architecture cleanup. I can't justify it beyond that, as much as I would love to. (So again, agree with your points above.)

[jeremyVignelles]

Thanks. By the way, if you have time, i'd love to have your opinion about #288

[jeremyVignelles]

(Just updated #288, read again if you already have)
Another project we should look at is CefSharp. I have already used it a while a go, but I can't remember if it needs WinForms or not. Like vlc, it is mainly a wrapper around c++ dlls (chromium in that case) and displays that in a winforms or wpf window.

[jeremyVignelles]

Got valuable advices from Martin Finkel, videolan's maintainer of vlc-winrt.
I'm pasting (and translating) his advice here for reference:

I agree that wrapping WinForms in WPF is not an ideal solution.
I understand your reluctance to add a new dependency, but to be exact, you are in fact removing one :
From Wikipedia, WPF uses DirectX internally anyway : "Rather than relying on the older GDI subsystem, WPF uses DirectX." https://en.wikipedia.org/wiki/Windows_Presentation_Foundation
See also https://github.com/Sascha-L/WPF-MediaKit/blob/3f44720670314aad4b4a3105fcf57826a3c4e652/Source/DirectShow/Controls/D3DRenderer.cs

So the question is : are we going to introduce a dependency to the DirectX SDK? (Which is included in the windows SDK starting from windows 8, source: https://msdn.microsoft.com/en-us/library/windows/desktop/ee663275(v=vs.85).aspx )

That would be great if you joined the discussion at https://gitter.im/Vlc-DotNet/Lobby 😉

[jeremyVignelles]

Using HwndHost is likely to cause the same airspace issues as many users have been complaining about (see #296). Using DirectX should also fix #296 as well, and we probably can reuse some code from vlc-winrt and join the efforts 😃

[adamfierek]

I found how to fix airspace effect. On days, I'll post description and sample project :)

[jeremyVignelles]

@kolorowezworki That would be great, please post it on #296, or even better, create a PR 😉

[adamfierek]

@jeremyVignelles I will, just give me one day or two ;)

[adamfierek]

https://github.com/kolorowezworki/Airhack
It's quite dirty, but it works ;)

[madallday]

We are currently using the old Vlc.DotNet version 2014.04.18, back when the WPF control actually behaved like a WPF control. Can I ask what the reasons were for completely gutting it? It worked before and now it seems the only way to upgrade is to apply a hack (Airhack :)). Is there any way we can apply the old WPF control to utilize the new core functionality?

[jeremyVignelles]

This will be fixed in 3.0.0 when #365 gets merged.
@madallday : I contacted the author of this plugin, and he told me that there were huge memory leaks back then in .net framework 4.0 that were fixed a long time afterwards. See https://arbel.net/2008/10/22/improper-use-of-interopbitmap-can-cause-a-memory-leak/ and https://support.microsoft.com/en-us/help/2494124/fix-insufficient-memory-to-continue-the-execution-of-the-program-error

As of 2018 (Happy new year!) this should not be an issue anymore.

The implementation I made in #365 is probably not perfect, and I'd like to have your point of view of the API that should be shipped in 3.0.0.