From 196b17265035ed935ffb80a48265db94a90190ce Mon Sep 17 00:00:00 2001 From: Kevin Lubick Date: Tue, 12 Apr 2022 08:23:07 -0400 Subject: [PATCH] [sksl] Make sksl tracing optional This removes the required dependency on our JSON code. In the Bazel rules, this dependency is pushed down into sksl instead of required by the cc_binary rules. It adds a stub version of SkVMDebugTrace.cpp and removes SkVMDebugTracePlayer unless the appropriate GN or Bazel flag is set (skia_enable_sksl_tracing and enable_sksl_tracing, respectively). There was an existing #define that CanvasKit used (CK_INCLUDE_SKSL_TRACE) and this was changed to SKSL_ENABLE_TRACING. Users of //:skia_core no longer need to specify a JSON dep, if sksl needs it (e.g. for tracing), then it will specify the dependency. This is a reland of https://skia-review.googlesource.com/c/skia/+/528837 Bug: skia:12541 Change-Id: I79612c69fdbefd3db9822a2b66df7552f7c13865 Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529278 Reviewed-by: John Stiles --- .bazelrc | 2 ++ BUILD.gn | 7 ++++++ bazel/BUILD.bazel | 5 +++++ bazel/cc_binary_with_flags.bzl | 1 + bazel/common_config_settings/BUILD.bazel | 5 +++++ example/BUILD.bazel | 4 ---- gn/gn_to_bp.py | 2 ++ gn/skia.gni | 1 + gn/skqp_gn_args.py | 5 +++-- gn/sksl.gni | 7 ++++-- modules/canvaskit/BUILD.bazel | 11 +++------- modules/canvaskit/BUILD.gn | 6 +++--- modules/canvaskit/canvaskit_bindings.cpp | 6 +++--- modules/canvaskit/compile.sh | 2 +- src/sksl/BUILD.bazel | 20 ++++++++++++++++-- src/sksl/tracing/SkVMDebugTrace.cpp | 27 ++++++++++++++++++++++++ 16 files changed, 86 insertions(+), 25 deletions(-) diff --git a/.bazelrc b/.bazelrc index 9cae8ba642..3e3eb53e41 100644 --- a/.bazelrc +++ b/.bazelrc @@ -24,6 +24,8 @@ build --flag_alias=disable_effect_serialization=no//bazel/common_config_settings build --flag_alias=enable_effect_serialization=//bazel/common_config_settings:enable_effect_serialization build --flag_alias=disable_skslc=no//bazel/common_config_settings:enable_skslc build --flag_alias=enable_skslc=//bazel/common_config_settings:enable_skslc +build --flag_alias=disable_sksl_tracing=no//bazel/common_config_settings:enable_sksl_tracing +build --flag_alias=enable_sksl_tracing=//bazel/common_config_settings:enable_sksl_tracing build --flag_alias=disable_tracing=no//bazel/common_config_settings:enable_tracing build --flag_alias=enable_tracing=//bazel/common_config_settings:enable_tracing build --flag_alias=disable_vma=no//bazel/common_config_settings:use_vulkan_memory_allocator diff --git a/BUILD.gn b/BUILD.gn index 255f83a29c..a03c03d085 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -711,6 +711,7 @@ if (skia_compile_sksl_tests) { # Build skslc. skia_executable("skslc") { defines = [ + "SKSL_ENABLE_TRACING", "SKSL_STANDALONE", "SK_DISABLE_TRACING", "SK_ENABLE_SPIRV_CROSS", @@ -727,6 +728,7 @@ if (skia_compile_sksl_tests) { } sources += skia_sksl_sources sources += skia_sksl_gpu_sources + sources += skia_sksl_tracing_sources include_dirs = [ "." ] deps = [ ":run_sksllex", @@ -1381,6 +1383,11 @@ skia_component("skia") { if (skia_enable_sksl) { deps += [ ":dehydrate_sksl" ] sources += skia_sksl_sources + + if (skia_enable_sksl_tracing) { + defines += [ "SKSL_ENABLE_TRACING" ] + sources += skia_sksl_tracing_sources + } } if (skia_build_for_debugger) { diff --git a/bazel/BUILD.bazel b/bazel/BUILD.bazel index cb29f0e26a..d24a45a1cc 100644 --- a/bazel/BUILD.bazel +++ b/bazel/BUILD.bazel @@ -33,6 +33,11 @@ GENERAL_DEFINES = [ "SK_ENABLE_SPIRV_VALIDATION", ], "//conditions:default": [], +}) + select({ + "//bazel/common_config_settings:enable_sksl_tracing_true": [ + "SKSL_ENABLE_TRACING", + ], + "//conditions:default": [], }) GPU_DEFINES = select({ diff --git a/bazel/cc_binary_with_flags.bzl b/bazel/cc_binary_with_flags.bzl index 566b08a2ce..3da75af6da 100644 --- a/bazel/cc_binary_with_flags.bzl +++ b/bazel/cc_binary_with_flags.bzl @@ -7,6 +7,7 @@ It is based off of https://github.com/bazelbuild/examples/tree/main/rules/starla """ _bool_flags = [ + "//bazel/common_config_settings:enable_sksl_tracing", "//bazel/common_config_settings:enable_skslc", "//bazel/common_config_settings:is_skia_dev_build", "//bazel/common_config_settings:use_icu", diff --git a/bazel/common_config_settings/BUILD.bazel b/bazel/common_config_settings/BUILD.bazel index 1cea4e2e80..c01c08bfc3 100644 --- a/bazel/common_config_settings/BUILD.bazel +++ b/bazel/common_config_settings/BUILD.bazel @@ -192,6 +192,11 @@ bool_flag( flag_name = "enable_skslc", ) +bool_flag( + default = False, + flag_name = "enable_sksl_tracing", +) + bool_flag( # See SkTraceVentCommon.h for more on this type of tracing. default = True, diff --git a/example/BUILD.bazel b/example/BUILD.bazel index da551b10d1..0e879ec968 100644 --- a/example/BUILD.bazel +++ b/example/BUILD.bazel @@ -23,7 +23,6 @@ cc_binary_with_flags( }, deps = [ "//:skia_core", - "//src/utils:json_srcs", "//tools/sk_app", ], ) @@ -46,7 +45,6 @@ cc_binary_with_flags( }, deps = [ "//:skia_core", - "//src/utils:json_srcs", "//tools/sk_app", ], ) @@ -69,7 +67,6 @@ cc_binary_with_flags( }, deps = [ "//:skia_core", - "//src/utils:json_srcs", "//tools/sk_app", ], ) @@ -87,7 +84,6 @@ cc_binary_with_flags( }, deps = [ "//:skia_core", - "//src/utils:json_srcs", # This DEPS is for the utility in the demo for creating a vulkan context. # Outside clients would not need it. "//tools/gpu/vk:VkTestUtils_src", diff --git a/gn/gn_to_bp.py b/gn/gn_to_bp.py index 8270a08e8f..f5869b5c7b 100755 --- a/gn/gn_to_bp.py +++ b/gn/gn_to_bp.py @@ -495,6 +495,8 @@ def generate_args(target_os, enable_gpu, renderengine = False): 'skia_use_fontconfig': 'false', 'skia_include_multiframe_procs': 'false', + # Required for some SKSL tests + 'skia_enable_sksl_tracing': 'true', } d['target_os'] = target_os if target_os == '"android"': diff --git a/gn/skia.gni b/gn/skia.gni index 89ec00e68e..f6ed7a8c0f 100644 --- a/gn/skia.gni +++ b/gn/skia.gni @@ -25,6 +25,7 @@ declare_args() { skia_enable_skottie = !(is_win && is_component_build) || (is_wasm && skia_canvaskit_enable_skottie) skia_enable_sksl = true + skia_enable_sksl_tracing = is_skia_dev_build skia_enable_skvm_jit_when_possible = is_skia_dev_build skia_enable_svg = !is_component_build skia_enable_tools = is_skia_dev_build diff --git a/gn/skqp_gn_args.py b/gn/skqp_gn_args.py index d2a6c903cc..5f90bb1a14 100644 --- a/gn/skqp_gn_args.py +++ b/gn/skqp_gn_args.py @@ -9,11 +9,12 @@ def GetGNArgs(api_level, debug, arch=None, ndk=None, is_android_bp=False): 'skia_enable_fontmgr_android': 'false', 'skia_enable_fontmgr_empty': 'true', 'skia_enable_pdf': 'false', - 'skia_enable_skshaper': 'false', 'skia_enable_skottie': 'false', + 'skia_enable_skshaper': 'false', + 'skia_enable_sksl_tracing': 'true', 'skia_enable_sktext': 'false', - 'skia_enable_tools': 'true', 'skia_enable_svg': 'false', + 'skia_enable_tools': 'true', 'skia_tools_require_resources': 'true', 'skia_use_dng_sdk': 'false', 'skia_use_expat': 'true', diff --git a/gn/sksl.gni b/gn/sksl.gni index 68c63a399c..fd504fb7bc 100644 --- a/gn/sksl.gni +++ b/gn/sksl.gni @@ -204,8 +204,6 @@ skia_sksl_sources = [ "$_src/sksl/spirv.h", "$_src/sksl/tracing/SkVMDebugTrace.cpp", "$_src/sksl/tracing/SkVMDebugTrace.h", - "$_src/sksl/tracing/SkVMDebugTracePlayer.cpp", - "$_src/sksl/tracing/SkVMDebugTracePlayer.h", "$_src/sksl/transform/SkSLBuiltinVariableScanner.cpp", "$_src/sksl/transform/SkSLEliminateDeadFunctions.cpp", "$_src/sksl/transform/SkSLEliminateDeadGlobalVariables.cpp", @@ -214,6 +212,11 @@ skia_sksl_sources = [ "$_src/sksl/transform/SkSLTransform.h", ] +skia_sksl_tracing_sources = [ + "$_src/sksl/tracing/SkVMDebugTracePlayer.cpp", + "$_src/sksl/tracing/SkVMDebugTracePlayer.h", +] + skia_sksl_gpu_sources = [ "$_src/sksl/codegen/SkSLCodeGenerator.h", "$_src/sksl/codegen/SkSLGLSLCodeGenerator.cpp", diff --git a/modules/canvaskit/BUILD.bazel b/modules/canvaskit/BUILD.bazel index 55ddd71c26..0cac4bca8a 100644 --- a/modules/canvaskit/BUILD.bazel +++ b/modules/canvaskit/BUILD.bazel @@ -138,9 +138,6 @@ CK_DEFINES = [ }) + select({ ":enable_runtime_effect_true": ["CK_INCLUDE_RUNTIME_EFFECT=1"], ":enable_runtime_effect_false": [], -}) + select({ - ":enable_sksl_tracing_true": ["CK_INCLUDE_SKSL_TRACE=1"], - ":enable_sksl_tracing_false": [], }) CK_RELEASE_OPTS = [ @@ -351,6 +348,9 @@ cc_binary_with_flags( "use_icu": [ "True", ], + "enable_sksl_tracing": [ + "True", + ], "shaper_backend": [ "harfbuzz_shaper", ], @@ -421,11 +421,6 @@ bool_flag( flag_name = "enable_runtime_effect", ) -bool_flag( - default = True, - flag_name = "enable_sksl_tracing", -) - bool_flag( default = True, flag_name = "include_matrix_js", diff --git a/modules/canvaskit/BUILD.gn b/modules/canvaskit/BUILD.gn index b4b8dbc23a..5f4b14c91b 100644 --- a/modules/canvaskit/BUILD.gn +++ b/modules/canvaskit/BUILD.gn @@ -369,9 +369,6 @@ skia_wasm_lib("canvaskit") { if (skia_canvaskit_enable_rt_shader) { defines += [ "CK_INCLUDE_RUNTIME_EFFECT" ] } - if (skia_canvaskit_enable_sksl_trace) { - defines += [ "CK_INCLUDE_SKSL_TRACE" ] - } if (!skia_canvaskit_enable_alias_font) { defines += [ "CANVASKIT_NO_ALIAS_FONT" ] } @@ -382,4 +379,7 @@ skia_wasm_lib("canvaskit") { if (!skia_canvaskit_enable_font) { defines += [ "CK_NO_FONTS" ] } + if (skia_enable_sksl_tracing) { + defines += [ "SKSL_ENABLE_TRACING" ] + } } diff --git a/modules/canvaskit/canvaskit_bindings.cpp b/modules/canvaskit/canvaskit_bindings.cpp index 38c8544171..54b6b561b2 100644 --- a/modules/canvaskit/canvaskit_bindings.cpp +++ b/modules/canvaskit/canvaskit_bindings.cpp @@ -89,7 +89,7 @@ #include "include/pathops/SkPathOps.h" #endif -#if defined(CK_INCLUDE_RUNTIME_EFFECT) && defined(CK_INCLUDE_SKSL_TRACE) +#if defined(CK_INCLUDE_RUNTIME_EFFECT) && defined(SKSL_ENABLE_TRACING) #include "include/sksl/SkSLDebugTrace.h" #endif @@ -1802,7 +1802,7 @@ EMSCRIPTEN_BINDINGS(Skia) { }), allow_raw_pointers()); #ifdef CK_INCLUDE_RUNTIME_EFFECT -#ifdef CK_INCLUDE_SKSL_TRACE +#ifdef SKSL_ENABLE_TRACING class_("DebugTrace") .smart_ptr>("sk_sp") .function("writeTrace", optional_override([](SkSL::DebugTrace& self) -> std::string { @@ -1830,7 +1830,7 @@ EMSCRIPTEN_BINDINGS(Skia) { } return effect; })) -#ifdef CK_INCLUDE_SKSL_TRACE +#ifdef SKSL_ENABLE_TRACING .class_function("MakeTraced", optional_override([]( sk_sp shader, int traceCoordX, diff --git a/modules/canvaskit/compile.sh b/modules/canvaskit/compile.sh index 3f2e9feedc..5bc9f3c0d5 100755 --- a/modules/canvaskit/compile.sh +++ b/modules/canvaskit/compile.sh @@ -230,6 +230,7 @@ echo "Compiling" skia_use_zlib=true \ skia_enable_gpu=${ENABLE_GPU} \ skia_build_for_debugger=${DEBUGGER_ENABLED} \ + skia_enable_sksl_tracing=${ENABLE_SKSL_TRACE} \ \ ${GN_SHAPER} \ ${GN_FONT} \ @@ -247,7 +248,6 @@ echo "Compiling" skia_canvaskit_enable_particles=${ENABLE_PARTICLES} \ skia_canvaskit_enable_pathops=${ENABLE_PATHOPS} \ skia_canvaskit_enable_rt_shader=${ENABLE_RT_SHADER} \ - skia_canvaskit_enable_sksl_trace=${ENABLE_SKSL_TRACE} \ skia_canvaskit_enable_matrix_helper=${ENABLE_MATRIX} \ skia_canvaskit_enable_canvas_bindings=${ENABLE_CANVAS} \ skia_canvaskit_enable_font=${ENABLE_FONT} \ diff --git a/src/sksl/BUILD.bazel b/src/sksl/BUILD.bazel index 3e0ae7ce7c..a354de7445 100644 --- a/src/sksl/BUILD.bazel +++ b/src/sksl/BUILD.bazel @@ -15,8 +15,7 @@ filegroup( ) cc_library( - name = "srcs", - visibility = ["//:__subpackages__"], + name = "core_srcs", deps = [ ":SkSLAnalysis_src", ":SkSLBuiltinMap_src", @@ -149,6 +148,23 @@ cc_library( ], ) +cc_library( + name = "tracing_srcs", + deps = [ + "//src/sksl/tracing:SkVMDebugTracePlayer_src", + "//src/utils:json_srcs", + ], +) + +cc_library( + name = "srcs", + visibility = ["//:__subpackages__"], + deps = [":core_srcs"] + select({ + "//bazel/common_config_settings:enable_sksl_tracing_true": [":tracing_srcs"], + "//bazel/common_config_settings:enable_sksl_tracing_false": [], + }), +) + generated_cc_atom( name = "GLSL.std.450_hdr", hdrs = ["GLSL.std.450.h"], diff --git a/src/sksl/tracing/SkVMDebugTrace.cpp b/src/sksl/tracing/SkVMDebugTrace.cpp index 18959572c0..72d4b50d13 100644 --- a/src/sksl/tracing/SkVMDebugTrace.cpp +++ b/src/sksl/tracing/SkVMDebugTrace.cpp @@ -7,6 +7,8 @@ #include "src/sksl/tracing/SkVMDebugTrace.h" +#ifdef SKSL_ENABLE_TRACING + #include "include/core/SkData.h" #include "include/core/SkRefCnt.h" #include "include/core/SkStream.h" @@ -386,3 +388,28 @@ bool SkVMDebugTrace::readTrace(SkStream* r) { } } // namespace SkSL + +#else // SKSL_ENABLE_TRACING + +#include + +namespace SkSL { + void SkVMDebugTrace::setTraceCoord(const SkIPoint &coord) {} + + void SkVMDebugTrace::setSource(std::string source) {} + + bool SkVMDebugTrace::readTrace(SkStream *r) { return false; } + + void SkVMDebugTrace::writeTrace(SkWStream *w) const {} + + void SkVMDebugTrace::dump(SkWStream *o) const {} + + std::string SkVMDebugTrace::getSlotComponentSuffix(int slotIndex) const { return ""; } + + std::string SkVMDebugTrace::getSlotValue(int slotIndex, int32_t value) const { return ""; } + + double SkVMDebugTrace::interpretValueBits(int slotIndex, int32_t valueBits) const { return 0; } + + std::string SkVMDebugTrace::slotValueToString(int slotIndex, double value) const { return ""; } +} +#endif