gstreamer: Break cyclic reference dependency for objects and callbacks #456
Conversation
The associated callbacks should capture weak references of required objects with which they are will interact otherwise it leads to leaked underlying gst resources on high level owner (Player) destruction due to the cyclic reference dependency (between PlayerInner, ServoSrc and the associated callbacks). PlayerInner -> PlaySignalAdapter callbacks (strong -> weak refs) ServoSrc -> ServoSrc callbacks (strong -> weak refs) Testing: Manually testing with the following WPT test ./mach test-wpt html/semantics/embedded-content/the-video-element/intrinsic_sizes.htm -r with enabled GST debugging (GST_DEBUG=3,gst-play*:7) Fixes: servo#455 Signed-off-by: Andrei Volykhin <[email protected]>
|
@jdm < Please take a look |
|
There is crash under heavy testing scenario, so please postpone review |
Instead of going this way, you might want to use the |
| signal_adapter.connect_media_info_updated({ | ||
| let observer = self.observer.clone(); | ||
| let weak_inner = weak_inner.clone(); | ||
|
|
||
| move |_, info| { | ||
| let Ok(metadata) = metadata_from_media_info(info) else { | ||
| return; | ||
| }; | ||
|
|
||
| let Some(strong_inner) = weak_inner.upgrade() else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
| signal_adapter.connect_media_info_updated({ | |
| let observer = self.observer.clone(); | |
| let weak_inner = weak_inner.clone(); | |
| move |_, info| { | |
| let Ok(metadata) = metadata_from_media_info(info) else { | |
| return; | |
| }; | |
| let Some(strong_inner) = weak_inner.upgrade() else { | |
| return; | |
| }; | |
| signal_adapter.connect_media_info_updated(glib::clone!( | |
| #[strong(rename_to = observer] | |
| self.observer, | |
| #[weak] | |
| inner, | |
| move |_, info| { | |
| let Ok(metadata) = metadata_from_media_info(info) else { | |
| return; | |
| }; |
would have the same effect btw, if you want to avoid a bit of boilerplate
There was a problem hiding this comment.
Wow. Thanks. I will look into glib:clone macr, but it also looks like a boilerplate...
There was a problem hiding this comment.
Well, it exists to reduce this kind of boilerplate :)
There was a problem hiding this comment.
@sdroege < If I am understanding correctly glib::clone macro will upgrade weak to strong at the beginning of the passing closure execution (no way to delay this right before variable usage)?
https://docs.rs/glib-macros/0.21.0/src/glib_macros/clone.rs.html#544
There was a problem hiding this comment.
Correct. Do you need to do it later?
There was a problem hiding this comment.
There are a risk that closure will be the latest owner of the object and this object will be destroyed on the wrong thread (like it happens in #456 (comment) but with higher chance of failure).
Anyway I will try to fix mentioned crash and return back to this ....
There was a problem hiding this comment.
There should be no "wrong thread" though. That seems like a deeper problem that should be fixed.
| inner.player.pipeline().connect("source-setup", false, { | ||
| let is_ready = self.is_ready.clone(); | ||
| let observer = self.observer.clone(); | ||
| let sender = sender.clone(); | ||
| let weak_inner = weak_inner.clone(); | ||
|
|
||
| move |args| { | ||
| let source = args[1].get::<gst::Element>().unwrap(); | ||
|
|
||
| let Some(strong_inner) = weak_inner.upgrade() else { | ||
| return None; | ||
| }; | ||
|
|
||
| let mut inner = strong_inner.lock().unwrap(); |
There was a problem hiding this comment.
Also here:
| inner.player.pipeline().connect("source-setup", false, { | |
| let is_ready = self.is_ready.clone(); | |
| let observer = self.observer.clone(); | |
| let sender = sender.clone(); | |
| let weak_inner = weak_inner.clone(); | |
| move |args| { | |
| let source = args[1].get::<gst::Element>().unwrap(); | |
| let Some(strong_inner) = weak_inner.upgrade() else { | |
| return None; | |
| }; | |
| let mut inner = strong_inner.lock().unwrap(); | |
| inner.player.pipeline().connect_closure("source-setup", false, glib::closure!( | |
| #[strong(rename_to = is_ready)] | |
| self.is_ready, | |
| #[strong(rename_to = observer)] | |
| self.observer, | |
| #[strong] | |
| sender, | |
| #[weak] | |
| inner, | |
| move |_pipeline: &gst::Element, source: &gst::Element| { | |
| let mut inner = inner.lock().unwrap(); |
There was a problem hiding this comment.
(in addition to what glib::clone! can do for closures/async blocks, glib::closure! also unpacks the arguments from a &[glib::Value] to the actual arguments for you)
2 participants
The associated callbacks should capture weak references of required objects with which they are will interact otherwise it leads to leaked underlying
gstresources on high level owner (Player) destruction due to the cyclic reference dependency (betweenPlayerInner,ServoSrcand the associated callbacks).PlayerInner->PlaySignalAdaptercallbacks (strong -> weak refs)ServoSrc->ServoSrccallbacks (strong -> weak refs)Testing: Manually testing with the following WPT test ./mach test-wpt html/semantics/embedded-content/the-video-element/intrinsic_sizes.htm -r with enabled GST debugging (GST_DEBUG=3,gst-play*:7)
Fixes: #455