kyamagu / skia-python

Python binding to Skia Graphics Library
https://kyamagu.github.io/skia-python/
BSD 3-Clause "New" or "Revised" License
240 stars 42 forks source link

Anti-Alias with complex skia.Path triggers shader compilation error and failed drawing #214

Closed stenson closed 2 months ago

stenson commented 10 months ago

Describe the bug First off, just want to say I'm a big fan of skia-python! But I recently heard from some users of my library coldtype (which uses skia-python extensively), that when they try to use coldtype with Python 3.12, the only option is to install 119.0b4 which triggers all kinds of issues. I've started looking into the issues (many of which are related to the image changes, I think), but there's one fundamental bug I'd like to figure out before the others, which is that I can't seem to reliably draw complex curves with anti-aliasing without triggering some obscure shader compilation errors.

To Reproduce I'm running this script on Python 3.12.0 on macOS 12.5.1 (a slightly modified version of the glfw example code here):

import contextlib, glfw, skia
from OpenGL import GL

WIDTH, HEIGHT = 640, 480

path = skia.Path()
path.moveTo(184, 445)
path.lineTo(249, 445)
path.quadTo(278, 445, 298, 438)
path.quadTo(318, 431, 331, 419)
path.quadTo(344, 406, 350, 390)
path.quadTo(356, 373, 356, 354)
path.quadTo(356, 331, 347, 312)
path.quadTo(338, 292, 320, 280) # <- comment out this line and shape will draw correctly with anti-aliasing
path.close()

@contextlib.contextmanager
def glfw_window():
    if not glfw.init():
        raise RuntimeError('glfw.init() failed')
    glfw.window_hint(glfw.STENCIL_BITS, 8)
    window = glfw.create_window(WIDTH, HEIGHT, '', None, None)
    glfw.make_context_current(window)
    yield window
    glfw.terminate()

@contextlib.contextmanager
def skia_surface(window):
    context = skia.GrDirectContext.MakeGL()
    (fb_width, fb_height) = glfw.get_framebuffer_size(window)
    backend_render_target = skia.GrBackendRenderTarget(
        fb_width,
        fb_height,
        0,  # sampleCnt
        0,  # stencilBits
        skia.GrGLFramebufferInfo(0, GL.GL_RGBA8))
    surface = skia.Surface.MakeFromBackendRenderTarget(
        context, backend_render_target, skia.kBottomLeft_GrSurfaceOrigin,
        skia.kRGBA_8888_ColorType, skia.ColorSpace.MakeSRGB())
    assert surface is not None
    yield surface
    context.abandonContext()

with glfw_window() as window:
    GL.glClear(GL.GL_COLOR_BUFFER_BIT)

    with skia_surface(window) as surface:
        with surface as canvas:
            canvas.drawCircle(100, 100, 40, skia.Paint(Color=skia.ColorGREEN, AntiAlias=True))

            paint = skia.Paint(Color=skia.ColorBLUE)
            paint.setStyle(skia.Paint.kStroke_Style)
            paint.setStrokeWidth(2)
            paint.setAntiAlias(True)

            canvas.drawPath(path, paint)

        surface.flushAndSubmit()
        glfw.swap_buffers(window)

        while (glfw.get_key(window, glfw.KEY_ESCAPE) != glfw.PRESS
            and not glfw.window_should_close(window)):
            glfw.wait_events()

On my setup, running that code produces this output:

Shader compilation error
------------------------
   1    #version 110
   2
   3    uniform vec4 sk_RTAdjust;
   4    uniform vec2 uatlas_adjust_S0;
   5    attribute vec4 fillBounds;
   6    attribute vec4 color;
   7    attribute vec4 locations;
   8    varying vec2 vatlasCoord_S0;
   9    varying vec4 vcolor_S0;
  10    void main() {
  11        vec2 unitCoord = vec2(float(gl_VertexID & 1), float(gl_VertexID >> 1));
  12        vec2 devCoord = mix(fillBounds.xy, fillBounds.zw, unitCoord);
  13        vec2 atlasTopLeft = vec2(abs(locations.x) - 1.0, locations.y);
  14        vec2 devTopLeft = locations.zw;
  15        bool transposed = locations.x < 0.0;
  16        vec2 atlasCoord = devCoord - devTopLeft;
  17        if (transposed) {
  18            atlasCoord = atlasCoord.yx;
  19        }
  20        atlasCoord += atlasTopLeft;
  21        vatlasCoord_S0 = atlasCoord * uatlas_adjust_S0;
  22        vcolor_S0 = color;
  23        gl_Position = vec4(devCoord, 0.0, 1.0);
  24        gl_Position = vec4(gl_Position.xy * sk_RTAdjust.xz + gl_Position.ww * sk_RTAdjust.yw, 0.0, gl_Position.w);
  25    }
  26
Errors:
ERROR: 0:11: Use of undeclared identifier 'gl_VertexID'
ERROR: 0:11: Use of undeclared identifier 'gl_VertexID'
ERROR: 0:12: Use of undeclared identifier 'unitCoord'
ERROR: 0:16: Use of undeclared identifier 'devCoord'
ERROR: 0:18: Use of undeclared identifier 'atlasCoord'
ERROR: 0:18: Use of undeclared identifier 'atlasCoord'
ERROR: 0:20: Use of undeclared identifier 'atlasCoord'
ERROR: 0:21: Use of undeclared identifier 'atlasCoord'
ERROR: 0:23: Use of undeclared identifier 'devCoord'

Expected behavior For the curve to be drawn on screen without a shader compilation error. With 87.5 running the same code, this is the graphical output:

image

Desktop (please complete the following information):

