Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Basic Wpf UserControl #1

Closed
wants to merge 14 commits into from
Closed

Basic Wpf UserControl #1

wants to merge 14 commits into from
+1,489 −246

Conversation

Ch4rg3r
Copy link
Contributor

@Ch4rg3r Ch4rg3r commented Jul 3, 2018

Added Wpf UserControl with Autosizing using WindowsFormsHost

@Ch4rg3r
Basic Wpf UserControl
e0c8001 
Added Wpf UserControl with Autosizing using WindowsFormsHost
mfkl
mfkl requested changes Jul 4, 2018
View reviewed changes
Copy link
Member

@mfkl mfkl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

/cc @jeremyVignelles

LibVLCSharp.Wpf/LibVLCSharp.Wpf.csproj Outdated
<ProjectGuid>{DA7A2677-0944-481F-A59B-9128FC54FD5F}</ProjectGuid>
<OutputType>Library</OutputType>
<AppDesignerFolder>Properties</AppDesignerFolder>
<RootNamespace>LibVLCSharp.Wpf</RootNamespace>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename LibVLCSharp.Wpf to LibVLCSharp.WPF everywhere please?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to wirte Wpf, to case all my namespaces and classes the C# way ( I have tools that complains when I don't)

LibVLCSharp.Wpf/VLCViewControl.xaml.cs Outdated

namespace LibVLCSharp.Wpf
{
public partial class VLCViewControl : UserControl, LibVLCSharp.Shared.IVideoView
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with the other platforms, can you rename VLCViewControl with VideoView?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I believe your control needs to be IDisposable and dispose of libvlc and mediaplayer in the dispose method

LibVlcSharp.Wpf.Sample/App.xaml Outdated
@@ -0,0 +1,9 @@
<Application x:Class="LibVlcSharp.Wpf.Sample.App"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename LibVlcSharp.Wpf.Sample to LibVLCSharp.WPF.Sample everywhere please.

LibVlcSharp.Wpf.Sample/App.xaml.cs Outdated
namespace LibVlcSharp.Wpf.Sample
{
/// <summary>
/// Interaktionslogik für "App.xaml"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment looks very german :D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can rename it of course. Yeah forgot this comment :D

@jeremyVignelles
Copy link
Collaborator

I really wonder if making a wpf control is a good thing. Look at what happened in Vlc.DotNet :
We had this kind of "WPF" control, which is just a wrapper over WinForms. We had airspace issues and user complained about that. Some migrated to Meta.Vlc, which provided a more WPF-ish way of doing that : an ImageSource. With that, you could use it like a Wpf image, use it everywhere like brushes...
I decided to replace the implementation of the WPF control which I considered useless
(Wrapping a component in a WinFormsHost is really easy). I quickly realized that implementing that with an ImageSource would come at a terrible cost : perf is bad...

Here's the situation now, in Vlc.DotNet :

  • User have the choice of their technology (which is good)
  • User tend to install the Wpf control by default, despite my numerous warnings that it comes at the cost of performance, and then complain that the perfs are poor when they use 50 streams.

For the full-length story, see ZeBobo5/Vlc.DotNet#249

With that experience, I would think twice before doing a "Wpf" control.
In Vlc.DotNet, we were limited to windows-only desktop apps, so not providing a WPF option was possible, but now that we should support Xamarin.Forms with a Wpf backend and Eto.Forms...

What would you think of having several Wpf implementations?
Like :
.Wpf.WinForms/.Xamarin.Forms.Wpf.WinForms
and
.Wpf.ImageSource/.Xamarin.Forms.Wpf.ImageSource

Users would have to read a short readme to take an enlighted decision, but if that could avoid the issues about perfs/about airspace.

@mfkl
Copy link
Member

mfkl commented Jul 4, 2018

Thanks for the input @jeremyVignelles

Unity's dude's take on WPF/D3D situation: sharpdx/SharpDX#599 (comment) TL;DR: "D3DImage is fundamentally broken and unsuitable for efficient D3D rendering in WPF."

So right now the choices are:

