Skip to content

Conversation

@tharkum
Copy link
Contributor

@tharkum tharkum commented Oct 29, 2025

The associated signal callbacks shouldn't capture strong or weak references of a owner GstPlay object with which they are interact otherwise it leads to leaked underlying media resources.

Note that upgrading the captured weak reference for a GstPlay object on another thread (the GstPlay streaming thread) is not safely because a GstPlay object could be destroyed on that thread after signal callback handling with mutex panic on the gst_play_finalize.

To solve the GstPlay ownership issue between client and Gstreamer was introduced the player shared state with minimal required states (metadata, pending playback rate, play state, ...).

Anyway the captured GstPlay weak reference will be used on sync source setup stage which is locking the client thread until a source will be ready.

For GstAppSrc and associated seek_data callback a capture of a weak reference will be used with explicit set empty callbacks on a player destruction.

Testing: Manually testing with the following WPT test
GST_DEBUG=3,gst-play*:7 ./mach test-wpt html/semantics/embedded-content/the-video-element/intrinsic_sizes.htm -r

Fixes (partially): #455

@tharkum
Copy link
Contributor Author

tharkum commented Oct 29, 2025

@jdm < Please take a look

@tharkum
Copy link
Contributor Author

tharkum commented Oct 29, 2025

There is crash under heavy testing scenario, so please postpone review

 0:30.53 pid:2649784    0: servoshell::backtrace::print
 0:30.53 pid:2649784    1: servoshell::crash_handler::install::handler
 0:30.57 pid:2649784    2: <unknown>
 0:30.57 pid:2649784    3: mozalloc_abort
 0:30.57 pid:2649784    4: abort
 0:30.57 pid:2649784    5: g_mutex_clear
 0:30.57 pid:2649784    6: <unknown>
 0:30.58 pid:2649784    7: g_object_unref
 0:30.58 pid:2649784    8: g_object_get
 0:30.58 pid:2649784    9: gst_play_get_position
 0:30.58 pid:2649784   10: <unknown>
 0:30.58 pid:2649784   11: g_cclosure_marshal_VOID__BOXEDv
 0:30.58 pid:2649784   12: g_signal_emit_valist
 0:30.58 pid:2649784   13: g_signal_emit
 0:30.58 pid:2649784   14: gst_bus_async_signal_func
 0:30.58 pid:2649784   15: <unknown>
 0:30.58 pid:2649784   16: g_main_context_dispatch
 0:30.58 pid:2649784   17: <unknown>
 0:30.58 pid:2649784   18: g_main_loop_run
 0:30.58 pid:2649784   19: <unknown>
 0:30.58 pid:2649784   20: <unknown>
 0:30.62 pid:2649784   21: start_thread
 0:30.62 pid:2649784              at ./nptl/pthread_create.c:442:8
 0:30.62 pid:2649784   22: __GI___clone3
 0:30.62 pid:2649784              at ./misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81:0

@sdroege
Copy link
Contributor

sdroege commented Oct 30, 2025

PlayerInner -> PlaySignalAdapter callbacks (strong -> weak refs)

Instead of going this way, you might want to use the gst::Bus on the Player instance directly instead of using the signals via callbacks. That makes it (in my experience) simpler to decouple ownership of values and avoid circular references.

…acks

The associated signal callbacks shouldn't capture strong or weak
references of a owner `GstPlay` object with which they are interact
otherwise it leads to leaked underlying media resources.

Note that upgrading the captured weak reference for a `GstPlay` object
on another thread (the `GstPlay` streaming thread) is not safely because
a `GstPlay` object could be destroyed on that thread after signal callback
handling with mutex panic on the `gst_play_finalize`.

To solve the `GstPlay` ownership issue between client and Gstreamer was
introduced the player shared state with minimal required states
(`metadata`, `pending playback rate`, `play state`, ...).

Anyway the captured `GstPlay` weak reference will be used on sync `source`
setup stage which is locking the client thread until a source will be ready.

For `GstAppSrc` and associated `seek_data` callback a capture of a weak reference
will be used with explicit set `empty` callbacks on a player destruction.

Fixes: servo#455

Signed-off-by: Andrei Volykhin <andrei.volykhin@gmail.com>
@tharkum tharkum force-pushed the gstreamer-break-cyclic-reference-dependency branch from cb36475 to 32098e8 Compare December 15, 2025 12:46
@tharkum tharkum changed the title gstreamer: Break cyclic reference dependency for objects and callbacks gstreamer: Decouple ownership of the GstPlay/AppSrc from signal callbacks Dec 15, 2025
@tharkum tharkum requested a review from sdroege December 15, 2025 12:50
@tharkum
Copy link
Contributor Author

tharkum commented Dec 16, 2025

@jdm < Please take a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants