GNOME Bugzilla – Bug 642174
Playbin2 cannot work with non-raw custom sinks
Last modified: 2011-02-18 12:02:57 UTC
Created attachment 180724 [details] [review] patch for playbin2 to set "caps" property of decodebin2 I made an sink element whose sink caps accepts "audio/mpeg, mpegversion = (int) { 2, 4 };", and attached to playbin2 like the following. gst-launch playbin2 uri=file:///foo.aac audio-sink=mysink (mysink is a bin element containing aacparse & aac2spdif-encoder & alsasink device="iec958") but it does not work. I read debug log and found that typefind was connected to "faad", not "mysink", in the auto-plugging. ---------- debug log begin ------- パイプラインを一時停止 (PAUSED) にしています... 0:00:00.025437089 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:3618:gst_decode_bin_plugin_init: binding text domain gst-plugins-base-0.10 to locale dir /usr/share/locale 0:00:00.026880461 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1048:gst_decode_bin_set_caps:<decodebin20> Setting new caps: video/x-raw-yuv; video/x-raw-rgb; video/x-raw-gray; audio/x-raw-int; audio/x-raw-float; text/plain; text/x-pango-markup; video/x-dvd-subpicture; subpicture/x-pgs 0:00:00.026924222 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1102:gst_decode_bin_set_subs_encoding:<decodebin20> Setting new encoding: (NULL) ...........(some LOG messages deleted) ........... 0:00:00.049423446 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1954:type_found:<decodebin20> typefind found caps audio/mpeg, framed=(boolean)false, mpegversion=(int)2, stream-format=(string)adts, level=(string)2, profile=(string)lc, channels=(int)2, rate=(int)48000 0:00:00.049507086 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:2408:gst_decode_chain_new:<decodebin20> Creating new chain 0x12d2c00 with parent group (nil) 0:00:00.049527966 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1343:analyze_new_pad:<decodebin20> Pad typefind:src caps:audio/mpeg, framed=(boolean)false, mpegversion=(int)2, stream-format=(string)adts, level=(string)2, profile=(string)lc, channels=(int)2, rate=(int)48000 0:00:00.049557607 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:3420:gst_decode_pad_new:<decodebin20> making new decodepad 0:00:00.050222412 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1233:gst_decode_bin_autoplug_continue:<decodebin20> autoplug-continue returns TRUE 0:00:00.050246492 28366 0x10b1080 LOG decodebin2 gstdecodebin2.c:2207:are_final_caps:<decodebin20> Checking with caps audio/mpeg, framed=(boolean)false, mpegversion=(int)2, stream-format=(string)adts, level=(string)2, profile=(string)lc, channels=(int)2, rate=(int)48000 0:00:00.050277452 28366 0x10b1080 LOG decodebin2 gstdecodebin2.c:2214:are_final_caps:<decodebin20> Caps are not final caps 0:00:00.052860074 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1282:gst_decode_bin_autoplug_sort:<decodebin20> autoplug-sort returns 0x1251c60 0:00:00.052888554 28366 0x10b1080 LOG decodebin2 gstdecodebin2.c:1480:analyze_new_pad:<typefind:src> Let's continue discovery on this pad 0:00:00.052906115 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1610:connect_pad:<decodebin20> pad typefind:src , chain:0x12d2c00 0:00:00.052964075 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1651:connect_pad:<decodebin20> autoplug select requested try ............... 0:00:00.053667641 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1812:connect_element:<decodebin20> Attempting to connect element faad0 [chain:0x12d2c00] further 0:00:00.053684761 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1825:connect_element:<decodebin20> got a source pad template src 0:00:00.053711962 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1836:connect_element:<decodebin20> got the pad for always template src 0:00:00.053743162 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1343:analyze_new_pad:<decodebin20> Pad faad0:src caps:audio/x-raw-int, endianness=(int)1234, signed=(boolean)true, width=(int)16, depth=(int)16, rate=(int)[ 8000, 96000 ], channels=(int)[ 1, 8 ] 0:00:00.053774322 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:3420:gst_decode_pad_new:<decodebin20> making new decodepad 0:00:00.053880043 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1233:gst_decode_bin_autoplug_continue:<decodebin20> autoplug-continue returns TRUE 0:00:00.053901323 28366 0x10b1080 LOG decodebin2 gstdecodebin2.c:2207:are_final_caps:<decodebin20> Checking with caps audio/x-raw-int, endianness=(int)1234, signed=(boolean)true, width=(int)16, depth=(int)16, rate=(int)[ 8000, 96000 ], channels=(int)[ 1, 8 ] 0:00:00.053930324 28366 0x10b1080 LOG decodebin2 gstdecodebin2.c:2214:are_final_caps:<decodebin20> Caps are final caps 0:00:00.053947124 28366 0x10b1080 LOG decodebin2 gstdecodebin2.c:1490:analyze_new_pad:<decodebin20> Pad is final. autoplug-continue:1 0:00:00.053966604 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:1913:expose_pad:<decodebin20> pad faad0:src, chain:0x12d2c00 0:00:00.054019804 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:3349:gst_decode_pad_set_blocked:<'':decodepad1> blocking pad: 1 ........... 0:00:00.054143805 28366 0x10b1080 DEBUG decodebin2 gstdecodebin2.c:2750:gst_decode_chain_is_complete:<decodebin20> Chain 0x12d2c00 is complete: 0 0:00:00.054160486 28366 0x10b1080 LOG decodebin2 gstdecodebin2.c:1939:expose_pad:<decodebin20> expose unlocking from thread 0x10b1080 Pipeline is PREROLLING ... ......... 0:00:00.054593529 28366 0x12ef160 DEBUG decodebin2 gstdecodebin2.c:2750:gst_decode_chain_is_complete:<decodebin20> Chain 0x12d2c00 is complete: 1 0:00:00.054613169 28366 0x12ef160 DEBUG decodebin2 gstdecodebin2.c:3103:gst_decode_bin_expose:<decodebin20> Exposing currently active chains/groups 0:00:00.054682930 28366 0x12ef160 DEBUG decodebin2 gstdecodebin2.c:3174:gst_decode_bin_expose:<decodebin20> About to expose dpad decodepad1 as src0 0:00:00.054703330 28366 0x12ef160 DEBUG decodebin2 gstdecodebin2.c:3188:gst_decode_bin_expose:<decodebin20> emitting new-decoded-pad 0:00:00.055027452 28366 0x12ef160 DEBUG decodebin2 gstdecodebin2.c:3192:gst_decode_bin_expose:<decodebin20> emitted new-decoded-pad 0:00:00.055050212 28366 0x12ef160 LOG decodebin2 gstdecodebin2.c:3197:gst_decode_bin_expose:<decodebin20> signalling no-more-pads ERROR: from element /GstPlayBin2:playbin20/GstPlaySink:playsink0: Internal GStreamer error: pad problem. Please file a bug at http://bugzilla.gnome.org/enter_bug.cgi?product=GStreamer. 0:00:00.082970321 28366 0x12ef160 DEBUG decodebin2 gstdecodebin2.c:3209:gst_decode_bin_expose:<decodebin20:src0> unblocking 0:00:00.083092242 28366 0x12ef160 DEBUG decodebin2 gstdecodebin2.c:3349:gst_decode_pad_set_blocked:<decodebin20:src0> blocking pad: 0 0:00:00.083135562 28366 0x12ef160 DEBUG decodebin2 gstdecodebin2.c:3211:gst_decode_bin_expose:<decodebin20:src0> unblocked 追加のデバッグ情報: gstplaysink.c(1885): gen_audio_chain (): /GstPlayBin2:playbin20/GstPlaySink:playsink0: Failed to configure the audio sink. ...................... -----------debug log end --------- I also read the source code of playbin2 and decodebin2, and found that playbin2 does not set "caps" property of the internal uridecodebin(decodebin2) even when custom sink is set. Thus in auto-plugging process, it seems to me that, custom sink caps are not recognized as final caps in decodebin2::analyze_new_pads(), so the auto-plugging wrongly continues to proceed auto-plugging. I wrote and tried the attached patch, which seems to be working.
Please take a look at the text sink related code in the autoplug-continue signal handler inside playbin2. This already does the same but only for the text sink. The autoplug-continue signal would be a better place for your code too, the caps property might not be flexible enough in some cases.
Created attachment 180761 [details] [review] patch for playbin2 to set "caps" property of decodebin2, updated.
thank you for your advice, and I re-newed the patch as the above.
Created attachment 180762 [details] [review] patch for playbin2 to work with custom sinks sorry, I sent a wrong patch. this is the updated version.
Review of attachment 180762 [details] [review]: ::: gst/playback/gstplaybin2.c @@ +2915,3 @@ + GstCaps *acaps; + + gst_object_ref (group->playbin->audio_sink); Why is it necessary to take a ref of the audio sink? This should be protected by some mutex instead I guess, apart from that the way how you ref the audio sink is not threadsafe @@ +2916,3 @@ + + gst_object_ref (group->playbin->audio_sink); + subcaps = gst_caps_make_writable (subcaps); Please rename subcaps to something more descriptive, it's not containing the subtitle caps anymore @@ +2917,3 @@ + gst_object_ref (group->playbin->audio_sink); + subcaps = gst_caps_make_writable (subcaps); + acaps = _get_first_sink_caps (group->playbin->audio_sink); You can assume that the pad from which you want to get the caps is called "sink", otherwise it wouldn't work with playbin2 anyway
Created attachment 180771 [details] [review] patch for playbin2 to work with custom sinks, updated
commit 140dca43f35d5438d8c5300bd150e9fb69beea59 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Tue Feb 15 17:46:22 2011 +0100 playbin2: Optimize autoplug-continue handler a bit Don't build merge the caps of all sinks but check them one-by-one until one supports the caps. Also get reffed caps from the sinkpads instead of a writable copy and add debug output if a sink claims to support ANY caps. commit 555e33800867d68bfed0e27546448e738acabf35 Author: Akihiro Tsukada <tskd2@yahoo.co.jp> Date: Tue Feb 15 17:24:28 2011 +0100 playbin2: Fix handling of non-raw custom sinks When autoplugging elements in decodebin2, check if the caps are supported by one of the sink before continuing autoplugging. Fixes bug #642174.
This patch (555e33) causes rhythmbox to freeze up during playback within a few seconds of the end of a track, when normally it would continue on to the next. Reverting these two causes git master to work again.
Can you get a backtrace?
As PLAY_BIN_LOCK() is double-locked from activate_group() and from a signal handler (autoplug_continue()), so the process is dead locked. using g_mutex_trylock() and conditional unlcok() seems to resolve the problem. I will update the patch soon.
Don't update the patch but provide a patch on top of what currently is in GIT please. And using g_mutex_trylock() is definitely not a solution here. I'll take a look at this in 1-2 hours
Unfortunately I don't think it's possible to get around using a recursive mutex here. Releasing it before setting uridecodebin to PAUSED can cause races commit 1351597381760f6c99c84d5ac97dcaaca1b93191 Author: Sebastian Dröge <sebastian.droege@collabora.co.uk> Date: Fri Feb 18 10:57:40 2011 +0100 playbin2: Use a recursive mutex for the playbin lock This lock is taken when activating a group, which could result in calling the autoplug-continue callback, which also needs this lock to access the sinks. See bug #642174.
Isn't the thread of activate_group() always same as that of signal handlers? As the comment in activate_group() says /* release the group lock before setting the state of the decodebins, they * might fire signals in this thread that we need to handle with the * group_lock taken. */ , I think the same is true for playbin lock. how about moving the state setting code in activate_group() to the place where in setup_next_source() and after releasing the playbin lock?
(In reply to comment #13) > Isn't the thread of activate_group() always same as that of signal handlers? Not always, otherwise it would've never started any playback at all instead of deadlocking when switching tracks. > As the comment in activate_group() says > /* release the group lock before setting the state of the decodebins, they > * might fire signals in this thread that we need to handle with the > * group_lock taken. */ > , I think the same is true for playbin lock. > how about moving the state setting code in activate_group() to the place where > in setup_next_source() and after releasing the playbin lock? Look at the code that calls group_set_locked_state_unlocked() in activate_group(). That has to be called after setting the state and needs to have the group and playbin lock. Another possible solution would be to release the playbin lock together with the group lock in activate_group() and take it again after setting the state but IMO this can lead to races because the playbin lock is also used to protect against concurrent group changes... which could easily happen at the short time when the state change is happening. That's why I used the solution that was committed now.
(In reply to comment #14) > Not always, .... If autoplug_continue_cb() and activate_group() are executed on separate threads, then using recursive mutex is not different from normal mutex... doesn't this still cause the probelm as in comment #12 ? > Look at the code that calls group_set_locked_state_unlocked() in > activate_group(). That has to be called after setting the state and needs to > have the group and playbin lock. I thought that changing the order of {set-state, allow-state-change, unlock} into {allow-state-change, unlock, set-state} would be OK. (I thought that set-state itself must not necessarily be protected by the lock.) Maybe I don't fully understand the problem. Sorry to bother you many times due to a bit;) poor quality of my code.
(In reply to comment #15) > (In reply to comment #14) > > Not always, .... > > If autoplug_continue_cb() and activate_group() are executed on separate > threads, then using recursive mutex is not different from normal mutex... > doesn't this still cause the probelm as in comment #12 ? Sometimes they are executed from the same thread and sometimes they're not. If they are executed from the same thread the recursive mutex will prevent the deadlock, if they are executed from different threads the recursive mutex will work like every other mutex. > > Look at the code that calls group_set_locked_state_unlocked() in > > activate_group(). That has to be called after setting the state and needs to > > have the group and playbin lock. > > I thought that changing the order of {set-state, allow-state-change, unlock} > into {allow-state-change, unlock, set-state} would be OK. > (I thought that set-state itself must not necessarily be protected by the > lock.) The unlock must happen after the set_state. The mutex is not necessary for the set_state but it should only be released after the group is completely activated to prevent simultanous group changes.