m116/m117 API gaps against m87.5

[jonathanhogg]

I'm a bit confused about this as the M116 migration docs says that Surface.MakeFromBackendRenderTarget is one of the "simple renamings", that "Where it is possible, when m87 APIs disappear, emulations with m116 is done" and that "we might add the new names and remove the old names, and document them as renamed".

However, it seems to have just disappeared completely. Has it been renamed? Or is thee just another way to do this now that I'm missing. I can't seem to make sense of the Skia class reference documentation on this.

[HinTak]

Both... :-). Apparently the same functionality is provided by "SkSurfaces::WrapBackendRenderTarget", but the transfer hasn't been straightforward, so it is disabled for the time being. Thanks for the report. If you read up on what that does and how is it different, that would be useful...

[jonathanhogg]

Thanks, but I am failing to find any documentation for this method at all. Can you point me at something?

[HinTak]

Okay, testing this HinTak/skia-m1xx-python@8f9d266 . If CI is successful, what platform are you on? I can attach a m117b2 wheel binary here.

edit: should be this instead
HinTak/skia-m1xx-python@d80e9c8

[jonathanhogg]

I'm on macOS 64-bit Intel, but can build from source if that's easier.

For reference, I'm wrapping an OpenGL framebuffer (constructed with moderngl) in a Surface. Code excerpt looks like:

            depth = COLOR_FORMATS[colorbits]
            self._texture = self.glctx.texture((self.width, self.height), 4, dtype=depth.moderngl_dtype)
            self._framebuffer = self.glctx.framebuffer(color_attachments=(self._texture,))
            backend_render_target = skia.GrBackendRenderTarget(self.width, self.height, 0, 0, skia.GrGLFramebufferInfo(self._framebuffer.glo, depth.gl_format))
            colorspace = skia.ColorSpace.MakeSRGBLinear() if linear else skia.ColorSpace.MakeSRGB()
            self._surface = skia.Surface.MakeFromBackendRenderTarget(self._graphics_context, backend_render_target, skia.kBottomLeft_GrSurfaceOrigin,
                                                                     depth.skia_colortype, colorspace)
            self._canvas = self._surface.getCanvas()

COLOR_FORMATS here encapsulates a bunch of type enumerations, like so:

ColorFormat = namedtuple('ColorFormat', ('moderngl_dtype', 'gl_format', 'skia_colortype'))

COLOR_FORMATS = {
    8: ColorFormat('f1', GL_RGBA8, skia.kRGBA_8888_ColorType),
    16: ColorFormat('f2', GL_RGBA16F, skia.kRGBA_F16_ColorType),
    32: ColorFormat('f4', GL_RGBA32F, skia.kRGBA_F32_ColorType)  # Canvas currently fails with 32bit color
}

[jonathanhogg]

OK, have managed to get your m117-public-try branch to build (not entirely obvious from the build instructions, but managed to get it working following the CI scripts) and am having a go with that.

I had to make this patch to get it to build:

diff --git a/skia b/skia
--- a/skia
+++ b/skia
@@ -1 +1 @@
-Subproject commit 26ce945468bd635624e76ff8a1f964284f16b33f
+Subproject commit 26ce945468bd635624e76ff8a1f964284f16b33f-dirty
diff --git a/src/skia/Surface.cpp b/src/skia/Surface.cpp
index 7d9ea8d..93e78ef 100644
--- a/src/skia/Surface.cpp
+++ b/src/skia/Surface.cpp
@@ -723,7 +723,7 @@ surface
         [] (SkSurface& surface, bool syncCpu) {
             auto direct = GrAsDirectContext(surface.recordingContext());
             if (direct) {
-                direct->flush(&surface, SkSurface::BackendSurfaceAccess::kNoAccess, GrFlushInfo());
+                direct->flush(&surface, SkSurfaces::BackendSurfaceAccess::kNoAccess, GrFlushInfo());
                 direct->submit(syncCpu);
             }
         },

