Media stream callbacks refactoring
Fixes https://code.videolan.org/videolan/LibVLCSharp/issues/72 and more.
Question:
- We currently store the
Stream
reference provided by the user provided. Should weCopyTo
our own stream to be safe(r)? - We currently do not expose the stream provided by the user. Should we?
/cc @jeremyVignelles
Merge request reports
Activity
mentioned in issue #73 (closed)
343 338 /// <param name="options"></param> 344 339 public Media(LibVLC libVLC, Stream stream, params string[] options) 345 340 : base(() => CtorFromCallbacks(libVLC, stream), Native.LibVLCMediaRelease) 346 { 347 if(options.Any()) 341 { 342 if (options.Any()) 348 343 Native.LibVLCMediaAddOption(NativeReference, options.ToString()); 349 344 } 350 345 346 static OpenMedia _openMedia; Hmm I think that's unrelated to stuff being
static
no?From my understanding, media callbacks are per media items.
If you want to provide multiple streams, then you need to create one
Media
for each stream (as a media represents a stream). If you want to play multiple streams, you need multiple mediaplayers (1 media played per mediaplayer). Right?so...
On one hand the C# spec says
While an instance of a class contains a separate copy of all instance fields of the class, there is only one copy of each static field.
On the other hand... this tests works fine.
[Test] public async Task CreateMultipleMediaFromStreamPlay() { var libVLC1 = new LibVLC(new[] { "--no-audio" }); var libVLC2 = new LibVLC(new[] { "--no-sout-display" }); var mp1 = new MediaPlayer(libVLC1); var mp2 = new MediaPlayer(libVLC2); mp1.Play(new Media(libVLC1, GetStreamFromUrl("http://www.quirksmode.org/html5/videos/big_buck_bunny.mp4"))); mp2.Play(new Media(libVLC2, GetStreamFromUrl("https://streams.videolan.org/streams/mp3/05-Mr.%20Zebra.mp3"))); await Task.Delay(10000); }
Maybe because your OpenMedia instance has not yet been garbage collected. I wouldn't expect it to work on the long run.
Moreover, your StreamData is static too. you can check on each instance if you want, but once you set the second stream, the first instance will be affected. this is why there was a dictionary I guess.
changed this line in version 4 of the diff
Should we
CopyTo
?No, for perfs and because the stream might be an infinite forward only stream.
Should we expose the Stream given?
Maybe, that could be easier for users. Don't have strong feelings here.
If I remember correctly, the current recommendation is to allocate a new Media, pass it to the player and Dispose() it immediately, to match libvlc_media_release or something. How does that work with streams?
If I remember correctly, the current recommendation is to allocate a new Media, pass it to the player and Dispose() it immediately, to match libvlc_media_release or something. How does that work with streams?
The stream lives as long as the
Media
instance lives. TheCallbackCloseMedia
rewinds the stream so you can play it again. CallingMedia.Dispose()
disposes of the stream.Edited by Martin FinkelThe stream lives as long as the Media instance lives.
This means you need to use the Media in two different ways... Doesn't sound great.
The CallbackCloseMedia rewinds the stream so you can play it again.
Only if seekable
Calling Media.Dispose() disposes of the stream.
Not sure you should do that. You are given a stream instance, but it's not your responsibility to Dispose() the stream.
Edited by Jérémy VIGNELLES- Resolved by Martin Finkel
added 2 commits
mentioned in merge request !5 (closed)
added 11 commits
-
f6e1bbac...5c799152 - 11 commits from branch
videolan:master
-
f6e1bbac...5c799152 - 11 commits from branch