GNOME Bugzilla – Bug 755454
glimagesink: always send a reconfigure event when starting
Last modified: 2018-11-03 11:41:54 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.
Why does it go to 1x1 at all?
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 !
Because of: width = MAX (1, width); height = MAX (1, height); In gst_glimage_sink_on_resize
That seems like the real problem to me then, it shouldn't just go to a random 1x1 :)
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.
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.
(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 :)
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).
Does the attached patch still fix the issue ?
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
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 ?
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.
The v4l2src part was a regression, there was already code to avoid the reallocation, but I broke it recently.
(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.
Should we push both attached patches ?
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 ?
-- 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.