My code is now failing with the following error:

      | TypeError: MakeFromBackendRenderTarget(): incompatible function arguments. The following argument types are supported:
    1. (context: skia.GrRecordingContext, backendRenderTarget: skia.GrBackendRenderTarget, origin: skia.GrSurfaceOrigin, colorType: skia.ColorType, colorSpace: skia.ColorSpace, surfaceProps: skia.SurfaceProps = None, releaseProc: void (void*) = None, releaseContext: capsule = None) -> skia.Surface

Invoked with: <skia.GrContext object at 0x10cdd1c70>, <skia.GrBackendRenderTarget object at 0x1119e8530>, <GrSurfaceOrigin.kBottomLeft_GrSurfaceOrigin: 1>, <ColorType.kRGBA_8888_ColorType: 4>, <skia.ColorSpace object at 0x1114f34b0>

I am creating the context with:

self._graphics_context = skia.GrDirectContext.MakeGL()

so I had expected it to be a GrDirectContext (and thus a subclass of GrRecordingContext) not a GrContext.

Will poke some more and report back…

[jonathanhogg]

Can confirm that m87.5 definitely returned a GrDirectContext object.

Looking at skia/GrContext.cpp there have been a significant number of changes to the class structure and I'm not feeling confident trying to parse it, so I'm going to leave this alone for a bit in case you can see more immediately what the problem is, @HinTak.

[HinTak]

skia.GrDirectContext should be an alias to skia.GrContext - upstream changes. Can you add "none", "none" to the end and see if that works?

[jonathanhogg]

Yes, I just noticed that. Adding the Nones makes no difference. Unfortunately, the error isn't making it clear exactly which argument it is unhappy with.

(Also, would it make more sense to have GrContext be an alias of GrDirectContext going by the official naming of the classes in the skia docs?)

[jonathanhogg]

Have boiled down a small failing repro from my code to aid in debugging (needs glfw and moderngl installed):

import skia
import glfw
import moderngl

glfw.init()
glfw.window_hint(glfw.CLIENT_API, glfw.OPENGL_API)
glfw.window_hint(glfw.OPENGL_PROFILE, glfw.OPENGL_CORE_PROFILE)
glfw.window_hint(glfw.CONTEXT_VERSION_MAJOR, 4)
glfw.window_hint(glfw.CONTEXT_VERSION_MINOR, 1)
glfw.window_hint(glfw.OPENGL_FORWARD_COMPAT, glfw.TRUE)
glfw.window_hint(glfw.DOUBLEBUFFER, glfw.TRUE)
glfw.window_hint(glfw.SAMPLES, 0)
glfw.window_hint(glfw.AUTO_ICONIFY, glfw.FALSE)
glfw.window_hint(glfw.CENTER_CURSOR, glfw.FALSE)
glfw.window_hint(glfw.RESIZABLE, glfw.FALSE)
window = glfw.create_window(100, 100, "Test", None, None)
glfw.make_context_current(window)
glctx = moderngl.create_context(410)
width, height = glfw.get_framebuffer_size(window)
glctx.screen.viewport = (0, 0, width, height)
GL_RGBA8 = 32856

graphics_context = skia.GrDirectContext.MakeGL()
texture = glctx.texture((width, height), 4, dtype='f1')
framebuffer = glctx.framebuffer(color_attachments=(texture,))
framebuffer_info = skia.GrGLFramebufferInfo(framebuffer.glo, GL_RGBA8)
backend_render_target = skia.GrBackendRenderTarget(width, height, 0, 0, framebuffer_info)
colorspace = skia.ColorSpace.MakeSRGB()
surface = skia.Surface.MakeFromBackendRenderTarget(graphics_context, backend_render_target,
                                                   skia.kBottomLeft_GrSurfaceOrigin,
                                                   skia.kRGBA_8888_ColorType, colorspace)
canvas = surface.getCanvas()