Additional context I realize these releases are still pre-release, but as they install by default with Python3.12, I wanted to get ahead of the skia updates to make sure Coldtype can continue working correctly. Thanks for all the hard work on this awesome library!

HinTak commented 10 months ago

I'll have a look and see if I can reproduce the problem on Linux with 120bX (I only really have access to Linux - the rest is just pushing into github's CI, with the pytests and the logs).

However, I had some exchanges with @kyamagu at the beginning about the needs of having another 87.X release, and this (just back-porting the CI changes specific to python 3.12 support) seems simple enough to do.

I'll have a look at both.

HinTak commented 10 months ago

It doesn't look like github let's me run CI without it mergeable against an existing branch (so I tried in #216 #215 and closed them myself), so I have to create a v87.5 branch first. I have done so on my playground https://github.com/HinTak/skia-m1xx-python/actions/runs/6998632212 instead.

If CI finishes over there, you @stenson can download the pip wheel from the artefact there instead. Or if you insist, we can put a v87.5 branch here and host the CI here later. (and possibly make a release out of a branch here).

HinTak commented 10 months ago

Oh, it runs nicely on 120bX here on Linux python 3.11... am afraid this is a Mac OS X GL problem, I think. GL on mac os X is behind, I think.

HinTak commented 10 months ago

CI finished, failing only later at doc building (a known issue) so I back-ported the doc building fix too. Grab the pip wheel from this https://github.com/HinTak/skia-m1xx-python/actions/runs/7000261476 if it makes you happier, but the earlier one should work again python 3.12 too.

stenson commented 10 months ago

Thanks for looking into this — I'm not too worried about an 87.5 build that works with python 3.12 as the 87.5 builds on previous versions of python work just fine, I'm more just interested in making sure future releases of skia-python will continue to work on macOS. When you say GL on mac is behind, I'm not sure I understand, isn't skia meant to be cross-platform?

HinTak commented 10 months ago

It is a bit more complicated than that - between m87 and m116 about 20% of skia API changed ... and google folks are quite aggressively changing things every release.

As for GL , which is exploiting graphic card's capabilility with driver support plus software emulation fallbacks, the last mile - software emulation fallbacks, is provided by ANGLE on windows (GL on directX, I think) from Google, OpenGL from Apple, and mesa on Linux. And I meant Apple's OpenGL is significantly behind mesa, I seem to have heard. And Google stick quite close to what latest mesa can do.

@stenson one question for you - is problem on arm CPU or on Intel CPU? We currently don't CI-test GL on arm-based macs since github CI 's arm mac image can't do it. I filed #217 and just closed it myself thinking that might be a issue.

The other thing is min-os version - I just bumped this up https://github.com/kyamagu/skia-python/pull/210/commits/b0e3f3c81e5d9357947639d0879164968e3bfe8c - and would appreciate you test and file problems against that artefact when it finishes building, if possible, for the next release. We don't want to set it too high (dropping support for old mac os x), but it might be necessary.

Skia isn't as cross-platform as one would like - sometimes it is underlying os limitations. I recently closed two font-related issues for which can be worked around if we bundle freetype on non-Linux, and use that instead of coretext(mac) or directwrite(windows) to load fonts. (#138 #195).

That said, if you have more problems and/or missing API using m119/120 vs m87, we definitely want to know.

HinTak commented 10 months ago

@stenson I have gone back and revisited some of the Image.* which got broken in the m87 to m116 transition and re-enabled them again: https://github.com/kyamagu/skia-python/pull/210/commits/fb46ad4887f0493a85b2110e9bb3cd56ee92c711 these might be what you think are missing? The initial work was done by somebody who noticed something missing too, and this being in textured backed images, probably on the general area of your interests.

If you have anything else obvious and simple enough to put into m120, let me know soon.

stenson commented 10 months ago

@HinTak sorry for the delay here — I'm not quite sure I understand how to test these changes before they make it onto pypi (I tried pip installing from the specific sha but skia compilation fails on my setup). That said, I've just tested 120.0b5 from pypi and I'm getting the exact same error from the start of this thread.

And I am on an armbased mac to answer your earlier question.

HinTak commented 10 months ago

@stenson 120 b5 just went out today. If CI is successful, binary snapshot builds are available under https://github.com/kyamagu/skia-python/actions . Anyway, the reason why I ask about arm vs Intel mac is because github CI doesn't run pytest with arm mac's so GL on arm macs is different from intel macs according to github's CI infrastructure.

I am thinking of converting this example to ru n headless to see if it works on Intel macs. The other thing is, you might look into the Metal equivalent as that's what Apple wants you to use instead of GL. We might switch on the Metal code in m121.

stenson commented 8 months ago

Been looking into this further — I've gotten the full coldtype library working with 121.0b5 with some hacks (always drawing to an image rather than drawing curves directly to an opengl-backed window), but even drawing an image to the opengl-backed window is much slower than the equivalent operation with python3.11 + skia-python m87.

If switching on the metal code is still an option, I think that'd be a great change to make sure skia-python can be performant on macOS.

pavpanchekha commented 4 months ago

I seem to have a similar error in a case with AntiAlias. The Python code is the following:

        path = skia.Path().moveTo(self.x1, self.y1).lineTo(self.x2, self.y2)
        paint = skia.Paint(Color=parse_color(self.color))
        paint.setStyle(skia.Paint.kStroke_Style)
        paint.setStrokeWidth(self.thickness)
        canvas.drawPath(path, paint)

The compilation error is:

Shader compilation error
------------------------
   1    #version 110
   2    
   3    
   4    float _determinant2(mat2 m) {
   5    return m[0].x*m[1].y - m[0].y*m[1].x;
   6    }
   7    const float PI = 3.14159274;
   8    const float PRECISION = 4.0;
   9    const float NUM_TOTAL_EDGES = 16383.0;
  10    uniform vec4 sk_RTAdjust;
  11    uniform vec3 utessControlArgs_S0;
  12    uniform vec4 uaffineMatrix_S0;
  13    uniform vec2 utranslate_S0;
  14    attribute vec4 pts01Attr;
  15    attribute vec4 pts23Attr;
  16    attribute vec2 argsAttr;
  17    attribute float curveTypeAttr;
  18    vec2 robust_normalize_diff_f2f2f2(vec2 a, vec2 b) {
  19        vec2 diff = a - b;
  20        if (diff == vec2(0.0)) {
  21            return vec2(0.0);
  22        } else {
  23            float invMag = 1.0 / max(abs(diff.x), abs(diff.y));
  24            return normalize(invMag * diff);
  25        }
  26    }
  27    vec2 unchecked_mix_f2f2f2f(vec2 a, vec2 b, float T) {
  28        return ((b - a) * (vec2(T)) + (a));
  29    }
  30    float wangs_formula_max_fdiff_p2_ff2f2f2f2f22(vec2 p0, vec2 p1, vec2 p2, vec2 p3, mat2 matrix) {
  31        vec2 d0 = matrix * (((vec2(-2.0)) * (p1) + (p2)) + p0);
  32        vec2 d1 = matrix * (((vec2(-2.0)) * (p2) + (p3)) + p1);
  33        return max(dot(d0, d0), dot(d1, d1));
  34    }
  35    float wangs_formula_conic_p2_fff2f2f2f(float _precision_, vec2 p0, vec2 p1, vec2 p2, float w) {
  36        vec2 C = (min(min(p0, p1), p2) + max(max(p0, p1), p2)) * 0.5;
  37        p0 -= C;
  38        p1 -= C;
  39        p2 -= C;
  40        float m = sqrt(max(max(dot(p0, p0), dot(p1, p1)), dot(p2, p2)));
  41        vec2 dp = ((vec2(-2.0 * w)) * (p1) + (p0)) + p2;
  42        float dw = abs(((-2.0) * (w) + (2.0)));
  43        float rp_minus_1 = max(0.0, ((m) * (_precision_) + (-1.0)));
  44        float numer = length(dp) * _precision_ + rp_minus_1 * dw;
  45        float denom = 4.0 * min(w, 1.0);
  46        return numer / denom;
  47    }
  48    void main() {
  49        float NUM_RADIAL_SEGMENTS_PER_RADIAN = utessControlArgs_S0.x;
  50        float JOIN_TYPE = utessControlArgs_S0.y;
  51        float STROKE_RADIUS = utessControlArgs_S0.z;
  52        mat2 AFFINE_MATRIX = mat2(uaffineMatrix_S0.xy, uaffineMatrix_S0.zw);
  53        vec2 TRANSLATE = utranslate_S0;
  54        vec2 p0 = pts01Attr.xy;
  55        vec2 p1 = pts01Attr.zw;
  56        vec2 p2 = pts23Attr.xy;
  57        vec2 p3 = pts23Attr.zw;
  58        vec2 lastControlPoint = argsAttr;
  59        float w = -1.0;
  60        if (curveTypeAttr != 0.0) {
  61            w = p3.x;
  62            p3 = p2;
  63        }
  64        float numParametricSegments;
  65        if (w < 0.0) {
  66            if (p0 == p1 && p2 == p3) {
  67                numParametricSegments = 1.0;
  68            } else {
  69                float _0_m = wangs_formula_max_fdiff_p2_ff2f2f2f2f22(p0, p1, p2, p3, AFFINE_MATRIX);
  70                numParametricSegments = max(ceil(sqrt(3.0 * sqrt(_0_m))), 1.0);
  71            }
  72        } else {
  73            float _1_n2 = wangs_formula_conic_p2_fff2f2f2f(PRECISION, AFFINE_MATRIX * p0, AFFINE_MATRIX * p1, AFFINE_MATRIX * p2, w);
  74            numParametricSegments = max(ceil(sqrt(_1_n2)), 1.0);
  75        }
  76        vec2 tan0 = robust_normalize_diff_f2f2f2(p0 == p1 ? (p1 == p2 ? p3 : p2) : p1, p0);
  77        vec2 tan1 = robust_normalize_diff_f2f2f2(p3, p3 == p2 ? (p2 == p1 ? p0 : p1) : p2);
  78        if (tan0 == vec2(0.0)) {
  79            tan0 = vec2(1.0, 0.0);
  80            tan1 = vec2(-1.0, 0.0);
  81        }
  82        float edgeID = float(gl_VertexID >> 1);
  83        if ((gl_VertexID & 1) != 0) {
  84            edgeID = -edgeID;
  85        }
  86        float numEdgesInJoin = 4.0;
  87        float turn = _determinant2(mat2(p2 - p0, p3 - p1));
  88        float combinedEdgeID = abs(edgeID) - numEdgesInJoin;
  89        if (combinedEdgeID < 0.0) {
  90            tan1 = tan0;
  91            if (lastControlPoint != p0) {
  92                tan0 = robust_normalize_diff_f2f2f2(p0, lastControlPoint);
  93            }
  94            turn = _determinant2(mat2(tan0, tan1));
  95        }
  96        float cosTheta = clamp(dot(tan0, tan1), -1.0, 1.0);
  97        float rotation = acos(cosTheta);
  98        if (turn < 0.0) {
  99            rotation = -rotation;
 100        }
 101        float numRadialSegments;
 102        float strokeOutset = sign(edgeID);
 103        if (combinedEdgeID < 0.0) {
 104            numRadialSegments = numEdgesInJoin - 2.0;
 105            numParametricSegments = 1.0;
 106            p3 = (p2 = (p1 = p0));
 107            combinedEdgeID += numRadialSegments + 1.0;
 108            float sinEpsilon = 0.01;
 109            bool tangentsNearlyParallel = abs(turn) * (1.0 / sqrt(dot(tan0, tan0) * dot(tan1, tan1))) < sinEpsilon;
 110            if (!tangentsNearlyParallel || dot(tan0, tan1) < 0.0) {
 111                if (combinedEdgeID >= 0.0) {
 112                    strokeOutset = turn < 0.0 ? min(strokeOutset, 0.0) : max(strokeOutset, 0.0);
 113                }
 114            }
 115            combinedEdgeID = max(combinedEdgeID, 0.0);
 116        } else {
 117            float maxCombinedSegments = (NUM_TOTAL_EDGES - numEdgesInJoin) - 1.0;
 118            numRadialSegments = max(ceil(abs(rotation) * NUM_RADIAL_SEGMENTS_PER_RADIAN), 1.0);
 119            numRadialSegments = min(numRadialSegments, maxCombinedSegments);
 120            numParametricSegments = min(numParametricSegments, (maxCombinedSegments - numRadialSegments) + 1.0);
 121        }
 122        float radsPerSegment = rotation / numRadialSegments;
 123        float numCombinedSegments = (numParametricSegments + numRadialSegments) - 1.0;
 124        bool isFinalEdge = combinedEdgeID >= numCombinedSegments;
 125        if (combinedEdgeID > numCombinedSegments) {
 126            strokeOutset = 0.0;
 127        }
 128        if (abs(edgeID) == 2.0) {
 129            float _2_x = ((cosTheta) * (0.5) + (0.5));
 130            strokeOutset *= (_2_x * JOIN_TYPE) * JOIN_TYPE >= 1.0 ? (1.0 / sqrt(_2_x)) : sqrt(_2_x);
 131        }
 132        vec2 tangent;
 133        vec2 strokeCoord;
 134        if (combinedEdgeID != 0.0 && !isFinalEdge) {
 135            vec2 A;
 136            vec2 B;
 137            vec2 C = p1 - p0;
 138            vec2 D = p3 - p0;
 139            if (w >= 0.0) {
 140                C *= w;
 141                B = 0.5 * D - C;
 142                A = (w - 1.0) * D;
 143                p1 *= w;
 144            } else {
 145                vec2 E = p2 - p1;
 146                B = E - C;
 147                A = ((vec2(-3.0)) * (E) + (D));
 148            }
 149            vec2 B_ = B * (numParametricSegments * 2.0);
 150            vec2 C_ = C * (numParametricSegments * numParametricSegments);
 151            float lastParametricEdgeID = 0.0;
 152            float maxParametricEdgeID = min(numParametricSegments - 1.0, combinedEdgeID);
 153            float negAbsRadsPerSegment = -abs(radsPerSegment);
 154            float maxRotation0 = (1.0 + combinedEdgeID) * abs(radsPerSegment);
 155            for (int _0_exp = 4;_0_exp >= 0; --_0_exp) {
 156                float testParametricID = lastParametricEdgeID + exp2(float(_0_exp));
 157                if (testParametricID <= maxParametricEdgeID) {
 158                    vec2 testTan = ((vec2(testParametricID)) * (A) + (B_));
 159                    testTan = ((vec2(testParametricID)) * (testTan) + (C_));
 160                    float cosRotation = dot(normalize(testTan), tan0);
 161                    float maxRotation = ((testParametricID) * (negAbsRadsPerSegment) + (maxRotation0));
 162                    maxRotation = min(maxRotation, PI);
 163                    if (cosRotation >= cos(maxRotation)) {
 164                        lastParametricEdgeID = testParametricID;
 165                    }
 166                }
 167            }
 168            float parametricT = lastParametricEdgeID / numParametricSegments;
 169            float lastRadialEdgeID = combinedEdgeID - lastParametricEdgeID;
 170            float angle0 = acos(clamp(tan0.x, -1.0, 1.0));
 171            angle0 = tan0.y >= 0.0 ? angle0 : -angle0;
 172            float radialAngle = ((lastRadialEdgeID) * (radsPerSegment) + (angle0));
 173            tangent = vec2(cos(radialAngle), sin(radialAngle));
 174            vec2 norm = vec2(-tangent.y, tangent.x);
 175            float a = dot(norm, A);
 176            float b_over_2 = dot(norm, B);
 177            float c = dot(norm, C);
 178            float discr_over_4 = max(b_over_2 * b_over_2 - a * c, 0.0);
 179            float q = sqrt(discr_over_4);
 180            if (b_over_2 > 0.0) {
 181                q = -q;
 182            }
 183            q -= b_over_2;
 184            float _5qa = (-0.5 * q) * a;
 185            vec2 root = abs(((q) * (q) + (_5qa))) < abs(((a) * (c) + (_5qa))) ? vec2(q, a) : vec2(c, q);
 186            float radialT = root.y != 0.0 ? root.x / root.y : 0.0;
 187            radialT = clamp(radialT, 0.0, 1.0);
 188            if (lastRadialEdgeID == 0.0) {
 189                radialT = 0.0;
 190            }
 191            float T = max(parametricT, radialT);
 192            vec2 ab = unchecked_mix_f2f2f2f(p0, p1, T);
 193            vec2 bc = unchecked_mix_f2f2f2f(p1, p2, T);
 194            vec2 cd = unchecked_mix_f2f2f2f(p2, p3, T);
 195            vec2 abc = unchecked_mix_f2f2f2f(ab, bc, T);
 196            vec2 bcd = unchecked_mix_f2f2f2f(bc, cd, T);
 197            vec2 abcd = unchecked_mix_f2f2f2f(abc, bcd, T);
 198            float u = ((w - 1.0) * (T) + (1.0));
 199            float v = (w + 1.0) - u;
 200            float uv = ((v - u) * (T) + (u));
 201            if (T != radialT) {
 202                tangent = w >= 0.0 ? robust_normalize_diff_f2f2f2(bc * u, ab * v) : robust_normalize_diff_f2f2f2(bcd, abc);
 203            }
 204            strokeCoord = w >= 0.0 ? abc / uv : abcd;
 205        } else {
 206            tangent = combinedEdgeID == 0.0 ? tan0 : tan1;
 207            strokeCoord = combinedEdgeID == 0.0 ? p0 : p3;
 208        }
 209        vec2 ortho = vec2(tangent.y, -tangent.x);
 210        strokeCoord += ortho * (STROKE_RADIUS * strokeOutset);
 211        vec2 devCoord = AFFINE_MATRIX * strokeCoord + TRANSLATE;
 212        gl_Position = vec4(devCoord, 0.0, 1.0);
 213        gl_Position = vec4(gl_Position.xy * sk_RTAdjust.xz + gl_Position.ww * sk_RTAdjust.yw, 0.0, gl_Position.w);
 214    }
 215    
Errors:
ERROR: 0:82: Use of undeclared identifier 'gl_VertexID'
ERROR: 0:83: Use of undeclared identifier 'gl_VertexID'
ERROR: 0:84: Use of undeclared identifier 'edgeID'
ERROR: 0:84: Use of undeclared identifier 'edgeID'
ERROR: 0:88: Use of undeclared identifier 'edgeID'
ERROR: 0:89: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:102: Use of undeclared identifier 'edgeID'
ERROR: 0:103: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:107: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:111: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:112: Use of undeclared identifier 'strokeOutset'
ERROR: 0:112: Use of undeclared identifier 'strokeOutset'
ERROR: 0:112: Use of undeclared identifier 'strokeOutset'
ERROR: 0:115: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:115: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:124: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:125: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:126: Use of undeclared identifier 'strokeOutset'
ERROR: 0:128: Use of undeclared identifier 'edgeID'
ERROR: 0:130: Use of undeclared identifier 'strokeOutset'
ERROR: 0:134: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:134: Use of undeclared identifier 'isFinalEdge'
ERROR: 0:152: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:154: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:157: Use of undeclared identifier 'maxParametricEdgeID'
ERROR: 0:161: Use of undeclared identifier 'maxRotation0'
ERROR: 0:162: Use of undeclared identifier 'maxRotation'
ERROR: 0:162: Use of undeclared identifier 'maxRotation'
ERROR: 0:163: Use of undeclared identifier 'maxRotation'
ERROR: 0:169: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:172: Use of undeclared identifier 'lastRadialEdgeID'
ERROR: 0:173: Use of undeclared identifier 'radialAngle'
ERROR: 0:173: Use of undeclared identifier 'radialAngle'
ERROR: 0:188: Use of undeclared identifier 'lastRadialEdgeID'
ERROR: 0:206: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:207: Use of undeclared identifier 'combinedEdgeID'
ERROR: 0:210: Use of undeclared identifier 'strokeOutset'

I assume it is the same issue—a bad GL stack on macOS—but I wanted to note it here for completeness.

HinTak commented 4 months ago

I am swtching on the METAL backend on mac os x for m125: https://github.com/kyamagu/skia-python/pull/244/commits/a4e875b66e4a408644e5f11ad5282fef9877b4ca ( on #244 ) - since I don't even own a mac, this is just blindly adding Metal code which has some Vulkan equivalent out. See if that can be used to improve things.

HinTak commented 4 months ago

244 metal-enabled build finishes. I don't know how to actually use it, but essentially all the vulkan Related API (I think there are 9 code blocks) has a Metal equivalent now. Contribution of examples / tests welcomed.

@kyamagu sorry about the noise with force-push and CI failures...

HinTak commented 4 months ago

v87.6 is out (should land in pypi in a few hours), mainly it is a build-matrix update.

HinTak commented 4 months ago

The build matrix update switches the macos build host from 11 to 12. I'd be interested to know if that, and python 12, has any impact on the shader errors appearing or disappearing (on the same hardware).

stenson commented 4 months ago

@HinTak i haven't had any luck yet with figuring out metal on macos with the m125 version, but the new 87.6 version works great for me on python3.12, no shader errors at all, as far as I can tell it performs just as well as python3.11 + 87.5

HinTak commented 4 months ago

okay, thanks for letting me know. We'll probably need to look for the shader error upstream... it is a bit unfortunate that it seems to be mac os x only. I'd like to know if @pavpanchekha has it with 87.6 or not.

pavpanchekha commented 4 months ago

I don't get any shader errors on 87.6 with Python 3.12. I do still get that mysterious segfault I told you about, HinTak, so probably that's not Skia-related. But it does now work great, no shader errors, everything shows up.

HinTak commented 4 months ago

A while ago we had somebody else's code doing intermittent/mysterious segfaults, and it turns out that it was a use-after-going-out-of-scope problem: referring to out-of-scope python/skia variables and structures sometimes work, as it depends on when the python interpreter removes it (that can be a while), so sometimes works and sometimes segfault. Keeping some variables longer/global was the solution.

I am at a loss about the shader error. Some people online say it can be caused by leftovers in the GPUCache (for mesa/linux), and clearing it would do. I couldn't find out where the mac os x equivalent is. That maybe an answer, as some cached files are probably invalid if you switch between 87 to 1xx. One thing to try is perhaps set up a 2nd user on your mac: GPUCache is per user (although i don't know where it is or how to clear it on mac os; I do know where it is on Linux/mesa), so you can start with an empty cache if you set up a new user, and don't ever run skia 87 on it, start with 1xx. See if that works?

