Render first video frame when preroll is available

[eerii]

In order to display the first frame of a video that doesn't autoplay we can use preroll. This guarantees that when the video is in the state PAUSED it has at least one buffer ready, which we can send to the video renderer.

The closure from the new_sample callback has been extracted since the logic for rendering video samples and preroll samples is the same.

Fixes servo/servo#31666. A detailed log of the reasoning behind this change and an alternative implementation (which required a new api function so this one was favoured) can be found on that issue.

[mrobinson]

@ceyusa Would you be willing to take a quick look at this one? Thanks!

[ceyusa]

overall it looks good to me....

I wonder how many variable clousures are there in the servo-media crate, but I don't remember many, and I'm worry it could make code difficult to follow. Do you think a "normal" method/function could make sense here?

And yeah, when the video has a poster, it's problem, but imo it's not a servo-media issue, but a servo one to choose what to show. So yeah, this behavior looks the correct one.

[mrobinson]

FWIW, we use this pattern a lot in Servo, but it does sometimes come with downsides such as stack size in debug mode. This mainly happens in layout though which is a deeply nested processing of the DOM tree.

[eerii]

I used the closure to try to make the less changes possible, but I can see a normal method being clearer, I can absolutely change it.

[eerii]

I am trying to separate the code in a method but I am encountering an issue with threads. This is the new code:

if let Some(video_renderer) = self.video_renderer.clone() {
    // Set video_sink callbacks.
    inner.lock().unwrap().video_sink.set_callbacks(
        gst_app::AppSinkCallbacks::builder()
            .new_preroll(|video_sink| {
                self.render_sample(
                    video_sink.pull_preroll().map_err(|_| gst::FlowError::Eos)?,
                )
            })
            .new_sample(|video_sink| {
                self.render_sample(
                    video_sink.pull_sample().map_err(|_| gst::FlowError::Eos)?,
                )
            })
            .build(),
    );
};

// Inside impl GStreamerPlayer

fn render_sample(&self, sample: gst::Sample) -> Result<gst::FlowSuccess, gst::FlowError> {
    let frame = self
        .render
        .lock()
        .unwrap()
        .get_frame_from_sample(sample)
        .map_err(|_| gst::FlowError::Error)?;
    self.video_renderer
        .clone()
        .ok_or(gst::FlowError::Error)?
        .lock()
        .unwrap()
        .render(frame);
    notify!(self.observer, PlayerEvent::VideoFrameUpdated);
    Ok(gst::FlowSuccess::Ok)
}

However, the compiler is complaining because the inner field of GStreamerPlayer does not implement Sync:

error[E0277]: `std::cell::RefCell<std::option::Option<std::sync::Arc<std::sync::Mutex<player::PlayerInner>>>>` cannot be shared between threads safely
   --> /home/eri/Code/web/media/backends/gstreamer/player.rs:630:34
    |
630 |                       .new_preroll(|video_sink| {
    |  ______________________-----------_^
    | |                      |
    | |                      required by a bound introduced by this call
631 | |                         render_sample(
632 | |                             Arc::new(Mutex::new(self)),
633 | |                             video_sink.pull_preroll().map_err(|_| gst::FlowError::Eos)?,
634 | |                         )
635 | |                     })
    | |_____________________^ `std::cell::RefCell<std::option::Option<std::sync::Arc<std::sync::Mutex<player::PlayerInner>>>>` cannot be shared between threads safely
    |
    = help: within `player::GStreamerPlayer`, the trait `std::marker::Sync` is not implemented for `std::cell::RefCell<std::option::Option<std::sync::Arc<std::sync::Mutex<player::PlayerInner>>>>`
    = note: if you want to do aliasing and mutation between multiple threads, use `std::sync::RwLock` instead
note: required because it appears within the type `GStreamerPlayer`
   --> /home/eri/Code/web/media/backends/gstreamer/player.rs:351:12
    |
351 | pub struct GStreamerPlayer {
    |            ^^^^^^^^^^^^^^^
    = note: required for `&player::GStreamerPlayer` to implement `std::marker::Send`
note: required because it's used within this closure
   --> /home/eri/Code/web/media/backends/gstreamer/player.rs:630:34
    |
630 |                     .new_preroll(|video_sink| {
    |                                  ^^^^^^^^^^^^
note: required by a bound in `gstreamer_app::app_sink::AppSinkCallbacksBuilder::new_preroll`
   --> /home/eri/.cargo/registry/src/index.crates.io-6f17d22bba15001f/gstreamer-app-0.22.0/src/app_sink.rs:75:74
    |
74  |     pub fn new_preroll<
    |            ----------- required by a bound in this associated function
75  |         F: FnMut(&AppSink) -> Result<gst::FlowSuccess, gst::FlowError> + Send + 'static,
    |                                                                          ^^^^ required by this bound in `AppSinkCallbacksBuilder::new_preroll`

I have tried multiple things, such as passing all the parameters to the render_sample instead of using self., making render_sample a function outside GStreamerPlayer instead of a method, cloning or moving references. The suggested change of using a RwLock means changing all of the code using inner, which seems excessive.

The original solution seems to work since all the calls to self. are done only once when the closure is defined, but I can't figure out how to do it on a separate function. I'm probably missing something, sorry.

[ceyusa]

Thanks a lot @eerii for your effort.

Given that the simplest solution right now is to keep the closure inside the thread. I guess a comment above the closure explaining it would be enough.

Other than that, lgtm

[eerii]

Thank you for the review! Is a comment like the one in the last commit right?