  • WinForms Host -> Airspace issues.
  • WPF -> bad performance (copy).
  • Just Not support WPF -> 1 less target for Xamarin.Forms.
  • Patch vlc/libvlc to provide a new API which would help bypass all those issues/bad components? Not sure that's even possible /cc @jbkempf

@jbkempf
Copy link

jbkempf commented Jul 4, 2018

You have no choice here, if you want to target most platforms: Winforms -> airspacing issues.

D3DImage does too many copies, and DXGI integration is not working on Windows 7, IIRC, unless people have D3D11 working, which is rare.

@jeremyVignelles
Copy link
Collaborator

jeremyVignelles commented Jul 4, 2018 via email

@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 4, 2018 •
edited
Loading

I know the way VLC.DotNet does this. I thought about DirectX and OpenGL too, but it seems they somehow require Handles too. Even Microsoft wasn't able to fix this - a fix was availible for a short time in PresentationFramework 4.5. Another way mentioned is using HwndHost like here on stackoverflow

Even SharpGL uses Images on WPF

EDIT: also in general i know these issues about airspacing, i wrote this to have an basic alternative to Xamarin.Forms, as this works with Wpf without using xamarin

Ch4rg3r added 2 commits July 4, 2018 12:46
@Ch4rg3r
Changes to WPF-UserControl
9e5ab1b 
- Renaming
- Disposable
@Ch4rg3r
Change #2
0f11012 
Renaming Folders
@jbkempf
Copy link

jbkempf commented Jul 4, 2018 •
edited
Loading

@Ch4rg3r ok, great.

My opinion is to use that WPF/Handle and try to fix the airspacing issues.

Performance is key!

@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 4, 2018

@jbkempf i guess that's true. The mentioned way on stackoverflow (no idea if it works) draws the WPF controls on Win32 on top of the WindowsFormsHost and forwards mouse actions to the WPF controls. This way the WPF is copied and drawn twice. Guess that has less performance impact

@jeremyVignelles
Copy link
Collaborator

jeremyVignelles commented Jul 4, 2018 •
edited
Loading

I agree with j-b : let's go for the perfs.
For the airspace issue, I think we can try the way that was suggested in Vlc.DotNet : https://github.com/kolorowezworki/Airhack

Thanks @Ch4rg3r for your time and reactivity!

Edit : the Vlc.DotNet's airspace issue :
ZeBobo5/Vlc.DotNet#296

@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 4, 2018 •
edited
Loading

@jeremyVignelles seems to be the implementation of what i meant, but simpler. And i just tested it. It works, enabling mouse actions for both background and foreground.
That project does not provide any license. Maybe a problem?

@jeremyVignelles
Copy link
Collaborator

How do you see the interaction with this project? I thought that this would be something to "recommend" for users facing this issue, but not something embedded in the library since most of the users, I guess, just want to display the video, not to hover things on top of that.

However I don't know the status of this project (licensing, packaging, support...), and maybe @kolorowezworki can answer that?

@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 4, 2018 •
edited
Loading

I would make VideoView from it, using the background as MediaPlayer, giving the User of this library the ability to add child elements to the foreground. I guess it requires a few changes

@Ch4rg3r
Updated VideoView for WPF supporting Child Elements
7ddc025 
Now Supports behaviour like container (example Grid). Child Elements can
be added in XAML or code behind and will automatically set to foreground
Elements (preventing airspace issue)
@mfkl
Copy link
Member

mfkl commented Jul 10, 2018

@Ch4rg3r Let us know when you think this is good for review/testing.

@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 10, 2018 •
edited
Loading

@mfkl the last change works for me. If you can test it's functionality, please do it. Maybe you find issues or things to be improved.

Major issues might be performance. But i cannot say why. Could be the way i use MediaPlayer in Examples, maybe it can be improved, but that's not part of the UserControl itself

mfkl
mfkl requested changes Jul 10, 2018
View reviewed changes
Copy link
Member

@mfkl mfkl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick GitHub glance. Will build and test later :) Keep up the good work 👍

LibVLCSharp.WPF/ForegroundWindow.xaml.cs Outdated
internal partial class ForegroundWindow : Window
{
private Window wndhost;
private ContentControl bckgnd;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "_" consistently for private fields please.

LibVLCSharp.WPF/ForegroundWindow.xaml.cs Outdated
wndhost.LocationChanged += Wndhost_LocationChanged;
try
{
Point locationFromScreen = bckgnd.PointToScreen(new Point(0, 0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new Point(0, 0) allocation can be cached in a readonly field.

LibVLCSharp.WPF/ForegroundWindow.xaml.cs Outdated
System.Windows.Point targetPoints = source.CompositionTarget.TransformFromDevice.Transform(locationFromScreen);
this.Left = targetPoints.X;
this.Top = targetPoints.Y;
Vector size = bckgnd.PointToScreen(new Point(bckgnd.ActualWidth, bckgnd.ActualHeight)) - bckgnd.PointToScreen(new Point(0, 0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new Point(0, 0) allocation can be cached in a readonly field.

LibVLCSharp.WPF/ForegroundWindow.xaml.cs Outdated
}
catch
{
this.Hide();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe log an error or something? What could possibly happen here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure when this actually can happen. Most likely would only happen, if no Window with Hwnd is provided, but then you wouldn't see anything anyway

LibVLCSharp.WPF/ForegroundWindow.xaml.cs Outdated

private void Wndhost_LocationChanged(object sender, EventArgs e)
{
Point locationFromScreen = bckgnd.PointToScreen(new Point(0, 0));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new Point(0, 0) allocation can be cached in a readonly field.

LibVLCSharp.WPF/VideoView.cs Outdated
if (mediaPlayer.IsPlaying)
mediaPlayer.Stop();

mediaPlayer.Dispose();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You may add _mediaPlayer.Hwnd = IntPtr.Zero

LibVLCSharp.WPF/VideoView.cs Outdated
libVLC = new Shared.LibVLC();
mediaPlayer = new Shared.MediaPlayer(libVLC);

this.SizeChanged += OnSizeChanged;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be unregistered in the Dispose method.

LibVLCSharp.WPF/VideoView.cs Outdated
controlHeight = this.Height;
controlWidth = this.Width;

this.Loaded += VideoView_Loaded;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be unregistered in the Dispose method.

LibVLCSharp/Shared/MediaPlayer.cs Outdated
@@ -631,6 +631,11 @@ struct Native
#endif
}

public void SetVideoFormatCallbacks()
{
throw new NotImplementedException();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's up with this? :D https://github.com/Ch4rg3r/libvlcsharp/blob/7ddc025392caf39ec1277c4f59585b672d88cf8b/LibVLCSharp/Shared/MediaPlayer.cs#L1272

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea :D i will delete it

LibVlcSharp.WPF.Sample/Controls.xaml.cs Outdated

private void StopButton_Click(object sender, RoutedEventArgs e)
{
parent.Dispatcher.Invoke(() => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

off topic/review question: Do you think it'd make sense for the binding to offer a dispatcher callback registration, so users dont have to do just that on most calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to implement it inside the Control to work for all calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like it's not needed anymore... just tested it without invoke (ThreadIDs are the same)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mfkl : I think it might be interesting to avoid a lot of questions like this one : ZeBobo5/Vlc.DotNet#444

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's not needed : You are in a UI event handler, which runs on the dispatcher.

Ch4rg3r added 2 commits July 10, 2018 13:17
@Ch4rg3r
Fixes
90adbfe 
- Fixed  naming
- Fixed disposing
@Ch4rg3r
Removed Dispatcher.Invoke
e93b7ad
@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 10, 2018

I guess that's it so far. Should be completly fixed and working

@jeremyVignelles
Copy link
Collaborator

Is it just me or did you forget to reference VideoLAN.LibVLC.Windows in the Sample project? The DLL is not found for me

@jeremyVignelles
Copy link
Collaborator

Thanks for your reactivity.
You did a really good job, and thank you for that. You even managed to solve the Airspace issue in a clean manner, congratulations for that.

For me, the WPF control is OK. However, the sample looks more like a test project. I think that we can choose only one of the two Examples and make it clean and commented for future users. We can keep the structure with a "launcher" window, or make a "Minimal" sample, as made in Vlc.DotNet.

I just saw one bug : When the video is playing, closing the video window doesn't stop the player (audio is still playing). Please Dispose() properly the video control when the window is closing, so that users do the same. You might have to do that in another thread...

@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 10, 2018 •
edited
Loading

I will consider that...
Well the Airspace fix wasn't my idea, i jsut changed necessary parts.
I did the sample this way, to show 3 possible ways of using the control with child elements...

EDIT: simple fix commited, player will stop now

@Ch4rg3r
Handling VideoView Unloaded event
92fdcb3 
Fix issue of window closing and player keeps playing
@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 14, 2018

Do you think MediaPlayer and LibVLC should use an instance, to prevent ObjectNullException??

@mfkl
Copy link
Member

mfkl commented Jul 16, 2018

Do you think MediaPlayer and LibVLC should use an instance, to prevent ObjectNullException??

I don't understand. What do you mean use an instance?

@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 16, 2018

I mean the something like

get
{
    if(_mediaplayer==null)   
        _mediaplayer= new MediaPlayer()
    return _mediaplayer
}

Same for LibVLC

@mfkl
Copy link
Member

mfkl commented Jul 16, 2018

No, if you're using the provided VideoView control, you agree to have the LibVLC and MediaPlayer object's lifecycles bound to the control's view lifecycle. They won't be null ever (since newed in the ctor and disposed on dispose()) and I don't see a reason for your lazy loading suggestion.
If a user needs more custom access, they have access to the classes and can do whatever they need.

@jeremyVignelles
Copy link
Collaborator

I call that a lazy initialization (as in the Lazy class).
I think that errors might be hard to spot in those kind of patterns, because a call to a getter isn't meant to actually modify something. Then, later, you don't pay attention, call that from multiple threads and crash... then you lock everything and have perf penalties...

@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 16, 2018

True, might be more often the case than NullException

@mfkl
Copy link
Member

mfkl commented Jul 18, 2018

Can you tick that checkbox so I can push on that PR please?
https://stackoverflow.com/questions/20928727/adding-commits-to-another-persons-pull-request-on-github

@mfkl
Copy link
Member

mfkl commented Jul 18, 2018

Made a PR on your fork. Please review the changes and next time use git mv to rename files ;-)

@Ch4rg3r
Merge pull request #1 from mfkl/master
0c8dfeb 
Fix renaming + cosmetics + rebase
@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 18, 2018

Merged.

@mfkl
Copy link
Member

mfkl commented Jul 19, 2018

https://twitter.com/martz2804/status/1019867718969782273

@Ch4rg3r thanks for your contribution, it's now live on nuget ;-) Feel free to keep those PRs coming!

@Ch4rg3r
Copy link
Contributor Author

Ch4rg3r commented Jul 19, 2018

Thanks for merging and for your work. Great library 👍

vlc-mirrorer pushed a commit that referenced this pull request Jul 19, 2018
@Ch4rg3r @mfkl
Add WPF support with custom control
0ce7e3c 
This uses a WindowsFormsHost for better performance (HWND).
Airspace issues have been worked around.

Merge from GitHub PR #1, initiated by Ch4rg3r
vlc-mirrorer pushed a commit that referenced this pull request Jul 19, 2018
@Ch4rg3r @mfkl
Add WPF support with custom control
d137e06 
This uses a WindowsFormsHost for better performance (HWND).
Airspace issues have been worked around.

Merge from GitHub PR #1, initiated by Ch4rg3r
@mfkl mfkl mentioned this pull request Jul 29, 2018
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyVignelles jeremyVignelles jeremyVignelles left review comments

@mfkl mfkl mfkl requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Ch4rg3r @jeremyVignelles @mfkl @jbkempf