pavpanchekha commented 4 months ago

I can try that, but not this coming week.

stenson commented 4 months ago

Thanks for the idea @HinTak, unfortunately I just created a new user on my mac and installed the 124.0b7 release without ever running an 87 skia-python with that user and I got all the same shader errors as I get when switching back and forth between an 87 install and a 1xx install. I'll try to look into that angle more, though.

HinTak commented 4 months ago

I am wondering if I can modify these lines:

   while (glfw.get_key(window, glfw.KEY_ESCAPE) != glfw.PRESS
            and not glfw.window_should_close(window)):
            glfw.wait_events()

To something like, display the window for 10 seconds then just quit by itself. That way, I can send it into github's CI as part of post-build tests.

stenson commented 4 months ago

@HinTak on my computer you can actually get rid of the while loop entirely and "Shader compilation error" will still be printed to stderr directly and the program will exit, i.e. with these two files I can run python glfw_test.py and immediately get a result on whether the shader failed to compile. Does that work for CI ?

minimal_glfw.py

import contextlib, glfw, skia
from OpenGL import GL

WIDTH, HEIGHT = 640, 480

path = skia.Path()
path.moveTo(184, 445)
path.lineTo(249, 445)
path.quadTo(278, 445, 298, 438)
path.quadTo(318, 431, 331, 419)
path.quadTo(344, 406, 350, 390)
path.quadTo(356, 373, 356, 354)
path.quadTo(356, 331, 347, 312)
path.quadTo(338, 292, 320, 280) # <- comment out this line and shape will draw correctly with anti-aliasing
path.close()

