From 1ad638f58f68fc9ed8bb0aa965a7752c42968de9 Mon Sep 17 00:00:00 2001 From: Martin Finkel Date: Mon, 26 Apr 2021 12:48:54 +0700 Subject: [PATCH 1/8] Media: Internally keep a reference to the last set Media libvlc_media_player_get_media increments the ref count of the media, which may easily cause a memory leak, as this is very unusual behavior for C# property getter to have these types of consequences. Users don't have the reflex to dispose every Media returned by the getter property. Holding a ref internally in the mediaplayer prevents from incrementing the native ref count of the media object. https://code.videolan.org/videolan/LibVLCSharp/-/issues/442 --- src/LibVLCSharp/Shared/MediaPlayer.cs | 28 +++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/src/LibVLCSharp/Shared/MediaPlayer.cs b/src/LibVLCSharp/Shared/MediaPlayer.cs index 0bbbb4b53..c2e93598d 100644 --- a/src/LibVLCSharp/Shared/MediaPlayer.cs +++ b/src/LibVLCSharp/Shared/MediaPlayer.cs @@ -34,11 +34,6 @@ readonly struct Native internal static extern void LibVLCMediaPlayerSetMedia(IntPtr mediaPlayer, IntPtr media); - [DllImport(Constants.LibraryName, CallingConvention = CallingConvention.Cdecl, - EntryPoint = "libvlc_media_player_get_media")] - internal static extern IntPtr LibVLCMediaPlayerGetMedia(IntPtr mediaPlayer); - - [DllImport(Constants.LibraryName, CallingConvention = CallingConvention.Cdecl, EntryPoint = "libvlc_media_player_event_manager")] internal static extern IntPtr LibVLCMediaPlayerEventManager(IntPtr mediaPlayer); @@ -623,8 +618,11 @@ public MediaPlayer(Media media) : base(() => Native.LibVLCMediaPlayerNewFromMedia(media.NativeReference), Native.LibVLCMediaPlayerRelease) { _gcHandle = GCHandle.Alloc(this); + _media = media; } + Media? _media; + /// /// Get the media used by the media_player. /// Set the media that will be used by the media_player. @@ -635,10 +633,19 @@ public Media? Media { get { - var mediaPtr = Native.LibVLCMediaPlayerGetMedia(NativeReference); - return mediaPtr == IntPtr.Zero ? null : new Media(mediaPtr); + return _media; + } + set + { + if(_media?.NativeReference != IntPtr.Zero) + { + _media?.Dispose(); + _media = null; + } + + _media = value; + Native.LibVLCMediaPlayerSetMedia(NativeReference, value?.NativeReference ?? IntPtr.Zero); } - set => Native.LibVLCMediaPlayerSetMedia(NativeReference, value?.NativeReference ?? IntPtr.Zero); } /// @@ -2408,6 +2415,11 @@ protected override void Dispose(bool disposing) { if (_gcHandle.IsAllocated) _gcHandle.Free(); + if (_media?.NativeReference != IntPtr.Zero) + { + _media?.Dispose(); + _media = null; + } } base.Dispose(disposing); From c3bab01c222d48722789c149d43e29b0df88cf93 Mon Sep 17 00:00:00 2001 From: Martin Finkel Date: Mon, 26 Apr 2021 13:16:48 +0700 Subject: [PATCH 2/8] Tests: Adjust LibVLC API coverage to reflect libvlc_media_player_get_media removal --- src/LibVLCSharp.Tests/LibVLCAPICoverage.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/LibVLCSharp.Tests/LibVLCAPICoverage.cs b/src/LibVLCSharp.Tests/LibVLCAPICoverage.cs index 0822249b8..62cbd4aa4 100644 --- a/src/LibVLCSharp.Tests/LibVLCAPICoverage.cs +++ b/src/LibVLCSharp.Tests/LibVLCAPICoverage.cs @@ -93,7 +93,8 @@ public async Task CheckLibVLCCoverage() List notImplementedOnPurpose = new List { "libvlc_printerr", "libvlc_vprinterr", "libvlc_clock", "libvlc_dialog_get_context", "libvlc_dialog_set_context", - "libvlc_event_type_name", "libvlc_log_get_object", "libvlc_vlm", "libvlc_media_list_player", "libvlc_media_library" + "libvlc_event_type_name", "libvlc_log_get_object", "libvlc_vlm", "libvlc_media_list_player", "libvlc_media_library", + "libvlc_media_player_get_media" }; List exclude = new List(); @@ -141,4 +142,4 @@ public async Task CheckLibVLCCoverage() Assert.Zero(missingApisCount); } } -} \ No newline at end of file +} From d1d0a893128e02f32b6fd2e81ef66aed4ed7d37c Mon Sep 17 00:00:00 2001 From: Martin Finkel Date: Tue, 11 May 2021 14:18:01 +0700 Subject: [PATCH 3/8] add media retain --- src/LibVLCSharp/Shared/MediaPlayer.cs | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/LibVLCSharp/Shared/MediaPlayer.cs b/src/LibVLCSharp/Shared/MediaPlayer.cs index c2e93598d..a493a8340 100644 --- a/src/LibVLCSharp/Shared/MediaPlayer.cs +++ b/src/LibVLCSharp/Shared/MediaPlayer.cs @@ -631,20 +631,23 @@ public MediaPlayer(Media media) /// public Media? Media { - get - { - return _media; - } + get => _media; set { - if(_media?.NativeReference != IntPtr.Zero) + if(_media != null) { - _media?.Dispose(); + _media.Dispose(); _media = null; } _media = value; - Native.LibVLCMediaPlayerSetMedia(NativeReference, value?.NativeReference ?? IntPtr.Zero); + + if(_media != null) + { + _media.Retain(); + } + + Native.LibVLCMediaPlayerSetMedia(NativeReference, _media?.NativeReference ?? IntPtr.Zero); } } From 84250ebb7d9ae619ef081c0070c9d3831a9adeca Mon Sep 17 00:00:00 2001 From: Martin Finkel Date: Tue, 11 May 2021 14:44:06 +0700 Subject: [PATCH 4/8] Add native ref check --- src/LibVLCSharp/Shared/Media.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/LibVLCSharp/Shared/Media.cs b/src/LibVLCSharp/Shared/Media.cs index 693dd69e5..da41c751d 100644 --- a/src/LibVLCSharp/Shared/Media.cs +++ b/src/LibVLCSharp/Shared/Media.cs @@ -436,7 +436,7 @@ MediaEventManager EventManager /// Get duration (in ms) of media descriptor object item. /// duration of media item or -1 on error - public long Duration => Native.LibVLCMediaGetDuration(NativeReference); + public long Duration => NativeReference == IntPtr.Zero ? -1 : Native.LibVLCMediaGetDuration(NativeReference); /// /// Parse the media asynchronously with options. @@ -486,7 +486,7 @@ void OnParsedChanged(object sender, MediaParsedChangedEventArgs mediaParsedChang /// Return true is the media descriptor object is parsed /// true if media object has been parsed otherwise it returns false - public bool IsParsed => Native.LibVLCMediaIsParsed(NativeReference) != 0; + public bool IsParsed => NativeReference == IntPtr.Zero ? false : Native.LibVLCMediaIsParsed(NativeReference) != 0; /// Get Parsed status for media descriptor object. /// a value of the libvlc_media_parsed_status_t enum From 946ba50c1d4dd402e436c11a4590cd7d1e2054e0 Mon Sep 17 00:00:00 2001 From: Martin Finkel Date: Mon, 26 Jul 2021 14:23:46 +0700 Subject: [PATCH 5/8] fix add option media usage --- src/LibVLCSharp/Shared/MediaPlayer.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/LibVLCSharp/Shared/MediaPlayer.cs b/src/LibVLCSharp/Shared/MediaPlayer.cs index a493a8340..efb8c0963 100644 --- a/src/LibVLCSharp/Shared/MediaPlayer.cs +++ b/src/LibVLCSharp/Shared/MediaPlayer.cs @@ -663,11 +663,9 @@ public Media? Media /// true if successful public bool Play() { - var media = Media; - if(media != null) + if(_media != null) { - media.AddOption(Configuration); - media.Dispose(); + _media.AddOption(Configuration); } return Native.LibVLCMediaPlayerPlay(NativeReference) == 0; } From fd809d20eb5d9f9d3ea80f9e873d41e7b45f8f11 Mon Sep 17 00:00:00 2001 From: Martin Finkel Date: Mon, 26 Jul 2021 14:24:00 +0700 Subject: [PATCH 6/8] Revert "Add native ref check" This reverts commit 84250ebb7d9ae619ef081c0070c9d3831a9adeca. --- src/LibVLCSharp/Shared/Media.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/LibVLCSharp/Shared/Media.cs b/src/LibVLCSharp/Shared/Media.cs index da41c751d..693dd69e5 100644 --- a/src/LibVLCSharp/Shared/Media.cs +++ b/src/LibVLCSharp/Shared/Media.cs @@ -436,7 +436,7 @@ MediaEventManager EventManager /// Get duration (in ms) of media descriptor object item. /// duration of media item or -1 on error - public long Duration => NativeReference == IntPtr.Zero ? -1 : Native.LibVLCMediaGetDuration(NativeReference); + public long Duration => Native.LibVLCMediaGetDuration(NativeReference); /// /// Parse the media asynchronously with options. @@ -486,7 +486,7 @@ void OnParsedChanged(object sender, MediaParsedChangedEventArgs mediaParsedChang /// Return true is the media descriptor object is parsed /// true if media object has been parsed otherwise it returns false - public bool IsParsed => NativeReference == IntPtr.Zero ? false : Native.LibVLCMediaIsParsed(NativeReference) != 0; + public bool IsParsed => Native.LibVLCMediaIsParsed(NativeReference) != 0; /// Get Parsed status for media descriptor object. /// a value of the libvlc_media_parsed_status_t enum From ade26bddefaba5e77ea1b9e04e8e001649c0522d Mon Sep 17 00:00:00 2001 From: Martin Finkel Date: Mon, 26 Jul 2021 14:28:24 +0700 Subject: [PATCH 7/8] nit --- src/LibVLCSharp/Shared/MediaPlayer.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/LibVLCSharp/Shared/MediaPlayer.cs b/src/LibVLCSharp/Shared/MediaPlayer.cs index efb8c0963..cde650de6 100644 --- a/src/LibVLCSharp/Shared/MediaPlayer.cs +++ b/src/LibVLCSharp/Shared/MediaPlayer.cs @@ -2416,7 +2416,8 @@ protected override void Dispose(bool disposing) { if (_gcHandle.IsAllocated) _gcHandle.Free(); - if (_media?.NativeReference != IntPtr.Zero) + + if (_media != null) { _media?.Dispose(); _media = null; From 8503a4fb7db97bcd90742b40a3d7a03abc8bbec1 Mon Sep 17 00:00:00 2001 From: Martin Finkel Date: Mon, 26 Jul 2021 14:38:06 +0700 Subject: [PATCH 8/8] add failing test --- src/LibVLCSharp.Tests/MediaPlayerTests.cs | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/src/LibVLCSharp.Tests/MediaPlayerTests.cs b/src/LibVLCSharp.Tests/MediaPlayerTests.cs index 8e3caf224..132738fb5 100644 --- a/src/LibVLCSharp.Tests/MediaPlayerTests.cs +++ b/src/LibVLCSharp.Tests/MediaPlayerTests.cs @@ -208,5 +208,18 @@ public void SetMediaPlayerRole() Assert.True(mp.SetRole(MediaPlayerRole.None)); Assert.AreEqual(MediaPlayerRole.None, mp.Role); } + + [Test] + public void MediaRefCountTestFail() + { + var media1 = new Media(_libVLC, new Uri(RealStreamMediaPath)); + var media2 = new Media(_libVLC, new Uri(RealStreamMediaPath)); + + var mp = new MediaPlayer(media1); + media1.Dispose(); + Debug.WriteLine(mp.Media.Mrl); + + mp.Media = media2; + } } }