Issue 10680 in skia: Runtime effects: Limit functionality for CPU and low-end GPU

47 views
Skip to first unread message

brianos… via monorail

unread,
Aug 31, 2020, 4:10:51 PM8/31/20
to bu...@skia.org
Status: New
Owner: ----
CC: michae...@google.com, johns...@google.com, ethann...@google.com
Area: SkSL
Priority: Medium
Type: Defect

New issue 10680 by brian...@google.com: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680

There are two big categories of functionality that can be used, but should be guarded (or only enabled by opting in, perhaps).

First is types: We'd like to prevent users from using any 'int' variables, to ensure that things work on low end GPUs without integer support. Nothing prevents users from using int today, and if you happen to be on a device that doesn't support them, the shader just won't compile (but will fail far too late).

Second, intrinsics: Pipeline stage compiles use the sksl_gpu.sksl pre-include, so nearly all GLSL builtin functions are declared. If you target the GPU backend, most of them will work, too. (eg Flutter recently posted a demo where they're using dFdx/dFdy). You could end up on a device that doesn't have one, though. These end up passed through by GrSkSLFP, and inserted into the combined-FP program, where they're handled by the backend generator.

In addition, many of these won't work on the CPU backend. We've implemented the most common intrinsics in the byte code generator + skvm, but there are a huge number of things here that just don't exist. Many of the missing bits are also functions related to integers, or to other types that should be disallowed (sampler, image, etc...)


Strategies:

* We could use the CPU backend as the arbiter of what's allowed, by compiling all the way to ByteCode in the SkRuntimeEffect factory. This easily handles the case of missing intrinsics (those are reported as an error by the byte code generator). It doesn't address types like 'int' - those are supported by the ByteCode (though not yet by SkVM). This is also extra work if we're not planning to use the CPU backend. Still - it might be a good way to guard unsupported intrinsics.

* For types, I think we're going to need something in the IR generator. Adding lots of checks against specific types feels really cumbersome, but we may be able to add a flag to the Type itself, indicating if it's allowed in runtime effects. That would make any specific checks much simpler against the type object itself. We'd still need to validate the types of variables, expressions, parameters, return types from functions, etc... But having the bit on the type will hopefully make it easier to read (and to expand support over time)?

--
You received this message because:
1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

bugdroid via monorail

unread,
Oct 1, 2020, 4:31:30 PM10/1/20
to bu...@skia.org

Comment #1 on issue 10680 by bugdroid: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c1

The following revision refers to this bug:
https://skia.googlesource.com/skia/+/8e2ef02855cde4121e61fe5dc825715fb8301b9c

commit 8e2ef02855cde4121e61fe5dc825715fb8301b9c
Author: Brian Osman <brian...@google.com>
Date: Thu Oct 01 20:30:33 2020

Support variables in the intrinsic map, clone them into Programs

As a first step, convert sksl_pipeline.sksl to an IntrinsicMap (rather
than inherited element list). This makes the new code operate on
sk_FragCoord (which was previously being shared by all runtime effect
programs).

The new unit test angered TSAN, and now runs without complaint.

Also finish converting the .fp intrinsics over, so those don't need an
inherited element list either. And while doing that, refactor that
parsing to match all of the others. FP was uniquely implementing
processIncludeFile itself, rather than reusing the pattern of other
pre-include parsing.

The meat of the CL is the subtle changes in Compiler, and the logic in
cloneBuiltinVariables. Note that we need to clone the global variable
declaration element (because one of the goals is to get rid of shared
and inherited program elements), but also the variable itself (and the
new copy needs to live in the program's symbol table).

Bug: skia:10589
Bug: skia:10679
Bug: skia:10680
Change-Id: Ied352f8434dac2b8eacb4e515b014b6af7b57d20
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/319023
Commit-Queue: Brian Osman <brian...@google.com>
Reviewed-by: John Stiles <johns...@google.com>

[modify] https://crrev.com/8e2ef02855cde4121e61fe5dc825715fb8301b9c/src/sksl/ir/SkSLVariableReference.cpp
[modify] https://crrev.com/8e2ef02855cde4121e61fe5dc825715fb8301b9c/tests/SkRuntimeEffectTest.cpp
[modify] https://crrev.com/8e2ef02855cde4121e61fe5dc825715fb8301b9c/src/sksl/SkSLCompiler.cpp
[modify] https://crrev.com/8e2ef02855cde4121e61fe5dc825715fb8301b9c/src/sksl/ir/SkSLVariableReference.h
[modify] https://crrev.com/8e2ef02855cde4121e61fe5dc825715fb8301b9c/src/sksl/SkSLIRGenerator.cpp
[modify] https://crrev.com/8e2ef02855cde4121e61fe5dc825715fb8301b9c/src/sksl/SkSLCompiler.h
[modify] https://crrev.com/8e2ef02855cde4121e61fe5dc825715fb8301b9c/src/sksl/SkSLIRGenerator.h

bugdroid via monorail

unread,
Nov 5, 2020, 4:45:32 PM11/5/20
to bu...@skia.org

Comment #3 on issue 10680 by bugdroid: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c3


The following revision refers to this bug:
https://skia.googlesource.com/skia/+/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190

commit bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190
Author: Brian Osman <brian...@google.com>
Date: Thu Nov 05 21:44:40 2020

Rearrange SkSL pre-include modules to hide things

This makes numerous internal and GLSL types or intrinsics hidden from
public (runtime effect) SkSL. In particular:

- Only core numeric types are visible to all program types. GLSL types
involving images, textures, and sampling are restricted to internal use.
- sk_Caps is no longer visible to runtime effects.
- The set of intrinsics available to runtime effects is now a separate,
curated list in sksl_public.sksl. It exactly matches the GLSL ES 1.00
spec order.
- The blend intrinsics are no longer visible, which also fixes a bug.
These are nice, but we're not going to offer them yet - they involve
enums, which creates complications.

Bug: skia:10680
Bug: skia:10709
Bug: skia:10913

Change-Id: I8fa1c94f6e4899f38530bb9cff33d147f6983ab3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332597

Commit-Queue: Brian Osman <brian...@google.com>
Reviewed-by: John Stiles <johns...@google.com>

[add] https://crrev.com/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190/src/sksl/sksl_public.sksl
[add] https://crrev.com/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190/src/sksl/generated/sksl_public.dehydrated.sksl
[modify] https://crrev.com/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190/BUILD.gn
[modify] https://crrev.com/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190/tests/SkRuntimeEffectTest.cpp
[modify] https://crrev.com/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190/RELEASE_NOTES.txt
[modify] https://crrev.com/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190/src/sksl/SkSLByteCodeGenerator.cpp
[modify] https://crrev.com/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190/src/sksl/SkSLCompiler.cpp
[modify] https://crrev.com/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190/src/sksl/generated/sksl_interp.dehydrated.sksl
[modify] https://crrev.com/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190/src/sksl/SkSLCompiler.h
[modify] https://crrev.com/bea0dc67f47c4d5f52e2f25d4a03c9e7cbbda190/src/sksl/sksl_interp.sksl

bugdroid via monorail

unread,
Nov 6, 2020, 2:29:00 PM11/6/20
to bu...@skia.org

Comment #4 on issue 10680 by bugdroid: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c4


The following revision refers to this bug:
https://skia.googlesource.com/skia/+/b06301ee122799a84d384359eb3628b870cd47d0

commit b06301ee122799a84d384359eb3628b870cd47d0
Author: Brian Osman <brian...@google.com>
Date: Fri Nov 06 19:28:34 2020

Reland "Rearrange SkSL pre-include modules to hide things"

This reverts commit 4cb5c5e1728933637ca9b8653ff95f876871dc9b, and fixes
the Chromium issue by declaring sign(x) in sksl_public.sksl.

Original description:


This makes numerous internal and GLSL types or intrinsics hidden from
public (runtime effect) SkSL. In particular:

- Only core numeric types are visible to all program types. GLSL types
involving images, textures, and sampling are restricted to internal use.
- sk_Caps is no longer visible to runtime effects.
- The set of intrinsics available to runtime effects is now a separate,
curated list in sksl_public.sksl. It exactly matches the GLSL ES 1.00
spec order.
- The blend intrinsics are no longer visible, which also fixes a bug.
These are nice, but we're not going to offer them yet - they involve
enums, which creates complications.

Bug: skia:10680
Bug: skia:10709
Bug: skia:10913

Cq-Include-Trybots: luci.chromium.try:linux-chromeos-rel,linux-rel
Change-Id: I42deeeccd725a9fe18314d091ce253404e3572e2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/332750
Commit-Queue: Brian Osman <brian...@google.com>
Commit-Queue: Brian Salomon <bsal...@google.com>
Auto-Submit: Brian Osman <brian...@google.com>
Reviewed-by: Brian Salomon <bsal...@google.com>
Reviewed-by: John Stiles <johns...@google.com>

[add] https://crrev.com/b06301ee122799a84d384359eb3628b870cd47d0/src/sksl/sksl_public.sksl
[add] https://crrev.com/b06301ee122799a84d384359eb3628b870cd47d0/src/sksl/generated/sksl_public.dehydrated.sksl
[modify] https://crrev.com/b06301ee122799a84d384359eb3628b870cd47d0/BUILD.gn
[modify] https://crrev.com/b06301ee122799a84d384359eb3628b870cd47d0/tests/SkRuntimeEffectTest.cpp
[modify] https://crrev.com/b06301ee122799a84d384359eb3628b870cd47d0/RELEASE_NOTES.txt
[modify] https://crrev.com/b06301ee122799a84d384359eb3628b870cd47d0/src/sksl/SkSLByteCodeGenerator.cpp
[modify] https://crrev.com/b06301ee122799a84d384359eb3628b870cd47d0/src/sksl/SkSLCompiler.cpp
[modify] https://crrev.com/b06301ee122799a84d384359eb3628b870cd47d0/src/sksl/generated/sksl_interp.dehydrated.sksl
[modify] https://crrev.com/b06301ee122799a84d384359eb3628b870cd47d0/src/sksl/SkSLCompiler.h
[modify] https://crrev.com/b06301ee122799a84d384359eb3628b870cd47d0/src/sksl/sksl_interp.sksl

brian… via monorail

unread,
Nov 13, 2020, 11:06:34 AM11/13/20
to bu...@skia.org

Comment #5 on issue 10680 by brian...@google.com: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c5

Addendum: To ensure maximum compatibility, we'd like to target the WebGL 1.0 spec. This is exactly the same as the GLSL ES 1.0 spec, with the additional restriction that ALL optional behavior is explicitly disallowed. In other words: Only constant-length for loops, no while or do-while loops, no non-constant indexing. See https://www.khronos.org/registry/webgl/specs/latest/1.0/#4.3

brian… via monorail

unread,
Dec 15, 2020, 4:37:07 PM12/15/20
to bu...@skia.org
Updates:
Blockedon: 11094 11088 11093 11096 11095

Comment #6 on issue 10680 by brian...@google.com: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c6

(No comment was entered for this change.)

bugdroid via monorail

unread,
Jan 6, 2021, 9:06:48 AM1/6/21
to bu...@skia.org

Comment #7 on issue 10680 by bugdroid: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c7


The following revision refers to this bug:
https://skia.googlesource.com/skia/+/cdde253e40829a8ec85d721cb941bb0a4f59d6a2

commit cdde253e40829a8ec85d721cb941bb0a4f59d6a2
Author: Brian Osman <brian...@google.com>
Date: Wed Jan 06 14:05:59 2021

Runtime effects: Disallow bitwise ops and integer remainder

Bug: skia:10680
Bug: skia:11088
Bug: skia:11127
Change-Id: I25ea288d03df13147b31bc4ca4b224bbe2fa924e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/350030
Reviewed-by: John Stiles <johns...@google.com>
Commit-Queue: Brian Osman <brian...@google.com>

[add] https://crrev.com/cdde253e40829a8ec85d721cb941bb0a4f59d6a2/tests/sksl/errors/golden/IllegalOperators.skvm
[modify] https://crrev.com/cdde253e40829a8ec85d721cb941bb0a4f59d6a2/tests/SkSLInterpreterTest.cpp
[modify] https://crrev.com/cdde253e40829a8ec85d721cb941bb0a4f59d6a2/src/sksl/SkSLIRGenerator.cpp
[modify] https://crrev.com/cdde253e40829a8ec85d721cb941bb0a4f59d6a2/gn/sksl_tests.gni
[add] https://crrev.com/cdde253e40829a8ec85d721cb941bb0a4f59d6a2/tests/sksl/errors/IllegalOperators.rte

bugdroid via monorail

unread,
Jan 8, 2021, 7:48:20 PM1/8/21
to bu...@skia.org

Comment #8 on issue 10680 by bugdroid: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c8


The following revision refers to this bug:
https://skia.googlesource.com/skia/+/54515b7b2b994b1d439a3a3bfa4f0517c9a981f4

commit 54515b7b2b994b1d439a3a3bfa4f0517c9a981f4
Author: Brian Osman <brian...@google.com>
Date: Sat Jan 09 00:47:48 2021

Support function calls in SkSL-to-SkVM

Bug: skia:10680
Change-Id: I8697bdc157d250f3c390c7f49074318aa8c7bdab
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/351918
Commit-Queue: Brian Osman <brian...@google.com>
Reviewed-by: Mike Klein <mtk...@google.com>

[modify] https://crrev.com/54515b7b2b994b1d439a3a3bfa4f0517c9a981f4/src/core/SkRuntimeEffect.cpp
[modify] https://crrev.com/54515b7b2b994b1d439a3a3bfa4f0517c9a981f4/tests/SkSLInterpreterTest.cpp
[modify] https://crrev.com/54515b7b2b994b1d439a3a3bfa4f0517c9a981f4/src/sksl/SkSLVMGenerator.cpp

bugdroid via monorail

unread,
Jan 14, 2021, 12:49:14 PM1/14/21
to bu...@skia.org

Comment #9 on issue 10680 by bugdroid: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c9


The following revision refers to this bug:

bugdroid via monorail

unread,
Feb 9, 2021, 3:00:40 PM2/9/21
to bu...@skia.org

Comment #10 on issue 10680 by bugdroid: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c10


The following revision refers to this bug:
https://skia.googlesource.com/skia/+/cb208d4c1f91489e02119b58876da319ac02b402

commit cb208d4c1f91489e02119b58876da319ac02b402
Author: Brian Osman <brian...@google.com>
Date: Tue Feb 09 19:57:06 2021

Stop exposing non-square matrices to runtime effects

The SkSL names of these types were inaccessible (in the private symbol
table), but we had inadvertently made them usable via the GLSL aliases.

Bug: skia:10680
Change-Id: I2e22db39c623c872dca837a22ff6f99cf5db46b4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/368251

Reviewed-by: John Stiles <johns...@google.com>
Commit-Queue: Brian Osman <brian...@google.com>

brian… via monorail

unread,
Mar 22, 2021, 1:35:25 PM3/22/21
to bu...@skia.org
Updates:
Status: Fixed

Comment #11 on issue 10680 by brian...@google.com: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c11

Marking this done. There are specific SkSL keywords that people can accidentally use in runtime effects (eg Issue 11301), but at this point, as a rule ... WebGL1 is the only thing that's accepted.

Git Watcher via monorail

unread,
Apr 19, 2021, 1:50:10 PM4/19/21
to bu...@skia.org

Comment #12 on issue 10680 by Git Watcher: Runtime effects: Limit functionality for CPU and low-end GPU
https://bugs.chromium.org/p/skia/issues/detail?id=10680#c12


The following revision refers to this bug:
https://skia.googlesource.com/skia/+/e49703faf265828903464f6a80a33e60f622cef0

commit e49703faf265828903464f6a80a33e60f622cef0
Author: Brian Osman <brian...@google.com>
Date: Mon Apr 19 15:15:24 2021

Remove custom SkVertices data and runtime effect `varying` support

This was an experimental feature. It worked (but only the GPU backend).
It was never adopted or used by anyone, to my knowledge. It's a large
amount of code, and a strange corner of SkSL for users to stumble into.

Bug: skia:10680
Change-Id: I0dda0364bce7dbffa58c32de4c7801ec2a6bc42e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/398222
Reviewed-by: Mike Reed <re...@google.com>
Commit-Queue: Brian Osman <brian...@google.com>

[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/RELEASE_NOTES.txt
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/gm/vertices.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/include/core/SkVertices.h
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/include/effects/SkRuntimeEffect.h
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/include/private/SkSLModifiers.h
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/resources/sksl/errors/BadModifiers.sksl
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/resources/sksl/errors/ModifiersInStruct.sksl
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/samplecode/Sample3D.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/core/SkCanvas.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/core/SkDraw.h
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/core/SkDraw_vertices.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/core/SkPicturePriv.h
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/core/SkRuntimeEffect.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/core/SkVertices.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/core/SkVerticesPriv.h
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/gpu/ops/GrDrawVerticesOp.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/sksl/SkSLAnalysis.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/sksl/SkSLIRGenerator.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/sksl/SkSLLexer.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/sksl/SkSLLexer.h
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/sksl/SkSLParser.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/sksl/codegen/SkSLPipelineStageCodeGenerator.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/sksl/generated/sksl_gpu.dehydrated.sksl
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/sksl/generated/sksl_public.dehydrated.sksl
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/src/sksl/lex/sksl.lex
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/tests/VerticesTest.cpp
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/tests/sksl/errors/BadModifiers.glsl
[modify] https://crrev.com/e49703faf265828903464f6a80a33e60f622cef0/tests/sksl/errors/ModifiersInStruct.glsl
Reply all
Reply to author
Forward
0 new messages