@contextlib.contextmanager
def glfw_window():
    if not glfw.init():
        raise RuntimeError('glfw.init() failed')
    glfw.window_hint(glfw.STENCIL_BITS, 8)
    window = glfw.create_window(WIDTH, HEIGHT, '', None, None)
    glfw.make_context_current(window)
    yield window
    glfw.terminate()

@contextlib.contextmanager
def skia_surface(window):
    context = skia.GrDirectContext.MakeGL()
    (fb_width, fb_height) = glfw.get_framebuffer_size(window)
    backend_render_target = skia.GrBackendRenderTarget(
        fb_width,
        fb_height,
        0,  # sampleCnt
        0,  # stencilBits
        skia.GrGLFramebufferInfo(0, GL.GL_RGBA8))
    surface = skia.Surface.MakeFromBackendRenderTarget(
        context, backend_render_target, skia.kBottomLeft_GrSurfaceOrigin,
        skia.kRGBA_8888_ColorType, skia.ColorSpace.MakeSRGB())
    assert surface is not None
    yield surface
    context.abandonContext()

with glfw_window() as window:
    GL.glClear(GL.GL_COLOR_BUFFER_BIT)

    with skia_surface(window) as surface:
        with surface as canvas:
            canvas.drawCircle(100, 100, 40, skia.Paint(Color=skia.ColorGREEN, AntiAlias=True))

            paint = skia.Paint(Color=skia.ColorBLUE)
            paint.setStyle(skia.Paint.kStroke_Style)
            paint.setStrokeWidth(2)
            paint.setAntiAlias(True)

            canvas.drawPath(path, paint)

        surface.flushAndSubmit()
        glfw.swap_buffers(window)

