GNOME Bugzilla – Bug 735844
basetextoverlay/pango: overlay negotiation fails when it should not
Last modified: 2014-09-18 16:30:22 UTC
Created attachment 285084 [details] Log of events in basetextoverlay and the sink that supports overlay I'm hitting an issue which was not happening in gstreamer 1.2, where the overlay composition negotiation fails. In my pipeline blending subtitles over video frames is not supported, as it is waaay to slow for my system to do that. So in all cases I _must_ be using an overlay. I've updated my sink code to handle overlay negotiation with proper caps features (namely, memory:SystemMemory and meta:GstVideoOverlayComposition). The issue is that sometimes overlay negotiation fails while seeking. I've attached a log of the relevant events between the textoverlay element and my sink. The overlay negotiation features is working (in the log: Downstream accepts the overlay meta: 1), but then caps setting fails because the pad is flushing, so the ALLOCATION query is not done. This results in the overlay being disabled. The caps are then never being re-negotiated. I'm not sure what regressed compared to gstreamer 1.2, it could be either the textoverlay code or a deeper issue related to events and pads.
What is the outcome of this? The downstream and the source pad will be flushing and the negotiation will be re-done before another buffer is pushed as upstream is still going to handle the seek after overlay has handled it. What is the bug that you are seeing exactly? Does it renegotiate after the seek completes?
It does not renegotiate as there's nothing that would make it renegotiate. We will have to check manually after sending the ALLOCATION query if it failed because the pad was flushing (which still is racy as only downstream might be flushing but our srcpad not yet). There were similar fixes necessary in the video/audio base classes. Best would be to find a generic solution for this, but that would likely require GstFlowReturns for events and queries... which is 2.0 stuff.
@thiagoss: caps setting failed as shown in the log, but renegotiation never happens for some reason. @slomo: I've also hit the case where caps settings works but then the ALLOCATION query fails. As you said, it cannot work since no renegotation is done. Note that I only hit this case when applying the next attached patch.
Created attachment 285121 [details] [review] pango: just forward the seek event to sink pads like other events Remove seek event special handling and just forward the seek event to sink pads, like other events. Flushing is already sent on the source pad when seeking so I don't think this code is necessary. Anybody remembers why this is needed ?
Comment on attachment 285121 [details] [review] pango: just forward the seek event to sink pads like other events Makes sense to me, no idea what this code was supposed to do. Probably similar changes needed in other subtitle renderers.
(In reply to comment #3) > @slomo: I've also hit the case where caps settings works but then the > ALLOCATION query fails. As you said, it cannot work since no renegotation is > done. Note that I only hit this case when applying the next attached patch. Yes, that's what the video/audio base classes catch both :) I think for fixing this properly we really need GstFlowReturns for events and queries.
@slomo: would that also fix the case where caps setting fails and no renegotation happens ?
You would then get GST_FLOW_FLUSHING from the CAPS event and could handle that accordingly
But the flush-start event goes through basetextoverlay before it goes downstream of textoverlay, so just set a bool if fully/successfully negotiated, and if not successfully negotiated, try again after a flush-stop ?
That should work in this case, yes, but doesn't fix it for the common case.
Created attachment 285375 [details] [review] basetextoverlay: just forward the seek event to sink pads like other events Just changes the title of the patch
Created attachment 285376 [details] [review] basetextoverlay: schedule reconfigure on source pad when negotiation fails When negotiation fails while flushing, just set the reconfigure flag on the source pad so that negotiation is retried on the next video buffer.
Created attachment 285377 [details] [review] basetextoverlay: do not ask for a bufferpool when checking for composition meta We do not need a bufferpool so do not ask for one
Created attachment 285378 [details] [review] forward video frame when mapping fails instead of dropping it With this patch at least video is shown when overlay cannot be used and the video frames cannot be mapped.
Created attachment 285379 [details] [review] basetextoverlay: get framerate from previously parsed video info
The last 3 patches are losely related to this issue, but they do not harm. I think all the patches can be backported to 1.4, in addition to a65b307349 to avoid a conflict.
commit f47c2442fe7e3c0235b4c9afd72359627a2df416 Author: Arnaud Vrac <avrac@freebox.fr> Date: Thu Jan 31 13:49:00 2013 +0100 basetextoverlay: get framerate from previously parsed video info commit 267a8c24af4f02ba6f3075bd589d3c5d1dc826e9 Author: Arnaud Vrac <avrac@freebox.fr> Date: Thu Jan 31 13:47:35 2013 +0100 basetextoverlay: do not ask for a bufferpool when checking for composition meta commit 76ce11299411b412702d3fab34b7f152dc211156 Author: Arnaud Vrac <avrac@freebox.fr> Date: Thu Sep 4 15:06:31 2014 +0200 basetextoverlay: schedule reconfigure on source pad when negotiation fails The source pad might be flushing while negotiating, resulting in set_caps or the ALLOCATION query failing. In this case set the reconfigure flag on the source pad so that negotiation is retried on the next buffer. commit ef5823cc9b0b825a8ca96eab02712d31db57a41d Author: Arnaud Vrac <avrac@freebox.fr> Date: Thu Jan 31 15:38:18 2013 +0100 basetextoverlay: just forward the seek event to sink pads like other events https://bugzilla.gnome.org/show_bug.cgi?id=735844
Comment on attachment 285378 [details] [review] forward video frame when mapping fails instead of dropping it This one looks wrong. What are you trying to fix here? If mapping a frame failed and we can't use the overlay meta, then we'll have to fail. Otherwise textoverlay will silently not do what it is expected to do.
Created attachment 285509 [details] [review] basetextoverlay: Do not fail the negotiation if query fails One of these fixes introduced a regression when the allocation query isn't supported. This patch seems to make it work again, but I didn't test the original racy issue. So please test this and confirm it is still fixed. The allocation query failure doesn't mean that the negotiation has failed as the element can allocate buffers itself. Instead, only fail if the pads are flushing and the allocation query failed. https://bugzilla.gnome.org/show_bug.cgi?id=727255
The bug number at the end of the commend was wrong, I'll try to remember to remove it before pushing :)
The patch seems wrong, since you do not handle the case where set_caps fails. In this case the caps must be reconfigured too.
Created attachment 285697 [details] [review] basetextoverlay: Do not fail the negotiation if query fails Update to also set renegotiation when setting caps fail
*** Bug 736177 has been marked as a duplicate of this bug. ***
Does this new patch help?
Comment on attachment 285697 [details] [review] basetextoverlay: Do not fail the negotiation if query fails This doesn't catch the case where the allocation query fails because of flushing downstream, right?
@@ -780,7 +781,7 @@ gst_base_text_overlay_negotiate (GstBaseTextOverlay * overlay, GstCaps * caps) if (!gst_pad_peer_query (overlay->srcpad, query)) { /* no problem, we use the query defaults */ GST_DEBUG_OBJECT (overlay, "ALLOCATION query failed"); - ret = FALSE; + allocation_ret = FALSE; } if (caps_has_meta && gst_query_find_allocation_meta (query, @@ -792,9 +793,10 @@ gst_base_text_overlay_negotiate (GstBaseTextOverlay * overlay, GstCaps * caps) overlay->attach_compo_to_buffer = attach; - if (!ret && overlay->video_flushing) { + if ((!ret || !allocation_ret) && overlay->video_flushing) { GST_DEBUG_OBJECT (overlay, "negotiation failed, schedule reconfigure"); gst_pad_mark_reconfigure (overlay->srcpad); + ret = FALSE; It sets allocation_ret to false and then if it failed and video is flushing it will mark for reconfigure and fail the setcaps. Or am I missing something here?
Downstream might be flushing already but textoverlay didn't receive the flush event yet
Is there a way detect this and still allow elements that accept the meta but fail the allocation query (fakesink)?
Well, flow returns for events and queries :)
Maybe something else that could be done for 1.x? :) When it fails the allocation, it should fallback and try to renegotiate the caps without the meta, and then, that will fail. Can't we mark the pad as pending a reconfigure at this moment and return the set caps failure upstream? Or is this also going to cause an issue anyway?
Maybe that's the best we can do in 1.x, yes. Can you prepare a patch?
*** Bug 736748 has been marked as a duplicate of this bug. ***
Created attachment 286319 [details] [review] basetextoverlay: Do not fail the negotiation if query fails Updated patch. Arnaud, could you try this one, please?
Please reopen if it still fails for you. master: commit 3ecb8bea22182bd79e34ede48cb16c78c40a8118 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Fri Sep 5 13:49:46 2014 -0300 basetextoverlay: Do not fail the negotiation if query fails The allocation query failure doesn't mean that the negotiation has failed as the element can allocate buffers itself. Instead, only fail if the pads are flushing and the allocation query failed. https://bugzilla.gnome.org/show_bug.cgi?id=735844 1.4: commit d6ec4cec8acffaac7c2d417ee67bd111d6f15523 Author: Thiago Santos <thiagoss@osg.samsung.com> Date: Fri Sep 5 13:49:46 2014 -0300 basetextoverlay: Do not fail the negotiation if query fails The allocation query failure doesn't mean that the negotiation has failed as the element can allocate buffers itself. Instead, only fail if the pads are flushing and the allocation query failed. https://bugzilla.gnome.org/show_bug.cgi?id=735844 commit 56775ad5bd52fc1ad48bbaf8cac86b17733f6bd6 Author: Arnaud Vrac <avrac@freebox.fr> Date: Thu Jan 31 13:49:00 2013 +0100 basetextoverlay: get framerate from previously parsed video info commit 8e0b8981190a34cdeb1b7ef7ce3882c948ea9c3b Author: Arnaud Vrac <avrac@freebox.fr> Date: Thu Jan 31 13:47:35 2013 +0100 basetextoverlay: do not ask for a bufferpool when checking for composition meta commit a5acd930a2af49a2f606163e3033ea7b945a79f0 Author: Arnaud Vrac <avrac@freebox.fr> Date: Thu Sep 4 15:06:31 2014 +0200 basetextoverlay: schedule reconfigure on source pad when negotiation fails The source pad might be flushing while negotiating, resulting in set_caps or the ALLOCATION query failing. In this case set the reconfigure flag on the source pad so that negotiation is retried on the next buffer.