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 733916 - basetextoverlay: Simple pipeline with textoverlay and fakesink fails to negotiate
basetextoverlay: Simple pipeline with textoverlay and fakesink fails to negot...
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
1.4.0
Other All
: Normal normal
: 1.4.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-07-29 13:10 UTC by Will Manley
Modified: 2014-08-26 14:25 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
basetestoverlay: do subset against all supported caps (1.03 KB, patch)
2014-07-29 20:03 UTC, Thiago Sousa Santos
reviewed Details | Review
basetextoverlay: always intersect with the filter caps (1.89 KB, patch)
2014-08-08 16:48 UTC, Thiago Sousa Santos
committed Details | Review
basetextoverlay: rework caps negotiation (7.22 KB, patch)
2014-08-08 16:48 UTC, Thiago Sousa Santos
reviewed Details | Review

Description Will Manley 2014-07-29 13:10:51 UTC
The following command fails:

    gst-launch-1.0 videotestsrc num-buffers=50 ! textoverlay ! fakesink

with the error:

    ERROR: from element /GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc0: 
    Internal data flow error.
    Additional debug info:
    gstbasesrc.c(2933): gst_base_src_loop (): 
    /GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc0:
    streaming task paused, reason not-negotiated (-4)

Given the simple nature of the pipeline it seems like it ought to succeed.

I suspect that this is due to the new `GstVideoOverlayComposition` mechanism
recently added to GStreamer.  This is all a little odd as adding an identity
element is enough to make it work:

    gst-launch-1.0 videotestsrc num-buffers=50 ! identity ! textoverlay ! fakesink

This is will GStreamer 1.4.0 with the Debian provided packages on my Debian testing machine.
Comment 1 Will Manley 2014-07-29 13:16:35 UTC
I'm not sure whether this is the same bug but I was also seeing similar failures with the pipeline:

    gst-launch-1.0 videotestsrc ! textoverlay ! decodebin ! videoconvert \
                   ! video/x-raw,format=RGB ! fakesink

which would be resolved by either removing textoverlay or decodebin.  This was how I originally came onto this subject as upgrading to 1.4 caused one of the tests in the stb-tester test suite to start failing.  I have since put in a workaround[1].

I'm holding off opening a second bug for this more complex pipeline until the simple pipeline failure is understood.

[1]: https://github.com/wmanley/stb-tester/commit/ad6936e
Comment 2 Thiago Sousa Santos 2014-07-29 20:03:00 UTC
Created attachment 281976 [details] [review]
basetestoverlay: do subset against all supported caps

Otherwise it will fail if upstream accepted the overlay meta
but rejects the allocation query (fakesink)
Comment 3 Sebastian Dröge (slomo) 2014-07-29 22:26:34 UTC
Comment on attachment 281976 [details] [review]
basetestoverlay: do subset against all supported caps

Typo in commit mssage :)


The same bug would also be in the other subtitle renderers then. They have the same code.

But can you explain why exactly this happens? I think there was a reason why we don't check for the full caps here but only the ones that we can handle directly... I just can't remember what it was.
Comment 4 Thiago Sousa Santos 2014-07-30 00:50:27 UTC
It has this code for negotiation:

  query = gst_query_new_allocation (target, TRUE);

  if (!gst_pad_peer_query (overlay->srcpad, query)) {
    /* no problem, we use the query defaults */
    GST_DEBUG_OBJECT (overlay, "ALLOCATION query failed");
  }

  if (gst_query_find_allocation_meta (query,
          GST_VIDEO_OVERLAY_COMPOSITION_META_API_TYPE, NULL))
    attach = TRUE;


So, even though fakesink will claim to accept the overlay meta it will reject the allocation query with the meta in the caps. So 'attach' is marked as false and then textoverlay will check if the negotiated caps (with the meta) is a subset of the raw formats (without the meta) and it fails.

I also feel like there is something else wrong here.
Comment 5 Sebastian Dröge (slomo) 2014-08-05 07:47:44 UTC
Why does fakesink claim to accept the overlay meta? It does not accept it :)
Comment 6 Thiago Sousa Santos 2014-08-06 22:18:41 UTC
(In reply to comment #5)
> Why does fakesink claim to accept the overlay meta? It does not accept it :)

