GNOME Bugzilla – Bug 796772
waylandsink: check for wl_shell unneeded when surface and display are passed by the application
Last modified: 2018-07-25 12:40:04 UTC
Created attachment 372985 [details] [review] patch for downgrade wl_shell check. I have an application that uses waylandsink and passed the surface and display handle to the waylandsink. When I use waylandsink in weston/fullscreen-shell waylandsink fails to check for wl_shell protocol extension. But this is not needed because the surface is properly configured by the application (via zwp_fullscreen_shell). If I comment out the wl_shell check and my scenario start working. Best Matteo Valdina
Review of attachment 372985 [details] [review]: Can you provide a small application that demonstrate this ? ::: ext/wayland/wldisplay.c @@ +354,3 @@ + if (!self->shell) { + g_warning ("Could not bind to wp_shell"); you mean wl ?
Yes, sorry my bad. It should be wl_shell. About the small demonstration app, I'll try but it is not easy. To run a Wayland client in these scenario requires a bunch of code. You also need to run the weston compositor with the fullscreen-shell.so
Ok, running weston with that is not a problem for me. What I'm trying to figure-out is if there is any corner case that I may be missing that would leave the pipeline dangling without appropriate error. I'm also wondering, why not add support for the scenario instead, so that gst-launch works without wl_shell ?
> I'm also wondering, why not add support for the scenario instead, so that > gst-launch works without wl_shell ? The point of relaxing the wl_shell constrains is to allow the application to run in a Wayland compositor that is not providing wl_shell (for example fullscreen-shell) and let the application to deal with it. In this way the application developer can use waylandsink in other wayland compositor or custom compositor. I know that there are couples of shells that likes wl_shell, for example, fullscreen_shell, xdg_shell, and ivi_shell. This is different from adding the support of fullscreen_shell that it is more for gst-launch or simpler application. Anyway, the support of fullscreen_shell is an easy task. I'll try to wrap-up a patch for that. About the corner case for wl_shell I see that the error condition is handled like a failure of wl_shell_surface.
Created attachment 372992 [details] [review] relaxed wl_shell check and zwp_fullscreen_shell support Here an updated patch with the relaxed wl_shell check and zwp_fullscreen_shell. I tested with these commands: weston -Bwayland-backend.so --shell=fullscreen-shell.so --no-config --use-pixman -Swayland-1 and gst-launch-1.0 videotestsrc ! waylandsink display=wayland-1
Hi, Any updates/comments on the patch? Best Matteo
There is conflict when applying on git master, can your rebase and re-submit please ?
I've manually merge the thing, which is scary, so please do rebase and re-submit. From the testing: gst-launch-1.0 videotestsrc ! waylandsink display=wayland-1 Ends up showing a centered 320x240 video. I was expecting the video to cover the screen, I think more work is needed, weston can scale the video and should in this case imho.
Hi, I'll rebase it not a problem. I worked on the 1.14 branch. About the center, it depends on the arguments passed to the fullscreen_shell. zwp_fullscreen_shell_v1_present_surface (display->fullscreen_shell, + window->area_surface, ZWP_FULLSCREEN_SHELL_V1_PRESENT_METHOD_DEFAULT, + NULL); I pass ZWP_FULLSCREEN_SHELL_V1_PRESENT_METHOD_DEFAULT that it is the default behavior but you can pass ZWP_FULLSCREEN_SHELL_V1_PRESENT_METHOD_ZOOM to zoom the view to fullscreen.
(In reply to Matteo Valdina from comment #9) > I pass ZWP_FULLSCREEN_SHELL_V1_PRESENT_METHOD_DEFAULT that it is the default > behavior but you can pass ZWP_FULLSCREEN_SHELL_V1_PRESENT_METHOD_ZOOM to > zoom the view to fullscreen. I think for video rendering, the zoom method is the one that makes the most sense. The GstVideoOverlay set_render_rectangle() method should deal with the case you want to place the video elsewhere.
Created attachment 373150 [details] [review] patch for downgrade wl_shell check and zwp_fullscreen_shell support. - Rebased GStreamer master. - Changed rendering to fullscreen (zoom) instead of default mode.
Thanks, I'll do a final check tomorrow and hopefully merge.
See commit e0535b44d4dceb3554dd67ff0402730d263e2e04
Thanks, for the quick review. The GitHub mirror is still maintained? I don't have access/view on gitlab.
The GitHub mirror is read-only, it will update itself eventually (it's automated). The gitlab migration is being delayed / held at the moment. The official repository is still on git.freedesktop.org