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 755454 - glimagesink: always send a reconfigure event when starting
glimagesink: always send a reconfigure event when starting
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-base
git master
Other All
: Normal normal
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2015-09-23 08:39 UTC by Julien Isorce
Modified: 2018-11-03 11:41 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
glimagesink: avoid duplicated reconfigure event (840 bytes, patch)
2015-09-23 08:45 UTC, Julien Isorce
none Details | Review
gtkglsink: Don't initially send a reconfigure (1.60 KB, patch)
2017-09-06 15:09 UTC, Nicolas Dufresne (ndufresne)
none Details | Review

Description Julien Isorce 2015-09-23 08:39:55 UTC
Not sure for how long this problem is there, but it always resize the window to 1x1 and then resize to the desired size.
To from 0x0 to 1x1 does not triggered a reconfigure event. As initial values are 0x0.
But change from 1x1 to 320x240 triggers a reconfigure event, Which potentially deactivate /reactive upstream pools which adds unnecessary delays.

Patch follows.
Comment 1 Sebastian Dröge (slomo) 2015-09-23 08:44:22 UTC
Why does it go to 1x1 at all?
Comment 2 Julien Isorce 2015-09-23 08:45:03 UTC
Created attachment 311928 [details] [review]
glimagesink: avoid duplicated reconfigure event

Not sure this is the best way to fix it. An other way is in gst_gl_window_x11_draw_unlocked, if gst_gl_window_get_surface_dimensions return 0x0 then do not call gst_gl_window_resize. But then it requires to duplicate this in other places like in gst_gl_window_wayland_egl.
Let me know !
Comment 3 Julien Isorce 2015-09-23 08:45:49 UTC
Because of:

width = MAX (1, width);
height = MAX (1, height);

In gst_glimage_sink_on_resize
Comment 4 Sebastian Dröge (slomo) 2015-09-23 08:58:26 UTC
That seems like the real problem to me then, it shouldn't just go to a random 1x1 :)
Comment 5 Matthew Waters (ystreet00) 2015-09-23 09:05:32 UTC
The MIN (1, width/height) is needed because you'll otherwise get errors on GLX creating 0x0 sized buffers.  If we decide to simply ignore all 0x0 resizes that could work as well however we need to be careful to keep track of state correctly with the early return.
Comment 6 Nicolas Dufresne (ndufresne) 2015-09-23 13:51:06 UTC
And the reconfigure is needed to communicate the window size, so unless we can ensure the window is created before we are negotiated, we will need this renegotiation.

The bug is also that upstream reallocate everything, but currently reconfigure event is just too generic. We need a way to tell upstream why we want to reconfigure. In this case it's for the the window size in the CompositonOverlayMeta params. Most upstream element should be able to ignore that.
Comment 7 Julien Isorce 2015-09-23 13:59:59 UTC
(In reply to Nicolas Dufresne (stormer) from comment #6)
> And the reconfigure is needed to communicate the window size, so unless we

If I am not mistaken the reconfigure event in that case is only needed if the window size changed. The window is initially created with size of the input video stream. So initial on_resize I think it is just there to set the viewport (the surface being created already with the right size).

At least in the case videotestsrc ! glimagesink there is no need for the reconfigure event to be sent. Which is sent without attached patch.
Then if the user resize the window it will trigger reconfigure event, which is wanted.

But maybe I am missing something :)
Comment 8 Nicolas Dufresne (ndufresne) 2015-09-23 14:08:18 UTC
Well, as long as the window manage does not decide otherwise, but yeah, this approach make sense (as long as later update induced by the window manager are reflected).
Comment 9 Julien Isorce 2017-07-28 20:28:09 UTC
Does the attached patch still fix the issue ?
Comment 10 Julien Isorce 2017-09-06 14:00:36 UTC
The issue can be reproduced with playbin video-sink=glimagesink (not with videotestsrc)

* Without patch:
  
-- glbufferpoolA create
-- glbufferpoolA config
-- glbufferpoolA start
-- glbufferpoolA alloc
-- glbufferpoolA alloc
Pipeline is PREROLLED ...
Setting pipeline to PLAYING ...
New clock: GstPulseSinkClock
-- glbufferpoolB create
-- glbufferpoolB config
-- glbufferpoolA finalize
-- glbufferpoolB start
-- glbufferpoolB alloc
-- glbufferpoolB alloc


* With attached patch:

-- glbufferpoolA create
-- glbufferpoolA config
-- glbufferpoolA start
-- glbufferpoolA alloc
-- glbufferpoolA alloc
Pipeline is PREROLLED ...
Setting pipeline to PLAYING ...
New clock: GstPulseSinkClock
Comment 11 Nicolas Dufresne (ndufresne) 2017-09-06 14:50:49 UTC
That patch seems to do the job for me with:

  gst-launch-1.0 v4l2src ! glimagesink

The camera will no longer re-alloc (hence restart) until the window is resized. Though it seems this pattern need fixing in other sinks, like this one:

  gst-launch-1.0 v4l2src ! glsinkbin sink=gtkglsink

Matthew, is that patch safe though ?
Comment 12 Nicolas Dufresne (ndufresne) 2017-09-06 15:09:23 UTC
Created attachment 359277 [details] [review]
gtkglsink: Don't initially send a reconfigure

This prevent the very first resize (when the widget is created) to
result in a renegotiation.

This one though is a bit of a hack, though the interesting aspect is that it
plays on the action of sending the reconfigure event, rather then ignoring
bunch more actions.

While this work for the common cases, I'm not sure if this is the right
approach. I still see the camera restarting even if camera resolution isn't
changed.
Comment 13 Nicolas Dufresne (ndufresne) 2017-09-06 15:28:42 UTC
The v4l2src part was a regression, there was already code to avoid the reallocation, but I broke it recently.
Comment 14 Matthew Waters (ystreet00) 2017-09-07 04:29:40 UTC
(In reply to Nicolas Dufresne (stormer) from comment #11)
> That patch seems to do the job for me with:
> 
>   gst-launch-1.0 v4l2src ! glimagesink
> 
> Matthew, is that patch safe though ?

As I said in comment 5, we need to ensure that the state tracking is still sane on all winsys'.  It seems like it's a good idea though.
Comment 15 Julien Isorce 2018-05-15 02:39:57 UTC
Should we push both attached patches ?
Comment 16 Nicolas Dufresne (ndufresne) 2018-05-15 10:11:15 UTC
These two were completly forgotten. Yours, for sure, mine is less trivial, can you review the code again, just make sure I'm not doing something silly ?
Comment 17 GStreamer system administrator 2018-11-03 11:41:54 UTC
-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/issues/227.