Skip to content
Snippets Groups Projects

Media stream callbacks refactoring

Closed Martin Finkel requested to merge mfkl/LibVLCSharp:fix-media-from-stream into master
1 unresolved thread

Fixes https://code.videolan.org/videolan/LibVLCSharp/issues/72 and more.

Question:

  • We currently store the Stream reference provided by the user provided. Should we CopyTo our own stream to be safe(r)?
  • We currently do not expose the stream provided by the user. Should we?

/cc @jeremyVignelles

Edited by Martin Finkel

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
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;
  • Author Maintainer

    Because the ctor is static so it needs to be a static ref. And because of iOS

  • Does that mean we can't play from two streams at the same time?

  • Author Maintainer

    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?

  • I might not be fully awoken given that it's 7:30 here, but doesn't static mean that it is shared between instances?

    When you add a new stream, CtorFromCallbacks is called. which sets those static fields, right?

  • Author Maintainer

    Yeah... you might be right. Checking now.

  • Author Maintainer

    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

  • Please register or sign in to reply
  • 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?

  • Author Maintainer

    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. The CallbackCloseMedia rewinds the stream so you can play it again. Calling Media.Dispose() disposes of the stream.

    Edited by Martin Finkel
  • The 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
  • Martin Finkel added 3 commits

    added 3 commits

    • 351bde92 - Only rewind stream if its seekable
    • 17345cae - Do not dispose of the user's stream when media gets disposed
    • bc9c3796 - Add comments

    Compare with previous version

  • Author Maintainer

    Fair enough

  • Martin Finkel added 1 commit

    added 1 commit

    Compare with previous version

  • Martin Finkel added 2 commits

    added 2 commits

    • af03da46 - Add dictionnary back
    • f6e1bbac - Add CreateMultipleMediaFromStreamPlay unit test

    Compare with previous version

  • Martin Finkel mentioned in merge request !5 (closed)

    mentioned in merge request !5 (closed)

  • Martin Finkel added 11 commits

    added 11 commits

    Compare with previous version

  • closed

  • Please register or sign in to reply
    Loading