GrDawnGpu tries to guarantee the CPU mapping invariants of GrDawnBuffer
objects that are managed by a GrStagingBufferManager by issuing and
tracking mapAsync requests following command buffer submission. However,
this logic suffered from some issues where:
1. The code always assumed mapAsync succeeds. If a mapAsync request
failed during shutdown due to a lost GPU connection, the GrDawnGpu
destructor would spin forever waiting for pending mapAsync requests
to complete.
2. If a client unmapped and then re-mapped a buffer that isn't managed
by the async staging buffer logic in GrDawnGpu, the buffer would
never get mapped and likely hit an assertion for a non-staging
buffer.
These are now fixed:
* GrDawnBuffer now has more explicit error handling to make it more
tolerant to mapAsync failures.
* GrGpuBuffer now relies on mapAsync completion callbacks instead of
spinning on `GrGpuBuffer::isMapped` as a buffer may never get mapped
in the case of an error. This also has the benefit of tracking the
mapAsync procedure state on a per-request basis intead of relying on
state stored in GrDawnBuffer. The existing `fBusyStagingBuffers` has
been replaced by a single reference counted `GrDawnAsyncWait` object.
* A synchronous/blocking map function has been provided to satisfy the
`GrGpuBuffer::onMap` contract. This method is not used in the existing
staging buffer use cases in practice but it solves part of problem #2
above.
* GrDawnGpu::onReadPixels now uses a GrDawnBuffer's blocking map
functionality using the public GrGpuBuffer API. This has the same
behavior as the existing blocking map that created and mapped a
wgpu::Buffer directly.
* The invariants around GrDawnBuffer lifetime and CPU mapping are
documented in class level comments.
Bug: skia:12512
Change-Id: I8bb92137fbd60c31066e4071bd696018b3563bb8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/533758
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Arman Uguray <armansito@google.com>
G3 prefers license() first.
This was done mechanically with a big find/replace
Change-Id: I8c33c7bc10a6bec42e966cad81c259954e841811
Bug: skia:13211
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/535898
Reviewed-by: Ben Wagner <bungeman@google.com>
Ran the following commands:
find -name "BUILD.bazel" -exec sed -i -e '1iload("//bazel:macros.bzl", "cc_library", "exports_files_legacy")\nexports_files_legacy()' {} +
buildifier --lint=fix --mode=fix -r .
This had the effect of making sure we can export all of our
files in G3 (until we no longer have legacy targets) and
making all of our cc_libraries shim-able.
bazel/macros.bzl has the human-contributed changes, the rest
were mechanical.
Change-Id: I8e24e30e74b038cfd072cdbe4078bfd1d213dd46
Bug: skia:13211
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/535359
Reviewed-by: Ben Wagner <bungeman@google.com>
This node was only used to detect recursion while inlining. We no longer
need to do this, because we disallow recursion in all programs.
The removal of one IRNode per inlined function actually allows for
slightly more aggressive inlining, since we restrict inlining based on
IRNode consumption. This allows the "ExponentialGrowth" tests to inline
a bit more deeply than before.
Change-Id: I894dbb1ca3096bb785b67facb01cc9c630f694c4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/534780
Reviewed-by: Arman Uguray <armansito@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
We enforce no-recursion in all programs now, not just Runtime Effects.
Change-Id: I3737329e4526fa1b7fdbb47ccb959f78f507f665
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/535119
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
- Disables MSL via SPIRV (we don't have the necessary DEPS)
- Adds new context type, configs, etc...
- Minor tweaks to the ANGLE test context code
Bug: angleproject:7155
Bug: skia:13272
Change-Id: I258ed19abba01ad96cfe6fca46b558af2340880e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/534569
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Bug: skia:13118
Change-Id: Ibfcc8df522a87f8ddf6e185121c0844d453b2012
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/534139
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
This will let us use this utility everywhere.
Bug: skia:12701
Change-Id: I9342d0b40a81789ed93e3ec4009e5602033d6691
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/534662
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This just plumbs through a flag that allows snippets to request a
dev2Local matrix uniform and uses it for gradients and the image shader.
Bug: skia:12701
Change-Id: If1eadff8d5e40d81d9e3794db4b7f816127c4b75
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529810
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This limits our error reporting to the first 16MB of SkSL code in a
program, and error marks are limited to a run of 255 characters or
less. In practice, these limits do not affect normal code in any way.
This gives us the same tight memory footprint we originally had when
positions were stored as `int32 fLine`.
Change-Id: Idef04344324870a7b92aca154feb5e1a0121d284
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/533699
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
These were not being generated because they were in the "settings" test
group; we have switch-rewrite settings that also need to be tested.
This was a blind spot in our golden output coverage; without these
tests, there is very little switch-statement usage in our corpus.
They are now in the SPIR-V test group as well.
Change-Id: Ic23b726d00c3047f2d19f7f6dc41e58e600e991c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/534141
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This is a reland of commit 866bd64c1b
Original change's description:
> Check that the GrBackendFormat of a promise image is textureable.
>
> Bug: chromium:1311844
> Change-Id: I13bae71305ae9520851cd1ea38a1da737b934dd1
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/532196
> Reviewed-by: Brian Salomon <bsalomon@google.com>
> Commit-Queue: Greg Daniel <egdaniel@google.com>
Bug: chromium:1311844
Change-Id: I01e2d0e4eb01ee7d97798db6eeff73fec76bf521
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/532408
Auto-Submit: Greg Daniel <egdaniel@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
We now have a new type of ProgramKind, private runtime shaders.
`sksl_rt_effect.sksl` is now only loaded for these kinds of program.
Rather than having a special-case check for sk_FragCoord in
SkRuntimeEffect, the symbol will no longer exist at all unless a private
options flag is set.
Change-Id: I9223baaf59d74c44d64f322cd57fc841625342b7
Bug: skia:12202
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/532784
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Right now nothing prevents it from working. (skslc doesn't use
SkRuntimeEffect::MakeInternal.) This will be fixed in a followup.
Change-Id: Ib8479220e1f194b035516d976a7369d926a07f5d
Bug: skia:12202
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/532783
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This has two benefits:
1) We get detailed error messages with accurate positions
2) We can actually test these in our golden .rts files.
Thanks to #2, add a new unit test file, and adjust some existing files
that were breaking these rules.
Change-Id: I0b65e2f06f79ce8cbea9bad4c3d27062ec9b6e6c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/532769
Reviewed-by: Arman Uguray <armansito@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Bug: skia:13169
Change-Id: Icf0b720d3e3a13d490aba8495cf9db83d1d62318
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/532762
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This will allow us to reuse this logic in Graphite.
Change-Id: I649dcd3893a1355af457a2583a6db3066fb87c9a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/532758
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Bug: skia:13170
Change-Id: I11ef0ea5ac3ae61b24a47805bb3290a37880cfee
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/532536
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:13173
Change-Id: Ifbcce77605dd781563568293fc501dfa31f143da
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528706
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:13118
Change-Id: Iab67fd1148182fdd29a38b69f27c51b13942a2b5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/532400
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
* Move Rectanizer classes to a shared location
* Have GrDrawOpAtlas store SkColorType and explicit bytes-per-pixel
instead of GrColorType.
Bug: skia:13118
Change-Id: Ib5c3d79394c89dce7f06e8eddf09a5f6a9543a7f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/531320
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Bug: skia:13171
Change-Id: I6dffb98ac2464f930995cf8ea57e422091d20fd2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/531743
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:13118
Change-Id: Ica760f58107de021b7823f69b94809dd2f313ac7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/531739
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
(This mirrors an optimization performed in the constant folder.)
Expressions like `OpIEqual %20 %20` or `OpFUnordNotEqual %15 %15` can be
replaced by `true` or `false` on sight. The GLSL spec makes it clear
that checking for NaN is optional:
4.7.1 Range and Precision
"... NaNs are not required to be generated. Support for signaling NaNs
is not required and exceptions are never raised. Operations and built-in
functions that operate on a NaN are not required to return a NaN as the
result."
Change-Id: I2e29b659a73582e9ade0eb61f70f7d362a007c50
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/531550
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Previously, every expression/statement type was responsible for
pruning or clearing the store-cache when branches were involved. This
was difficult to reason about and easy to get wrong, particularly if
the details are not fresh in your mind.
Now, `writeLabel` takes care of the details for you. Pass in the
location of the branch(es) which use the label, and the proper cache
updating behavior will occur automatically.
Some of the label enum types are not strictly necessary and exist for
the benefit of a reader. Specifically:
- `kBranchlessBlock` and `kBranchIsOnPreviousLine` are synonyms
- `kBranchIsBelow` and `kBranchesOnBothSides` are also synonyms
The hope is that extra enum names will be easier for a reader to
follow, versus fewer but very-verbose enum names (like
`kBranchIsBelowOrOnBothSides`).
This change earned some very minor switch-related dividends. Previously,
every label in a switch was treated as a forward-branch, but in fact,
the very first label in a switch is privileged. This is because we are
branching from the previous line, and the store cache is trustworthy in
this case. (Versus "branching from above," where the store cache needs
to be pruned before it can be trusted.)
Change-Id: I38b539069c22be9f0777b632f60f0eab2409d687
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/531540
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
We now have two functions `writeOpLoad` and `writeOpStore` which are
in charge of writing SpvOpLoad and SpvOpStore instructions.
`writeOpStore` also keeps track of pointer stores in a "store cache."
Subsequent loads from that same pointer will be found in the cache and
will return the value stored in that pointer instead.
Such a cache definitely cannot work in the face of control flow, so we
make the following concessions:
- `pruneReachableOps` is now `pruneConditionalOps`. Any pointers that
are altered inside a potentially-unreachable block are cleared from
the cache entirely.
- The entire store cache is cleared at all OpLabels within a loop.
The cache also cannot work in the presence of swizzled stores, so we
make another significant concession:
- The entire store cache is cleared whenever we store into a non-memory
pointer (e.g., assigning into a swizzled LValue, such as `foo.xz`).
Despite these significant limitations, this manages to dramatically
shrink many real-world examples.
Change-Id: I0981a0cf7b45b064e153e9ada271494c8e00cad5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/530054
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, we would determine the operandType by calling
`getActualType`. This function converts half-precision types to full-
precision ones, which seems to have been unintentional. Fortunately, the
operand type is not actually emitted into the SPIR-V by most code paths
(most paths use the resultType instead) so it was not a significant
impact in practice. A few matrix-based paths emitted ops using this type
and these paths now emit RelaxedPrecision as expected.
Change-Id: I32f4c0327427476fee6b78953284818b7970b6e8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/530543
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Arman Uguray <armansito@google.com>
Previously, we only handled the simple case of extracting from an
OpConstantComposite. Now we also handle the complex case of extracting
from an OpCompositeConstruct, where vectors can be composed of other
vectors.
This is particularly challenging because OpCompositeConstruct can
contain SpvIds from almost any other instruction, so we need to be
able to decode those instructions and figure out their type. For
instance:
%5 = OpFAdd Vec2 %1 %2
%6 = OpFAdd Scalar %3 %4
%7 = OpCompositeConstruct FloatVec3 %5 %6
%8 = OpCompositeExtract %7 2
The %8 (OpCompositeExtract) could be replaced with %6 but we need to
peek at the type in *both* OpFAdd instructions to decode this. It
only works when the affected instructions are in-cache, so many
opportunities are currently not optimized because their code still
uses the original, uncached form of writeInstruction.
Change-Id: I5719ae6284f32e1d6f2c898eca282c22b94fc764
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529743
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Change-Id: I3a9e9e90b0ac1b1099830eaca06506bcce794144
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/530055
Commit-Queue: Brian Salomon <bsalomon@google.com>
Auto-Submit: Brian Salomon <bsalomon@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Bug: skia:13219
Change-Id: I57c5c2aa40e6eb85d5e6045d6f3374d0379efd39
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/530337
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This removes the assert from the SPIR-V generator so the
tests compile. The generated SPIR-V is incorrect. The next
CL fixes the generator, and restores the assert.
Change-Id: I77b507cf7fb5eac481322887000bd1c73cd5c899
Bug: skia:13219
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/530336
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This is a reland of commit 60ff0facbf
Structs are now deduplicated using a [Type*, SpvId] map.
Original change's description:
> Use op cache when emitting types.
>
> We no longer need to maintain a separate `fTypeMap` for mapping types
> to SpvIds, since the op cache handles this automatically.
>
> We also now support deduplicating instructions that don't have a result,
> such as decorations. (In particular, we needed to avoid emitting the
> SpvDecorationArrayStride op every time the array type was accessed, but
> this op doesn't have a result ID.)
>
> Change-Id: I779b8c8e3de5973b8f487b28c0a8ece9a1041845
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529732
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I9f6a78d58e8af38a1fd690a8860d8b5aa3193be6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529748
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Change-Id: I800fa2a1fb0e64ad478c76ea2d5cda176ea8f48b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529746
Auto-Submit: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This reverts commit 60ff0facbf.
Reason for revert: Broke D3D bots
Original change's description:
> Use op cache when emitting types.
>
> We no longer need to maintain a separate `fTypeMap` for mapping types
> to SpvIds, since the op cache handles this automatically.
>
> We also now support deduplicating instructions that don't have a result,
> such as decorations. (In particular, we needed to avoid emitting the
> SpvDecorationArrayStride op every time the array type was accessed, but
> this op doesn't have a result ID.)
>
> Change-Id: I779b8c8e3de5973b8f487b28c0a8ece9a1041845
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529732
> Reviewed-by: Brian Osman <brianosman@google.com>
> Commit-Queue: John Stiles <johnstiles@google.com>
> Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I0e2187f88f2a945fd6f88ce75ff815e03d2f7df5
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529747
Auto-Submit: Brian Osman <brianosman@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
We no longer need to maintain a separate `fTypeMap` for mapping types
to SpvIds, since the op cache handles this automatically.
We also now support deduplicating instructions that don't have a result,
such as decorations. (In particular, we needed to avoid emitting the
SpvDecorationArrayStride op every time the array type was accessed, but
this op doesn't have a result ID.)
Change-Id: I779b8c8e3de5973b8f487b28c0a8ece9a1041845
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529732
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, we stringized the types and put them into fTypeMap. Using
the op cache is a simpler mechanism that should work equally well.
Output diffs are almost all ID reorderings. In a few cases we
managed to deduplicate function types that stringize differently but
come out the same in SPIR-V (e.g. no float/half distinction).
Change-Id: If7de5b2dafa12d05c3c2c497a243e9e3908dfee7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529805
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Change-Id: I3f6e25ec7b31339bfc9bd2435bc9226e6d9be06b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529498
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: Ic19d7591c90f75f04dd1c58b406f2b770f9780c7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529351
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
If we start with an OpConstantComposite, then we do an
OpCompositeExtract from it, we can look up the result directly from op
cache and avoid doing any work. This helps our matrix code a lot.
Change-Id: Idfbdc0c69676b9c1e91cdc57bf0d6382b9b5d8d4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529339
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
https://en.cppreference.com/w/cpp/container/span/subspan
The real std::span::subspan in C++20 allows the "count" argument to
default to `std::dynamic_extent` (a fancy way of spelling ~0U). I didn't
think it would be worth adding `skstd::dynamic_extent`, but I did have
a use for an unbounded subspan, so I added a single-argument version to
SkSpan.
Change-Id: I297cc452cf2db727a3f9869ff8f46f3527e19370
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529603
Reviewed-by: Arman Uguray <armansito@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, we didn't usually generate OpConstantComposite ops for
matrices. Now, a matrix assembled from constants should come out as a
constant.
Change-Id: I458718901686dffb84e4079a81017d61195420d3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529338
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
The output changes here are almost entirely a wash, because we already
had support for caching scalars and vectors. Almost all changes are just
inconsequential reorderings of IDs, and the removal of RelaxedPrecision
decorators on constants (which were not meaningful).
Change-Id: I45340c4a240cb504b7c4a934b3db178d2f39ec99
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528709
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Change-Id: Ia799cdff5288efe5d5d53e8d8f77cf32f3343371
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529131
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
Rearrange SkGlyphRect so that leftTop is negative. This makes
calculating width and height = rightBottom + leftTop. Then we
can use this as the dimensions of the glyph in the digest.
Rename: topLeft to leftTop.
Bug: skia:13192
Change-Id: If06ebf6e595f380f8fbb9a3381e87945e312eceb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529336
Reviewed-by: Ben Wagner <bungeman@google.com>
Commit-Queue: Herb Derby <herb@google.com>
Make the glyph buffers use the same Direct position calculations
for Bitmap and GPU processing. This will create fewer changes when
switching to SkGlyphDigest based API.
Bug: skia:13192
Change-Id: I7e3e44dcfc1a4bad014d0ebe2bef3c1b28c712f4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529277
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Herb Derby <herb@google.com>
Bug: skia:12701
Change-Id: Ib02db7160599d3c15a01597c746ce5131f2998e0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/526298
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This is a reland of commit ae5e846047
Original change's description:
> [graphite] Move Graphite into Skia base directories.
>
> Change-Id: Ie0fb74f3766a8b33387c145bd1151344c25808cb
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528708
> Reviewed-by: Kevin Lubick <kjlubick@google.com>
> Reviewed-by: Greg Daniel <egdaniel@google.com>
> Commit-Queue: Jim Van Verth <jvanverth@google.com>
Change-Id: Ia575fd49206ad0b665a6a9153317e738bb321446
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/529059
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
* Wire up the WGSLCodeGenerator to SkSLCompiler.
* Wire up build rules to generate WGSL from unit tests.
* Include HelloWorld.sksl as the first complete program.
Bug: skia:13092
Change-Id: I283cf5971b6856126b9fc23340afacff5cc54697
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/526760
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Arman Uguray <armansito@google.com>
Bug: skia:12701
Change-Id: Iaac8b6fd0f2d9d1f8d5771f8204047a9127184c1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528041
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
Previously, we used unscoped blocks for two similar functions:
- Rewrite one statement as two simpler statements:
`int a, b;` -> `int a; int b;`
- Group together multiple statements without braces. e.g. the inliner
uses unscoped Blocks to rearrange statements.
Conceptually, these are different from the debugger's perspective. The
compound statements should be treated as one unit; the grouped
statements should be treated individually (and the enclosing Block
should be ignored). A Block now contains a BlockKind enum to
distinguish between these cases.
Change-Id: Ie14a570bb46992689fb96b8fd3b67f2ca6e5239f
Bug: skia:13189
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528655
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Change-Id: If3d7e222023efb957eba66adc1f9ca601c2ffd67
Bug: skia:13189
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528916
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This CL defines a const label string in GrSurfaceProxy 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: I5a7f7708707e3d486eb1f8931db4c49afba5a33b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/525236
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Change-Id: Ie0fb74f3766a8b33387c145bd1151344c25808cb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528708
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
It was decided that being able to report positions from C++ code is not
worth the cost of having all of these captures everywhere.
Change-Id: I94981d9f780bd95df8b56198210c9cdd5f16239c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528366
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
`writeComposite` can write and deduplicate constants, so it's preferable
to manually emitting an OpCompositeConstruct opcode.
Change-Id: I0c4ac8f8a456c8561c0b6a90cd316934f20895e8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528638
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
`writeComposite` can write and deduplicate constants, so it's preferable
to manually emitting an OpCompositeConstruct opcode.
Change-Id: Ie5c23af76822da762eadac8ff0ab0c6cc0febd31
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528637
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Some GPUs (Adrenos in particular) perform noticeably better when we
use OpConstantComposite instead of OpCompositeConstruct. This also gives
us some deduplication of redundant ops.
Change-Id: I53b7a3e1cf61e51647a661a08ff4c7b53ee60f10
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528636
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Change-Id: I484067cb1f6025dc9e6770c51c99bfc2c5925652
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528365
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
This adopts a trick from SkVM to avoid sorting entirely.
Change-Id: I586c8a3613b48241842a7d8eba1c9d68a4717f83
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528368
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I238d29ba0250224fa593845ae65192653f58faff
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528156
Reviewed-by: Kevin Lubick <kjlubick@google.com>
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Blocks did not previously track their position, creating problems
reporting some errors (which will be improved in followup CLs). Fixing
block positions changed the reporting of do loop errors, requiring do
loop position tracking to be updated as part of this change.
Change-Id: I3bd048a62d912914edf679f42607de1b5eafc2b9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528045
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Caveat: on my machine, Nanobench doesn't detect any change (pro or con)
on this CL.
I'm working under the assumption that function calls have a non-zero
cost--they may be inlined (bloating code size), or not (incurring the
costs of a function call, register push/popping, etc). This CL avoids
making six calls to $blend_set_color_saturation by using two half3
variables. These half3s are used to swizzle the result--they contain two
zeros and a one, so multiplying them by a scalar will put the result in
the desired component. I've also made some very minor simplifications to
the math that were made possible by reordering.
Change-Id: I0c1ef88d165365376078846324be8bb723548512
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528043
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
A ternary of the form `anything ? value : value` can be reduced to a
comma-expression of the form `anything, value`. This seems like a rare
case in real code, but it's easy enough to detect with our existing
toolbox.
The `anything` test-expression will be eliminated from the expression
if it has no side effects, using our existing constant-folding rules
for the comma expression.
Change-Id: I1285b04cd6a08f1bed614aa1aa6f37ea2447de91
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528439
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Previously, when we vectorized scalars, we would not place them in the
constant buffer, so we could emit simple vectors like (0,0,0) or (1,1,1)
more than once. Now, we use `writeConstructorSplat` to vectorize, which
knows how to write constants.
Change-Id: Ic97c0ce5415fd46ff8c7fb7dac9205844633ef3a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527921
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: I76a01cf18583fab619459f258119b780ff978be5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527840
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Today I learned that `mix(a, b, 1)` can reduce precision. Ternaries do
not suffer from this problem.
Change-Id: I58814d00193ccbff53960030d163d31c49234f6c
Bug: skia:9320
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528161
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
The inliner can do a better job with functions that only have a single
return by eliding a temp variable. In this case, it was simple to adapt.
Change-Id: I9a5ee26cf546db1b2647cdf95d4cdba6649ea19b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528160
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
SkSL errors now identify the specific range of code they are describing,
rather than just the line number.
Change-Id: Ifabb3148476f9b4cd8e532f23e5b38e1cf33a87e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528039
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
While I was fixing up Chrome's uses, I found some failures
there that I did not see in Skia, and tracked them down
to a few other places where we include SkColorSpace
and it is not strictly necessary
- SkCustomMesh.h
- GrColorInfo.h
- GrColorSpaceXform.h
- SkColorSpaceXformSteps.h
For these files (and their .cpp files), I added enforcement
of include-what-you-use, and then fixed the myriad of places
which were depending on these transitive includes.
One change to help Chrome is the manual overloads of
SkImage::MakeFromAdoptedTexture instead of using default
parameters. This makes it so callers of that function
do not need to include SkColorSpace if they were going
to pass nullptr for it anyway.
Bug: skia:13052
Change-Id: I16bf8ed5e258225d887f562f2c189623b1ca9c23
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527056
Reviewed-by: Robert Phillips <robertphillips@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
This means that the UniformManager w/in the SkPipelineDataGatherer will
now collect all the uniforms into a single block of memory.
Bug: skia:12701
Change-Id: I504a014d37662619191d9b519b4e1add69eac8bb
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527837
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This removes the use of the dubious void* 'srcs' array.
Bug: skia:12701
Change-Id: I5078d48c119d4e84e83a78b10d0bd4a9a1d8cd8c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527777
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This reverts commit 2e6f60f423.
Reason for revert: draws black incorrectly in various iPhone 8 tests
Original change's description:
> Fix color fringes on blend_hue and blend_saturation.
>
> Previously, we checked for division against zero, but didn't do anything
> to prevent division against extraordinarily small values. Now, we only
> saturate if the delta between max and min is greater than 0.00001.
>
> Change-Id: I7d1df3430941c7e1a7f94e597d5449f9259612d6
> Bug: skia:9320
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527498
> Auto-Submit: John Stiles <johnstiles@google.com>
> Reviewed-by: Greg Daniel <egdaniel@google.com>
> Commit-Queue: Greg Daniel <egdaniel@google.com>
Bug: skia:9320
Change-Id: Id83376080eed684577b3592c5e1bee3c80fc3fc9
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/528038
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Derek Sollenberger <djsollen@google.com>
Commit-Queue: Derek Sollenberger <djsollen@google.com>
This addresses (hopefully) all of the remaining suboptimal positions in
SkSL error reporting.
Change-Id: I5bc977b03d51153b841a89fa687e54e3e9cb6ec3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527976
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This cleans up a lot of positions produced by DSLParser to make them
actually match the ranges of the elements being parsed.
Change-Id: Ic3a9d62c99c4b5f92b84a597a2ceba386bbcc334
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527501
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
This is moving to the world where the SkPipelineDataGatherer holds
a UniformManager. In that world the UniformManager will need to
incrementally allocate the uniforms it is collecting.
Bug: skia:12701
Change-Id: I6ed26948d7d0f2979130c4908d0c34c32604cd75
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527736
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
With UniformData being stored in a arena and TextureData still being
held by unique pointer, the PipelineDataCache needs to be able to handle
both.
Longer term, TextureData will also be held in an arena and this
split will go away.
Bug: skia:12701
Change-Id: Iab6c6542fbce4f886410f985e6c93ddaa125152e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527504
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
We already performed a matching simplification for `0 - x`, so this
seemed like a straightforward improvement.
Performing this simplification causes the expressions in the test code
to match on both sides (e.g. `-one == -one`) which allows them to fold
away.
Change-Id: Idf87a98024dd6831b45d0384285ead2e2e039493
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527656
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
We use multiplication by 1 or -1 to branchlessly choose one of `min` and
`max` in the same function.
Change-Id: I44cf747feeae75a9c3e00f36e112e0a429871e86
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527596
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Hard-light is just overlay with the parameters reversed.
Change-Id: I6cf5963b1252cba3a7b71a56f4094a070188f8b2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527503
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
These functions were functionally almost identical, except:
- Sometimes sda/dsa are flipped
- Sometimes the saturation is not updated
We now have one method (blend_hslc) which can do all four blend
operations. It takes two new parameters ("flip" and "saturate") to
handle these four variations.
This reduces our shader count on some of our most shader-heavy slides
(e.g. aaxfermodes, xfermodeimagefilter) at a pretty reasonable cost.
Change-Id: Ifa8a48399851a9badb5d50038de1e25e60d44ebd
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527281
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This will allow expressions like `-x == -x` or `!y == !y` to be detected
as matching expressions (which enables various constant-folding paths).
(Also, migrated the analysis code into a separate cpp.)
Change-Id: I3e317fdaed3762f8fa19e684a5ed557fc9348c7c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527617
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Followup CLs will improve their output.
Change-Id: I07059348f68cd6cd3154c31a41f81018b26a44e5
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527616
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Previously, we checked for division against zero, but didn't do anything
to prevent division against extraordinarily small values. Now, we only
saturate if the delta between max and min is greater than 0.00001.
Change-Id: I7d1df3430941c7e1a7f94e597d5449f9259612d6
Bug: skia:9320
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527498
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
Change-Id: Ibff49d1928d7f82d04930c8cfd9d574780732c0d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527497
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We were not propagating the position into a double-negated expression,
leading to an assertion failure in PrefixExpression.
Change-Id: I1970ff1a06d9631582626c68e151f12f6b3ef278
Bug: oss-fuzz:46381
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527507
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
The SkPipelineDataGatherer is going to subsume all the UniformManagers
scattered across the code so it will need the Layout in its ctor and
be plumbed where ever there is a UniformManager (in order to replace it).
Bug: skia:12701
Change-Id: I4f42bf50023e9e66c90f9a14833b976e214e1cc1
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/526275
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
The unused variable was breaking compilation for some versions of clang
Bug: skia:12701
Change-Id: I8ded5bf3d18136b5679878090186746b88292c3d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527236
Commit-Queue: Robert Phillips <robertphillips@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
We didn't have any existing tests that exercised this path; it is
separate from most operators since it has no C++ equivalent.
Change-Id: I95b538dad01f8c8b122954fb5f66337371a398a8
Bug: oss-fuzz:46289
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/527196
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
This is currently unused and can be re-obtained from the paint params
key if needed. Currently, it is getting in the way of updating how we
collect uniform data.
Bug: skia:12701
Change-Id: I5057cdfa42252b03b0f9756290735bf4b20dcb6b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/526272
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
These aren't currently used (we recompute them from the PaintParamsKey)
and are only getting in the way as we change how we accumulate uniforms.
We can always re-add them if the recomputation becomes a hot spot.
Bug: skia:12701
Change-Id: Id91ea0d27543666c75c154fc585d954c5f89aded
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/526270
Reviewed-by: Jim Van Verth <jvanverth@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This is a reland of commit 1aedd5dc11
Original change's description:
> Always apply mipmap sharpening on GPU
>
> Bug: skia:13078
>
> Change-Id: If459a96eba09fb10e967bc364435f79b83fdc1ec
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/522099
> Reviewed-by: Michael Ludwig <michaelludwig@google.com>
> Commit-Queue: Brian Salomon <bsalomon@google.com>
Bug: skia:13078
Change-Id: Ic05b38fc07566f090d609431f2738d64dfdc8a66
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524218
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
This is a reland of commit 355f0f9fa2
Original change's description:
> Change GPU LOD bias to be just shy of -.5.
>
> We want to ensure that when a MIP level is 1:1 with device space
> that kNearest picks that level instead of a larger level.
>
> Bug: skia:13078
>
> Change-Id: I703d08ab394e1d39b31d16946067a2ead415c72a
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524224
> Reviewed-by: Michael Ludwig <michaelludwig@google.com>
> Commit-Queue: Brian Salomon <bsalomon@google.com>
Bug: skia:13078
Change-Id: I7fc765a8718d770ebdac68adf9c59ff15d8c8451
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/526517
Commit-Queue: Brian Salomon <bsalomon@google.com>
Auto-Submit: Brian Salomon <bsalomon@google.com>
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
This reverts commit 355f0f9fa2.
Reason for revert: blocking chrome roll, should be #if defined(...) for the guard
Original change's description:
> Change GPU LOD bias to be just shy of -.5.
>
> We want to ensure that when a MIP level is 1:1 with device space
> that kNearest picks that level instead of a larger level.
>
> Bug: skia:13078
>
> Change-Id: I703d08ab394e1d39b31d16946067a2ead415c72a
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524224
> Reviewed-by: Michael Ludwig <michaelludwig@google.com>
> Commit-Queue: Brian Salomon <bsalomon@google.com>
Bug: skia:13078
Change-Id: I42d6e99509a87f0354f104f2c0177e78cf0d0e21
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/526462
Auto-Submit: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Bug: skia:13114
Change-Id: I653ab746927abdd1491e070e2e27252bf056d233
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/526024
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
We want to ensure that when a MIP level is 1:1 with device space
that kNearest picks that level instead of a larger level.
Bug: skia:13078
Change-Id: I703d08ab394e1d39b31d16946067a2ead415c72a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524224
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
This doesn't reuse memory w/in the gatherer but sets us up to do so.
Bug: skia:12701
Change-Id: If5ed962e73609b113581ed2bd681e872fe3214c0
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/526029
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
We were not freeing the memory allocated by copyToMask in
one place.
Change-Id: I4404395e8bc2beb43eca5226e947c42b22805ef7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/526076
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
This doesn't actually change the memory allocation but sets up to do so.
90% of the CL is just renaming SkPipelineData to SkPipelineDataGatherer.
The main interesting changes are those to:
ExtractPaintData
in DrawPass.cpp
and SkPipelineData.h/.cpp
Bug: skia:12701
Change-Id: I3e18f9b3f16166649de1bf1f4399d5521d817eb3
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524763
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
This moves the Build-Debian10-BazelClang-x86_64-Release-IWYU
job from experimental to on when a file in one of the
folders that we enforce IWYU is modified (currently
for svg, sksl, and now, debugger).
Change-Id: Ia6fe1e7b30fc486db3eb081b6a64bc4c250cbf0b
Bug: skia:13052
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/525796
Reviewed-by: Derek Sollenberger <djsollen@google.com>
Commit-Queue: Kevin Lubick <kjlubick@google.com>
The builtin variable scanner did not check builtin code for the presence
of sk_FragColor, etc. We currently get away with this because none of
the existing builtin code uses a builtin variable.
Now FindAndDeclareBuiltinVariables checks shared program elements too.
Change-Id: Ifb3ee3857ef73b18d9e4f406970f0f67681dd4be
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/525042
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Bug: skia:12845
Change-Id: If3ac2b6ba2c8e28328ee5805a29fc83353220364
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524756
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Jim Van Verth <jvanverth@google.com>
Most of these are pretty mechanical generated changes.
IWYU noticed one issue with DSLCore.h, which was fixed here.
Change-Id: I5629565ad3c2817daa71907c62f932d93f9d78ab
Bug: skia:12541
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524617
Reviewed-by: Ben Wagner <bungeman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This closes one of the last gaps in SkSL's constant-folding abilities.
Change-Id: I65c0f2e5fe11a7d47ab2069b2992403fca78b8a7
Bug: skia:12819
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524761
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Arman Uguray <armansito@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
The expression `!x ? y : z` can be optimized to `x ? z : y`, saving a
bit-not. SkVM now supports this optimization.
Change-Id: I06a0d2a716947de1021ba66b054b92e25568c641
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524226
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
SkVM has a `bit_clear` opcode dedicated to the operation `x & ~y`, but
the optimizer was not smart enough to combine a bit-and with a bit-not
and replace it with a bit-clear. Now, it can.
Change-Id: Ida5345c3def0a4bf7afa08bb7f7835e1e2e37677
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524225
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Arman Uguray <armansito@google.com>
Previously, our ID canonicalization was simply "lower ID numbers before
higher ID numbers" and was done separately at every opcode by taking
the min and max of (x.id, y.id).
Now, this logic is factored out into a helper function
`canonicalizeIdOrder` and has two rules:
- Immediate values go last; that is, "x + 1" instead of "1 + x".
- If both/neither are immediate, lower IDs before higher IDs (as
before)
This change lets us remove a lot of simplification logic. We no longer
need to check for both `x + 0` and `0 + x` when removing no-op
arithmetic; now we can be certain that the immediate will always come
last, so just checking for `x + 0` is sufficient.
Change-Id: I66cc5c23bba414041c0bc556521d3db57fac504d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/524222
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Bug: skia:13086
Change-Id: I0fc3243fb3f3974a32726237358522171ae33c41
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/523419
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
This is needed for accurate error reporting when we start reporting
ranges rather than line numbers.
Change-Id: If465317e04685e91ab7c408d29e82028b5d59d1a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/523425
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
Bug: skia:13078
Change-Id: If459a96eba09fb10e967bc364435f79b83fdc1ec
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/522099
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Brian Salomon <bsalomon@google.com>
This test fails on Nvidia GPUs on OpenGL due to an issue that only
affects GLSL. Disabling this test to reduce developer noise until we
have a way to re-enable it more selectively in dm.
Bug: skia:13034,skia:13035
Change-Id: I60e0d976774bd474676380583af24865e88471c4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/523976
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Arman Uguray <armansito@google.com>
A few tests were divided into a Runtime Effect-compatible .rts test, and
a Runtime Effect-incompatible .sksl test.
Change-Id: Ib52554892685bdc44fe3622ab314960ee0962b90
Bug: skia:13042
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/523377
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
In a few cases, this involved splitting a test into two (an ES2-
compatible portion and a ES3+ portion).
Change-Id: Ie6f18f787cf7c10696a2841ff538bbe2b95bf50d
Bug: skia:13042
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/523187
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: Arman Uguray <armansito@google.com>
The debugger is now more aware of scopeless blocks and treats them as
a combined statement--in particular, individual inner statements of the
scopeless Block are not counted as stopping points when stepping in the
debugger. Only the Block itself is used as a stopping point. This
improves stepping over multiple var-declarations in one statement.
Change-Id: Ic3ab4715cd0158109d8389ea0650b661d3a8b65e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/523185
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
We are going to cache (and uniquify) the UniformBlocks and TextureBlocks
separate from each other.
Bug: skia:12701
Change-Id: I03837c4a38a9bdeb4224a697eab119fca24e8f8c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/522916
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
We were unintentionally trying to run all GMs in 3 minutes
which sometimes took a bit longer. This uses the batch
mode that we use for unit tests too.
Change-Id: Icf9b415881d57772584a16423bdaff14d862c19d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/522936
Reviewed-by: Joe Gregorio <jcgregorio@google.com>
A few tests received minor tweaks to make them Runtime Effect-friendly.
Change-Id: I9b4f66b0974c41d38324dfbb31ac9849338f600a
Bug: skia:13042
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/523186
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
These are wrapped in an unscoped Block. Previously, we didn't assign any
position to the block, so it was implicitly given the position of its
enclosing statement.
Change-Id: Id320eb1db583acd6ae42deba2fbb0b61033c3936
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/522922
Reviewed-by: Arman Uguray <armansito@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
A few tests received minor tweaks to make them Runtime Effect-friendly.
Change-Id: Icbcedb84b7882e42f21425b2d40d7819705c359e
Bug: skia:13042
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/522918
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This is useful because it will allow these tests to be supported in
Android CTS, where only Runtime Effects are API-accessible.
This CL updates the C++ test harness so that .rts files in the error/
directory are found, and tweaks error tests as necessary to make them
Runtime Effect-compatible. For instance, Runtime Effects enforce the
parameters on main(), which adds extra errors that we don't want. And
some error tests require ES3 (e.g. array constructors) and so those
tests remain as .sksl files.
In this CL, only tests beginning with A are updated. The remaining tests
will be updated in followup CLs.
Change-Id: I70b064df4f0b3ed02d6bc8cc9add7ee844a78691
Bug: skia:13042
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/522424
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
`test_expect_fail` did a lot of fiddly string manipulation work in one
big monolithic function. Now it has helper functions
`get_expected_errors` and `check_expected_errors`. This hopefully makes
the logic a bit easier to understand.
There aren't any functional changes here, just restructuring.
Change-Id: I0961054b475255b6159b4dd05b98b6054b144d71
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/522422
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Ethan Nicholas <ethannicholas@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
PS1 regenerates the Bazel files. Use it as the base change when
comparing patchsets.
IWYU seems to do a good job of working with MyFile.cpp and
MyFile.h, but if there is just a MyHeader.h, it doesn't always
seem to throw errors if the includes aren't correct. This was
observed with include/sksl/DSL.h This might be due to the fact
that headers are not compiled on their own, so they are never
sent directly to the IWYU binary.
This change sets enforce_iwyu_on_package() on the all sksl
packages and then fixes the includes until all those checks
are happy. There were a few files that needed fixes outside
of the sksl folder. Examples include:
- src/gpu/effects/GrConvexPolyEffect.cpp
- tests/SkSLDSLTest.cpp
To really enforce this, we need to add a CI/CQ job that runs
bazel build //example:hello_world_gl --config=clang \
--sandbox_base=/dev/shm --features skia_enforce_iwyu
If that failed, a dev could make the changes described in
the logs and/or run the command locally to see those
prescribed fixes.
I had to add several entries to toolchain/IWYU_mapping.imp
in order to fix some private includes and other atypical
choices. I tried adding a rule there to allow inclusion of
SkTypes.h to make sure defines like SK_SUPPORT_GPU, but
could not get it to work for all cases, so I deferred to
using the IWYU pragma: keep (e.g. SkSLPipelineStageCodeGenerator.h)
Change-Id: I4c3e536d8e69ff7ff2d26fe61a525a6c2e80db06
Bug: skia:13052
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/522256
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
We are going to cache the uniform data and the texture data separately.
Begin by making the uniform data something that could be cached on its own.
Bug: skia:12701
Change-Id: If3ea3a9b6050faf0810549d4076ae44732656a9e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/522178
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
The error was being reported at the position of the var declaration,
rather than the position of the reference. And since the declaration
was in a module, its position was both incorrect (with respect to the
program source) and could be past the end.
Change-Id: I443b9fbbe016c43b93d457abfefd17025e451d8a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/521522
Reviewed-by: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Ethan Nicholas <ethannicholas@google.com>
By setting the compile time flag:
SK_EXPERIMENTAL_SIMULATE_DRAWGLYPHRUNLIST_WITH_SLUG_STRIKE_SERIALIZE
will cause all SkTextBlobs to be rendered by analyzing the Slug
to create strike cache differences and serialize the Blob to a Slug.
Then the serialized strike cache differences are used to populate
the strike cache using a different TypefaceIDs using SkTypeface_Remote
as a proxy for the real SkTypeface. This will create a hard break
between the original glyph data, and the proxied glyph data.
It will then deserialize the Slug doing TypefaceID translation to
the SkTypeface_Remote, and draw the unflattened Slug.
Bug: chromium:1278340
Change-Id: I0f1980dee966b1092a99741793aed9d138451f4d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/510228
Reviewed-by: Robert Phillips <robertphillips@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Herb Derby <herb@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>
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>
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>
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>
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>
