After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 761702 - rtsp-thread-pool: explain why GSource is a part of GstRTSPThreadImpl
rtsp-thread-pool: explain why GSource is a part of GstRTSPThreadImpl
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-rtsp-server
git master
Other Linux
: Normal enhancement
: 1.9.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-02-08 10:57 UTC by Patricia Muscalu
Modified: 2016-04-06 08:57 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
rtsp-thread-pool: explained why GSource is a part of GstRTSPThreadImpl (1.69 KB, patch)
2016-02-08 10:57 UTC, Patricia Muscalu 		none Details | Review
explained why GSource is a part of ThreadImpl (1.00 KB, patch)
2016-04-06 08:13 UTC, Patricia Muscalu 		committed Details | Review

Description Patricia Muscalu 2016-02-08 10:57:17 UTC 
Created attachment 320612 [details] [review]
rtsp-thread-pool: explained why GSource is a part of GstRTSPThreadImpl

Currently, there is no explanation why it is necessary to add source information to GstRTSPThreadImpl. The suggested patch adds a comment explaining the reason
of the source being a part of TheadImpl.
The patch adds as well error handling in _thread_stop().
Comment 1 Sebastian Dröge (slomo) 2016-02-16 16:51:53 UTC 
Review of attachment 320612 [details] [review]:

::: gst/rtsp-server/rtsp-thread-pool.c
@@ +180,3 @@
     g_source_set_callback (impl->source, (GSourceFunc) do_quit,
         thread, (GDestroyNotify) gst_rtsp_thread_unref);
+    if (G_UNLIKELY (g_source_attach (impl->source, thread->context) <= 0)) {

From the code of g_source_attach() it's actually impossible to get 0 here (and < 0 anyway as it's unsigned).
Comment 2 Tim-Philipp Müller 2016-04-04 17:02:37 UTC 
Patricia?
Comment 3 Patricia Muscalu 2016-04-05 06:23:23 UTC 
Sebastian, you're right. Only the FIXME comment should be included in the suggested patch.
Comment 4 Sebastian Dröge (slomo) 2016-04-05 11:12:29 UTC 
Can you update the patch?
Comment 5 Patricia Muscalu 2016-04-06 08:13:02 UTC 
Created attachment 325462 [details] [review]
explained why GSource is a part of ThreadImpl
Comment 6 Tim-Philipp Müller 2016-04-06 08:55:00 UTC 
Thanks, pushed:

commit f0891e2cdf32489d9a3393188e4782d9f8f91d09
Author: Patricia Muscalu <patricia@axis.com>
Date:   Wed Apr 6 10:09:46 2016 +0200

    rtsp-thread-pool: explained why GSource is a part of ThreadImpl
    
    Clarified why it is necessary to add source information to
    GstRTSPThreadImpl. See the reported bug in GLib:
    https://bugzilla.gnome.org/show_bug.cgi?id=720186
    for more information.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=761702