glfw_test.py

import subprocess

result = subprocess.run(["python", "minimal_glfw.py"], capture_output=True)

failed = "Shader compilation error" in result.stderr.decode("utf-8")

print("failed", failed)
HinTak commented 4 months ago

I have put a slightly straightened version of the code into https://github.com/HinTak/skia-m1xx-python/pull/6 and change it to only build for mac os. (So it will finish either way in about 20 minutes rather than 5 hours - building for aarch64 linux is very slow...). Just to confirm, the code fails with a bang, right? Instead of drawing with flaws and exit normally with piles of messages on stderr?

I just cut it out, prepend it with import skia, cut out 4 spaces from each line, run it directly on Linux against 126b8 (it just been tagged so should land public any time now). It pops out a quick window, then exits quietly and normally. In CI against m120 (just a convenient old pull I can put extra stuff in), it segfaults against 3.9 - I'll play in that pull a bit...

stenson commented 4 months ago

I think that's all correct — I just ran your test_ShaderError with python3.12 & skia-python==124.0b7 and I got this on stderr (the same error we've been seeing before):

Shader compilation error
------------------------
   1    #version 110
   2
   3    uniform vec4 sk_RTAdjust;
   4    uniform vec2 uatlas_adjust_S0;
   5    attribute vec4 fillBounds;
   6    attribute vec4 color;
   7    attribute vec4 locations;
   8    varying vec2 vatlasCoord_S0;
   9    varying vec4 vcolor_S0;
  10    void main() {
  11        vec2 unitCoord = vec2(float(gl_VertexID & 1), float(gl_VertexID >> 1));
  12        vec2 devCoord = mix(fillBounds.xy, fillBounds.zw, unitCoord);
  13        vec2 atlasTopLeft = vec2(abs(locations.x) - 1.0, locations.y);
  14        vec2 devTopLeft = locations.zw;
  15        bool transposed = locations.x < 0.0;
  16        vec2 atlasCoord = devCoord - devTopLeft;
  17        if (transposed) {
  18            atlasCoord = atlasCoord.yx;
  19        }
  20        atlasCoord += atlasTopLeft;
  21        vatlasCoord_S0 = atlasCoord * uatlas_adjust_S0;
  22        vcolor_S0 = color;
  23        gl_Position = vec4(devCoord, 0.0, 1.0);
  24        gl_Position = vec4(gl_Position.xy * sk_RTAdjust.xz + gl_Position.ww * sk_RTAdjust.yw, 0.0, gl_Position.w);
  25    }
  26
