New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Copy GLXFBConfig from shared context. #1456

Merged

romainguy merged 4 commits into google:master from roxlu:rox/feature-glx-copy-fbconfig-from-shared-gl-context

Jul 31, 2019

Contributor

roxlu commented Jul 30, 2019

Fixes #1453


          Copy GLXFBConfig from shared context.

93cc90c

googlebot commented Jul 30, 2019

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

googlebot added the cla: no label

roxlu mentioned this pull request

Using Filament with shared context exits with BadMatch on Linux #1453

Closed


          Merge branch 'master' into rox/feature-glx-copy-fbconfig-from-shared-…

7fde001

…gl-context

googlebot commented Jul 30, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

googlebot added cla: yes and removed cla: no labels

romainguy requested changes

View reviewed changes

Collaborator

romainguy left a comment

Thanks for your contribution! I left a bunch of comments that are mostly style related.

filament/backend/src/opengl/PlatformGLX.cpp Outdated

@@ @@ -46,6 +46,11 @@ struct GLXFunctions { @@
                   PFNGLXCREATEPBUFFERPROC createPbuffer;
                   PFNGLXDESTROYPBUFFERPROC destroyPbuffer;
                   PFNGLXMAKECONTEXTCURRENTPROC setCurrentContext;
+                  PFNGLXQUERYCONTEXTPROC queryContext; /* When creating a shared GL context, we query the used GLX_FBCONFIG_ID to make sure our display framebuffer attributes match; otherwise making our context current results in a BadMatch https://gist.github.com/roxlu/c282d642c353ce96ef19b6359c741bcb */

Collaborator

romainguy Jul 30, 2019

Our code style limits a line width to 100 characters. Can you move those comments above the definitions and use multiple lines wrapped at 80 or 100 columns please?

Contributor Author

roxlu Jul 31, 2019

Done.

filament/backend/src/opengl/PlatformGLX.cpp

-                  // Create a context
-                  static int attribs[] = { GLX_DOUBLEBUFFER, True, None };
+                  if (nullptr != sharedGLContext) {

Collaborator

romainguy Jul 30, 2019

Remove

Contributor Author

roxlu Jul 31, 2019

Done.

filament/backend/src/opengl/PlatformGLX.cpp Outdated

+                  if (nullptr != sharedGLContext) {
+                      int r = -1;
+                      int used_fb_id = -1;

Collaborator

romainguy Jul 30, 2019

Please use camelCase to name variables. The existing code makes the same mistake.

Contributor Author

roxlu Jul 31, 2019

Done.

filament/backend/src/opengl/PlatformGLX.cpp Outdated

+                      GLXContext shared_ctx = (GLXContext)((void*)sharedGLContext);
+                      r = g_glx.queryContext(mGLXDisplay, shared_ctx, GLX_FBCONFIG_ID, &used_fb_id);
+                      if (0 != r) {

Collaborator

romainguy Jul 30, 2019

if (r != 0) or if (r) if you prefer

Contributor Author

roxlu Jul 31, 2019

Done.

filament/backend/src/opengl/PlatformGLX.cpp Outdated

+                      r = g_glx.queryContext(mGLXDisplay, shared_ctx, GLX_FBCONFIG_ID, &used_fb_id);
+                      if (0 != r) {
+                          printf("Error: failed to get GLX_FBCONFIG_ID from shared GL context.");

Collaborator

romainguy Jul 30, 2019

Don't printf, use utils::slog.* (utils::slog.e << "Error: …" in this case)

Contributor Author

roxlu Jul 31, 2019

Thanks, changed this. The first check on if(mGLXDisplay == nullptr) from the existing code uses printf(). I've changed this into utils::slog.e too.

Contributor Author

roxlu Jul 31, 2019

Done.

filament/backend/src/opengl/PlatformGLX.cpp Outdated

+                      int num_configs = 0;
+                      GLXFBConfig* fb_configs = g_glx.getFbConfigs(mGLXDisplay, 0, &num_configs);
+                      if (nullptr == fb_configs) {

Collaborator

romainguy Jul 30, 2019

Change the order of the test, or just use !fbConfigs

Contributor Author

roxlu Jul 31, 2019

Done.

filament/backend/src/opengl/PlatformGLX.cpp Outdated

+                      for (int i = 0; i < num_configs; ++i) {
+                          r = g_glx.getFbConfigAttrib(mGLXDisplay, fb_configs[i], GLX_FBCONFIG_ID, &fb_id);
+                          if (0 != r) {

Collaborator

romainguy Jul 30, 2019

See comments above

Contributor Author

roxlu Jul 31, 2019

Done.

filament/backend/src/opengl/PlatformGLX.cpp Outdated

-                  if (mGLXConfig == nullptr || config_count == 0) {
+                    int config_count = 0;
+                    mGLXConfig = g_glx.chooseFbConfig(mGLXDisplay, DefaultScreen(mGLXDisplay),
+                                                      attribs, &config_count);

Collaborator

romainguy Jul 30, 2019

Continuation indents are 8 spaces in our code base, we don't align parameters in this way

Contributor Author

roxlu Jul 31, 2019

Done.

filament/backend/src/opengl/PlatformGLX.cpp Outdated Show resolved Hide resolved

roxlu added 2 commits

July 31, 2019 11:36


          Style fixes

75d76f9


          Merge branch 'rox/feature-glx-copy-fbconfig-from-shared-gl-context' o…

dad1f7e

…f github.com:roxlu/filament into rox/feature-glx-copy-fbconfig-from-shared-gl-context

romainguy approved these changes

View reviewed changes

Collaborator

romainguy left a comment

Thank you!

romainguy added the kokoro:run label

Collaborator

romainguy commented Jul 31, 2019

I'll wait for the CI builds to turn green before merging this.

kokoro-team removed the kokoro:run label

romainguy merged commit 0faa391 into google:master

This was referenced Jun 1, 2022

GLX + GLFW: BadMatch with combination of shared context & native window handle #5617

Open

PlatformCocoaGL: If a share context is specified, use its pixel format #5624

Closed

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

Labels