X11: fix usage of required configuration values #15148

AlwinEsch · 2018-12-27T23:33:11Z

Description

Before has Kodi checked only the EGL_NATIVE_VISUAL_ID and ignored
others. With this was e.g. on some addons the needed EGL_DEPTH_SIZE
not available.

This separates a part of IsSuitableVisual(...) into a separate function
(SuitableCheck(...)) which is checked by GetEGLConfig(...) and takes the
necessary configuration from the list.

Motivation and Context

Sit for a while because the addons which use GL to switch to the 4.0 version so that they work again and was almost senseless.

Thought all the time to have done something wrong in the rework and now found that the error lies in Kodi.

In addition, I found in the end that it was with the override KODI_GL_INTERFACE=GLX ./Kodi-x11 went and finally found that the error lies in Kodi itself.

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Bug fix (non-breaking change which fixes an issue)
Clean up (non-breaking change which removes non-working, unmaintained functionality)
Improvement (non-breaking change which improves existing functionality)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that will cause existing functionality to change)
Cosmetic change (non-breaking change that doesn't touch code)
None of the above (please explain below)

Checklist:

My code follows the Code Guidelines of this project
My change requires a change to the documentation, either Doxygen or wiki
I have updated the documentation accordingly
I have read the Contributing document
I have added tests to cover my change
All new and existing tests passed

xbmc/windowing/X11/GLContextEGL.cpp

yol · 2019-01-04T19:10:20Z

@AlwinEsch Can you make the changes? IMO this should go into v18 (@MartijnKaijser) so we have working viz and screen savers on X11.

AlwinEsch · 2019-01-05T20:04:09Z

Is updated.

Sit still to change the addons, unbelievably complex to convert these to the shader type :-| .

yol · 2019-01-06T13:10:39Z

@FernetMenta Can you comment on why there is this custom visual selection code in GetEGLConfig (that - strictly speaking - misses some checks such as EGL_RENDERABLE_TYPE and EGL_SURFACE_TYPE) as opposed to using eglChooseConfig like in the PB code?

FernetMenta · 2019-01-07T10:41:52Z

@pkerling when selecting a config for rendering to the main window GetEGLConfig returns a config that matches the native visual. Such a config has all the attributes required by Kodi core.

yol · 2019-01-07T21:54:17Z

when selecting a config for rendering to the main window GetEGLConfig returns a config that matches the native visual. Such a config has all the attributes required by Kodi core.

I believe you that it does (after all it is working fine so far), but that does not really answer my question :-) Which is why eglChooseConfig is basically reimplemented here but without the priority logic and ignoring important bits such as EGL_RENDERABLE_TYPE and EGL_SURFACE_TYPE. Theoretically, you might get a config that is only suitable for GLES. And if the first config in the list that has the right visual does not fit the other criteria, we currently just bail out instead of looking for a fitting one. Fortunately, I think you will usually get a window-surface-/GL-capable RGBA8888 config first so it hasn't been a problem.

xbmc/windowing/X11/GLContextEGL.cpp

AlwinEsch · 2019-01-10T17:30:16Z

I had to add a second commit. When switching I noticed another error.

Some addons need the stuff described above, but they have always been removed by Kodi with glClear.

AlwinEsch · 2019-01-10T17:45:41Z

Here the difference between of second commit (screensaver.rsxs.solarwinds):

Before:

After:

The shadow behind

yol

Few minors then I think we are good to go

xbmc/windowing/X11/GLContextEGL.h

xbmc/windowing/X11/GLContextEGL.cpp

yol · 2019-01-22T09:44:16Z

xbmc/windowing/X11/GLContextEGL.cpp

