Make tests pass with debug build (#833)

* remove unused param and fix incorrect signature

* in debug mode skia enables trivial_abi

related skia change:
https://github.com/JetBrains/skia/commit/3cd41698078075347badbeb64e546d8539c29ff8

* make paragraph tests pass with skiko.debug

* extract clone break iterator into separate test

it crashes with skiko.debug=true

* set matrices instead of pointers

before that it was failed with assert in debug mode

set matrices instead of pointers JS/Native

* remove unused function it was leading to wrong overload resolution and crash

* fix quality level in png encoding/decoding

* fail tests before they crash

* skip `breakIteratorCloneTest` with Js target

skiko/build.gradle.kts

@@ -1217,6 +1217,7 @@ tasks.withType<Test>().configureEach {
         )
         systemProperty("skiko.test.ui.enabled", System.getProperty("skiko.test.ui.enabled", canRunUiTests.toString()))
         systemProperty("skiko.test.ui.renderApi", System.getProperty("skiko.test.ui.renderApi", "all"))
+        systemProperty("skiko.test.debug", buildType == SkiaBuildType.DEBUG)
 
         // Tests should be deterministic, so disable scaling.
         // On MacOs we need the actual scale, otherwise we will have aliased screenshots because of scaling.

skiko/buildSrc/src/main/kotlin/properties.kt

@@ -75,7 +75,7 @@ enum class SkiaBuildType(
     DEBUG(
         "Debug",
         flags = arrayOf("-DSK_DEBUG"),
-        clangFlags = arrayOf("-std=c++17", "-g"),
+        clangFlags = arrayOf("-std=c++17", "-g", "-DSK_TRIVIAL_ABI=[[clang::trivial_abi]]"),
         msvcCompilerFlags = arrayOf("/Zi /std:c++17"),
         msvcLinkerFlags = arrayOf("/DEBUG"),
     ),

skiko/src/commonMain/kotlin/org/jetbrains/skia/Font.kt

@@ -585,7 +585,7 @@ class Font : Managed {
     val spacing: Float
         get() = try {
             Stats.onNativeCall()
-            _nGetSpacing(_ptr, NullPointer)
+            _nGetSpacing(_ptr)
         } finally {
             reachabilityBarrier(this)
         }
@@ -729,4 +729,4 @@ private external fun _nGetPaths(ptr: NativePointer, glyphs: InteropPointer, coun
 private external fun _nGetMetrics(ptr: NativePointer, metrics: InteropPointer)
 
 @ExternalSymbolName("org_jetbrains_skia_Font__1nGetSpacing")
-private external fun _nGetSpacing(ptr: NativePointer, glyphsArr: NativePointer): Float
+private external fun _nGetSpacing(ptr: NativePointer): Float

skiko/src/commonMain/kotlin/org/jetbrains/skia/paragraph/Paragraph.kt

@@ -200,6 +200,12 @@ class Paragraph internal constructor(ptr: NativePointer, text: ManagedString?) :
     //     // TODO: update _text
     //     return this;
     // }
+
+    /**
+     * `from` and `to` are ignored by skia
+     * and change applies to the whole paragraph text
+     * see: https://github.com/JetBrains/skia/blob/51072f3e6d263eeffed4c3038655ab1bf9cf8439/modules/skparagraph/src/ParagraphImpl.cpp#L1069
+     */
     fun updateFontSize(from: Int, to: Int, size: Float): Paragraph {
         return try {
             if (_text != null) {
@@ -219,6 +225,10 @@ class Paragraph internal constructor(ptr: NativePointer, text: ManagedString?) :
         }
     }
 
+    /**
+     * `from` and `to` are ignored by skia
+     * and change applies to the whole paragraph text
+     */
     fun updateForegroundPaint(from: Int, to: Int, paint: Paint?): Paragraph {
         return try {
             if (_text != null) {
@@ -239,6 +249,10 @@ class Paragraph internal constructor(ptr: NativePointer, text: ManagedString?) :
         }
     }
 
+    /**
+     * `from` and `to` are ignored by skia
+     * and change applies to the whole paragraph text
+     */
     fun updateBackgroundPaint(from: Int, to: Int, paint: Paint?): Paragraph {
         return try {
             if (_text != null) {

skiko/src/commonTest/kotlin/org/jetbrains/skia/BreakIteratorTests.kt

 package org.jetbrains.skia
 
+import org.jetbrains.skia.impl.reachabilityBarrier
 import org.jetbrains.skiko.OS
 import org.jetbrains.skiko.hostOs
-import org.jetbrains.skiko.tests.SkipJsTarget
-import org.jetbrains.skiko.tests.SkipJvmTarget
-import org.jetbrains.skiko.tests.SkipNativeTarget
-import kotlin.test.Test
-import kotlin.test.assertContentEquals
-import kotlin.test.assertEquals
-import kotlin.test.assertFailsWith
-import kotlin.test.assertTrue
+import org.jetbrains.skiko.tests.*
+import kotlin.test.*
 
 private fun BreakIterator.asSequence() = generateSequence { next().let { n -> if (n == -1) null else n } }
 
@@ -27,13 +22,38 @@ class BreakIteratorTests {
         val boundary = BreakIterator.makeWordInstance()
         boundary.setText("家捷克的软件开发公司 ,software development company")
 
-        assertContentEquals(listOf(1, 3, 4, 6, 8, 10, 11, 12, 20, 21, 32, 33, 40), boundary.asSequence().toList())
+        val boundariesList = listOf(1, 3, 4, 6, 8, 10, 11, 12, 20, 21, 32, 33, 40)
+        assertContentEquals(boundariesList, boundary.asSequence().toList())
+        assertEquals(-1, boundary.next())
+
+        //todo check what happens if we will continue with next() after -1
 
         assertEquals(0, boundary.first())
         assertEquals(40, boundary.last())
+    }
+
+    @Test
+    @SkipJsTarget
+    fun breakIteratorCloneTest() {
+        // Wasm and iOS builds of Skia do not include required data to implement those iterators,
+        // see `third_party/externals/icu/flutter/README.md`.
+        if (hostOs == OS.Ios)
+            return
+
+        if (isDebugModeOnJvm)
+            throw Error("This test is usually crashes in DEBUG mode")
+
+        val boundary = BreakIterator.makeWordInstance()
 
         val boundaryCloned = boundary.clone()
-        assertContentEquals(boundary.asSequence().toList(), boundaryCloned.asSequence().toList())
+
+        boundaryCloned.setText("家捷克的软件开发公司 ,software development company")
+        val boundariesList = listOf(1, 3, 4, 6, 8, 10, 11, 12, 20, 21, 32, 33, 40)
+        assertContentEquals(boundariesList, boundaryCloned.asSequence().toList())
+
+        assertEquals(0, boundaryCloned.first())
+        assertEquals(40, boundaryCloned.last())
+        reachabilityBarrier(boundary)
     }
 
     @Test

skiko/src/commonTest/kotlin/org/jetbrains/skia/ParagraphTest.kt

@@ -104,9 +104,9 @@ class ParagraphTest {
         ), paragraph.getRectsForRange(8, 21, RectHeightMode.TIGHT, RectWidthMode.TIGHT),0.01f)
 
         paragraph = paragraph
-            .updateFontSize(8, 21, 48.0f)
-            .updateForegroundPaint(8, 21, Paint().apply { color = Color.RED })
-            .updateBackgroundPaint(8, 21, Paint().apply { color = Color.BLACK })
+            .updateFontSize(0, text.length, 48.0f)
+            .updateForegroundPaint(0, text.length, Paint().apply { color = Color.RED })
+            .updateBackgroundPaint(0, text.length, Paint().apply { color = Color.BLACK })
             .updateAlignment(Alignment.RIGHT)
             .markDirty()
 

skiko/src/commonTest/kotlin/org/jetbrains/skia/TextLineTest.kt

 package org.jetbrains.skiko
 
-import org.jetbrains.skia.*
+import org.jetbrains.skia.Font
+import org.jetbrains.skia.ManagedString
+import org.jetbrains.skia.TextLine
+import org.jetbrains.skia.Typeface
 import org.jetbrains.skia.impl.use
 import org.jetbrains.skia.shaper.ShapingOptions
+import org.jetbrains.skia.tests.assertCloseEnough
+import org.jetbrains.skia.tests.makeFromResource
+import org.jetbrains.skiko.tests.isDebugModeOnJvm
+import org.jetbrains.skiko.tests.runTest
 import kotlin.test.Test
 import kotlin.test.assertContentEquals
 import kotlin.test.assertEquals
 import kotlin.test.assertNotEquals
-import org.jetbrains.skia.tests.makeFromResource
-import org.jetbrains.skia.tests.assertCloseEnough
-import org.jetbrains.skiko.tests.runTest
 
 class TextLineTest {
     private val inter36: suspend () -> Font = suspend {
@@ -228,6 +232,9 @@ class TextLineTest {
 
     @Test
     fun dontCrashOnStringsWithoutGlyphs() = runTest {
+        if (isDebugModeOnJvm)
+            throw Error("This test is usually crashes in DEBUG mode")
+
         val failedOn = "\uFE0F" // Variation Selector-16
         val textLine = TextLine.make(failedOn, jbMono36(), ShapingOptions.DEFAULT)
         assertCloseEnough(expected = 0f, actual = textLine.width)

skiko/src/commonTest/kotlin/org/jetbrains/skiko/tests/TestUtils.kt

@@ -15,3 +15,5 @@ expect annotation class SkipJsTarget
 expect annotation class SkipJvmTarget
 
 expect fun makeFromFileName(path: String?): Data
+
+expect val isDebugModeOnJvm: Boolean

skiko/src/jsTest/kotlin/TestUtils.js.kt

@@ -34,3 +34,5 @@ actual annotation class SkipJvmTarget
 actual annotation class SkipNativeTarget
 
 actual fun makeFromFileName(path: String?): Data = Data(0)
+
+actual val isDebugModeOnJvm: Boolean = false

skiko/src/jvmMain/cpp/common/Font.cc

@@ -382,7 +382,7 @@ extern "C" JNIEXPORT void JNICALL Java_org_jetbrains_skia_FontKt__1nGetMetrics
 }
 
 extern "C" JNIEXPORT jfloat JNICALL Java_org_jetbrains_skia_FontKt__1nGetSpacing
-  (JNIEnv* env, jclass jclass, jlong ptr, jshortArray glyphsArr) {
+  (JNIEnv* env, jclass jclass, jlong ptr) {
     SkFont* instance = reinterpret_cast<SkFont*>(static_cast<uintptr_t>(ptr));
     return instance->getSpacing();
 }

skiko/src/jvmMain/cpp/common/Image.cc

@@ -80,7 +80,7 @@ extern "C" JNIEXPORT jlong JNICALL Java_org_jetbrains_skia_ImageKt__1nEncodeToDa
       switch (skFormat) {
         case SkEncodedImageFormat::kPNG: {
           SkPngEncoder::Options options = SkPngEncoder::Options();
-          options.fZLibLevel = std::max((int)(quality / 10), 9);
+          options.fZLibLevel = std::max(0, std::min((int)(quality / 10), 9));
           SkData* data = SkPngEncoder::Encode(nullptr, instance, options).release();
           return reinterpret_cast<jlong>(data);
         }

skiko/src/jvmMain/cpp/common/RuntimeShaderBuilder.cc

@@ -97,7 +97,7 @@ Java_org_jetbrains_skia_RuntimeShaderBuilderKt__1nUniformFloatMatrix33
   (JNIEnv* env, jclass jclass, jlong builderPtr, jstring uniformName, jfloatArray uniformMatrix33) {
     SkRuntimeShaderBuilder* runtimeShaderBuilder = jlongToPtr<SkRuntimeShaderBuilder*>(builderPtr);
     std::unique_ptr<SkMatrix> matrix33 = skMatrix(env, uniformMatrix33);
-    runtimeShaderBuilder->uniform(skString(env, uniformName).c_str()) = matrix33.get();
+    runtimeShaderBuilder->uniform(skString(env, uniformName).c_str()) = *matrix33;
 }
 
 extern "C" JNIEXPORT void JNICALL
@@ -105,7 +105,7 @@ Java_org_jetbrains_skia_RuntimeShaderBuilderKt__1nUniformFloatMatrix44
   (JNIEnv* env, jclass jclass, jlong builderPtr, jstring uniformName, jfloatArray uniformMatrix44) {
     SkRuntimeShaderBuilder* runtimeShaderBuilder = jlongToPtr<SkRuntimeShaderBuilder*>(builderPtr);
     std::unique_ptr<SkM44> matrix44 = skM44(env, uniformMatrix44);
-    runtimeShaderBuilder->uniform(skString(env, uniformName).c_str()) = matrix44.get();
+    runtimeShaderBuilder->uniform(skString(env, uniformName).c_str()) = *matrix44;
 }
 
 extern "C" JNIEXPORT void JNICALL

skiko/src/jvmMain/cpp/common/interop.cc

@@ -521,19 +521,6 @@ namespace skija {
             return java::lang::Throwable::exceptionThrown(env) ? nullptr : res;
         }
 
-        std::unique_ptr<SkIRect> toSkIRect(JNIEnv* env, jobject obj) {
-            if (obj == nullptr)
-                return std::unique_ptr<SkIRect>(nullptr);
-            else {
-                return std::unique_ptr<SkIRect>(new SkIRect{
-                    env->GetIntField(obj, left),
-                    env->GetIntField(obj, top),
-                    env->GetIntField(obj, right),
-                    env->GetIntField(obj, bottom)
-                });
-            }
-        }
-
         std::unique_ptr<SkIRect> toSkIRect(JNIEnv* env, jintArray rectInts) {
             if (rectInts == nullptr)
                 return std::unique_ptr<SkIRect>(nullptr);

skiko/src/jvmMain/cpp/common/interop.hh

@@ -226,7 +226,7 @@ namespace skija {
         void onLoad(JNIEnv* env);
         void onUnload(JNIEnv* env);
         jobject fromSkIRect(JNIEnv* env, const SkIRect& rect);
-        std::unique_ptr<SkIRect> toSkIRect(JNIEnv* env, jobject obj);
+        std::unique_ptr<SkIRect> toSkIRect(JNIEnv* env, jintArray obj);
     }
 
     namespace Path {

skiko/src/jvmTest/kotlin/TestUtils.jvm.kt

@@ -23,3 +23,8 @@ actual typealias SkipJvmTarget = org.junit.Ignore
 actual annotation class SkipNativeTarget
 
 actual fun makeFromFileName(path: String?): Data = Data.Companion.makeFromFileName(path)
+
+actual val isDebugModeOnJvm: Boolean
+    get() {
+        return System.getProperty("skiko.test.debug") == "true"
+    }
\ No newline at end of file

skiko/src/nativeJsMain/cpp/Font.cc

@@ -360,7 +360,7 @@ SKIKO_EXPORT void org_jetbrains_skia_Font__1nGetMetrics
 
 
 SKIKO_EXPORT KFloat org_jetbrains_skia_Font__1nGetSpacing
-  (KNativePointer ptr, KShort* glyphsArr) {
+  (KNativePointer ptr) {
     SkFont* instance = reinterpret_cast<SkFont*>(ptr);
     return instance->getSpacing();
 }

skiko/src/nativeJsMain/cpp/Image.cc

@@ -74,7 +74,7 @@ SKIKO_EXPORT KNativePointer org_jetbrains_skia_Image__1nEncodeToData
        switch (skFormat) {
          case SkEncodedImageFormat::kPNG: {
            SkPngEncoder::Options options = SkPngEncoder::Options();
-           options.fZLibLevel = std::max(quality / 10, 9);
+           options.fZLibLevel = std::max(0, std::min(quality / 10, 9));
            SkData* data = SkPngEncoder::Encode(nullptr, instance, options).release();
            return reinterpret_cast<KNativePointer>(data);
          }

skiko/src/nativeJsMain/cpp/RuntimeShaderBuilder.cc

 #include <iostream>
 #include "SkRuntimeEffect.h"
+#include "SkMatrix.h"
 #include "common.h"
 
 static void deleteRuntimeShaderBuilder(SkRuntimeShaderBuilder* builder) {
@@ -81,13 +82,15 @@ SKIKO_EXPORT void org_jetbrains_skia_RuntimeShaderBuilder__1nUniformFloatMatrix2
 SKIKO_EXPORT void org_jetbrains_skia_RuntimeShaderBuilder__1nUniformFloatMatrix33
   (KNativePointer builderPtr, KInteropPointer uniformName, KFloat* uniformMatrix33) {
     SkRuntimeShaderBuilder* runtimeShaderBuilder = reinterpret_cast<SkRuntimeShaderBuilder*>(builderPtr);
-    runtimeShaderBuilder->uniform(skString(uniformName).c_str()) = uniformMatrix33;
+    std::unique_ptr<SkMatrix> matrix33 = skMatrix(uniformMatrix33);
+    runtimeShaderBuilder->uniform(skString(uniformName).c_str()) = *matrix33;
 }
 
 SKIKO_EXPORT void org_jetbrains_skia_RuntimeShaderBuilder__1nUniformFloatMatrix44
   (KNativePointer builderPtr, KInteropPointer uniformName, KFloat* uniformMatrix44) {
     SkRuntimeShaderBuilder* runtimeShaderBuilder = reinterpret_cast<SkRuntimeShaderBuilder*>(builderPtr);
-    runtimeShaderBuilder->uniform(skString(uniformName).c_str()) = uniformMatrix44;
+    std::unique_ptr<SkM44> matrix44 = skM44(uniformMatrix44);
+    runtimeShaderBuilder->uniform(skString(uniformName).c_str()) = *matrix44;
 }
 
 SKIKO_EXPORT void org_jetbrains_skia_RuntimeShaderBuilder__1nChildShader

skiko/src/nativeTest/kotlin/org/jetbrains/skiko/tests/TestUtils.native.kt

@@ -21,3 +21,5 @@ actual annotation class SkipJvmTarget
 actual typealias SkipNativeTarget = kotlin.test.Ignore
 
 actual fun makeFromFileName(path: String?): Data = Data.Companion.makeFromFileName(path)
+
+actual val isDebugModeOnJvm: Boolean = false
\ No newline at end of file