jazzfool / iced_video_player

Video player component for Iced
Apache License 2.0
119 stars 17 forks source link

Memory leak when not dropping Video state manually #14

Open cyl3x opened 1 day ago

cyl3x commented 1 day ago

I use this player together with some buttons to switch between different rtsp streams. Currently my implementation looks more or less like this. If I don't drop the video state manually, I end up with ~100mb of stale memory, although I assumed it would be dropped as soon as I replaced it with the new video. Is this an incorrect assumption or am I replacing the video incorrectly?

Code ```rust use std::time::Duration; use iced::widget::{button, row, text, Column}; use iced::{Length, Padding}; use iced_video_player::VideoPlayer; use url::Url; mod config { #[derive(Clone, Debug)] pub struct Video { pub name: String, pub url: Url, } } pub struct Video { videos: Vec, video: Option, } #[derive(Debug, Clone)] pub enum Message { SetVideo(usize), } impl Video { pub fn new(videos: Vec) -> Self { let mut video = Self { videos, video: None, }; if !video.videos.is_empty() { video.update(Message::SetVideo(0)); } video } pub fn view(&self) -> iced::Element { if self.videos.is_empty() { return text("No videos").into(); } let mut column = Column::new(); if let Some(video) = &self.video { column = column.push(VideoPlayer::new(video)); } let buttons = self.videos.iter().enumerate().map(|(idx, video)| { button(text(&video.name).center()) .on_press(Message::SetVideo(idx)) .width(Length::Fill) .height(36) .into() }); let button_row = row(buttons) .spacing(16) .padding(Padding::ZERO.right(16.0)) .width(Length::Fill) .height(Length::Shrink); column.push(button_row).spacing(16).into() } pub fn update(&mut self, message: Message) { self.is_error = false; match message { Message::SetVideo(idx) => { let pipeline = iced_video_player::Video::from_pipeline( format!("uridecodebin uri={} ! videoconvert ! videoscale ! videorate ! appsink name=iced_video caps=video/x-raw,format=RGBA,pixel-aspect-ratio=1/1", &self.videos[idx].url), Some(true), ); // Manually dropping if let Some(video) = std::mem::replace(&mut self.video, None) { std::mem::drop(video); } self.video = match pipeline { Ok(video) => Some(video), Err(err) => None, }; } } } } ```
jazzfool commented 1 day ago

I did find a leak relating to GPU textures/buffers and have fixed it on master (cd978e3) - whether this accounts for as much as 100MB of stale memory however really depends on the video resolution.

Besides that, testing your code, after switching videos it seems GStreamer takes around 5-10s to clean up its memory. Dropping the video manually vs reassigning should not make a difference (and I couldn't observe any).

If your leak is still occurring then it may be a GStreamer issue. Perhaps it could be the buffering in uridecodebin if you are streaming video?

cyl3x commented 1 day ago

I am having some trouble testing. With c44141c the from_pipeline method was removed and therefore I cannot easily build a pipeline with uridecodebin. I would like to see this method back because to build my own pipeline now I would have to have gst as a separate dependency (as iced_video_player doesn't expose gst) and rebuild the app_sink extraction of the Video::new or of the old Video::from_pipeline method.

I tried to simply play with playbin, but in the following panic with my cam rtsp streams (but the test stream from #13 runs fine):

thread '<unnamed>' panicked at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/time.rs:957:23:
cannot convert float seconds to Duration: value is either too big or NaN
stack backtrace:
   0: rust_begin_unwind
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:74:14
   2: core::panicking::panic_display
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:264:5
   3: core::time::Duration::from_secs_f64::panic_cold_display
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panic.rs:101:13
   4: core::time::Duration::from_secs_f64
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/time.rs:957:23
   5: iced_video_player::video::Video::from_gst_pipeline::{{closure}}
             at /home/cyl3x/.cargo/git/checkouts/iced_video_player-b7cdbf0a371f9213/cd978e3/src/video.rs:248:38

FYI, my try to write me a from_pipeline function:

fn from_pipeline(uri: &url::Url) -> Result<iced_video_player::Video, iced_video_player::Error> {
    gst::init()?;

    let pipeline = format!("uridecodebin uri={} ! videoconvert ! videoscale ! videorate ! appsink name=iced_video caps=video/x-raw,format=NV12,pixel-aspect-ratio=1/1", uri.as_str());
    let pipeline = gst::parse::launch(pipeline.as_ref())?
        .downcast::<gst::Pipeline>()
        .map_err(|_| iced_video_player::Error::Cast)?;

    let app_sink = pipeline
        .by_name("iced_video")
        .and_then(|elem| elem.downcast::<gst_app::AppSink>().ok())
        .ok_or(iced_video_player::Error::AppSink("iced_video".to_string()))?;

    iced_video_player::Video::from_gst_pipeline(pipeline, app_sink)
}

resulted in:

thread 'main' panicked at /home/cyl3x/.cargo/registry/src/index.crates.io-6f17d22bba15001f/glib-0.20.4/src/object.rs:2310:13:
property 'av-offset' of type 'GstPipeline' not found
stack backtrace:
   0: rust_begin_unwind
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/std/src/panicking.rs:665:5
   1: core::panicking::panic_fmt
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/panicking.rs:74:14
   2: <T as glib::object::ObjectExt>::set_property::{{closure}}
             at /home/cyl3x/.cargo/registry/src/index.crates.io-6f17d22bba15001f/glib-0.20.4/src/object.rs:2310:13
   3: core::option::Option<T>::unwrap_or_else
             at /rustc/eeb90cda1969383f56a2637cbd3037bdf598841c/library/core/src/option.rs:1015:21
   4: <T as glib::object::ObjectExt>::set_property
             at /home/cyl3x/.cargo/registry/src/index.crates.io-6f17d22bba15001f/glib-0.20.4/src/object.rs:2309:21
   5: iced_video_player::video::Internal::set_av_offset
             at /home/cyl3x/.cargo/git/checkouts/iced_video_player-b7cdbf0a371f9213/cd978e3/src/video.rs:144:9
   6: ...

so maybe you have a clue or I'll try again when I have time

jazzfool commented 11 hours ago

With b4fceaf, your implementation of from_pipeline should now work. I would like to bring back Video::from_pipeline but there's really no good way to do it. It's not as simple as searching for iced_video, since although it works in your case, it would not work, e.g., with playbin, since you must first find the video-sink pad, then search by_name.

Also regarding your RTSP stream not working with playbin - that is a strange error. It results from GStreamer reporting an invalid framerate. 82dea04 no longer does framerate calculations anyway, but perhaps you could try ffprobe or request DESCRIBE to see what framerate your RTSP stream is reporting?