Errors:
ERROR: 0:11: Use of undeclared identifier 'gl_VertexID'
ERROR: 0:11: Use of undeclared identifier 'gl_VertexID'
ERROR: 0:12: Use of undeclared identifier 'unitCoord'
ERROR: 0:16: Use of undeclared identifier 'devCoord'
ERROR: 0:18: Use of undeclared identifier 'atlasCoord'
ERROR: 0:18: Use of undeclared identifier 'atlasCoord'
ERROR: 0:20: Use of undeclared identifier 'atlasCoord'
ERROR: 0:21: Use of undeclared identifier 'atlasCoord'
ERROR: 0:23: Use of undeclared identifier 'devCoord'
HinTak commented 4 months ago

BTW, Metal-enabled build is out: https://github.com/kyamagu/skia-python/releases/tag/v126.0b8

On CI against m120, the code is failing on multiple places:

context = skia.GrDirectContext.MakeGL()
assert context is not None                                # assert here

(fb_width, fb_height) = glfw.get_framebuffer_size(window) # segfault against 3.9, 3.11

assert surface is not None
# strerr "GLFWError: (65545) b'NSGL: Failed to find a suitable pixel format'"

@kyamagu do you remember why there is a lineCIBW_TEST_REQUIRES_MACOS: pytest pillow pyopengl (missing glfw) vs CIBW_TEST_REQUIRES: pytest pillow glfw in the CI yml?