while True:
    framebuffer.clear()
    graphics_context.resetContext()
    path = skia.Path()
    path.addRect(0, 0, width, height)
    paint = skia.Paint()
    paint.setColor(0xff00ff00)
    paint.setStyle(skia.Paint.Style.kFill_Style)
    canvas.drawPath(path, paint)
    surface.flushAndSubmit()
    glctx.copy_framebuffer(glctx.screen, framebuffer)
    glfw.swap_buffers(window)
    glfw.poll_events()

[Edit: More complete example that can be run under m87.5 successfully (at least on macOS). Result should be a green square window. The code is drawing a green rectangle into an off-screen GL framebuffer and then copying this into the window framebuffer.]

[HinTak]

The other possibility is to switch
skia.GrDirectContext.MakeGL() to skia.GrContext.MakeGL(). Yes, I wrote in the readme.m116 that GrDirectContext is the proper one aligned with upstream. The short one is just shorter :-). Basically one of them get all the methods of the other. I put in an alias, but maybe the alias doesn't work too well.

[HinTak]

The actual names being picked is near line 526, and the alias is at line 1198. If you modified the end of the first to "GrDirectContext", and switch the two sides of line 1198 around? Am afraid if it is some subtlety about inheritance, etc, things are going to break in one way or the other.

[HinTak]

Here is the first place :

skia-python/src/skia/GrContext.cpp

Line 526 in f71fdee

py::class_<GrDirectContext, sk_sp<GrDirectContext>, GrRecordingContext>(m, "GrContext")

, the 2nd place is 6 lines from the end.

[jonathanhogg]

Sadly neither change makes a difference (although having the type called GrDirectContext makes more sense to me).

From the Python side, everything seems to be the type it is supposed to be:

In [3]: isinstance(graphics_context, skia.GrRecordingContext)
Out[3]: True

In [4]: isinstance(backend_render_target, skia.GrBackendRenderTarget)
Out[4]: True

In [5]: isinstance(skia.GrSurfaceOrigin.kBottomLeft_GrSurfaceOrigin, skia.GrSurfaceOrigin)
Out[5]: True

In [6]: isinstance(colorspace, skia.ColorSpace)
Out[6]: True

In [7]: isinstance(skia.ColorType.kRGBA_8888_ColorType, skia.ColorType)
Out[7]: True

So the issue seems to be somewhere in the C++ wrapping and what types it thinks the arguments are. Sadly, I can see no obvious errors in the definitions.

[jonathanhogg]

OK, good news! After mucking about a bit trying to figure out what is different between m117 and m87.5 in the definitions, I have made it work.

Here's my diffs:

