wxMediaCtrl wayland support

[martinetd]

wxMediaCtrl wayland support in #2257 seems to be incomplete.
In particular, the mediaplayer does not work under wxgtk3 with wayland (unset DISPLAY to make sure) - the most obvious thing that happens is the video stays black while sound plays, and the console gets filled with "Failed to flush Wayland connection" messages.
That problem is fixed in the first commit:

gstreamer creates a new connection to the wayland display by default, and
gst_video_overlay_set_window_handle() only works if both the parent surface
(part of the gtk window) and the gstreamer surface are on the same display,
so we need to tell gstreamer about our wl_display when it asks

(see https://gitlab.freedesktop.org/gstreamer/gst-plugins-base/-/issues/635 for details as I could reproduce this with a simpler client)

With that fixed the video was being drawn on the full window, the second commit sets where it is drawn only for gtk+wayland.
Tested on X11, weston and sway.

Please tell me if there are things you'd like fixed or concerns.

cc @varigigi (not sure if watching)

[vadz]

Thanks for fixing this and sorry for the delay with the reply!

I don't know anything about Wayland, but if this makes it work, let's merge it, of course. I'm a bit surprised because I thought that @varigigi did test it with Wayland, but maybe I misunderstood something.

Anyhow, I'll merge this (maybe with some slight reformatting) soon if there are no comments from @paulcor.

Thanks again!

[martinetd]

No worry about the delay, I wasn't sure about the process so I opened a trac ticket yesterday but I'll close it when this is merged :)

I was also a bit surprised it didn't work, maybe it depends on the gstreamer version? I only tested with 1.0 (debian bullseye 1.18.4-2)

[varigigi]

@vadz, @martinetd, when I was testing wxwidgets on wayland, I was not able to fully test the mediactr: it was working fine with x11 and xwayland, but only partially with pure wayland with no x11 support.
At that time, I assumed it was a problem in my Gstreamer customizations, but following this thread, now I'm not so sure.
The patches proposed @martinetd fix the rendering problems with the demo webview.
Probably some additional work may still be needed maybe in mediactrl.cpp too: whenever I try to run the mediaplayer demo, it complains with the message "Could not find a suitable video sink", even if I can actually play any video using gstreamer pipeline from the command line.
My feeling is that the method TryVideoSink only work if x11 support is enabled, but I could be wrong.
Once again, everything works fine if x11 support is enabled.

[martinetd]

Thanks for the comment! that clears it up a bit.

I could get mediaplayer to work without Xwayland on my laptop with the autovideosink, so there might be something to look at - can you paste what you get with GST_DEBUG=3 to get an idea of what it tries?

[varigigi]

This is what I'm getting:

root@imx8mm-var-dart:/usr/share/wxwidgets/samples/mediaplayer# GST_DEBUG=3 ./mediaplayer
0:00:00.035652074 4201 0xaaaac568a0f0 WARN GST_ELEMENT_FACTORY gstelementfactory.c:462:gst_element_factory_make: no such element factory "xvimagesink"!
0:00:00.035732326 4201 0xaaaac568a0f0 WARN GST_ELEMENT_FACTORY gstelementfactory.c:462:gst_element_factory_make: no such element factory "ximagesink"!
mediaplayer.cpp(1504): assert "bOK" failed in wxMediaPlayerNotebookPage(): Could not create media control!

It looks it does not even try the autovideosink that is actually present:

root@imx8mm-var-dart:/usr/share/wxwidgets/samples/mediaplayer# gst-inspect-1.0 | grep -i sink | grep -i video
fbdevsink: fbdevsink: fbdev video sink
kms: kmssink: KMS video sink
autodetect: autovideosink: Auto video sink
video4linux2: v4l2sink: Video (video4linux2) Sink
decklink: decklinkvideosink: Decklink Video Sink
waylandsink: waylandsink: wayland video sink
inter: intervideosink: Internal video sink
opengl: glimagesinkelement: OpenGL video sink
vulkan: vulkansink: Vulkan video sink
debugutilsbad: fpsdisplaysink: Measure and show framerate on videosink
debugutilsbad: fakevideosink: Fake Video Sink

