Skip to content
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.

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

Fix a native crash on makeGL #869

Merged
merged 2 commits into from Feb 9, 2024

Conversation

igordmn
Copy link
Collaborator

@igordmn igordmn commented Feb 9, 2024

It is a regression after #811

After that PR, OpenGL functions are linked in runtime via function loadOpenGLLibrary. We didn't call it in makeGL functions. Now we call.

  • load library before calling makeGL
  • add isLoaded to avoid loading it multiple times
  • throw RenderException on macOs/iOS, as OpenGL there is deprecated

Fixes JetBrains/compose-multiplatform#3972

@igordmn igordmn force-pushed the igor.demin/fix-opengl-context-regression branch from 8a29d8e to 7a4b50a Compare February 9, 2024 12:20
It is a regression after #811

After that PR, OpenGL functions are linked in runtime via function `loadOpenGLLibrary`. We didn't call it in `makeGL` functions. Now we call.

Adding also `isLoaded` to avoid loading it multiple times

Fixes https://youtrack.jetbrains.com/issue/COMPOSE-642/Fix-Skiko-API-regression-for-creating-OpenGL-context
@igordmn igordmn force-pushed the igor.demin/fix-opengl-context-regression branch from 7a4b50a to 29d7328 Compare February 9, 2024 12:58
fbFormat
)
)
loadOpenGLLibrary()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we ever create BackendRenderTarget before initialising context? From a first look this one seems redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems it isn't needed indeed. But not because we init context. Because we can create it without context and even without OpenGL. Checked it just now.

@igordmn igordmn merged commit 27e5d7e into master Feb 9, 2024
5 checks passed
@igordmn igordmn deleted the igor.demin/fix-opengl-context-regression branch February 9, 2024 15:20
@eymar
Copy link
Collaborator

eymar commented Feb 22, 2024

This change affected CfW (k/js) running in Safari :) https://kotlinlang.slack.com/archives/C01F2HV7868/p1708540690334899?thread_ts=1708527218.429189&cid=C01F2HV7868
I'll prepare a fix.

igordmn added a commit that referenced this pull request Apr 16, 2024
By request from user in DM who uses LWJGL+Skiko on macOS.

Let Skia throw an error if it isn't supported, not Skiko.

Skiko works well without it, as it doesn't use any OpenGL inside Skiko classes (particularly SkiaLayer).

The original error was added in #869, the main purpose of which was to fix a Windows issue, macOs was added just by an assumption that it is no longer can work

## Testing
Existing tests works (SkiaLayerTest, DirectContextTest)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lwjgl crash in skiko 0.7.84
3 participants