Dev/graphite backend#236
Open
ramezgerges wants to merge 14 commits into
Open
Conversation
5 tasks
Adds an sk_graphite_* C ABI living inside :core so SkiaSharp's P/Invoke
layer can drive Graphite without binding directly to C++. Mirrors the
existing sk_gr_context_* / gr_* shim shape:
include/c/sk_graphite.h -- core: context, recorder, recording,
surface, image, backend texture,
texture info, async readback,
ImageProvider hook.
include/c/sk_graphite_vulkan.h -- Vulkan-only entry points
include/c/sk_graphite_metal.h -- Metal-only entry points
include/c/sk_graphite_dawn.h -- Dawn-only entry points
src/c/sk_graphite{,_vulkan,_metal,_dawn}.cpp
The implementations are wrapped in `#if defined(SK_GRAPHITE)` etc., with
no-op stubs in the else branch so the C ABI surface is consistent
whether or not the backend is built.
BUILD.gn / gn/core.gni: the :graphite target's all_dependent_configs
apply SK_GRAPHITE/SK_DAWN to dependents but not to :core itself. The
new shim cpp files live in :core, so add those defines explicitly when
the respective skia_enable_* args are set; otherwise the bodies
collapse to the stubs and never link the real symbols.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
macOS builds libSkiaSharp.dylib in two steps -- GN/ninja produces libskia.a, then xcodebuild compiles src/xamarin/*.cpp and links the dylib against libskia.a via -lskia. The macOS linker pulls object files from libskia.a lazily: only those that resolve undefined symbol references in the linkage roots get included. SkiaKeeper.c is the existing anchor file -- it holds one void* reference per C-API module so the linker keeps the whole object file alive in the final dylib. The new sk_graphite_*.cpp shim files weren't on the anchor list, so libskia.a contained them but the dylib didn't export their symbols. Add one anchor per shim cpp. The per-backend cpps have stub bodies in their !SK_GRAPHITE / !SK_VULKAN / !SK_METAL / !SK_DAWN branches, so anchoring is safe when those backends are disabled at build time.
Threads Skia's Graphite/Dawn backend through Emscripten's bundled
WebGPU shim (-sUSE_WEBGPU=1) so it links cleanly into libSkiaSharp.a
under is_canvaskit=true. Five surgical edits:
* include/c/sk_graphite_dawn.h + src/c/sk_graphite_dawn.cpp -- new
sk_graphite_dawn_backend_texture_new C entry point that wraps a
caller-supplied WGPUTexture (e.g. canvas.getContext('webgpu')
.getCurrentTexture()) via BackendTextures::MakeDawn. The wrapper
does not retain or release the WGPUTexture; any SkSurface/SkImage
that wraps it will retain for its own lifetime.
* src/gpu/graphite/dawn/DawnCaps.cpp -- guard YCbCr/biplanar texture
formats (R8BG8Biplanar420Unorm, R10X6BG10X6Biplanar420Unorm,
R8BG8A8Triplanar420Unorm, OpaqueYCbCrAndroid, DualSourceBlending,
TextureFormatsTier1) under #if !defined(__EMSCRIPTEN__). These
wgpu::TextureFormat enum values do not exist in Emscripten's
bundled webgpu_cpp.h and would fail to compile.
* src/gpu/graphite/dawn/DawnBuffer.cpp -- replace SKGPU_LOG(...) macro
with explicit SKGPU_LOG_D / SKGPU_LOG_E; Emscripten's preprocessor
rejects the macro form in this position.
* third_party/dawn/BUILD.gn -- skip the native Dawn CMake build AND
the libdawn_combined.a link step when is_canvaskit=true. On
Emscripten the wgpu_* symbols are provided by -sUSE_WEBGPU=1 at
the consumer's final link step; building native Dawn for WASM is
both unnecessary and breaks (the Dawn CMake build assumes desktop
toolchains).
Build prerequisite (out-of-tree): externals/dawn requires one extra
#include in third_party/externals/dawn/include/tint/tint.h --
'src/tint/api/common/bindings.h'. Dawn isn't a git submodule of skia
(it is checked out via DEPS), so we cannot land that patch here.
Apply it separately if a fresh DEPS sync is needed and Dawn pre-m146.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ramezgerges
force-pushed
the
dev/graphite-backend
branch
from
2e90471 to
bf5e889
Compare
The five Graphite C-API types (Context, Recorder, Recording, BackendTexture, TextureInfo) were hand-rolled with three or four overload-incomplete static-inline reinterpret_cast helpers per type in sk_graphite.cpp. Move them into sk_types_priv.h under the same DEF_MAP_WITH_NS pattern that already handles GrDirectContext / VulkanYcbcrConversionInfo / etc. Each invocation generates 8 const/non-const ref+ptr As/To overloads, gated by SK_GRAPHITE so non-Graphite builds skip both the includes and the helpers. The per-backend cpps (sk_graphite_vulkan/metal/dawn) now use the generated ToGraphiteContext / ToGraphiteBackendTexture / ToGraphiteTextureInfo instead of raw reinterpret_cast. Net: -10 source lines, consistent convention with the rest of the C-API map declarations.
Replace the misleading comment ("SKGPU_LOG unavailable on Emscripten")
with the actual reason: SKGPU_LOG expands via SKIA_LOG to
`if constexpr (priority <= ...)`, which requires a compile-time
constant. The call site passes a runtime variable set by the switch
above, so the original code fails to compile.
Only surfaces in WASM because the function is gated by
__EMSCRIPTEN__; upstream doesn't build Graphite-on-WASM, so the bug
sits there without anyone noticing.
* DawnCaps.cpp + third_party/dawn/BUILD.gn — hoist explanatory comments above the preprocessor gate and prefix each with "mono/skia:" so grep surfaces every local upstream patch. * SkiaKeeper.c — drop the "mono/skia:" prefix from the Graphite comments. The whole src/xamarin/ directory is mono-only territory, so the marker adds nothing.
The previous block explained what the anchors are doing, but the file-level convention (one anchor per .cpp to keep the linker from pruning archive members) already covers that. The Graphite includes and four anchors below speak for themselves.
Three related fixes to remove silent fallback returns from the
sk_graphite_* C shim:
* Closed-enum exhaustiveness. ToInsertStatus and the sampleCount()
getter on sk_graphite_texture_info handled every Skia value but
still ended in a `return X;` fallback. Drop the unreachable
default values; end the switches with SkUNREACHABLE so the
compiler can enforce exhaustiveness via -Wswitch-enum and any
future Skia milestone that grows the enum produces a compile
error here instead of a runtime mis-mapping.
* Open-enum honesty for BackendApi. skgpu::BackendApi has kMock and
kUnsupported values we don't expose on the public C ABI. The four
*_get_backend functions used to silently map them to kVulkan,
which is a lie. Add UNKNOWN_SK_GRAPHITE_BACKEND = -1 to the C
enum, list kMock/kUnsupported explicitly, drop the `default:`
blanket case, and end with SkUNREACHABLE. Null-handle early
returns also use UNKNOWN now rather than guessing Vulkan.
* Bool-returning sample-count validator. ToGraphiteSampleCount used
to silently clamp invalid input to k1, which means the caller had
no way to know they had passed a bad value. Switch to a
bool-returning out-parameter form and propagate the failure up
through sk_graphite_make_context_options (also bool-returning out
param) and the per-backend ContextFactory factories
(sk_graphite_context_make_{vulkan,metal,dawn}), which now return
nullptr when the C options struct is invalid. The managed binding
validates options up front and throws ArgumentException with a
clear message; the null path is a defense-in-depth backstop.
The shim was double-checking input that the managed binding already
controls: null handles, null info pointers, negative budgets, zero
or negative widths, out-of-range plane indices. Move that work to
managed where the failure mode is a proper ArgumentException with a
diagnostic message, and let the C shim be a straight 1-to-1 mapping
to skgpu::graphite::*.
Kept in the shim:
* if (!opts) return true; in sk_graphite_make_context_options — a
documented "null = use Skia defaults" semantic.
* fGpuBudgetInBytes >= 0 and recorderBudgetBytes >= 0 sentinels —
documented "negative = use Skia default" behaviour mirroring
the C struct comment.
* if (!bt.isValid()) return nullptr; — validates Skia's own return
value, not a defensive null check.
* Vulkan memory-allocator fallback in sk_graphite_context_make_vulkan
— real setup logic, not a guard.
* FfiImageProvider's findOrCreate boundary nulls — Skia is calling
into us through a C++ vtable; we can't trust the inputs.
The plain make_recorder entry was a forwarder to the _with_image_provider variant with a null third argument. Two C entry points, two C# P/Invokes, two SkiaApi declarations, two stub branches in the !SK_GRAPHITE block — all for what's a single C++ call. Collapse to one entry that takes the optional image-provider directly; callers that don't want one pass null.
…lags Five sites in the Graphite C ABI were carrying a binary flag as int32_t with a "0 = no, 1 = yes" comment instead of the C bool type used elsewhere in the same headers (e.g. sk_graphite_backend_is_- available, sk_graphite_context_is_device_lost, fProtectedContext): * sk_graphite_image_make_texture(... mipmapped) * sk_graphite_surface_make_render_target(... mipmapped, ...) * sk_graphite_image_provider_proc_t callback (mipmapped) * sk_graphite_texture_info_get_mipmapped return * sk_graphite_vk_texture_info_t::fMipmapped * sk_graphite_dawn_backend_context_init_t::fNonYielding Switch them all to bool to match the rest of the surface and drop the "0 = no, 1 = yes" comments. Managed side gains a slightly nicer shape (no `? 1 : 0` / `!= 0` adapters around every call), and the binary nature of each flag is now self-documenting from the type. The two-state SubmitInfo enums (fSync, fMarkBoundary) are kept as distinct enums on purpose — the named YES_/NO_ constants read more clearly at call sites than bare bools and mirror Skia's enum class.
Both sk_graphite_sync_to_cpu_t and sk_graphite_mark_frame_boundary_t were two-value enums shaped exactly like a bool. Mirroring Skia's `enum class SyncToCpu : bool` / `MarkFrameBoundary : bool` shape in the C ABI never paid off — at call sites the YES_/NO_ constants are just ceremony around a true/false. Drop both enums and make the fields in sk_graphite_submit_info_t plain `bool`. Same translation to Skia's enum class happens in sk_graphite_context_submit, only now the input is a clean boolean instead of an enum-equals-named-constant expression.
…ints
Drop the opaque sk_graphite_{vk,mtl,dawn}_backend_context_t handles plus
their paired _new / _delete functions. sk_graphite_context_make_* now take
const _init_t* directly. The CFRetain (Metal) and AddRef (Dawn) work that
the wrappers used to do moves inline into _make_*, bracketed by the
ContextFactory::Make* call so locals release on scope exit whether Skia
took its own refs or not. The Vulkan dispatch lambda was already
capturing fGetProc + fGetProcUserData by value, so the wrapper was
purely cosmetic on that path.
Net: 6 fewer exported symbols, ~60 lines removed, one consistent
managed lifecycle across all three backends.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The DEF_MAP_WITH_NS helpers added in 70150e1 reference the C-side typedefs sk_graphite_context_t/recorder_t/recording_t/backend_texture_t /texture_info_t. They live in include/c/sk_graphite.h, but sk_types_priv.h only pulled in the C++ skgpu::graphite::* headers — so any translation unit that includes sk_types_priv.h without first including sk_graphite.h fails to compile when SK_GRAPHITE is defined. Xamarin TUs (sk_managedtracememorydump.cpp, sk_compatpaint.cpp, etc.) are the affected callers and broke the WASM build (the first target that defines SK_GRAPHITE while compiling them). Native builds masked the issue because they don't currently set skia_enable_graphite=true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ramezgerges
force-pushed
the
dev/graphite-backend
branch
from
e1cf751 to
ff92039
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of Change
SkiaSharp Issue
Related to https://github.com/mono/SkiaSharp/issues/
API Changes
Expose
skgpu::graphite::*through the C shimAdds a C ABI for Skia's Graphite backend so that downstream language bindings (in our case mono/SkiaSharp#3968) can wrap it without touching the C++ types directly.
What's in
Four new headers + their
.cppimplementations underinclude/c/andsrc/c/:sk_graphite.hContext,Recorder,Recording,BackendTexture,TextureInfo,ImageProvider,Surface::RenderTarget/WrapBackendTexture,Image::WrapTexture/TextureFromImage,asyncRescaleAndReadPixelson surfaces, context budgets/cleanupsk_graphite_vulkan.hVulkanBackendContext,ContextFactory::MakeVulkan,VulkanTextureInfo,VulkanBackendTexturefactoriessk_graphite_metal.hMtlBackendContext,ContextFactory::MakeMetal,MtlBackendTexturesk_graphite_dawn.hDawnBackendContext,ContextFactory::MakeDawn,DawnBackendTexture57 total
SK_C_APIsymbols. The core file (sk_graphite.{h,cpp}) is guarded bySK_GRAPHITE; per-backend files are additionally guarded bySK_VULKAN/SK_METAL/SK_DAWNso platforms with one backend disabled still link clean.Conventions
Matches the rest of the C shim in
include/c/:sk_<type>_<action>(sk_graphite_recorder_snap,sk_graphite_context_make_recorder, etc.).*_deletereleases them, raw_t*parameters are non-owning. Same rules assk_canvas_*/sk_image_*.nullptron failure; boolean operations returnfalse. No exceptions across the ABI.Make*factory returns ask_graphite_context_t*so callers can stay backend-agnostic once the context is up.Test coverage
End-to-end smokes live in the consuming PR (mono/SkiaSharp#3968) — both a standalone C++ test that drives the upstream Skia API directly (proves Skia's Graphite works on the build agent) and a C-only test that drives this shim (proves the ownership/error contracts above hold without any managed code in the picture). Lavapipe is the canonical software ICD for headless CI.
Out of scope (deferred to a follow-up)
The Graphite surface is large; this PR covers the parts SkiaSharp ships in v1. Not included:
PrecompileContext/precompile/*— pipeline-cache warming.PersistentPipelineStorage— client-supplied cache blob.BackendSemaphore— cross-queue / cross-context sync.YUVABackendTextures— multi-plane Y'UV wrapping.asyncRescaleAndReadPixels{,YUV420,YUVA420}onSkImage(only theSkSurfacevariant is wired).InsertRecordingInfo::fTargetSurfaceretargeting.These can be added incrementally — none of them are blocked by anything in this PR.
Behavioral Changes
None.
Required SkiaSharp PR
Requires mono/SkiaSharp#3968
PR Checklist
skiasharpat time of PR