[martinetd]

oh an imx board.. I also couldn't get gstreamer to work well with one on a frankendebian and was blaming my hacked up install, will give it a shot with a slightly better setup on Monday -- I'll report if I find something.
No trace of autovideosink is weird, even if TryVideoSink() fails we inconditionally call gst_element_factory_make() with it so I believe it should log something? might need to add a couple of printfs to understand what it's doing :/

[varigigi]

I've just realized that you patched the file src/unix/mediactrl_gstplayer.cpp
This means that wxUSE_GSTREAMER_PLAYER is enabled in your build, but not in mine.
Can you share your configure options?

[varigigi]

Never mind, please just ignore my previous message.
I cleaned up all my local changes from gstreamer and wxwidgets, and wxUSE_GSTREAMER_PLAYER is now enabled.
However it crashes calling the method gst_element_set_context in the bus_sync_handler function.
The following patch fixes the crash

@@ -241,9 +240,11 @@ static GstBusSyncReply bus_sync_handler(GstBus * WXUNUSED(bus), GstMessage* msg,
         gst_message_parse_context_type(msg, &type) &&
         !g_strcmp0 (type, GST_WAYLAND_DISPLAY_HANDLE_CONTEXT_TYPE))
     {
+        GdkDisplay *display = gtk_widget_get_display (be->GetControl()->m_wxwindow);
+        struct wl_display *display_handle = gdk_wayland_display_get_wl_display (display);
         GstContext *context = gst_context_new (GST_WAYLAND_DISPLAY_HANDLE_CONTEXT_TYPE, TRUE);
         GstStructure *s = gst_context_writable_structure(context);
-        gst_structure_set(s, "display", G_TYPE_POINTER, be->m_display_info.dpy, NULL);
+        gst_structure_set(s, "handle", G_TYPE_POINTER, display_handle, NULL);
         gst_element_set_context(GST_ELEMENT(msg->src), context);
 
         return GST_BUS_DROP;

but I've just derived it from the official gst-plugins-bad 1.16 code
https://github.com/GStreamer/gst-plugins-bad/blob/1.16/tests/examples/waylandsink/main.c#L94
https://github.com/GStreamer/gst-plugins-bad/blob/1.16/gst-libs/gst/wayland/wayland.c#L44
However, even if the crash is gone, the videos do not play.
Any suggestion?

[martinetd]

Thanks for keeping at it.
For bus_sync_handler, my impression was that the mediactrl could work with non-gtk3 backends so wanted to use something non-gtk-dependant, will have to look for a fallback or why it works for me here... I'm afraid I don't know why the video still doesn't play with that fixed :/

I've got some more urgent work planned for this morning but hopefully will be done before the end of the day and come back to this PR ASAP, I need to see this work on imx anyway :)

[varigigi]

I think you are right, the gtk functions should not be present in this portion of the code, or at least they should be wrapped in a

#ifdef __WXGTK3__
        ...
#endif

Please let me know if you need me to perform specific tests.

[martinetd]

Please let me know if you need me to perform specific tests.

I just got started on my board (imx8mp by the way, but hopefully it's close enough).
Funnily enough I ran into the exact same problem where only xvimage and ximage sinks were tried, and certainly just because I didn't have wxUSE_GSTREAMER_PLAYER... I assume we'd want to make that work too at some point :|
Adding gstreamer1.0-plugins-bad or imx-gst1.0-plugin to they package sysroot fixes that issue as you likely found out... So I could reproduce your segfault... expanding on it below.

I think you are right, the gtk functions should not be present in this portion of the code, or at least they should be wrapped in a

#ifdef __WXGTK3__
        ...
#endif

Well, that's fine, but it won't work without an equivalent code path without gtk -- gstreamer on wayland needs to be given a pointer to the wayland display at some point or creates a new connection, which won't allow subsurfaces to be created.
More to the point, be->m_display_info.dpy comes from wxGetDisplayInfo() which seems to do the right thing for all backends, and for gtk3 it'll be gdk_wayland_display_get_wl_display() under wayland alright. I don't know why it wasn't called directly in the bus handler, I'll move that over there as m_display_info has no other use.

The problem seems to be setting either handle or display -- on my laptop if I set handle, gstreamer doesn't seem to get a hold of the display at all.
Looking in more details, it seems to be sink specific...
gst-plugins-bad/ext/wayland/gstwaylandsink.c uses gst_wayland_display_handle_context_get_handle in gst-plugins-bad/gst-libs/gst/wayland/wayland.c which gets "handle" -- it also doesn't check the return value of gst_structure_get, so fills in random junk in display and segfault when it tries to use it later.
but my laptop uses gst-plugins-base/ext/gl/gstglimagesink.c which calls gst_gl_handle_set_context in gst-plugins-base/gst-libs/gst/gl/gstglutils.c which looks for "display" . . . . I didn't think I could dislike gstreamer more than I already did, but consider it done!

I'm not sure what we can do to please both worlds.
For starters I'll send gstreamer a PR for the bad plugin to check the return value of gst_wayland_display_handle_context_get_handle at the very least... Could make it look for both variable for compatibility?

But short term we've got the short handle of the stick, I'll try to look into why the auto sink didn't pick the "good" gstglimagesink on the imx board, but I'm afraid it might not be able to use it...
And different sinks mean different problems for playback hence your video not showing, I'll look at that tomorrow it should be easy to reproduce now.

[varigigi]

Being wayland only supported via Linux+GTK3, I was actually suggesting to wrap all the changes, not just the internal handler calls.
Just to be aligned, which iMX8MP BSP are you working with?
I'm using Yocto Dunfel from FSLC community...

[martinetd]

wayland is only supported via gtk3 right now but ultimately it could work with others right? e.g. there's no reason the qt backend wouldn't work with wayland, but I have no idea if it's compatible with mediactrl (I suppose gstreamer should work as long as we give it somewhere to draw on, but not sure how compatible the event loops would be...)

I'm using whatever is in their lf-5.10.y-1.0.0 release right now -- it looks like gatesgarth. The plan will be to move to debian however (with quite some pain involved around the proprietary blobs..) so this is really just for testing.

Anyway:

• https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/-/merge_requests/2292 open for the plugin (although we won't be able to rely on it, I've used the occasion to ask a bunch of questions so maybe someone who understands this a bit more will help)
• taking a look at why the gl variants of the sinks don't get used now...

[martinetd]

Ok so, progress!
I've just pushed an extra three patches:

• the first one just sets both "handle" and "display" attributes as discussed. The PR I opened for the wayland sink will likely get merged so we should be ok to only set display in the long term - but if we want to handle older gstreamer versions with that plugin we should set both for a while. I can add a comment about removing handle later if that helps.
• the second commits fixes the wayland sink for me on my laptop (tested with GST_PLUGIN_FEATURE_RANK=waylandsink:257 to make it use the wayland sink) -- they apparently require to set the render rectangle earlier, it's probably a good thing.
• things worked like that, but the Move helper really seemed more appropriate than the draw callback for moving the window, so I just went ahead with that.

Might need some light cleanup & comments added but I think we're good for gstplayer now -- @varigigi if you have time would you mind testing on the imx board again for wayland sink there? I'll probably get around to it in the next few days but I think you'll be faster

Then there's the matter of the non-gstplayer variant, I honestly didn't look at it yet, but I assume it'd require similar patches. What do we want to do about it?

[varigigi]

Great job! the mediaplayer is fluently using the video accelerations from Gstreamer in Wayland:

• SoC: NXP iMX8M-MINI from Variscite DART-MX8M-MINI
• display: LVDS 800x480
• video: BigBuckBunny.mp4 (H264) 1280x720
• rescaling + H264 decoding: the CPU around 12%

Looking forward to see it in WxWidgets 3.1.6
Thanks for your valuable effort

[martinetd]

@varigigi thanks for testing! I also confirmed just now. I know we're not testing the evk but hardware decoding is indeed impressive, I'm getting 20% cpu (+8% from weston) with 1080p@30fps bbb resized to almost full screen on a 4k display -- there's no way that'd work without acceleration :)

@vadz I've rebased, added comments, and squashed the fixes so we're back to two logically separated commits (no code change from what @varigigi tested)
I think we're good on my end, please ask if there's anything you'd like to adjust.

the non-gstplayer variant of wxMediaCtrl would likely need similar fixes but I won't have time to look at it right now and this is probably worth getting as is already.

[vadz]

Thanks again!

I don't really know anything about the actual functions used by this code, but I'm quite ready to merge this without looking into this if it works for you and others. The only uncomfortable point is unsetting DISPLAY globally, if we could at least do it locally/temporarily, it would alleviate 99% of my concerns. If not, I guess we'll just have to leave with my concerns and merge it nevertheless...

[vadz]

Sorry, this is not really related to this PR and shouldn't be done in it, but I just wanted to mention that if anybody is interested in copying wxGtkObject from include/wx/gtk/private/object.h to some wxGstObject and using it here, it would be welcome!

[martinetd]

We're getting closer...!

Now my last qualm is about positioning. I didn't notice it earlier with the code going through resize on every frame (expose_event_callback), but now I'm doing it in Move it's obvious there's a difference in behaviour with X11 if the window isn't resized.

• gtk X11: the video extends over all the window below the notebook tab
• wayland, initial view: the video is where mediaplayer.cpp requested it (on the black box when no video is playing), which is what I had been expecting all along...
• wayland, after remove: the video spreads over all the window below the notebook tab like X11

Just to make sure I tried to make realize get the widget dimension through m_ctrl->m_wxwindow like I do in the Move handler and the dimension really changed at some point: if I log the size obtained that way in the expose event callback, I get the same x/y but the width/height change after two events:

got size 34 131 578 396
got size 34 131 578 396
(mediaplayer:2254974): Gtk-WARNING **: 15:29:25.173: Negative content height -4 (allocation 20, extents 12x12) while allocating gadget (node scale, owner GtkScale)
(mediaplayer:2254974): Gtk-WARNING **: 15:29:25.173: Negative content height -2 (allocation 0, extents 1x1) while allocating gadget (node trough, owner GtkScale)
(mediaplayer:2254974): Gtk-WARNING **: 15:29:25.173: Negative content height -4 (allocation 20, extents 12x12) while allocating gadget (node scale, owner GtkScale)
(mediaplayer:2254974): Gtk-WARNING **: 15:29:25.173: Negative content height -2 (allocation 0, extents 1x1) while allocating gadget (node trough, owner GtkScale)
got size 34 131 586 500

I'm not sure how gst_player_video_overlay_video_renderer_set_render_rectangle is expensive but a "solution" would be to move it back to expose_event_callback like I did before, I've done that for now in a separate commit, but it's really called a few times per second (not once per frame though) so it's probably not great...

So:

• do we want the size of the video to change like it actually does? if not X11 will need fixing.
• can we somehow catch this and only resize in that case (e.g. compare video size not to 0 but to previous size or something)?

in both cases figuring where that change comes from might help...

That being said I guess we've got enough improvements for one iteration, can always look at improving further later.

[vadz]

Thanks again for another update!

I don't understand why did it still not run the GitHub CI builds for it even though I had already approved running them, but I've just done it again. If they pass (and I see no reason they shouldn't), I'm perfectly fine with merging this as is.

If I understand you correctly (sorry, still didn't have time to actually test it myself), the positioning is indeed a bug, but as long as it's the same bug with X11 and Wayland, we could probably live with it -- even though, of course, it would be nice to fix it in a later PR.

[martinetd]

Ok, let's get this in and I'll take a look at positioning as time permits, it doesn't strike me as normal but we're indeed aligned with X11 and wayland now.
Once that's fixed I'll revert my last commit from this PR and live happily ever after -- hopefully :-D

[vadz]

Great, thanks again for your work on this, Dominique!

[swt2c]

👍 Thanks a lot for your work @martinetd. I had been trying for some time to get wxMediaCtrl working with Wayland, so I'm glad to see it happen!

[martinetd]

Great, if there are other users it's less likely to break when I'm not looking :D Thanks for the tests and review to you three.

I need to fiddle with some other things for a while, but I'll come back for the resize issue definitely.