kyamagu commented 4 months ago

@HinTak I remember that for some reason, pyopengl works, but glfw doesn't on the CI. Didn't look in depth at that time.

HinTak commented 4 months ago

Hi, @stenson and @pavpanchekha : Can I have both of you inserting this snipplet just after the context = skia.GrDirectContext.MakeGL() line (this snipplet needs a valid GL context, so it needs to be insert a bit after the windows creation, etc), for both under m87 and under current (m1xx)? ie. I want 2 x 4 lines, from both of you.

from OpenGL.GL import glGetString, GL_VENDOR, GL_RENDERER, GL_VERSION, GL_SHADING_LANGUAGE_VERSION
print(glGetString(GL_VENDOR))
print(glGetString(GL_RENDERER))
print(glGetString(GL_VERSION))
print(glGetString(GL_SHADING_LANGUAGE_VERSION))

Mine reports now:

b'AMD'
b'AMD Radeon R5 Graphics (radeonsi, stoney, LLVM 18.1.1, DRM 3.57, 6.8.10-300.fc40.x86_64)'
b'4.5 (Compatibility Profile) Mesa 24.0.8'
b'4.50'

(was 6.8.9-300.fc40.x86_64 and 24.0.7 before the most recent reboot)

Github CI reports (when one gets a GL context - see further below):

 # mac os x 12 
 "Apple Inc."
 "Apple Software Renderer"
 "2.1 APPLE-19.5.1"
 "1.20"

# mac os x 13
 "Apple Inc."
 "Apple Software Renderer"
 "2.1 APPLE-20.5.2"
 "1.20"

# mac os x 14
 "Apple Inc."
 "Apple Software Renderer"
 "2.1 APPLE-21.0.19"
 "1.20"

@kyamagu I think you are right, pyopengl (its GLUT part) works, but glfw does not work in CI.

HinTak commented 4 months ago

Hi, @stenson and @pavpanchekha - I also checked upstream m87 and m116 diff - they added a whole lot of conditionals based on those 4 answers (which is specific to your hard/ware software combination - mine is simple and quite generic - "Mesa" is basically the generic software render on linux; but if I had a NVidia graphic card and using their official graphics driver I would go down the non-Mesa code path).

According to https://www.glfw.org/faq#macos , glfw should have these 4 lines (<-). If your report some very low/wrong values, can you see if these improve the situation?

    if not glfw.init():
        raise RuntimeError('glfw.init() failed')
    glfw.window_hint(glfw.VISIBLE, glfw.FALSE)
    glfw.window_hint(glfw.STENCIL_BITS, 8)
    # see https://www.glfw.org/faq#macos
    glfw.window_hint(glfw.CONTEXT_VERSION_MAJOR, 3)                    #    <- 
    glfw.window_hint(glfw.CONTEXT_VERSION_MINOR, 2)                     #  <-
    glfw.window_hint(glfw.OPENGL_FORWARD_COMPAT, True)                     #  <-
    glfw.window_hint(glfw.OPENGL_PROFILE, glfw.OPENGL_CORE_PROFILE)    #   <-
    context = glfw.create_window(640, 480, '', None, None)

glfw simply doesn't work on github CI, with or without these 4 lines, but pyopengl (it GLUT part) does.

stenson commented 4 months ago

@HinTak thanks for looking into that! I'm seeing this on my setup:

b'Apple'
b'Apple M3 Pro'
b'2.1 Metal - 88'
b'1.20'

And the four additional lines mentioned do seem to completely fix the issue both for this test as well as for the coldtype library! Apologies that it hadn't occurred to me that glfw config might be the issue here.

Assuming this works for other macOS setups, would it make sense for those lines to be added to the glfw examples in this repo?

pavpanchekha commented 4 months ago

Hi @HinTak, here's what I get:

