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 735042 - videoaggregator: race of vagg->info on pad removal
videoaggregator: race of vagg->info on pad removal
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Linux
: Normal normal
: 1.5.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-08-19 10:26 UTC by Matthew Waters (ystreet00)
Modified: 2014-10-03 11:19 UTC
See Also:
GNOME target: ---
GNOME version: ---

Attachments
test application that shows the issue (5.47 KB, text/plain)
2014-08-19 10:34 UTC, Matthew Waters (ystreet00) 		  Details
Possible patch, WIP (6.75 KB, patch)
2014-09-18 15:38 UTC, Thibault Saunier 		none Details | Review
aggregator: Take lock to ensure set_caps is not called concurently (2.95 KB, patch)
2014-10-03 10:44 UTC, Thibault Saunier 		committed Details | Review
videoaggregator: Do not to release VIDEO_AGGREGATOR_LOCK while setting format info (7.36 KB, patch)
2014-10-03 10:44 UTC, Thibault Saunier 		committed Details | Review

Description Matthew Waters (ystreet00) 2014-08-19 10:26:54 UTC 
gst_videoaggregator_release_pad() calls update_converters() with the videoaggregator lock but update_src_caps without that lock.  In that time, the aggregate() function (from aggregator) could be called.

How it may manifest is with the following critical and backtrace:

GStreamer-CRITICAL **: _gst_util_uint64_scale: assertion 'denom != 0' failed

(gdb) bt

+ Trace 233972

  • #0 g_logv
    at gmessages.c line 1038
  • #1 g_log
    at gmessages.c line 1071
  • #2 g_return_if_fail_warning
    at gmessages.c line 1080
  • #3 _gst_util_uint64_scale
    at gstutils.c line 479
  • #4 gst_videoaggregator_aggregate
    at gstvideoaggregator.c line 1198
  • #5 aggregate_func
    at gstaggregator.c line 423
  • #6 g_main_dispatch
    at gmain.c line 3064
  • #7 g_main_context_dispatch
    at gmain.c line 3663
  • #8 g_main_context_iterate
    at gmain.c line 3734
  • #9 g_main_context_iteration
    at gmain.c line 3795
  • #10 gst_task_func
    at gsttask.c line 317
  • #11 g_thread_pool_thread_proxy
    at gthreadpool.c line 307
  • #12 g_thread_proxy
    at gthread.c line 764
  • #13 start_thread
    from /usr/lib/libpthread.so.0
  • #14 clone
    from /usr/lib/libc.so.6 

update_converters() does not deal with the framerate and if the aggregate function is called in between update_converters() and update_src_caps(), there could be a 0/1 framerate in vagg->info which fails with the above backtrace.
Comment 1 Matthew Waters (ystreet00) 2014-08-19 10:34:08 UTC 
Created attachment 283868 [details]
test application that shows the issue

Based off test from bug #734830
Comment 2 Thibault Saunier 2014-09-18 15:38:24 UTC 
Created attachment 286516 [details] [review]
Possible patch, WIP

I understand the theorical race but can't reproduce here with you test application. 

We might want to apply something like what I attach here, but I would need to think better about that and do more testing.
Comment 3 Sebastian Dröge (slomo) 2014-09-24 07:30:53 UTC 
Isn't this now setting the caps on the srcpad while holding the lock? That could lead to deadlocks
Comment 4 Thibault Saunier 2014-10-03 10:44:21 UTC 
Created attachment 287659 [details] [review]
aggregator: Take lock to ensure set_caps is not called concurently

Avoiding to be in an inconsistent state where we do not have
actual negotiate caps set as srccaps and leading to point where we
try to unref ->srccaps when they have already been set to NULL.
Comment 5 Thibault Saunier 2014-10-03 10:44:31 UTC 
Created attachment 287660 [details] [review]
videoaggregator: Do not to release VIDEO_AGGREGATOR_LOCK while setting format info

We should be able to always keep the VIDEO_AGGREGATOR_LOCK while
negotiating caps, this patch introduce that change.

That also implies that we do not need the SETCAPS_LOCK anymore because
now VIDEO_AGGREGATOR_LOCK guarantees that setcaps is not called from
several threads and the gst_aggregator_set_caps method is now
protected.
Comment 6 Thibault Saunier 2014-10-03 11:18:33 UTC 
Attachment 287659 [details] pushed as 1b4547f - aggregator: Take lock to ensure set_caps is not called concurently
Attachment 287660 [details] pushed as 3637296 - videoaggregator: Do not to release VIDEO_AGGREGATOR_LOCK while setting format info