libvlcsharp

libvlcsharp copied to clipboard

Description of Change

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.

Issues Resolved

• fixes https://code.videolan.org/videolan/LibVLCSharp/-/issues/442

API Changes

None, but the usage is subtly changed. Docs should probably mentioned it somewhere.

Platforms Affected

• Core (all platforms)

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

I have not tested this thoroughly.

PR Checklist

• [x] Rebased on top of the target branch at time of PR
• [x] Changes adhere to coding standard

This is a breaking change for those who already did that properly. If they Disposed the media by themselves, the media ends up getting disposed multiple times.

Aren't there some cases where the libvlc media player decides to change the media internally? like making it null on Stop or when playing a playlist?

This is a breaking change for those who already did that properly. If they Disposed the media by themselves, the media ends up getting disposed multiple times.

I know... :-( But trying to find ways to fix it anyway, for 3.x. If it's not possible, the fix will be 4.x only.

Aren't there some cases where the libvlc media player decides to change the media internally? like making it null on Stop or when playing a playlist?

Tried to find these cases but failed. We don't expose the medialistplayer object so that helps somewhat to reduce potentially affected use cases.

Thinking out loud: Should we remove IDisposable from Media and handle disposal differently than in C#? I see several advantages:

• Avoids risk of multiple disposal, and makes it clear there is a breaking change
• Users don't have to wonder if they need to Dispose() it or not
• MediaPlayer becomes in charge of managing the memory properly to avoid memory leaks.

Drawbacks : How to reuse a Media between two Play() ? If it is released as soon as the player starts another media, we would end up parsing the media over and over again...

Yes.. I see what you mean :-) My problem with this design is that it creates a layer of abstraction over LibVLC and I'd like to avoid that because it is bound to grow with time, and become problematic as the underlying LibVLC API changes.

This awkward libvlcsharp API won't be fixed for 3.x, because as you noted it's a subtle breaking change which has a high risk of breaking user-code. However, we can definitly fix it for 4.x and document it as a breaking change.

I don't understand yet why LibVLC must increment the ref count when calling get_media on a media that already has a refcount > 1. Maybe changing LibVLC is an option?

libvlc_media_player_new_from_media also increments the media ref count, as well as libvlc_media_player_set_media...

I don't think so. If you don't count the usages, libvlc won't be able to know that the media is still kept for later use and that it is not expected to be destroyed

So I tried to call media_release repeatedly and it seems that libvlc is fine with it... no crash.

2 solutions that I have so far. Aiming for something simple...

1. what's currently in this branch. Proxy the media by keeping a single ref in the binding to prevent from incrementing the native ref count. Advantage: Can use that property getter (which is really convenient) as much as we want, like a normal C# property. Disadvantage: ???
2. rename the property from mediaplayer.Media to mediaplayer.NewMedia (or make it a method), and document that users need to dispose that. Advantage: Doesn't circumvent the native API, uses the native ref count as intended. Disadvantage: People won't read the doc and dispose it, so it will cause leak. That's for sure.

Still looking for other solutions. No special hurry as this is v4 anyway. If solution 1 doesn't break usercode, we might actually fix this for v3

1. Disadvantage : You can't reuse Media for A/B loops
2. I'd be in favor of making it a method. Renaming the property doesn't really add value.
3. We could add IDisposable annotations (see my proposal here ) and recommend using an analyzer like IDisposableAnalzyer that would spot such issues, but work is needed here

This seems to work quite well so far. Tried to stress the implementation a bit with various tests on the netcore sample. Still not fully convinced this is the way to go though

Only issue I noticed, unrelated to this PR, is that now that the Media is properly getting disposed since last release, the debugger crashes when trying to inspect media object that have been disposed. We maybe should generalize these checks https://github.com/videolan/libvlcsharp/pull/205/commits/56b69d9374300997c4edda48808e17a87b1cf307 for the whole interop calls.

Added a failing test.

While this approach may seem like a good idea, for now there is a still a few undiscovered gotchas. And the retain/dispose routine is different if you create a mediaplayer from a media or if you set a media on a mediaplayer instance...

There needs to be a solution for this problem for libvlcsharp 4.

Gotta merge this one soon.