Why doesn't it? It can take and handle (in its own special way) any input. I guess it just can't provide the proper negotiation atm. This seems more like a design decision for what fakesink should do, it could go both ways.
Comment 7 Sebastian Dröge (slomo) 2014-08-07 07:07:22 UTC
Then you would have to add code to fakesink to let it handle the overlay composition meta.

But what do you mean with "fakesink will claim to accept the overlay meta"?
Comment 8 Thiago Sousa Santos 2014-08-07 14:04:22 UTC
(In reply to comment #7)
> Then you would have to add code to fakesink to let it handle the overlay
> composition meta.
> 
> But what do you mean with "fakesink will claim to accept the overlay meta"?

This:


:00:00.195544786 22567      0x226c940 DEBUG               basesink gstbasesink.c:4831:gst_base_sink_default_query:<fakesink0> Checking if requested caps video/x-raw(memory:SystemMemory, meta:GstVideoOverlayComposition), format=(string)I420, width=(int)320, height=(int)240, framerate=(fraction)30/1, pixel-aspect-ratio=(fraction)1/1, interlace-mode=(string)progressive are a subset of pad caps ANY result 1
0:00:00.195558989 22567      0x226c940 DEBUG               GST_PADS gstpad.c:3589:gst_pad_query:<fakesink0:sink> sent query 0x7f37f8003400 (accept-caps), result 1
0:00:00.195571263 22567      0x226c940 DEBUG                default gstutils.c:2857:gst_pad_query_accept_caps:<fakesink0:sink> query returned 1
0:00:00.195578643 22567      0x226c940 DEBUG               basesink gstbasesink.c:3157:gst_base_sink_event:<fakesink0> received event 0x2385df0 caps event: 0x2385df0, time 99:99:99.999999999, seq-num 21, GstEventCaps, caps=(GstCaps)"video/x-raw\(memory:SystemMemory\,\ meta:GstVideoOverlayComposition\)\,\ format\=\(string\)I420\,\ width\=\(int\)320\,\ height\=\(int\)240\,\ framerate\=\(fraction\)30/1\,\ pixel-aspect-ratio\=\(fraction\)1/1\,\ interlace-mode\=\(string\)progressive";
0:00:00.195618295 22567      0x226c940 DEBUG               basesink gstbasesink.c:3069:gst_base_sink_default_event:<fakesink0> caps 0x2385df0
0:00:00.195626448 22567      0x226c940 DEBUG               GST_PADS gstpad.c:5156:gst_pad_send_event_unchecked:<fakesink0:sink> sent event, ret ok
0:00:00.195634299 22567      0x226c940 LOG                 GST_PADS gstpad.c:4677:store_sticky_event:<fakesink0:sink> stored sticky event caps
0:00:00.195641358 22567      0x226c940 DEBUG               GST_PADS gstpad.c:4683:store_sticky_event:<fakesink0:sink> notify caps
0:00:00.195649502 22567      0x226c940 LOG           GST_PROPERTIES gstobject.c:463:gst_object_dispatch_properties_changed:<fakesink0> deep notification from sink (caps)
0:00:00.195658113 22567      0x226c940 LOG           GST_PROPERTIES gstobject.c:463:gst_object_dispatch_properties_changed:<pipeline0> deep notification from sink (caps)
0:00:00.195666700 22567      0x226c940 LOG                 GST_PADS gstpad.c:4845:gst_pad_push_event_unchecked:<timeoverlay0:src> sent event 0x2385df0 to (caps) peerpad <fakesink0:sink>, ret ok
Comment 9 Thiago Sousa Santos 2014-08-08 16:48:14 UTC
Created attachment 282933 [details] [review]
basetextoverlay: always intersect with the filter caps

Avoids returning values that upstream can't produce
Comment 10 Thiago Sousa Santos 2014-08-08 16:48:26 UTC
Created attachment 282934 [details] [review]
basetextoverlay: rework caps negotiation

Make textoverlay negotiate caps more correctly.

1) Check what caps we received in the video-sink
2) If it already has the overlay meta -> use it directly
3) If it doesn't, textoverlay try adding the overlay meta and using it,
   if downstream doesn't support it, just use what is received in the
   video-sink
