GNOME Bugzilla – Bug 655719
wayland video sink: initial implementation
Last modified: 2012-03-13 15:53:59 UTC
I have a minimal implementation of gstreamer video sink for wayland.Sink is just creating its own window and rendering into that.(using wayland-shm buffer) I would like to get it in upstream gst-plugins-bad. git://gitorious.org/gst-wayland/gst-wayland.git Wayland is still under heavy development. I followed the instructions in http://wayland.freedesktop.org/building.html to set up wayland . Expecting some opinions from you guys :)
What are the advantages of using a wayland-specific output instead of xvimagesink or glimagesink? I don't mind it going into -bad, especially if it will encourage/enable average people to test the wayland code. Also, please provide a patch.
(In reply to comment #1) > What are the advantages of using a wayland-specific output instead of > xvimagesink or glimagesink? > > I don't mind it going into -bad, especially if it will encourage/enable average > people to test the wayland code. > > Also, please provide a patch. It should have no X-dependency .. As per wayland design wiki, "Wayland is a protocol for a compositor to talk to its clients as well as a C library implementation of that protocol. The compositor can be a standalone display server running on Linux kernel modesetting and evdev input devices, an X application, or a wayland client itself. The clients can be traditional applications, X servers (rootless or fullscreen) or other display servers. EGL is the only GL binding API that lets us avoid dependencies on existing window systems, in particular X.A more subtle point is that libGL.so includes the GLX symbols, so linking to that library will pull in all the X dependencies.The problem with X is that... it's X. When you're an X server there's a tremendous amount of functionality that you must support to claim to speak the X protocol, yet nobody will ever use this." Currently the wayland sink has only s/W decoder support. git://gitorious.org/~sreerenj/vaapi/sree-gstreamer-vaapi.git has the patches for enabling h/W acceleration (libva) to wayland. I will give pull request to Gwenole Beauchesne to merge it with his master.
That's a justification for not requiring X in the display server, and is not a justification for using not-X in the client. Also, gst-plugins-gl can be compiled using EGL, iirc.
I am not sure whether EGL is in gst-plugins-gl or not...But most probably "its there" i think :) . The Wayland architecture integrates the display server, window manager and compositor into one process .And it can act as a toolkit for creating clients and compositors.A not-X client can take compositor, and a new window manager into a single process. Surely ,I am not trying to say that wayland will replace X anytime soon :) Shall I needs to provide the patches or some one(maintainers) will directly take the patches from git://gitorious.org/gst-wayland/gst-wayland.git ? (if you think it can push in to plugins-bad...)
I would have some naming nitpicks: typedef struct _GstWayLandSink GstWayLandSink; only use camer case for separate words. that is just call it GstWaylandSink ("wayland" is one name and not "way land" to my knowledge). in the header you have typedef struct wl_display WlDisplay; typedef struct wl_buffer WlBuffer; and then struct display { ... } struct window { ... } some copy'n'paste bugs? is the wl_buffer thing meant to be the window and is the "wl_" prefix missing on the structs? in the c-file I saw a g_usleep (50000), that I hope you will replace. Always good to add FIXME comments for the things you know that are WIP.
(In reply to comment #5) > I would have some naming nitpicks: > > typedef struct _GstWayLandSink GstWayLandSink; > > only use camer case for separate words. that is just call it GstWaylandSink > ("wayland" is one name and not "way land" to my knowledge). > Yup,You are right.I already got the same comment from Wayland developers:) Thanks for the reminder. > > in the header you have > typedef struct wl_display WlDisplay; > typedef struct wl_buffer WlBuffer; > and then > struct display { ... } > struct window { ... } > I will change the structuring later. > some copy'n'paste bugs? is the wl_buffer thing meant to be the window and is > the "wl_" prefix missing on the structs? > struct wl_surface *surface; struct wl_buffer *buffer; are the needed structures. wl_buffer is the wayland-shm buffer.(To supply them to the compositor) > in the c-file I saw a g_usleep (50000), that I hope you will replace. Always > good to add FIXME comments for the things you know that are WIP. Yup.I put the g_usleep to make sure that there is no dead lock even compositor didn't replied. Which needs to be remove. It is still a prototype to show the video playback in wayland. :) Thanks for the comments.
Created attachment 194992 [details] [review] Initial commit
Created attachment 194993 [details] [review] patch2
Created attachment 194995 [details] [review] patch3
Created attachment 194996 [details] [review] patch4
Created attachment 194997 [details] [review] patch5
Created attachment 194998 [details] [review] patch6
Created attachment 194999 [details] [review] patch7
Created attachment 195000 [details] [review] patch8
Created attachment 195001 [details] [review] patch9
Created attachment 195002 [details] [review] patch10
You should really squash the fixup patches into the ones that introduce the dead code or indentation problems. No need to have this clutter up the history at all.
As Rene proposed, squashing all patches together to 3 and making others as obsolete.
Created attachment 200519 [details] [review] Patch1
Created attachment 200520 [details] [review] patch2
Created attachment 200521 [details] [review] patch3
Adding one more patch which enables the wayland 0.85 (one of the mature release) support to waylandsink.
Created attachment 207809 [details] [review] Fixes to enable wayland 0.85 support
This has been hanging in bugzilla for the last 6 months... ping "maintainers" :)
Created attachment 208620 [details] [review] Fix in shell_surface_set_fullscreen
Review of attachment 200519 [details] [review]: ::: ext/wayland/gstwaylandsink.c @@ +3,3 @@ + * + * Copyright: Intel Corporation + * Copyright: Sreerenj Balachandran <sreerenj.balachandran@intel.com> Please add year like in header file. @@ +21,3 @@ + + +/* The waylandsink is currently just a prototype . It creates its own window and render the decoded video frames to that.*/ this is a good place to have the standard gtk-doc blob (even if the element is not part of the docs yet). @@ +84,3 @@ + +/*Fixme: Add more interfaces */ +GST_BOILERPLATE (GstWayLandSink, gst_wayland_sink, GstVideoSink, according to http://wayland.freedesktop.org/ it is "Wayland" and not "WayLand", "WayLand" would also imply "way_land". @@ +131,3 @@ + "wayland video sink", "Sink/Video", + "Output to wayland surface", + "Sreerenj Balachandran <sreerenj.balachandran@intel.com>,"); drop the ',' at the end @@ +163,3 @@ + g_param_spec_pointer ("wayland-display", "WayLand Display", + "WayLand Display id created by the application ", + G_PARAM_READWRITE)); G_PARAM_READWRITE|G_PARAM_STATIC_STRINGS The display id is a pointer? Call it "display handle" maybe? @@ +165,3 @@ + G_PARAM_READWRITE)); + + /*Fixme: not using now */ Please use a #if 0 then, to keep it compiled out (together with a little comment telling why you'd like to keep it) @@ +227,3 @@ +gst_wayland_sink_dispose (GObject * object) +{ + GstWayLandSink *sink = GST_WAYLAND_SINK (object); don't add an empty dispose method. @@ +237,3 @@ + GstWayLandSink *sink = GST_WAYLAND_SINK (object); + + gst_caps_replace (&sink->caps, NULL); Or move the gst_caps_replace to dispose, its an unref op and those should in theory be in dispose, so that dispose could be called multiple times to break ref-cycles (won't be the case here). Up to you :) @@ +385,3 @@ + + /* We intersect those caps with our template to make sure they are correct */ + intersection = gst_caps_intersect (allowed_caps, caps); if you just want to know that the intersection is non-empty use gst_caps_can_intersect() @@ +543,3 @@ + guint len = GST_BUFFER_SIZE (buffer) / sink->height; + + /*for (i = 0; i < sink->height; i++) { remove? @@ +584,3 @@ + GST_VERSION_MINOR, + "waylandsink", + "WayLand Video Sink", plugin_init, VERSION, "LGPL", "gst-wayland", "") "gst-wayland" -> GST_PACKAGE_ORIGIN
Review of attachment 200520 [details] [review]: ::: ext/wayland/gstwaylandsink.c @@ +134,3 @@ +GType +gst_wlbuffer_get_type (void) +{ Use G_DEFINE_TYPE?
Review of attachment 207809 [details] [review]: ::: ext/wayland/gstwaylandsink.c @@ +594,3 @@ + window->shell_surface = wl_shell_get_shell_surface (display->shell, + window->surface); +// wl_shell_surface_set_toplevel (window->shell_surface); no c++ style comment please
sreerenj, sorry for the duplicated comments above (e.g. WayLand->Wayland). If you could take a look at the other comments and provide a squashed patch, I'll push it.
Stefan: did you mean to push this?
Tim: Do you have any objection for that ? :)
It's fine that it was pushed, but we'd still like the things noted by Stefan fixed. Obviously, patches against current -bad are appropriate at this point.
Hm,,,I didn't see the commit !!.. Thanks Stefan. I will add the changes you mentioned (Hopefully I can make it as a proper element soon :) ). David Schleef :Thanks for the comments,,,, Of course, everything will be on top of current -bad.
Tim, nope, wanted to push the consolidated patch. But I trust sreerenj to come of with a followup to further clean this, thus I'd keep it in if thats okay. Once the next cleanup patch is applied, we should also close this ticket.
Created attachment 209222 [details] [review] waylandsink: Fix warnings, proper structuring, dead code removal, adding doc section.