diff --git a/skia b/skia
--- a/skia
+++ b/skia
@@ -1 +1 @@
-Subproject commit 26ce945468bd635624e76ff8a1f964284f16b33f
+Subproject commit 26ce945468bd635624e76ff8a1f964284f16b33f-dirty
diff --git a/src/skia/GrContext.cpp b/src/skia/GrContext.cpp
index fcb52ec0..1e974c93 100644
--- a/src/skia/GrContext.cpp
+++ b/src/skia/GrContext.cpp
@@ -523,7 +523,7 @@ py::class_<GrRecordingContext, sk_sp<GrRecordingContext>, GrImageContext>(
     //     py::overload_cast<>(&GrRecordingContext::priv, py::const_))
     ;
 
-py::class_<GrDirectContext, sk_sp<GrDirectContext>, GrRecordingContext>(m, "GrContext")
+py::class_<GrDirectContext, sk_sp<GrDirectContext>, GrRecordingContext>(m, "GrDirectContext")
     .def("resetContext", &GrDirectContext::resetContext,
         R"docstring(
         The :py:class:`GrContext` normally assumes that no outsider is setting
@@ -1195,7 +1195,7 @@ py::class_<GrDirectContext, sk_sp<GrDirectContext>, GrRecordingContext>(m, "GrCo
         )docstring")
     ;
 
-m.attr("GrDirectContext") = m.attr("GrContext");
+m.attr("GrContext") = m.attr("GrDirectContext");
 
 initGrContext_gl(m);
 initGrContext_vk(m);
diff --git a/src/skia/Surface.cpp b/src/skia/Surface.cpp
index 7d9ea8d4..565441bc 100644
--- a/src/skia/Surface.cpp
+++ b/src/skia/Surface.cpp
@@ -723,7 +723,7 @@ surface
         [] (SkSurface& surface, bool syncCpu) {
             auto direct = GrAsDirectContext(surface.recordingContext());
             if (direct) {
-                direct->flush(&surface, SkSurface::BackendSurfaceAccess::kNoAccess, GrFlushInfo());
+                direct->flush(&surface, SkSurfaces::BackendSurfaceAccess::kNoAccess, GrFlushInfo());
                 direct->submit(syncCpu);
             }
         },
@@ -1055,9 +1055,9 @@ surface
         [] (GrRecordingContext* context, const GrBackendRenderTarget& target,
             GrSurfaceOrigin origin, SkColorType colorType,
             sk_sp<SkColorSpace> colorSpace,
-            const SkSurfaceProps* surfaceProps, SkSurfaces::RenderTargetReleaseProc releaseProc, SkSurfaces::ReleaseContext releaseContext) {
+            const SkSurfaceProps* surfaceProps) {
             return SkSurfaces::WrapBackendRenderTarget(
-                context, target, origin, colorType, colorSpace, surfaceProps, releaseProc, releaseContext);
+                context, target, origin, colorType, colorSpace, surfaceProps, NULL, NULL);
         },
         R"docstring(
         Wraps a GPU-backed buffer into :py:class:`Surface`.
@@ -1090,7 +1090,7 @@ surface
         )docstring",
         py::arg("context"), py::arg("backendRenderTarget"), py::arg("origin"),
         py::arg("colorType"), py::arg("colorSpace"),
-        py::arg("surfaceProps") = nullptr, py::arg("releaseProc") = nullptr, py::arg("releaseContext") = nullptr)
+        py::arg("surfaceProps") = nullptr)
     .def_static("MakeRenderTarget",
         py::overload_cast<GrRecordingContext*, skgpu::Budgeted, const SkImageInfo&, int,
         GrSurfaceOrigin, const SkSurfaceProps*, bool>(

It seems that something in pybind11 is not liking the releaseProc and releaseContext argument types – even when not supplied.

[jonathanhogg]

OK, so my stuff seems to work flawlessly now except for skia.Image.MakeFromTexture().

May take a break before looking at that one… 😉

[jonathanhogg]

I've managed to get skia.Image.MakeFromTexture() working – it wasn't hard, just needed a header included and commenting the code back in.

Testing a bunch of my examples, the remaining gap I have is skia.Shaders.Lerp(), which I can see no replacement for and no way to emulate it using what shaders exists, which is a shame.

[jonathanhogg]

Figured out how to replace Lerp() as well. So I think all my stuff works now. I've collected up my patches in the linked PR.

There are some other bits that should probably be changed to match what I've done (like Surface.MakeFromBackendTexture(), which I assume is also going to be broken).

[HinTak]

Thanks a lot! I'll tidy up and make it pass through CI before including your work in #197 .

[jonathanhogg]

No worries. I was just looking at the tests: commenting back in the Blend and Lerp shader tests works for me.

[HinTak]

I'll say we'll release 117.0b3, and try not to put anymore comment-out blocks in, when m118 comes out. We have about 115 skipped tests at the moment, down from 125 since 116.0b2. This way, by about 130b16, we'd reach "no unaccounted for regression" and ready for 131 proper :-). @kyamagu

[kyamagu]

Thanks!

[HinTak]

M118 is out so I think we can release v117.0b3 in the next few days. Updating the Readme at the moment, so closing this.