Skip to content

Commit

Permalink
Fix a native crash on makeGL (#869)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
igordmn committed Feb 9, 2024
1 parent 2a8e25d commit 27e5d7e
Show file tree
Hide file tree
Showing 10 changed files with 129 additions and 35 deletions.
2 changes: 1 addition & 1 deletion skiko/src/awtMain/cpp/windows/OpenGLLibrary.cc
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ extern "C" {
return wglGetCurrentContext();
}

JNIEXPORT void JNICALL Java_org_jetbrains_skiko_OpenGLLibraryKt_loadOpenGLLibraryWindows(JNIEnv *env, jobject obj) {
JNIEXPORT void JNICALL Java_org_jetbrains_skiko_OpenGLLibrary_1jvmKt_loadOpenGLLibraryWindows(JNIEnv *env, jobject obj) {
OpenGL32Library = LoadLibrary("opengl32.dll");
if (OpenGL32Library == nullptr) {
auto code = GetLastError();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package org.jetbrains.skiko

import org.jetbrains.skia.BackendRenderTarget
import org.jetbrains.skiko.util.joinThreadCatching
import kotlin.test.*

class BackendRenderTargetTest {
@Test
fun makeGL() {
// use a new thread to be sure that there is no OpenGL context in it
joinThreadCatching {
// this method doesn't fail, even if there is no OpenGL Context in the thread or there is no OpenGL support
BackendRenderTarget.makeGL(100, 100, 1, 8, 0, 0x8058)
}
}
}
26 changes: 26 additions & 0 deletions skiko/src/awtTest/kotlin/org/jetbrains/skiko/DirectContextTest.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package org.jetbrains.skiko

import org.jetbrains.skia.DirectContext
import org.jetbrains.skiko.util.joinThreadCatching
import kotlin.test.*

class DirectContextTest {
@Test
fun makeGL() {
// use a new thread to be sure that there is no OpenGL context in it
joinThreadCatching {
// Test only that the method doesn't crash VM, and only throw an exception.
// To properly test it, we have to use third-party created OpenGL context.
// Testing with our own created OpenGL context won't be fair, as these functions can do additional
// initialization (i.e. if we call our own glMakeCurrent, we have to call `loadOpenGLLibrary`)
var actualException: RenderException? = null
try {
DirectContext.makeGL()
} catch (e: RenderException) {
actualException = e
}
assertTrue(actualException != null, "makeGL didn't fail")
}
}
}

19 changes: 19 additions & 0 deletions skiko/src/awtTest/kotlin/org/jetbrains/skiko/util/Threads.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package org.jetbrains.skiko.util

/**
* Runs body in another thread, join it and throws a last exception in this thread if any
*/
fun joinThreadCatching(body: () -> Unit) {
var exception: Throwable? = null
val thread = object : Thread() {
override fun run() = body()
}
thread.setUncaughtExceptionHandler { _, e ->
exception = e
}
thread.start()
thread.join()
if (exception != null) {
throw exception!!
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import org.jetbrains.skia.impl.Library.Companion.staticLoad
import org.jetbrains.skia.impl.Managed
import org.jetbrains.skia.impl.NativePointer
import org.jetbrains.skia.impl.Stats
import org.jetbrains.skiko.RenderException
import org.jetbrains.skiko.loadOpenGLLibrary

class BackendRenderTarget internal constructor(ptr: NativePointer) : Managed(ptr, _FinalizerHolder.PTR) {
companion object {
Expand All @@ -16,16 +18,9 @@ class BackendRenderTarget internal constructor(ptr: NativePointer) : Managed(ptr
fbFormat: Int
): BackendRenderTarget {
Stats.onNativeCall()
return BackendRenderTarget(
_nMakeGL(
width,
height,
sampleCnt,
stencilBits,
fbId,
fbFormat
)
)
val ptr =_nMakeGL(width, height, sampleCnt, stencilBits, fbId, fbFormat)
if (ptr == NullPointer) throw RenderException("Can't create OpenGL BackendRenderTarget")
return BackendRenderTarget(ptr)
}

fun makeMetal(width: Int, height: Int, texturePtr: NativePointer): BackendRenderTarget {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,17 @@ package org.jetbrains.skia

import org.jetbrains.skia.impl.*
import org.jetbrains.skia.impl.Library.Companion.staticLoad
import org.jetbrains.skiko.RenderException
import org.jetbrains.skiko.loadOpenGLLibrary

class DirectContext internal constructor(ptr: NativePointer) : RefCnt(ptr) {
companion object {
fun makeGL(): DirectContext {
Stats.onNativeCall()
return DirectContext(_nMakeGL())
loadOpenGLLibrary()
val ptr = _nMakeGL()
if (ptr == NullPointer) throw RenderException("Can't create OpenGL DirectContext")
return DirectContext(ptr)
}

fun makeMetal(devicePtr: NativePointer, queuePtr: NativePointer): DirectContext {
Expand Down
14 changes: 14 additions & 0 deletions skiko/src/commonMain/kotlin/org/jetbrains/skiko/OpenGLLibrary.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
package org.jetbrains.skiko

/**
* Load OpenGL library into memory.
*
* Should be called before any OpenGL operation.
*
* The current implementation loads OpenGl lazily on Windows, and at app startup on Linux/macOs, native and web targets
*
* Some Windows machines don't have OpenGL (usually CI machines), so we'll fallback to other renderers on them.
*
* @throws RenderException if OpenGL library can't be loaded.
*/
internal expect fun loadOpenGLLibrary()
29 changes: 29 additions & 0 deletions skiko/src/jvmMain/kotlin/org/jetbrains/skiko/OpenGLLibrary.jvm.kt
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package org.jetbrains.skiko

import org.jetbrains.skia.impl.Library

private external fun loadOpenGLLibraryWindows()

private var isLoaded = false

@Synchronized
internal actual fun loadOpenGLLibrary() {
if (!isLoaded) {
when {
// On Windows it is linked dynamically
hostOs.isWindows -> {
Library.staticLoad()
loadOpenGLLibraryWindows()
}
// it was deprecated in macOS 10.14 and isn't supported in Skiko
// see https://developer.apple.com/library/archive/documentation/GraphicsImaging/Conceptual/OpenGL-MacProgGuide/opengl_intro/opengl_intro.html
hostOs.isMacOS -> throw RenderException("OpenGL on macOS isn't supported")
// Do nothing as we don't know for sure which OS supports OpenGL.
// If there is support, we assume that it is already linked.
// If there is no support, there will be a native crash
// (to throw a RenderException we need to investigate case by case)
else -> Unit
}
isLoaded = true
}
}
23 changes: 0 additions & 23 deletions skiko/src/jvmMain/kotlin/org/jetbrains/skiko/OpenGLLibrary.kt

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package org.jetbrains.skiko

internal actual fun loadOpenGLLibrary() {
when (hostOs) {
// It is deprecated. See https://developer.apple.com/documentation/opengles
OS.Ios -> throw RenderException("OpenGL on iOS isn't supported")
// Do nothing as we don't know for sure which OS supports OpenGL.
// If there is support, we assume that it is already linked.
// If there is no support, there will be a native crash
// (to throw a RenderException we need to investigate case by case)
else -> Unit
}
}

0 comments on commit 27e5d7e

Please sign in to comment.