Remove incomplete IsExtSupported from CWinSystemEGL #1949

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
Member

popcornmix commented Dec 17, 2012

In the log on Pi, we get:
18:17:44 T:3040780928 NOTICE: GL: Reverting to POT textures

which is strange because we support nPOT.

This means (in xbmc/cores/VideoRenderers/LinuxRendererGLES.cpp):

  // determine whether GPU supports NPOT textures
  if (!g_Windowing.IsExtSupported("GL_TEXTURE_NPOT"))

is returning false.

But:

bool CRenderSystemGLES::IsExtSupported(const char* extension)
{
   ...
  else if (strcmp( extension, "GL_TEXTURE_NPOT" ) == 0)
  {
    // GLES supports non-power-of-two textures as standard.
        return true;

it should be returning true!

Trouble is, that xbmc/windowing/egl/WinSystemEGL.cpp overrides this
function with one that doesn't return true. I think this function
should be removed.

(Note: GL_TEXTURE_NPOT is not the correct extension name for GLES - it would need to be GL_OES_texture_npot if this is meant to query the GLES driver)

Member

jmarshallnz commented Dec 17, 2012

I don't know this code well, but shouldn't eglQueryString() be returning the extensions? What extensions does it return - perhaps it's just a matter of the incorrect extension?

Member

theuni commented Dec 17, 2012

The extensions and the query are correct, problem is that we're asking the wrong place :)

We'll have 2 lists of extensions, windowing (egl) and rendering (gles). I believe the issue is that we're calling into windowing and we never actually get to the render extensions. Could you please try this as a quick test?

diff --git a/xbmc/windowing/egl/WinSystemEGL.cpp b/xbmc/windowing/egl/WinSystemEGL.cpp
index a71e105..1698e26 100644
--- a/xbmc/windowing/egl/WinSystemEGL.cpp
+++ b/xbmc/windowing/egl/WinSystemEGL.cpp
@@ -372,7 +372,7 @@ bool CWinSystemEGL::IsExtSupported(const char* extension)
   name += extension;
   name += " ";

-  return m_extensions.find(name) != std::string::npos;
+  return (m_extensions.find(name) != std::string::npos || CRenderSystemGLES::IsExtSupported(extension));
 }

 bool CWinSystemEGL::PresentRenderImpl(const CDirtyRegionList &dirty)
Member

popcornmix commented Dec 17, 2012

@theuni
Your fix works for me:
NOTICE: GL: NPOT texture support detected

Member

theuni commented Dec 17, 2012

Ok. It's a bit circuitous to go renderer->windowing->renderer, but I think that's probably safest for frodo, because there are several places that query windowing directly (where they shouldn't).

So rather than fixing up each and risking missing one, I'll push this change instead and we'll clean it up when the rendering/windowing split happens.

Thanks for bringing this up!

popcornmix closed this Dec 17, 2012

Member

theuni commented Dec 18, 2012

I was assuming that this behavior changed with the egl rewrite, but looking at the git logs, it seems as though this (forced npot) is how it as been all along.

For that reason, I'm hesitant to "fix" this just before release, since it's essentially enabling an un-tested code-path.

Ping @davilla and @gimli. Thoughts? Don't confuse this with the NPOT texture stuff, this will only affect video rendering and fbo's. In fact, fbo's will be disabled for gles as it is.

@popcornmix Do you recall whether this once worked as intended? Or have we been forcing npot since the early days of rpi?

Contributor

davilla commented Dec 18, 2012

rc2 on 19th. release pushed back so there's some time to verify.

Member

theuni commented Dec 18, 2012

ok. @popcornmix Could you please confirm that video playback works and looks normal with this fixed? This really only affects omxplayer afaik, since all other egl+gles players that i know of use the bypass render method.

Contributor

davilla commented Dec 18, 2012

aml, with dvdplayer/amcodec, I've seen that message "NOTICE: GL: Reverting to POT textures"

Member

theuni commented Dec 18, 2012

right, but it doesn't matter because we don't actually render. omxplayer renders frames itself.

Member

popcornmix commented Dec 18, 2012

Video playback is okay with this fix on Pi.
We also use the bypass render method (the frames are displayed from OpenMAX on a different layer to the GL).

popcornmix deleted the popcornmix:is_ext_supported branch Jan 13, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment