Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Render first video frame when preroll is available #419

Merged
merged 2 commits into from
Apr 4, 2024
Merged

Render first video frame when preroll is available #419

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bc21c3d
Render first video frame when preroll is available
eerii Apr 3, 2024
b8925a5
Add comment explaining closure
eerii Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 26 additions & 14 deletions backends/gstreamer/player.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,27 +618,39 @@ impl GStreamerPlayer {
}
});

self.video_renderer.clone().map(|video_renderer| {
let render = self.render.clone();
let observer = self.observer.clone();
if let Some(video_renderer) = self.video_renderer.clone() {
// Creates a closure that renders a frame using the video_renderer
// Used in the preroll and sample callbacks
let render_sample = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let render = self.render.clone();
let observer = self.observer.clone();
move |sample: gst::Sample| {
let frame = render
.lock()
.unwrap()
.get_frame_from_sample(sample)
.map_err(|_| gst::FlowError::Error)?;
video_renderer.lock().unwrap().render(frame);
notify!(observer, PlayerEvent::VideoFrameUpdated);
Ok(gst::FlowSuccess::Ok)
}
};

// Set video_sink callbacks.
inner.lock().unwrap().video_sink.set_callbacks(
gst_app::AppSinkCallbacks::builder()
.new_preroll(|_| Ok(gst::FlowSuccess::Ok))
.new_preroll({
let render_sample = render_sample.clone();
move |video_sink| {
render_sample(video_sink.pull_preroll().map_err(|_| gst::FlowError::Eos)?)
}
})
.new_sample(move |video_sink| {
let sample = video_sink.pull_sample().map_err(|_| gst::FlowError::Eos)?;
let frame = render
.lock()
.unwrap()
.get_frame_from_sample(sample)
.map_err(|_| gst::FlowError::Error)?;
video_renderer.lock().unwrap().render(frame);
notify!(observer, PlayerEvent::VideoFrameUpdated);
Ok(gst::FlowSuccess::Ok)
render_sample(video_sink.pull_sample().map_err(|_| gst::FlowError::Eos)?)
})
.build(),
);
});
};

let (receiver, error_handler_id) = {
let inner_clone = inner.clone();
Expand Down
Loading