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 776458 - urisourcebin: Always configure typefind
urisourcebin: Always configure typefind
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other Linux
: Normal normal
: 1.11.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2016-12-24 00:48 UTC by Seungha Yang
Modified: 2017-01-10 13:28 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
urisourcebin: Allow configure typefind element for filesrc (4.17 KB, patch)
2016-12-24 02:11 UTC, Seungha Yang
none Details | Review
urisourcebin: Remove unused signal handler variable (1.16 KB, patch)
2016-12-24 08:43 UTC, Seungha Yang
committed Details | Review
urisourcebin: Use GList for typefind elements (2.92 KB, patch)
2016-12-24 08:44 UTC, Seungha Yang
committed Details | Review
urisourcebin: Always configure typefind element (4.24 KB, patch)
2016-12-24 08:45 UTC, Seungha Yang
none Details | Review
urisourcebin: Configure typefind element for non-streaming uri (4.12 KB, patch)
2017-01-10 12:56 UTC, Seungha Yang
committed Details | Review

Description Seungha Yang 2016-12-24 00:48:20 UTC
When "file://" uri used, adaptivedemux is not configured in urisourcebin. Is it intended behaviour? If not, we may need to configure typefind element in urisourcebin always.
Comment 1 Seungha Yang 2016-12-24 02:11:10 UTC
Created attachment 342441 [details] [review]
urisourcebin: Allow configure typefind element for filesrc

To ensure configuring adaptivedemux if needed,
setup typefind element even if uri is not matched to streaming protocol.
Comment 2 Sebastian Dröge (slomo) 2016-12-24 06:21:44 UTC
Comment on attachment 342441 [details] [review]
urisourcebin: Allow configure typefind element for filesrc

Makes sense, yes. Having a typefind there should be done in any case to support cases like your's
Comment 3 Sebastian Dröge (slomo) 2016-12-24 06:30:02 UTC
Review of attachment 342441 [details] [review]:

::: gst/playback/gsturisourcebin.c
@@ +1902,3 @@
+    GST_URI_SOURCE_BIN_UNLOCK (urisrc);
+
+    gst_element_no_more_pads (GST_ELEMENT_CAST (urisrc));

no_more_pads() here looks wrong, it's not done in any of the other cases but instead done "outside"

@@ +2185,3 @@
         G_CALLBACK (source_new_pad), urisrc);
   } else {
+    if (urisrc->is_stream || num_srcpads == 1) {

Why special case 1 srcpad here? The other case should handle 1 srcpad just fine
Comment 4 Seungha Yang 2016-12-24 07:09:00 UTC
(In reply to Sebastian Dröge (slomo) from comment #3)
> Review of attachment 342441 [details] [review] [review]:
> ::: gst/playback/gsturisourcebin.c
> @@ +1902,3 @@
> +    GST_URI_SOURCE_BIN_UNLOCK (urisrc);
> +
> +    gst_element_no_more_pads (GST_ELEMENT_CAST (urisrc));
> 
> no_more_pads() here looks wrong, it's not done in any of the other cases but
> instead done "outside"

It seems that we can remove it, because playbin3 does not connect no-more-pads signal of urisourcebin. Also, no-more-pads does not posted if a source element have only sometimes pads.

Do we need no-more-pads signal in playbin3 usecase? Note that decodebin3 also never post it.

> @@ +2185,3 @@
>          G_CALLBACK (source_new_pad), urisrc);
>    } else {
> +    if (urisrc->is_stream || num_srcpads == 1) {
> 
> Why special case 1 srcpad here? The other case should handle 1 srcpad just
> fine

I'm not sure whether there is an source element which has two static source pad. (I've never seen it in my usecase). But, for safe reason, I restrict it to one.

Anyway, I can change it :)
Comment 5 Seungha Yang 2016-12-24 08:43:54 UTC
Created attachment 342445 [details] [review]
urisourcebin: Remove unused signal handler variable

Remove never used handler id
Comment 6 Seungha Yang 2016-12-24 08:44:33 UTC
Created attachment 342446 [details] [review]
urisourcebin: Use GList for typefind elements

We need typefind elements per source element's srcpad
Comment 7 Seungha Yang 2016-12-24 08:45:24 UTC
Created attachment 342447 [details] [review]
urisourcebin: Always configure typefind element
Comment 8 Jan Schmidt 2017-01-09 13:27:31 UTC
Review of attachment 342445 [details] [review]:

OK
Comment 9 Jan Schmidt 2017-01-09 13:28:34 UTC
Review of attachment 342446 [details] [review]:

OK
Comment 10 Jan Schmidt 2017-01-09 13:36:21 UTC
Review of attachment 342447 [details] [review]:

::: gst/playback/gsturisourcebin.c
@@ +2114,3 @@
       GST_DEBUG_PAD_NAME (pad), GST_ELEMENT_NAME (element));
+
+  /* FIXME: What should we do if fail to setup typefind here ? */

There's no need to do extra - if setup_typefind fails, it will already have posted an error on the bus.

@@ +2115,3 @@
+
+  /* FIXME: What should we do if fail to setup typefind here ? */
+  setup_typefind (urisrc, pad);

I think this code should be:

caps = gst_pad_get_current_caps (pad);
if (caps == NULL)
  setup_typefind (urisrc, pad);
else {
  handle_new_pad (urisrc, pad, caps);
  gst_caps_unref (caps);
}

That way, if the pad does already have caps, we can skip the typefind element entirely (which will just get the same caps and then fire the have-type signal anyway)
Comment 11 Seungha Yang 2017-01-10 12:56:12 UTC
Created attachment 343240 [details] [review]
urisourcebin: Configure typefind element for non-streaming uri

To ensure configuring adaptivedemux if needed,
setup typefind element even if uri is not matched to streaming protocol.
Comment 12 Jan Schmidt 2017-01-10 13:08:59 UTC
Review of attachment 343240 [details] [review]:

Looks great, thanks!
Comment 13 Jan Schmidt 2017-01-10 13:27:58 UTC
Thanks, pushed:

commit 7bd7b2209ad124d65ce33c804c15edc4cc193cb4
Author: Seungha Yang <sh.yang@lge.com>
Date:   Tue Jan 10 21:52:34 2017 +0900

    urisourcebin: Configure typefind element for non-streaming uri
    
    To ensure configuring adaptivedemux if needed,
    setup typefind element even if uri is not matched to streaming protocol.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776458

commit 5d0628ff52ca3194a870fdf6af771fce61924984
Author: Seungha Yang <sh.yang@lge.com>
Date:   Sat Dec 24 16:44:26 2016 +0900

    urisourcebin: Use GList for typefind elements
    
    We need typefind elements per source element's srcpad
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776458

commit 46c6e92abdf4d20bdd12e6f59def1c335ce3b0b7
Author: Seungha Yang <sh.yang@lge.com>
Date:   Sat Dec 24 16:15:45 2016 +0900

    urisourcebin: Remove unused signal handler variable
    
    Remove never used handler id
    
    https://bugzilla.gnome.org/show_bug.cgi?id=776458