Renderer API Wrapper

[anlumo]

I added a few APIs and fixed the memory management. Now it's actually working with OpenGL for me!

It is based on #20.

[8176135]

The events example no longer builds because of the removal of create_event_context

[8176135]

If you can also provide an example using the new render api wrapper, that would be very helpful for a newcomer like me :D.

[sztomi]

I think most commits can be squashed into one (the fixups by definition of course), but probably the following ones as well (unless each commit is meaningful on its own). I'd put a potential example into a separate commit maybe.

[anlumo]

The events example no longer builds because of the removal of create_event_context

I just looked into it a bit, and I'm not sure how this example is even supposed to work. There are two separate threads that reference the same mpv object, but I don't see who is supposed to own it.

I think this happened due to the lifetime changes, making this example really hard to get to work in the way it's supposed to.

[anlumo]

If you can also provide an example using the new render api wrapper, that would be very helpful for a newcomer like me :D.

Well, it was way more work than I anticipated, but I added a working example.

The biggest problem was that I had to combine two event loops, mpv's and glutin's (which just wraps winit). In order to avoid idle polling (which isn't an issue for my own project, since I'm doing stuff during that time anyways), I had to wrap two more mpv APIs, the render update callback and the wakeup callback.

However, the example is now quite clean, it only polls events and redraws the window when actually necessary.

I also had to fix another bug in the process, the EndFile event wasn't forwarded to the example if the file was played successfully, since is_positive() returns false for 0.

[anlumo]

I think most commits can be squashed into one (the fixups by definition of course), but probably the following ones as well (unless each commit is meaningful on its own). I'd put a potential example into a separate commit maybe.

I've done a squash where it makes sense. I'm a fan of having smaller commits, I hope it's ok this way.

[anlumo]

One remaining issue is that the renderer context has to be dropped before the mpv player itself. Currently this cannot be verified by the compiler, so some restructuring might be needed (such as the mpv player owning the renderer).

[ParadoxSpiral]

Hi,
first of all thanks for you looking into this!

Regarding

I just looked into it a bit, and I'm not sure how this example is even supposed to work. There are two separate threads that reference the same mpv object, but I don't see who is supposed to own it.

and

One remaining issue is that the renderer context has to be dropped before the mpv player itself. Currently this cannot be verified by the compiler, so some restructuring might be needed (such as the mpv player owning the renderer).

