Bug 684658 – GstVideoDecoder locking: stuck on shutdown with mewmew.mkv and assrender

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 684658 - GstVideoDecoder locking: stuck on shutdown with mewmew.mkv and assrender


Summary:	GstVideoDecoder locking: stuck on shutdown with mewmew.mkv and assrender


Status:	RESOLVED FIXED

Product:	GStreamer
Classification:	Platform
Component:	gst-plugins-base
Version:	git master
Hardware:	Other Linux

Importance:	Normal major
Target Milestone:	0.11.x
Assigned To:	GStreamer Maintainers
QA Contact:	GStreamer Maintainers

URL:
Whiteboard:

Depends on:
Blocks:

Reported:	2012-09-23 12:59 UTC by Tim-Philipp Müller
Modified:	2012-09-25 23:00 UTC

See Also:
GNOME target:	---
GNOME version:	---

Attachments
videodecoder: don't take STREAM_LOCK on upstream events (3.32 KB, patch) 2012-09-24 09:26 UTC, Tim-Philipp Müller	none	Details \| Review
videodecoder: don't take STREAM_LOCK on upstream events (v2) (4.60 KB, patch) 2012-09-24 10:00 UTC, Tim-Philipp Müller	committed	Details \| Review

Description Tim-Philipp Müller 2012-09-23 12:59:50 UTC

+++ This bug was initially created as a clone of Bug #683192 +++

For some reason mewmew-vorbis-ssa.mkv is stuck on the first frame with totem and the playback test.
With totem there are invalid (-1) timestamp warnings, see bug #683189.
I'm sure this used to work not too long ago, not sure what changed, might be in videodecoder.
Things work though if assrender is removed from the equation.


When shutting down the above, we deadlock. I wonder if locking in videodecoder is right here: it pushes out buffers while still holding the GST_VIDEO_DECODER_STREAM_LOCK, and when it gets a QoS event in another thread, it takes the object lock and waits on the STREAM_LOCK as well and is stuck, which is then the reason the sink can't shut down.

+ Trace 230892

Thread 6 (Thread 0x7fd62d325700 (LWP 10604))

#0 __lll_lock_wait
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S line 136
#1 _L_lock_997
from /lib/x86_64-linux-gnu/libpthread.so.0
#2 __pthread_mutex_lock
at pthread_mutex_lock.c line 82
#3 gst_video_decoder_src_event_default
at gstvideodecoder.c line 1272
#4 gst_pad_send_event_unchecked
at gstpad.c line 4807
#5 gst_pad_push_event_unchecked
at gstpad.c line 4503
#131 gst_pad_push_event
at gstpad.c line 4629
#132 gst_base_sink_send_qos
at gstbasesink.c line 2445
#133 gst_base_sink_perform_qos
at gstbasesink.c line 2578
#134 gst_base_sink_chain_unlocked
at gstbasesink.c line 3233
#135 gst_base_sink_chain_main
at gstbasesink.c line 3318

Thread 1 (Thread 0x7fd644a92700 (LWP 10443))

#0 __lll_lock_wait
at ../nptl/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S line 136
#1 _L_lock_1145
from /lib/x86_64-linux-gnu/libpthread.so.0
#2 __pthread_mutex_lock
at pthread_mutex_lock.c line 101
#3 g_mutex_lock
at /tmp/buildd/glib2.0-2.32.3/./glib/gthread-posix.c line 208
#4 gst_base_sink_change_state
at gstbasesink.c line 4748
#5 gst_xvimagesink_change_state
at xvimagesink.c line 1759
#27 gst_element_set_state_func
at gstelement.c line 2524
#28 main
at gst-launch.c line 1156

Comment 1 Tim-Philipp Müller 2012-09-23 15:27:30 UTC

Note that this is a blocker for the next bug-fix release, not 1.0.0

Comment 2 Sebastian Dröge (slomo) 2012-09-24 08:50:53 UTC

<__tim> slomo, do you have an opinion on https://bugzilla.gnome.org/show_bug.cgi?id=684658 ?
<__tim> does the lock-taking look correct?
<slomo> __tim: using the stream lock for upstream events sounds wrong
<slomo> __tim: everything else seems correct

Comment 3 Tim-Philipp Müller 2012-09-24 09:26:48 UTC

Created attachment 225047 [details] [review]
videodecoder: don't take STREAM_LOCK on upstream events

Dumb patch, question is if it should wait or not.

Comment 4 Tim-Philipp Müller 2012-09-24 10:00:49 UTC

Created attachment 225049 [details] [review]
videodecoder: don't take STREAM_LOCK on upstream events (v2)

Protect qos variables with object lock in some other places as well.

Comment 5 Tim-Philipp Müller 2012-09-24 10:47:52 UTC

Ok, pushed this for now:

 commit 62c111f1e481f558e9d65a8eface9877178d3051
 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk>
 Date:   Mon Sep 24 10:16:09 2012 +0100

    videodecoder: don't take STREAM_LOCK on upstream events
    
    Don't try to take STREAM_LOCK on upstream events such as QOS.
    Protect qos-related variables with object lock instead. Fixes
    possible deadlock when shutting down in certain situations.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=684658

There's another place that needs fixing, namely the GST_QUERY_CONVERT code path, it also shouldn't take the stream lock, but I'll leave that for another time. Keeping bug open for that.

Comment 6 Olivier Crête 2012-09-24 15:30:39 UTC

The problem is that the output_state pointer is protected by the STREAM LOCK, it should probably use the object lock and refcounting and more cleverness...

Comment 7 Tim-Philipp Müller 2012-09-25 23:00:51 UTC

There's bug #684832 now for the remaining issue, so let's close this one then.