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 642174 - Playbin2 cannot work with non-raw custom sinks
Playbin2 cannot work with non-raw custom sinks
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
0.10.31
Other Linux
: Normal blocker
: 0.10.33
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2011-02-12 15:24 UTC by Akihiro Tsukada
Modified: 2011-02-18 12:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch for playbin2 to set "caps" property of decodebin2 (2.49 KB, patch)
2011-02-12 15:24 UTC, Akihiro Tsukada
needs-work Details | Review
patch for playbin2 to set "caps" property of decodebin2, updated. (1.78 KB, patch)
2011-02-13 06:25 UTC, Akihiro Tsukada
none Details | Review
patch for playbin2 to work with custom sinks (1.78 KB, patch)
2011-02-13 06:34 UTC, Akihiro Tsukada
needs-work Details | Review
patch for playbin2 to work with custom sinks, updated (2.94 KB, patch)
2011-02-13 12:27 UTC, Akihiro Tsukada
committed Details | Review

Description Akihiro Tsukada 2011-02-12 15:24:52 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.
Comment 1 Sebastian Dröge (slomo) 2011-02-12 15:59:55 UTC
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.
Comment 2 Akihiro Tsukada 2011-02-13 06:25:58 UTC
Created attachment 180761 [details] [review]
patch for playbin2 to set "caps" property of decodebin2, updated.
Comment 3 Akihiro Tsukada 2011-02-13 06:27:09 UTC
thank you for your advice, and I re-newed the patch as the above.
Comment 4 Akihiro Tsukada 2011-02-13 06:34:53 UTC
Created attachment 180762 [details] [review]
patch for playbin2 to work with custom sinks

sorry, I sent a wrong patch. this is the updated version.
Comment 5 Sebastian Dröge (slomo) 2011-02-13 09:29:21 UTC
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
Comment 6 Akihiro Tsukada 2011-02-13 12:27:40 UTC
Created attachment 180771 [details] [review]
patch for playbin2 to work with custom sinks, updated
Comment 7 Sebastian Dröge (slomo) 2011-02-15 16:48:56 UTC
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.
Comment 8 David Schleef 2011-02-18 06:22:35 UTC
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.
Comment 9 Sebastian Dröge (slomo) 2011-02-18 08:12:53 UTC
Can you get a backtrace?
Comment 10 Akihiro Tsukada 2011-02-18 08:43:24 UTC
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.
Comment 11 Sebastian Dröge (slomo) 2011-02-18 08:48:36 UTC
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
Comment 12 Sebastian Dröge (slomo) 2011-02-18 10:02:48 UTC
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.
Comment 13 Akihiro Tsukada 2011-02-18 10:53:08 UTC
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?
Comment 14 Sebastian Dröge (slomo) 2011-02-18 11:16:17 UTC
(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.
Comment 15 Akihiro Tsukada 2011-02-18 11:50:16 UTC
(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.
Comment 16 Sebastian Dröge (slomo) 2011-02-18 12:02:57 UTC
(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.