b'Apple'
b'Apple M1'
b'2.1 Metal - 88.1'
b'1.20'

I'm not using glfw, I'm using SDL2. Should I try to use glfw instead and try those four lines?

HinTak commented 4 months ago

@pavpanchekha I happened to have seen the SDL2 equivalent for those 4 lines while I was reading up on it. According to https://stackoverflow.com/questions/20931528/shader-cant-be-compiled, 3 of them are:

SDL_GL_SetAttribute(SDL_GL_CONTEXT_MAJOR_VERSION, 3);
SDL_GL_SetAttribute(SDL_GL_CONTEXT_MINOR_VERSION, 2); 
SDL_GL_SetAttribute(SDL_GL_CONTEXT_PROFILE_MASK, SDL_GL_CONTEXT_PROFILE_CORE);

This is in C code, so something similar should be possible with python SDL. Could you try to look it up and post in here?

HinTak commented 4 months ago

@stenson thanks for confirming that. I am almost certain that the difference you see is due to changes in this file , e.g. https://github.com/google/skia/blob/229d94a8807ea5d2e3d5ecef898a7a1146ca8840/src/gpu/ganesh/gl/GrGLCaps.cpp#L4695 Between m87 and m116, google folks added a lot of "if Apple / if metal" in the file similar to this line. I am glad we get to the bottom of it now. Yes, we should add those 4 lines to our documentation / code. As I have some exchange with @kyamagu above, actually mac os x testing inside CI has never worked (even in m87 days) and so the examples have always been largely Linux based... as it turns out, linux/mesa and Apple's implementation of opengl differs a bit, and upstream skia actively exploits those differences.

I am trying to figure out why glfw testing for mac os x never worked: https://github.com/FlorianRhiem/pyGLFW/issues/80

HinTak commented 4 months ago

@pavpanchekha found the SDL python equivalent in https://gist.github.com/hurricanerix/3be8221128d943ae2827 :

sdl2.SDL_GL_SetAttribute(sdl2.SDL_GL_CONTEXT_MAJOR_VERSION, 3)
sdl2.SDL_GL_SetAttribute(sdl2.SDL_GL_CONTEXT_MINOR_VERSION, 2)
sdl2.SDL_GL_SetAttribute(sdl2.SDL_GL_CONTEXT_FORWARD_COMPATIBLE_FLAG, 1)
sdl2.SDL_GL_SetAttribute(sdl2.SDL_GL_CONTEXT_PROFILE_MASK, sdl2.SDL_GL_CONTEXT_PROFILE_CORE)

edit: added the 3rd line as best I can see.

pavpanchekha commented 4 months ago

Ok, I tried this:

            sdl2.SDL_GL_SetAttribute(sdl2.SDL_GL_CONTEXT_MAJOR_VERSION, 3)
            sdl2.SDL_GL_SetAttribute(sdl2.SDL_GL_CONTEXT_MINOR_VERSION, 2)
            sdl2.SDL_GL_SetAttribute(sdl2.SDL_GL_CONTEXT_FORWARD_COMPATIBLE_FLAG, True)
            sdl2.SDL_GL_SetAttribute(sdl2.SDL_GL_CONTEXT_PROFILE_MASK,
                                     sdl2.SDL_GL_CONTEXT_PROFILE_CORE)

It worked: no shader compiler errors. I'll add it to our porting page.

HinTak commented 4 months ago

@pavpanchekha thanks for the confirmation!

@stenson you mentioned that you saw some slow down with m1xx compared to m87. Do you still see it now?

@pavpanchekha @stenson I'll update README.m116, the examples/tutorials, and the CI (this last one still doesn't work, but might as well prepare for when it does) soon. Think of anywhere else I should insert a note/snipplet of code in? Currently I have examples for glfw, sdl2 and glut settings.

The way I understand this issue now, is that, between m87 and m116, Google folks added Apple-specific hardware/software detection code in skia, to optimze performance and/or work around graphics hardware/software bugs. So it is now necessary to request a matching OpenGL profile expected from the new code paths for best behavior from Apple hardware. In m87, it is likely that Apple users were getting some kind of generic non-Mesa/non-Nvidia behaviour. So @stenson it might be interesting to know if you see any improvements in speed over m87 now.

HinTak commented 4 months ago

This development in skia in the last few years is probably driven by Apple switching to Arm hardware on the desktop - I see on github CI the mac os x 14 runner is arm-based, and is about 1/2 the speed of of the Intel based 12/13 runners in building skia. (So optimization becomes important).

pavpanchekha commented 4 months ago

Thanks for all your help, HinTak. I don't see anything else you might need to add.

stenson commented 4 months ago

All sounds great to me. I don't have any hard stats on performance, but it does seem equal if not better just from looking at some informal numbers from my library.

HinTak commented 4 months ago

When #249 get merged, this should automatically close. No need to do it by hand.

The glfw CI testing issue will continue in https://github.com/FlorianRhiem/pyGLFW/issues/80 . There is an update of Google's own Skia & SDL c++ example to m126 in https://github.com/HinTak/skia-c-examples/ (it was removed in m93 when they want to get rid of all SDL dependency within Skia's repo), and https://github.com/HinTak/skia-python-examples/ contains three versions of this python example code, one for each of glfw, OSX glut, and pysdl2, as well as a port of the google SDL c++ example to skia-python & PySDL2 . @pavpanchekha I think you might be interested to read it - there is something clever from Google using skia to render directly to SDL's window.

@pavpanchekha @stenson there is a paypal link somewhere on my profile page, if you feel like buying me a few cups of coffee :-).

HinTak commented 4 months ago

The glfw issue has gotten further upstream: https://github.com/glfw/glfw/issues/2570