This will use the recently added Bazel toolchain feature
to enforce proper includes for all files in //src/svg/...
In the future, I envision a CI/CQ job that will run
bazel build with a few different configurations and the
--feature skia_enforce_iwyu on to make sure we don't
regress.
Change-Id: Ibb9f816ab626415c11bd2b9b74c503297c4b0723
Bug: skia:13052
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/521036
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
PS1 regenerates the Bazel files.
It is recommended to review this CL with a diff from PS1.
Example output when a file does not pass the test:
tools/sk_app/CommandSet.h should add these lines:
#include "include/core/SkTypes.h"
#include "include/private/SkTArray.h"
#include "tools/skui/InputState.h"
#include "tools/skui/Key.h"
#include "tools/skui/ModifierKey.h"
namespace sk_app { class Window; }
tools/sk_app/CommandSet.h should remove these lines:
- #include "tools/sk_app/Window.h"
The full include-list for tools/sk_app/CommandSet.h:
#include "include/core/SkString.h"
#include "include/core/SkTypes.h"
#include "include/private/SkTArray.h"
#include "tools/skui/InputState.h"
#include "tools/skui/Key.h"
#include "tools/skui/ModifierKey.h"
#include <functional>
#include <vector>
class SkCanvas;
namespace sk_app { class Window; }
---
This makes use of Bazel's toolchain features
https://bazel.build/docs/cc-toolchain-config-reference#features
to allow us to configure compiler flags when compiling
individual files. This analysis is off by default, and can
be turned on with --features skia_enforce_iwyu. When enabled,
it will only be run for files that have opted in.
Example:
bazelisk build //example:hello_world_gl --config=clang \
--sandbox_base=/dev/shm --features skia_enforce_iwyu
There are two ways to opt files in:
- Add enforce_iwyu = True to a generated_cc_atom rule
- Add enforce_iwyu_on_package() to a BUILD.bazel file
(which enforces IWYU for all rules in that file)
Note that Bazel does not propagate features to dependencies
or dependents, so trying to enable the feature on cc_library
or cc_executable targets will only impact any files listed in
srcs or hdrs, not deps. This may be counter-intuitive when
compared to things like defines.
IWYU supports a mapping file, which we supply to help properly
handle things system headers (//toolchain/IWYU_mapping.imp)
Suggested Review Order:
- toolchain/build_toolchain.bzl to see how we get the IWYU
binaries into the toolchain
- toolchain/BUILD.bazel and toolchain/IWYU_mapping.imp
to see how the mapping file is made available for
all compile steps
- toolchain/clang_toolchain_config.bzl, where we define the
skia_enforce_iwyu feature to turn on any verification at
all and skia_opt_file_into_iwyu to enable the check for
specific files using a define.
- toolchain/clang_trampoline.sh, which is the toolchain is
configured to call instead of clang directly (see line 83
of clang_toolchain_config.bzl). This bash script used to
just forward all arguments directly onto clang. Now it
inspects them and either calls clang directly (if
it does not find the define in the arguments or we are
linking [bazel sometimes links with clang instead of ld])
or calls clang and then include-what-you-use. In all cases,
the trampoline sends the arguments to clang and IWYU
unchanged).
- //tools/sk_app/... to see enforcement enabled (and fixed)
for select files, as an example of that method.
- //experimental/bazel_test/... to see enforcement enabled
for all rules in a BUILD.bazel file.
- all other files.
Change-Id: I60a2ea9d5dc9955b6a8f166bd449de9e2b81a233
Bug: skia:13052
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519776
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: Leandro Lovisolo <lovisolo@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
This is a short term solution. The linked bug has more details about
where we want to land. As is, this doesn't use much of the resource key
system but ideally should be updated to do so if we can handle resource
tracking w/in the CommandBuffer efficiently.
Alternatively we may pursue a direction where that is moot because it's
cached globally or held directly by the RenderStep.
As-is, this is a simple approach and should produce equivalent behavior
from a GPU command perspective in the single Recorder situation we are
in currently.
Cq-Include-Trybots: luci.skia.skia.primary:Test-Mac12-Clang-MacBookPro16.2-GPU-IntelIrisPlus-x86_64-Debug-All-Graphite,Test-Mac11-Clang-MacMini9.1-GPU-AppleM1-arm64-Release-All-Graphite,Test-Mac11-Clang-MacMini9.1-GPU-AppleM1-arm64-Debug-All-ASAN_Graphite,Test-Mac10.15.7-Clang-MacBookPro11.5-GPU-RadeonHD8870M-x86_64-Debug-All-Graphite,Build-Mac-Clang-x86_64-Release-Graphite,Build-Mac-Clang-x86_64-Debug-Graphite,Build-Mac-Clang-arm64-Release-iOS_Graphite,Build-Mac-Clang-arm64-Release-Graphite,Build-Mac-Clang-arm64-Debug-iOS_Graphite,Build-Mac-Clang-arm64-Debug-Graphite_NoGpu,Build-Mac-Clang-arm64-Debug-Graphite,Build-Mac-Clang-arm64-Debug-ASAN_Graphite
Bug: skia:13059
Change-Id: I6a110fafe2b69a8bfbc3d859b02d916c973baf26
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/520737
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
Cq-Include-Trybots: luci.skia.skia.primary:Test-Mac12-Clang-MacBookPro16.2-GPU-IntelIrisPlus-x86_64-Debug-All-Graphite,Test-Mac11-Clang-MacMini9.1-GPU-AppleM1-arm64-Release-All-Graphite,Test-Mac11-Clang-MacMini9.1-GPU-AppleM1-arm64-Debug-All-ASAN_Graphite,Test-Mac10.15.7-Clang-MacBookPro11.5-GPU-RadeonHD8870M-x86_64-Debug-All-Graphite,Build-Mac-Clang-x86_64-Release-Graphite,Build-Mac-Clang-x86_64-Debug-Graphite,Build-Mac-Clang-arm64-Release-iOS_Graphite,Build-Mac-Clang-arm64-Release-Graphite,Build-Mac-Clang-arm64-Debug-iOS_Graphite,Build-Mac-Clang-arm64-Debug-Graphite,Build-Mac-Clang-arm64-Debug-ASAN_Graphite,Build-Mac-Clang-arm64-Debug-Graphite_NoGpu
Bug: skia:12703
Change-Id: Iea08a885565811c442e8177b3fa0c668db95fd0f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/521008
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
DrawList's recording functions that paralleled the defined Renderers are
removed in favor of a single recordDraw() call that takes a Renderer.
Device has to do too much logic around determining draw ordering that
is coupled with the selected Renderer's requirements (e.g. stencil)
that it is easier to have a Renderer selector that Device uses, and
then it can inspect the requirements of that Renderer and modify the
ordering tracking as needed.
Right now, this renderer selection process is just in Device, but I
can imagine it living in its own object that the Gpu exposes so that
all wrappers around DrawContext make consistent decisions. This will
also come in handy if Renderers are created per Gpu instead of static.
Bug: skia:12703
Change-Id: I73b5254b7c4183f4c4e54cf5e2993ab8143bd4bd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/521006
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
Bug: skia:12974
Change-Id: I6ec7736edc871241b0cf78413b0d5f7ff9abd8c5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/520736
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
The KeyContext is used in the addToKey methods but must appear in the
AddToKey methods bc the latter can call the former.
Bug: skia:12701
Change-Id: I3143afec8337b1e3e12f1c3cc198714009ca6930
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/520539
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
In preparation for ag/Ib9bec1e1d72169a18e6ad1ce8f9008a65dbe5a71,
which will define it in SkUserConfigManual first.
Bug: b/224771432
Change-Id: I122895f089ce8949c028bccfcb971bd7d1c6ca72
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/521001
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>
These logs were always intended to be temporary. Remove them, along with
RENDERENGINE_ABORTF. This is in preparation for turning *all* SK_ABORT
messages into ones that will appear in stack traces (b/224771432).
Revert "Convert RENDERENGINE_ABORTF to LOG_ALWAYS_FATAL"
This reverts commit 98fbe5eb8d.
Revert "Add more debugging info for backend texture failure"
This reverts commit 13f244c95c.
Revert "Add Android Framework specific logging to SkSurface::MakeFromBackendTexture"
This reverts commit 04a9672c0a.
Bug: b/206415266
Bug: b/224771432
Change-Id: I5a0d97b4e7d14e2a4222dc84c9049ebb4f9e7e0c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/521000
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>
Change-Id: I677ad3073725219897bec9f82d88ad3ba3fedb53
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/521002
Commit-Queue: Herb Derby <herb@google.com>
Auto-Submit: Herb Derby <herb@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
Bug: skia:12974
Change-Id: I70f3ec7901cd32c2f61b23b3f41675fb1db16614
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/516805
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
The block increment parameter, after dividing by address align, has to
fit into 16 bits. SkTBlockList with either large T or a large
"itemsPerBlock" hint can pretty easily exceed this. However, both the
items per block and the block increment are just hints to control
allocation patterns. SkBlockAllocator can have larger blocks than that
based on growth policy, up to its actual allocation size limit.
SkTBlockList also does not need itemsPerBlock in its implementation, so
if the request exceeds what the allocator can do, it's not problematic.
So clamping to the highest storable value is nicer than asserting that
the caller respected the internal limits.
Change-Id: I82b1c20034fd264b65c7eae4d6758caa6b574fb1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/520656
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
We no longer intend to run visual GMs from SkQP.
Change-Id: Ib04958a3d445078d65b72e852afc69781873b93c
Bug: skia:13031
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/520546
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Leon Scroggins <scroggo@google.com>
Commit-Queue: Leon Scroggins <scroggo@google.com>
Today, if the arc command flags are not separated by whitespace, the
parser fails to parse the string. I noticed this when trying to parse a
path similar to the one in the test case when playing around with
PathKit.FromSVGString.
Change-Id: I40967c07dfa03d76d26ac2e060b3ef7ac488d0fc
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/520256
Reviewed-by: Florin Malita <fmalita@google.com>
Commit-Queue: Dan Field <dnfield@google.com>
For discrete GPUs the draw buffers need to be unmapped so that
didModifyRange: is called to updated the managed buffer. In addition,
they should be tracked on the CommandBuffer.
Bug: skia:13033
Change-Id: I931b1bfd438bc75652c04734219690506fad5ea6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/520537
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Support for GM tests was removed from SkQP at
http://review.skia.org/516336. However, the Java harness still expects
us to supply a non-null GM array, and will crash if we don't.
Change-Id: I1f093254e680bf8d40dcb303cd29ae7b44e09b0a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/520538
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Leon Scroggins <scroggo@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This CL switches almost all instances of line tracking over to track
Positions instead. This does not yet add full range support - only the
start offsets will be correct currently. Followup CLs will extend the
ranges to fully cover their nodes.
Change-Id: Ie49aee02f35dcb30a3adb8a35f3e4914ba6939d2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/518137
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:12701
Change-Id: I1fd8dede3eb216c28408bd613119448704c0e7c7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/512356
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This reverts commit 5fe4b6faeb.
Reason for revert: relanding with guards
Original change's description:
> Revert "[skottie] Fix text-on-path tracking"
>
> This reverts commit ca973cbea0.
>
> Reason for revert: g3 image diffs
>
> Original change's description:
> > [skottie] Fix text-on-path tracking
> >
> > Tracking and line spacing computations require knowledge of cumulative
> > values for the whole line => we need two passes:
> >
> > 1) compute cumulative values
> > 2) compute per-fragment position adjustments
> >
> > Currently, #1 is implemented in the main onSync() loop (as we iterate
> > to compute fragment props) and #2 is post-applied via adjustLineProps(),
> > after the main loop.
> >
> > The problem is adjustLineProps() is executed after positioning glyphs on
> > path, and tracking is not taken into account for path positioning
> > (instead it moves glyphs horizontally, unrelated to the path).
> >
> > To fix this, we need tracking adjustments to be applied before
> > positioning on path (which is performed in fragmentMatrix()).
> >
> > - move the cumulative tracking computation to a dedicate lambda
> > (compute_linewide_props)
> > - move the fragment position adjustments to the main onSync() loop
> > (that way they participate in path positioning)
> > - to avoid executing the first pass unnecessarily, add flags to detect
> > the presence of tracking and line spacing animators.
> >
> >
> > Change-Id: Ieef2afb53ffe14177eba0ef41dc5c71149cab070
> > Reviewed-on: https://skia-review.googlesource.com/c/skia/+/518696
> > Reviewed-by: Ben Wagner <bungeman@google.com>
> > Commit-Queue: Florin Malita <fmalita@chromium.org>
> > Commit-Queue: Florin Malita <fmalita@google.com>
>
> Change-Id: Ia99fbb3d7d98eb6a59ff00d796bcc05bc6db63a3
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519597
> Auto-Submit: Florin Malita <fmalita@google.com>
> Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Change-Id: Ia39602178099d7fa016997f02e1bdf705b16bb2e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519598
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Florin Malita <fmalita@google.com>
This CL defines a const string in GrGPUResource and have the
constructors accept it. The label string is then plumbed through the
system for other components to accept it.
Bug: chromium:1164111
Change-Id: I6cc759f9263dedd4b2cc0c3ca7cf280be5d74174
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/508798
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Previously, this wrapped an SkTLazy<T>, but in C++17, SkTLazy is just a
thin wrapper on top of std::optional. This CL removes the middle-man.
Change-Id: I78f88d24a7933dfac4b637367a3d1e7ee80b3070
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519622
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Ben Wagner <bungeman@google.com>
This test fails on P400 GPUs (used in our Golo machines) but the wasm
tests do not honor the dm_flags test exclusions. (Another good reason to
implement skia:13034.)
Failing on tree: http://screen/4PxKQrjxaXpL9Q2
Change-Id: I086fb3293b3f4eaad877064470002525a7d6e75f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519621
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Bug: b/212296687
Change-Id: I9f5b5966e7cc497f8c8591463801ef558297f3ee
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519620
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
This makes the fixed-function blending happier
Bug: skia:12701
Change-Id: I398977a3ce9c25949535f73a83b9eb774d2d1c35
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519616
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
Bug: skia:12845
Change-Id: Ic036dea6b58682a6463f8c34d915730c4bfe677b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519617
Reviewed-by: Robert Phillips <robertphillips@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
This reverts commit ca973cbea0.
Reason for revert: g3 image diffs
Original change's description:
> [skottie] Fix text-on-path tracking
>
> Tracking and line spacing computations require knowledge of cumulative
> values for the whole line => we need two passes:
>
> 1) compute cumulative values
> 2) compute per-fragment position adjustments
>
> Currently, #1 is implemented in the main onSync() loop (as we iterate
> to compute fragment props) and #2 is post-applied via adjustLineProps(),
> after the main loop.
>
> The problem is adjustLineProps() is executed after positioning glyphs on
> path, and tracking is not taken into account for path positioning
> (instead it moves glyphs horizontally, unrelated to the path).
>
> To fix this, we need tracking adjustments to be applied before
> positioning on path (which is performed in fragmentMatrix()).
>
> - move the cumulative tracking computation to a dedicate lambda
> (compute_linewide_props)
> - move the fragment position adjustments to the main onSync() loop
> (that way they participate in path positioning)
> - to avoid executing the first pass unnecessarily, add flags to detect
> the presence of tracking and line spacing animators.
>
>
> Change-Id: Ieef2afb53ffe14177eba0ef41dc5c71149cab070
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/518696
> Reviewed-by: Ben Wagner <bungeman@google.com>
> Commit-Queue: Florin Malita <fmalita@chromium.org>
> Commit-Queue: Florin Malita <fmalita@google.com>
Change-Id: Ia99fbb3d7d98eb6a59ff00d796bcc05bc6db63a3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519597
Auto-Submit: Florin Malita <fmalita@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bug: chromium:1299266
Change-Id: Ic3d0f5e96c9c1d3d0b50cc20b41143481cdbb042
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/519324
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
Tracking and line spacing computations require knowledge of cumulative
values for the whole line => we need two passes:
1) compute cumulative values
2) compute per-fragment position adjustments
Currently, #1 is implemented in the main onSync() loop (as we iterate
to compute fragment props) and #2 is post-applied via adjustLineProps(),
after the main loop.
The problem is adjustLineProps() is executed after positioning glyphs on
path, and tracking is not taken into account for path positioning
(instead it moves glyphs horizontally, unrelated to the path).
To fix this, we need tracking adjustments to be applied before
positioning on path (which is performed in fragmentMatrix()).
- move the cumulative tracking computation to a dedicate lambda
(compute_linewide_props)
- move the fragment position adjustments to the main onSync() loop
(that way they participate in path positioning)
- to avoid executing the first pass unnecessarily, add flags to detect
the presence of tracking and line spacing animators.
Change-Id: Ieef2afb53ffe14177eba0ef41dc5c71149cab070
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/518696
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Florin Malita <fmalita@chromium.org>
Commit-Queue: Florin Malita <fmalita@google.com>
Moved the MatrixFoldingES2.sksl test case for matrix construction with
side-effects into a new PreserveSideEffects.sksl test and added new test
cases for various vector and matrix types and constructors. The new test
is written such that none of its contents should be folded away.
Note: This test does not pass on NVIDIA GPUs when using OpenGL as
discussed in skia:13035. Notably, NONE of the increments are executed on
those GPUs as ALL increment expressions seemingly get subjected to
constant-folding. The test is disabled on NVIDIA GPU bots.
This also means that the remaining MatrixFoldingES2.sksl tests now work
on NVIDIA GPUs when using OpenGL (with the exception of Tegra3 + OpenGL
ES).
Bug: skia:13035, skia:11919
Change-Id: I561bb62fe2b6b814ba80fbc492d3885bbcd6b65b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/518278
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Arman Uguray <armansito@google.com>