You restructued the EventContext to not live separately from Mpv. The EventContext having a disjoint lifetime was my idea to get around that. Because it has a reference to the Mpv struct, it has to be dropped first. The Mpv struct can be safely shared between threads because all the libmpv-sys methods it calls are thread safe. The wait_event method however is not thread-safe, so it must only be called from a single thread, this is enforced by only being able to create a single EventContext and the wait_event method taking &mut self. I imagined that the RenderContext would be created similarly, with a _does_not_outlive phantom reference to Mpv to be dropped first since this worked quite well. The only problem is that you cannot store Mpv and EventContext in the same struct (#21 ) which I'm not sure how to fix.

[anlumo]

It's been a hot minute since I changed that, so I don't remember exactly why I did it, but I think it was because it was impossible to store the EventContext in a struct along with the Mpv instance, because it's self-referential. This works fine for example projects where everything is held in local variables, but not if you want to store them in structs.

[ParadoxSpiral]

With these changes (event_context_mut) if one uses events either Mpv must be stored in a Mutex or only used in a single thread. If storing the structs together is important, then we could forego making Mpv + Events usable in parallel alltogether and just pass responsibility to the caller.

The main reason I had in mind when designing EventContext was to be able to process Events in a loop and still be able to use Mpv from other threads without using a Mutex (you would have to busy poll an infinite timeout would block other Mpv call sites). Maybe I was overthinking that.

Alternatively we could create a method that, when notified of new events, calls wait_event internally until no new events exist, and then passes a Vec<Event> to a user provided function. You sort of did this with set_wakeup_callback, but the way that is implemented is unsound because the user provided function could call Mpv from within the callback method which is not allowed (https://github.com/mpv-player/mpv/blob/master/libmpv/client.h#L1878).

[anlumo]

Alternatively we could create a method that, when notified of new events, calls wait_event internally until no new events exist, and then passes a Vec<Event> to a user provided function.

No you can't, because this is running on an internal thread where you're not allowed to do much. This is why I had to resort to user events in the example, because even calling request_redraw in the update callback would lead to panics (because Display uses Rc internally, which doesn't cope well with multiple threads).

Now that I write this, I realize that the callback closures have to be Send. I've pushed a commit for that. The Rust compiler didn't catch it, since they're executed from an unsafe function.

Anyways, since the crate doesn't know anything about the application setup, it can't message the main thread about calling wait_event either.

You sort of did this with set_wakeup_callback, but the way that is implemented is unsound because the user provided function could call Mpv from within the callback method which is not allowed

In order to do this, you'd have to move the instance into the closure, which would make it impossible to use it elsewhere (like the actual drawing call). This restriction might be tricked by wrapping it in an Arc though.

Unfortunately, handling multithreaded C APIs is always a mess in Rust, because C APIs tend to be pretty free-form in what you're allowed to do. There might not be a perfect way to explain to the Rust compiler what's allowed in the wakeup closure and what's not possible.

What would be possible is to set a thread-local variable to true while executing the callbacks, and then check in every Mpv function whether that flag is true and panic if it's the case. However, accessing thread-local data adds a lot of overhead to calls.

[anlumo]

Hmm maybe I should actually try that thing with Mpv within the wakeup callback. Since set_wakeup_callback takes &mut self, you can't actually use Arc<Mpv> to work around this.

[MoAlyousef]

Hi
Thanks for libmpv-rs and this render_gl api, it's compact and nice. I was able to get the fork to work with fltk-rs (based on the glium example):

/*
[dependencies]
fltk = {version = "1.1", features=["enable-glwindow", "no-pango"]}
libmpv = { git = "https://github.com/anlumo/libmpv-rs" }
*/
use fltk::{enums::Mode, prelude::*, *};
use libmpv::{
    render::{OpenGLInitParams, RenderContext, RenderParam, RenderParamApiType},
    FileState, Mpv,
};
use std::os::raw::c_void;

pub fn get_proc_address(win: &window::GlutWindow, name: &str) -> *mut c_void {
    win.get_proc_address(name) as _
}

fn main() {
    let args: Vec<String> = std::env::args().collect();
    if args.len() < 2 {
        println!("Usage: mpv <video file>");
        std::process::exit(1);
    }
    let a = app::App::default().with_scheme(app::Scheme::Gleam);
    app::get_system_colors();
    let mut win = window::Window::default().with_size(800, 600);
    let mut mpv_win = window::GlutWindow::new(5, 5, 790, 530, None);
    mpv_win.set_mode(Mode::Opengl3);
    let mut btn = button::Button::new(360, 545, 80, 40, "@||");
    win.end();
    win.make_resizable(true);
    win.show();
    mpv_win.make_current();

    let mut mpv = Mpv::new().expect("Error while creating MPV");
    let render_context = RenderContext::new(
        unsafe { mpv.ctx.as_mut() },
        vec![
            RenderParam::ApiType(RenderParamApiType::OpenGl),
            RenderParam::InitParams(OpenGLInitParams {
                get_proc_address,
                ctx: mpv_win.clone(),
            }),
        ],
    )
    .expect("Failed creating render context");
    mpv.event_context_mut().disable_deprecated_events().unwrap();
    mpv.playlist_load_files(&[(&args[1], FileState::AppendPlay, None)])
        .unwrap();

    btn.set_callback(move |b| {
        let prop: bool = mpv.get_property("pause").unwrap();
        mpv.set_property("pause", !prop).unwrap();
        if prop {
            b.set_label("@||");
        } else {
            b.set_label("@>");
        }
    });

    mpv_win.draw(move |w| {
        render_context
            .render::<window::GlutWindow>(0, w.w() as _, w.h() as _, true)
            .expect("Failed to draw on GlutWindow");
        w.swap_buffers();
    });

    app::add_idle(move || {
        mpv_win.redraw();
    });
    
    a.run().unwrap();
}

[anlumo]

Nice! Note that you're probably spinning in a tight loop redrawing the same image over and over again in your draw closure, until there's a new one available from the video.

[voidentente]

How is this coming along? I have found that the fork consistently throws segfaults when used with OpenGl when creating RenderContext on Windows 10 using mpv-dev-x86_64-1.109. I have tried out the software renderer - which unsurprisingly works - but that's hardly an alternative for resolutions larger than HD

[voidentente]

Btw, debugging tells me that that segfault happens in gpa_wrapper when calling params.get_proc_address, whose pointer is 0xabababababababab.

When RenderContext::new is called, the structure is still valid.
params[1].0.get_proc_address is 48 81 ec 88 00 00 00 4c and the other field is ctx: context, gl_window, and last_framebuffer_dimensions.

When the params arrive at gpa_wrapper, the layout seems invalid.
params.get_proc_address points at 0xabababababababab, while params.ctx contains
the valid get_proc_address and another ctx, that finally points at context, gl_window,
and last_framebuffer_dimensions. It seems something happens double here...

[voidentente]

See:

impl<C> From<OpenGLInitParams<C>> for mpv_opengl_init_params {
    fn from(val: OpenGLInitParams<C>) -> Self {
        Self {
	    // vvv Callsite vvv
            get_proc_address: Some(gpa_wrapper::<OpenGLInitParams<C>>),
            get_proc_address_ctx: Box::into_raw(Box::new(val)) as *mut c_void,
            extra_exts: ptr::null(),
        }
    }
}

unsafe extern "C" fn gpa_wrapper<GLContext>(ctx: *mut c_void, name: *const i8) -> *mut c_void {
    if ctx.is_null() {
        panic!("ctx for get_proc_address wrapper is NULL");
    }

    // vvv This resolves to *mut OpenGLInitParams<OpenGLInitParams<C>> vvv
    let params: *mut OpenGLInitParams<GLContext> = ctx as _;
    let params = &*params;
    (params.get_proc_address)(
        &params.ctx,
        CStr::from_ptr(name)
            .to_str()
            .expect("Could not convert function name to str"),
    )
}

[voidentente]

Fixed version:

impl<C> From<OpenGLInitParams<C>> for mpv_opengl_init_params {
    fn from(val: OpenGLInitParams<C>) -> Self {
        Self {
            get_proc_address: Some(gpa_wrapper::<C>),
            get_proc_address_ctx: Box::into_raw(Box::new(val)) as *mut c_void,
            extra_exts: ptr::null(),
        }
    }
}

unsafe extern "C" fn gpa_wrapper<C>(ctx: *mut c_void, name: *const i8) -> *mut c_void {
    if ctx.is_null() {
        panic!("ctx for get_proc_address wrapper is NULL");
    }

    let params: *mut OpenGLInitParams<C> = ctx as _;
    let params = &*params;

    (params.get_proc_address)(
        &params.ctx,
        CStr::from_ptr(name)
            .to_str()
            .expect("Could not convert function name to str"),
    )
}