4) Check if the allocation query also supports the meta to enable
   really using it

Before it wasn't really doing renegotiation of any kind, just
re-checking if it should use the overlay meta or not

Also had to update the caps in the test as memory:SystemMemory seems
to be required when you use a caps feature otherwise intersection/subset
checks will fail.
Comment 11 Sebastian Dröge (slomo) 2014-08-11 07:29:32 UTC
Review of attachment 282934 [details] [review]:

Make sense, now the same code is only required in all the other subtitle overlay elements :)

::: ext/pango/gstbasetextoverlay.c
@@ +715,2 @@
   GstQuery *query;
+  gboolean attach = TRUE;

Shouldn't the default be FALSE?
Comment 12 Thiago Sousa Santos 2014-08-11 16:22:56 UTC
master:

commit a080c0ebbf989930dde3b3575317057bb13dd4da
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Fri Aug 8 12:46:47 2014 -0300

    basetextoverlay: rework caps negotiation
    
    Make textoverlay negotiate caps more correctly.
    
    1) Check what caps we received in the video-sink
    2) If it already has the overlay meta -> use it directly
    3) If it doesn't, textoverlay try adding the overlay meta and using it,
       if downstream doesn't support it, just use what is received in the
       video-sink
    4) Check if the allocation query also supports the meta to enable
       really using it
    
    Before it wasn't really doing renegotiation of any kind, just
    re-checking if it should use the overlay meta or not
    
    Also had to update the caps in the test as memory:SystemMemory seems
    to be required when you use a caps feature otherwise intersection/subset
    checks will fail.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733916

commit c20e044ef0021f0046cc6941dbc8618dc4d37e0b
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Thu Aug 7 17:35:05 2014 -0300

    basetextoverlay: always intersect with the filter caps
    
    Avoids returning values that upstream can't produce
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733916

commit 33c2372ae3b0dbc70bfe8abe3e1a95cd1881ba22
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Mon Aug 11 12:22:44 2014 -0300

    assrender: improve negotiation
    
    Check if downstream supports overlay meta, if possible use it and
    if not fallback to no-overlay caps
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733916

commit 7895d8836945706c037e2308a75a3728b3d988db
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Mon Aug 11 12:21:28 2014 -0300

    assrender: always intersect with the filter caps
    
    Avoids returning values that peers can't use
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733916

commit eee178988ad2d7ad03163bb59cdf9b4cc79a517d
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Mon Aug 11 11:35:01 2014 -0300

    dvbsuboverlay: improve negotiation
    
    Check if downstream supports overlay meta, if possible use it and
    if not fallback to no-overlay caps
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733916

commit 627b6ac4617b370bb71012cced10c71672265eba
Author: Thiago Santos <thiagoss@osg.samsung.com>
Date:   Mon Aug 11 11:33:28 2014 -0300

    dvbsuboverlay: always intersect with the filter in getcaps
    
    Avoids returning unsupported caps to peers
    
    https://bugzilla.gnome.org/show_bug.cgi?id=733916


1.4:
base
commit d79cd056e7490d1fd4a8a232b905896b3104fee0
commit 93643695be6e83af204a6ba29e58b6566a92f18d
bad
commit f64b974c4d4a9f6732c9f63d6575f615f4e80cd5
commit 9f9493598c561cc6e736e3800557c82ceac405c3
commit 28fc15f236374f4423e2565cf0a85cf8aa3fed04
commit e0a878aef3fa381f181f9ff17ce2b31667ac0635
Comment 13 Arnaud Vrac 2014-08-26 13:51:21 UTC
Those patches prevent using overlay composition in some cases, for example when a videoconvert element is in the pipeline. I don't think any element or sink actually sets and negotiates the OVERLAY_COMPOSITION feature correctly, so those patches should be reverted in 1.4 at least, IMHO.
Comment 14 Arnaud Vrac 2014-08-26 14:25:52 UTC
Please ignore my comment, there's an issue with my code. It's getting pretty complicated to get caps negotation to work with features...