+}
+
+bool CGLContextEGL::SuitableCheck(EGLDisplay& eglDisplay, EGLConfig& config)
+{
  if (config == EGL_NO_CONFIG)


You can move this block to IsSuitableVisual

Your mean to let the

if (config == EGL_NO_CONFIG) { CLog::Log(LOGERROR, "Failed to determine egl config for visual info"); return false; }

away?

And:

bool CGLContextEGL::IsSuitableVisual(XVisualInfo *vInfo) { EGLConfig config = GetEGLConfig(m_eglDisplay, vInfo); if (config == EGL_NO_CONFIG) { CLog::Log(LOGERROR, "Failed to determine egl config for visual info"); return false; } return true; }

If yes makes not direct sense, before was it only to report a failed GetEGLConfig and to prevent the further check. But now becomes everything done inside the GetEGLConfig and can report EGL_NO_CONFIG frequently.

Why should GetEGLConfig frequently report EGL_NO_CONFIG? In that case we will not get any display

I have removed the IsSuitableVisual call complete, where this becomes called is the GetEGLConfig called directly after.

Now think that it is a remnant of the conversion of the old CGLContextGLX where this call was necessary.

The CLog::Log(LOGERROR, "Failed to determine egl config for visual info"); should never come, only for the worst case where a "eglGetConfigs" call returns true with a empty entry in list.

Point still stands: This function is never called with EGL_NO_CONFIG, so having the log message makes no sense

AlwinEsch · 2019-01-26T08:54:20Z

jenkins build this please

AlwinEsch · 2019-01-29T11:57:25Z

How is the state to bring in? The most OpenGL related addons need this.

yol · 2019-01-30T14:24:52Z

xbmc/windows/GUIWindowScreensaver.cpp

+    if (m_renderClearCnt == 0)
+      return;
+
+    /* make five render calls with clear to prevent mix with cached display data */


Yeah that's really crazy, but without that, when you start some visualizations (which used previous screen data and no "glClear") it flickers to the wild (for example, some modes in Screensaver Solar Winds)

This hack should under no circumstances go in.

I just noticed another thing: The screensavers render directly to display, right? If so, not calling glClear will not solve the problem for example on Wayland. We call eglSwapBuffers to display the content, but this is not a simple "front-back-buffer-switch" which it might have been in former times. Modern graphics drivers have multiple buffers ready and cycle through them as they see fit. Spec says: "The contents of ancillary buffers are always undefined after calling eglSwapBuffers. The contents of the color buffer are undefined if the value of the EGL_SWAP_BEHAVIOR attribute of surface is not EGL_BUFFER_PRESERVED." So the color buffer content is undefined when not calling glClear and the screensaver cannot rely on any specific content still being there.

yol · 2019-01-30T14:27:09Z

xbmc/windowing/X11/GLContextEGL.cpp

  m_eglConfig = GetEGLConfig(m_eglDisplay, vInfo);
  XFree(vInfo);

  if (m_eglConfig == EGL_NO_CONFIG)
  {
-    CLog::Log(LOGERROR, "failed to get eglconfig for visual id");
+    CLog::Log(LOGWARNING, "failed to get matching eglconfig for visual 0x%x of the window and is not suitable", visualid);


Suggested change

CLog::Log(LOGWARNING, "failed to get matching eglconfig for visual 0x%x of the window and is not suitable", visualid);

CLog::Log(LOGERROR, "failed to get suitable eglconfig for visual 0x%x", visualid);

Leave as error

AlwinEsch · 2019-02-02T01:32:18Z

xbmc/windows/GUIWindowScreensaver.cpp

@@ -68,6 +68,9 @@ bool CGUIWindowScreensaver::OnMessage(CGUIMessage& message)

  case GUI_MSG_WINDOW_INIT:
    {
+      // override the clear colour - we must never clear fullscreen
+      m_clearBackground = 0;


Found another way, the way before with count was really not nice.

There are also a few other places in Kodi that go that way. Also, 3D video rendering must avoid a glClear is called (over other way).

See #15148 (comment) - not calling glClear does not mean that the buffer contents are preserved.

Why is it then also needed on other fullscreen places in Kodi? In a visualization I can maybe understand, this are combinated with other parts on screen and could get mixed up.

There must be somewhere and somehow a way possible.

Why is it then also needed on other fullscreen places in Kodi?

How do you know it is needed? It might just be a performance optimization (no use clearing the buffer if we draw over everything anyway) - do you have a specific place where you know that it is really necessary?

Does the complete screen content have to be copied with every render call to have it back at the next frame?

Yes and no. There are extensions that can be used instead of the often unsupported EGL_BUFFER_PRESERVED. But they will not give you the last frame, but instead an arbitrary previous frame and tell you how old it is (EGL_EXT_buffer_age) - this also works with Wayland. But it will not help with the solarwinds screensaver because I guess that it is implemented in a way that it needs the exact previous frame. Then yes, you need to copy when swapping buffers. This is also what the driver would usually do for you anyway when setting EGL_BUFFER_PRESERVED. See also e.g. https://community.arm.com/graphics/b/blog/posts/mali-performance-3-is-egl_5f00_buffer_5f00_preserved-a-good-thing

Other option is to modify solarwinds screensaver to always draw everything.

AlwinEsch · 2019-02-02T17:56:26Z

Have now also tested with wayland over xbmc/screensavers.rsxs#20 and runs without problems

AlwinEsch · 2019-02-07T14:25:08Z

Have removed the last commit. Must be for Leia now a separate request created?

yol

Should be OK now

yol · 2019-02-08T09:00:58Z

Afaik we have not branched yet, so no backport needed

Before has Kodi checked only the EGL_NATIVE_VISUAL_ID and ignored others. With this was e.g. on a some addons the needed EGL_DEPTH_SIZE not available. This separates a part of IsSuitableVisual (...) into a separate function (SuitableCheck (...)) which is checked by GetEGLConfig and takes the necessary configuration from the list.

This calls GetEGLConfig which becomes also called direct after the place where IsSuitableVisual is called.

yol · 2019-02-10T08:53:27Z

@AlwinEsch thanks for your persistence! happy to see the screensavers and visualizations get some love :)

AlwinEsch added Type: Fix non-breaking change which fixes an issue Component: Binary add-ons Component: GUI engine v18 Leia labels Dec 27, 2018

AlwinEsch added this to the Leia 18.0-Final milestone Dec 27, 2018

AlwinEsch requested a review from FernetMenta December 27, 2018 23:33

yol reviewed Dec 28, 2018

View reviewed changes

xbmc/windowing/X11/GLContextEGL.cpp Outdated Show resolved Hide resolved

AlwinEsch force-pushed the fix-gl branch from 7cf0c82 to 9514476 Compare January 5, 2019 20:01

yol reviewed Jan 7, 2019

View reviewed changes

xbmc/windowing/X11/GLContextEGL.cpp Outdated Show resolved Hide resolved

AlwinEsch force-pushed the fix-gl branch 2 times, most recently from 8540151 to 44f42e3 Compare January 10, 2019 17:19

yol reviewed Jan 22, 2019

View reviewed changes

AlwinEsch force-pushed the fix-gl branch 3 times, most recently from b70439b to f982588 Compare January 24, 2019 00:13

MartijnKaijser removed this from the Leia 18.0-Final milestone Jan 28, 2019

yol reviewed Jan 30, 2019

View reviewed changes

AlwinEsch force-pushed the fix-gl branch 3 times, most recently from 511c6fd to e8aef1b Compare February 2, 2019 01:25

AlwinEsch commented Feb 2, 2019

View reviewed changes

AlwinEsch force-pushed the fix-gl branch 2 times, most recently from 7d97867 to 40c111e Compare February 7, 2019 14:24

yol approved these changes Feb 8, 2019

View reviewed changes

AlwinEsch added 2 commits February 8, 2019 18:28

X11: remove not needed CGLContextEGL::IsSuitableVisual

1ce33cc

This calls GetEGLConfig which becomes also called direct after the place where IsSuitableVisual is called.

AlwinEsch force-pushed the fix-gl branch from 40c111e to 1ce33cc Compare February 8, 2019 17:28

AlwinEsch merged commit efb8798 into xbmc:master Feb 10, 2019

AlwinEsch deleted the fix-gl branch February 10, 2019 00:09

Rechi added this to the Leia 18.1-rc1 milestone Feb 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

X11: fix usage of required configuration values #15148

X11: fix usage of required configuration values #15148

AlwinEsch commented Dec 27, 2018 •

edited

yol commented Jan 4, 2019

AlwinEsch commented Jan 5, 2019

yol commented Jan 6, 2019

FernetMenta commented Jan 7, 2019

yol commented Jan 7, 2019

AlwinEsch commented Jan 10, 2019

AlwinEsch commented Jan 10, 2019 •

edited

yol left a comment

yol Jan 22, 2019

AlwinEsch Jan 23, 2019

AlwinEsch Jan 23, 2019

AlwinEsch Jan 23, 2019

yol Jan 24, 2019

AlwinEsch Jan 24, 2019

yol Jan 30, 2019

AlwinEsch commented Jan 26, 2019

AlwinEsch commented Jan 29, 2019

yol Jan 30, 2019

AlwinEsch Jan 30, 2019

yol Jan 31, 2019

yol Jan 30, 2019

AlwinEsch Feb 2, 2019 •

edited

yol Feb 3, 2019

AlwinEsch Feb 3, 2019 •

edited

yol Feb 3, 2019

AlwinEsch commented Feb 2, 2019

AlwinEsch commented Feb 7, 2019

yol left a comment

yol commented Feb 8, 2019

yol commented Feb 10, 2019

	CLog::Log(LOGWARNING, "failed to get matching eglconfig for visual 0x%x of the window and is not suitable", visualid);
	CLog::Log(LOGERROR, "failed to get suitable eglconfig for visual 0x%x", visualid);

X11: fix usage of required configuration values #15148

X11: fix usage of required configuration values #15148

Conversation

AlwinEsch commented Dec 27, 2018 • edited

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Checklist:

yol commented Jan 4, 2019

AlwinEsch commented Jan 5, 2019

yol commented Jan 6, 2019

FernetMenta commented Jan 7, 2019

yol commented Jan 7, 2019

AlwinEsch commented Jan 10, 2019

AlwinEsch commented Jan 10, 2019 • edited

yol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlwinEsch commented Jan 26, 2019

AlwinEsch commented Jan 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlwinEsch Feb 2, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlwinEsch Feb 3, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlwinEsch commented Feb 2, 2019

AlwinEsch commented Feb 7, 2019

yol left a comment

Choose a reason for hiding this comment

yol commented Feb 8, 2019

yol commented Feb 10, 2019

AlwinEsch commented Dec 27, 2018 •

edited

AlwinEsch commented Jan 10, 2019 •

edited

AlwinEsch Feb 2, 2019 •

edited

AlwinEsch Feb 3, 2019 •

edited