Daniel Stone 2018-03-02 16:30:35 UTC
Sorry for chiming in so late here.

Firstly, get_{depth,bpp}_from_format are wrong. There is a very very small and fixed depth/bpp -> format mapping inside the kernel, which is pretty much:
  * 8/8 -> C8
  * 15/16 -> XRGB1555
  * 16/16 -> RGB565
  * 24/24 -> RGB888
  * 24/32 -> XRGB8888
  * 30/32 -> XRGB2101010
  * 32/32 -> ARGB8888

Anything else will silently be assumed to be XRGB8888.

So you should delete all but those formats, but _also_ preferentially use drmModeAddFB2, for which you can just pass a format directly. (GBM_FORMAT_* == DRM_FORMAT_*, so no need to add a mapping; you can rely on this and it is both API and ABI guaranteed.) Then you can just use drmModeAddFB as a fallback for the above formats only.

You can also delete the GBM_BO_FORMAT_* handling inside those two functions, as gbm_bo_get_format() only ever returns GBM_FORMAT_* enums. Very intuitive, I know.

I don't quite follow what the comment in gst_gl_window_gbm_egl_set_window_handle() means (wrt external objects and DRM pageflips), but I'm happy to try to answer if you can elaborate.

As a final nitpick, GBM is not Mesa-specific: it's also implemented by the Imagination, ARM, and Vivante proprietary drivers. Those tend to advertise EGL_KHR_platform_gbm rather than EGL_MESA_platform_gbm in the extension string.
[reply] [−] Comment 23 Nicolas Dufresne (stormer) [developer] 2018-03-02 16:53:53 UTC
Hi Daniel, as this code is merged, can you file a separate bug for these errors, and specify which files are affect ?
[reply] [−] Comment 24 Carlos Rafael Giani [reporter] 2018-03-02 17:08:16 UTC
Daniel: set_window_handle is part of the GstVideoOverlay functionality. This is how you can embed a GStreamer video sink inside, say, a Gtk interface. set_window_handle() is then called to supply the code with an existing window handle. What this handle is, is platform specific. In X, it is a Window id.

Since I do not see why anybody would want to embed GBM buffer objects anywhere, I added that comment. I wrote the GBM support with a single fullscreen video output in mind, just like the Vivante FB support.

As for the rest, thanks for the info! I have one remaining question: do you see anything wrong with how the BOs and the page flipping are done in draw_cb() ? I think this is proper triple buffering. Do you?

(In reply to Daniel Stone from comment #1)
> Daniel: set_window_handle is part of the GstVideoOverlay functionality. This
> is how you can embed a GStreamer video sink inside, say, a Gtk interface.
> set_window_handle() is then called to supply the code with an existing
> window handle. What this handle is, is platform specific. In X, it is a
> Window id.
> 
> Since I do not see why anybody would want to embed GBM buffer objects
> anywhere, I added that comment. I wrote the GBM support with a single
> fullscreen video output in mind, just like the Vivante FB support.

Thanks for the explanation! That makes sense to me.

> As for the rest, thanks for the info! I have one remaining question: do you
> see anything wrong with how the BOs and the page flipping are done in
> draw_cb() ? I think this is proper triple buffering. Do you?

I had a look at it, and I think the way you've done it is fine, yeah.

(In reply to Daniel Stone from comment #1)
> Daniel Stone 2018-03-02 16:30:35 UTC
> Sorry for chiming in so late here.
> 
> Firstly, get_{depth,bpp}_from_format are wrong. There is a very very small
> and fixed depth/bpp -> format mapping inside the kernel, which is pretty
> much:
>   * 8/8 -> C8
>   * 15/16 -> XRGB1555
>   * 16/16 -> RGB565
>   * 24/24 -> RGB888
>   * 24/32 -> XRGB8888
>   * 30/32 -> XRGB2101010
>   * 32/32 -> ARGB8888
> 
> Anything else will silently be assumed to be XRGB8888.

Is this also true for RGB332, BGR233, and NV12 ?

Created attachment 369272 [details] [review]
gbm window initialization patch

This patch is a response to https://bugzilla.gnome.org/show_bug.cgi?id=782923#c16 . It changes the GBM surface creation to match the way other window systems initialize their windows.

Daniel, does this look good to be merged now?

(In reply to Sebastian Dröge (slomo) from comment #5)
> Daniel, does this look good to be merged now?

Yes, with the caveat that I have ~no idea how that particular part of GStreamer works.

commit 9324ada729bcbf7da46de6d9f7c96af26f9bed74 (HEAD -> master)
Author: Carlos Rafael Giani <dv@pseudoterminal.org>
Date:   Sun Mar 4 16:41:14 2018 +0100

    gl/gbm: Initialize window handle (= gbm surface) like other window systems
    
    https://bugzilla.gnome.org/show_bug.cgi?id=793997

Thanks, it looks sensible to me in that regard.

There are also other things left to be fixed here from what I understand, so let's keep this bug open :)

Daniel, in comment 3 there was a question for you

(In reply to Carlos Rafael Giani from comment #3)
> (In reply to Daniel Stone from comment #1)
> > Daniel Stone 2018-03-02 16:30:35 UTC
> > Firstly, get_{depth,bpp}_from_format are wrong. There is a very very small
> > and fixed depth/bpp -> format mapping inside the kernel, which is pretty
> > much:
> >   * 8/8 -> C8
> >   * 15/16 -> XRGB1555
> >   * 16/16 -> RGB565
> >   * 24/24 -> RGB888
> >   * 24/32 -> XRGB8888
> >   * 30/32 -> XRGB2101010
> >   * 32/32 -> ARGB8888
> > 
> > Anything else will silently be assumed to be XRGB8888.
> 
> Is this also true for RGB332, BGR233, and NV12 ?

Oops. And yes: when you provide depth/bpp to drmModeAddFB, the above is the complete list of formats which work with it. No other format can be used with that ioctl.

-- 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/422.