I keep bumping into the need for this
Change-Id: I69384f7d590c163fd6244bdc64cc5e48450fecd4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/540171
Commit-Queue: Robert Phillips <robertphillips@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This doesn't improve the quality of final code, so we can skip this pass
during normal program compilation, but there's no excuse for leaving
Nops in a dehydrated module that we always load into memory at startup.
Change-Id: I7ff94f2713cff658799c469a46b526544dfc318c
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/540040
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
This describes the capabilities of a particular Skia rendering context
(GPU context, or the CPU backend). At the moment, it only contains the
supported SkSL version (with a new enum added to specify the current
value as "100" and a new ES3 value as "300".
SkCapabilities can not be retrieved from an SkCanvas - the client must
have a concrete way of knowing what their destination device that will
do the actual rendering is (GrCaps or SkSurface).
This CL doesn't make use of the SkCapabilities yet, that's coming in
follow-up CLs that alter the SkSL compiler and SkRuntimeEffect API.
Bug: skia:11209
Change-Id: I4e9fd21ff7ffd79f1926c5c2eb34e10b3af4bc9b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/537876
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
This makes expressions with `initialPreLocal` eligible for constant
folding. In practice, this doesn't do much today because matrix constant
folding doesn't support no-op-arithmetic elimination, but in a followup,
we could easily simplify `initialPreLocal * foo` --> `foo` when it's the
identity matrix.
Change-Id: I4c6faf2aa550b6b9776dd8a5da9ea5ec06cd8330
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/540316
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Robert Phillips <robertphillips@google.com>
Moving it into the helper class serves a few purposes:
1. Consolidate the more complicated stroke vertex count calculations so
that they can be reused between graphite and ganesh more easily.
2. Gives us an object that the PatchWriter can pass to its
PatchAllocators that is itself not a template, so the allocators can
be easily reused across different PatchWriter configurations.
3. Gives us a good place to start experimenting with tighter tolerance
tracking (particularly around internal rotation and join rotation).
Bug: skia:13056, skia:13012
Change-Id: I6086d459a21ac7ab8e833a988cc7c403983c3dd2
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/537083
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
This effectively rolls back http://review.skia.org/538045.
Change-Id: I9c457a6c9c9ba98d4c2e3a6af03c5dd80c76beb9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/540082
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
At present this has no effect. There isn't any dead path in the built-in
code (as you would hope) and we don't do any sufficiently-interesting
transformations that might cause unreachable code to appear.
Change-Id: I6798ae21f8721cf9f6f61ecb1765c8d8df7a9d0d
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/540039
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Change-Id: Iafd9cf15ac3c42bda7fa6b6857a82dc2eae6d234
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/540296
Reviewed-by: Heather Miller <hcm@google.com>
Commit-Queue: Eric Boren <borenet@google.com>
Commit-Queue: Heather Miller <hcm@google.com>
This should address the root cause of http://review.skia.org/538045.
We currently don't eliminate unreferenced names from the symbol table,
although that'd be free money if we wanted to do it.
Change-Id: I712e23e6a4f3cb48c2b5ae68f042e13031d5f632
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/540038
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
In b/224677119, we found that turning on *all* of Skia tracing caused
regressions. We are separately turning it off, but we do find some value
in select tracepoints. Add back in the ones we care about. The remaining
tracepoints can still be turned on manually with the sysprop.
All of these tracepoints are already traced on other platforms (e.g.
Chrome, Flutter). Convert them to TRACE_EVENT0_ALWAYS, introduced in
I13fd77445e0d2a05e46b75629b37bab5f571b02e. (A better long term solution
will be to use categories, as tracked in b/232405757.)
For GrOps, we had an Android-specific tracepoint, added in
I18ac03676da058ba4af5bd6a0c375b3f17c3c399, as well as separate
TRACE_EVENT0 tracepoints with basically the same timing and label.
Remove the Android-specific version, and make the other one
TRACE_EVENT0_ALWAYS. This removes redundant child-parent pairs.
Bug: b/231627558
Bug: b/224677119
Bug: b/232226246
Test: perfetto
Change-Id: I03ce62062a0bde723d3b6ac026bd978b249ad9a4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/540156
Commit-Queue: Leon Scroggins <scroggo@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
This is largely a re-implementation of the logic in
src/gpu/ganesh/gradients/GrGradientShader.cpp and
src/shaders/gradients/SkTwoPointConicalGradient.cpp. For simplicity,
most everything is in the shader now, but with an eye towards moving
computations that don't use sk_FragCoord into SkKeyHelpers.cpp.
Bug: skia:13302
Change-Id: I73d2e1f6c6ac0580aa897ffff3cfa87f4ed62b11
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539569
Commit-Queue: James Godfrey-Kittle <jamesgk@google.com>
Reviewed-by: John Stiles <johnstiles@google.com>
Constructor::Convert is a very heavy hammer which does error-checking
and lots of logic to identify the right constructor type to use. It cuts
out a lot of redundant work to call the desired method
`ConstructorCompound::Make` directly instead.
Change-Id: Icf9e513dbce223c2b27ccd0d11dc08eea849ff39
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539821
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Uses the initializer_list constructor to avoid generating a bunch of
insert calls, and then moves everything right into layout().
Change-Id: I09f5538e20ea6f7d3b6117020302a41e18b66295
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539822
Reviewed-by: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Colors are premultiplied, so the specular component (which is additive)
must be modulated by alpha explicitly.
Change-Id: If5f7b456b0ce518b1806e9b1e754229c3a6d0981
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539559
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Florin Malita <fmalita@google.com>
Reviewed-by: Jorge Betancourt <jmbetancourt@google.com>
Change-Id: Iac6b8a0b74fe23959f5ee814f4a30a5458ccf21e
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539936
Reviewed-by: John Stiles <johnstiles@google.com>
Auto-Submit: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
We can access shader caps bits directly from SkSL, using sk_Caps. We
don't need to pass in caps flags from fGpu->caps()->shaderCaps().
Also, you might get different caps bits depending on which
backend/device you are using, so it's a little risky to bake it into
a shared cache; SkSL can apply the right caps bits at compile time.
Change-Id: I896b3915f40fbe7f58dfe5ffb51a462b3d2096ea
Bug: skia:13302
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539572
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>
Reviewed-by: James Godfrey-Kittle <jamesgk@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
ATRACE_ANDROID_FRAMEWORK_ALWAYS and TRACE_EVENT0_ALWAYS create a
SkAndroidFrameworkTraceUtilAlways, which formats the input, requiring a
string copy. Mimic the split in SkAndroidFrameworkTraceUtil, which has
two constructors; one for formatting, and one for static strings. This
allows skipping the copy when it's unnecessary.
Make TRACE_EVENT0_ALWAYS call the cheaper constructor, since it never
has formatted input. ATRACE_ANDROID_FRAMEWORK_ALWAYS has few callers;
only one of them forces an unnecessary copy.
Bug: b/224677119
Test: perfetto
Change-Id: I8e699ae1496c94e08e6f7fce3616254b1a627a7f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539896
Commit-Queue: Leon Scroggins <scroggo@google.com>
Reviewed-by: Herb Derby <herb@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
ConstructorCompound::Make() will now turn `float2(myFloat2)` into the
equivalent simpler form `myFloat2` by simply returning the inner
argument as-is. In practice, this codepath is difficult to reach because
`Constructor::Convert` will interpret the above constructor as a cast,
and ConstructorCompoundCast already had a similar optimization in place
(to eliminate same-type casts).
This CL seems large but it's mostly comments. (`is_safe_to_eliminate`
could easily be a single-line return statement if the goal were to
minimize LOC!)
Change-Id: If75d1583e5fe27f7fd903175ebbd9185c67026f4
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539820
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Previously, each Expression would clone itself at its existing position
and then the position would potentially be overwritten at the end. Now,
rewriting the position happens as part of the clone operation.
This seemed simpler than the previous two-step operation, and also opens
up the possibility of passing the updated position to the clone's
children (but I didn't experiment with that here.)
Change-Id: I07d0c0221fcab460d0db358d059ceecf8a0a7dc7
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539571
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
`optimize_constructor_swizzle` already does the same transformation, but
in a far more complex way. In a followup CL, I intend to limit
`optimize_constructor_swizzle` to only operating on ConstructorCompound
objects.
Change-Id: Ieb4210a9ea9efb1a021c4b2e15ba55863000ef93
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539819
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
We added a `clone(Position pos)` overload late; this CL goes back and
uses it in all the places that needed it.
Change-Id: I6924ac9a91ebbd148531dccd25218d6f497ed0d6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539568
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
In cases where we know the type of constructor being made, it is much
more efficient to create it directly, instead of relying on
Constructor::Convert to deduce the type. This also helps us catch
legitimate bugs that Constructor::Convert will paper over; e.g. Convert
will happily accept mismatched types and just cast them (which was
occurring here, due to calling `scalarTypeForLiteral` in the wrong
spot).
Change-Id: I972046e14cc78708a3f1b8a4a31016d91e60a431
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539508
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
In follow up changes, the PatchAllocator duck type will get a little
more involved with tessellation-specific code, so I figured it'd be good
to buffer it with this new type between GrVertexChunkBuilder (which can
be used more generally if we update ops to do so).
Bug: skia:13012
Change-Id: Ib51d7beceba98cb10678dd04b431e923518ed6d8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539037
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
OpenGL docs specifically insist that the sequence (comma) operator
should not be treated as a constant-expression so that attempts to
declare multidimensional arrays with a comma will fail:
http://screen/vJEpAe9yNmbzZTm
(See "12.43 Sequence operator and constant expressions" in the OpenGL
ES3 documentation or read skia:13311 for details.)
In practice, we don't get much benefit from optimizing away unused
comma-expressions; it improves some synthetic tests, but realistically
this will not help Skia in any real-world scenario. The constant folder
no longer attempts this optimization, and comma-expressions are now
rejected in a constant-expression context.
Change-Id: Ic5dea6ff90e36614b548c1ce89a444e81da944ae
Bug: skia:13311
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539565
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Previously, this slide used a bunch of code to render a gray ramp. My
first impression was that the test slide was broken, but this was the
actual intent. We now use the shaders.skia.org zoomy-neurons.
Change-Id: Icd08462e30ab328b533525d74fa6064bccb76e8a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539202
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Most shader text displayed in SkSL mode is passed through, exactly as
written--comments and all--and is unaffected by the optimization
setting. However, runtime effects go through the pipeline stage which
can subject them to optimizations. This is unexpected in "SkSL" mode,
where the goal is to display the shader as close to the original input
as possible.
Now, when Viewer displays a runtime effect in "SkSL" mode, it will not
have optimizations applied (other than some basic stuff like constant
folding, which is currently non-negotiable in SkSL).
Change-Id: I9bd3d8f7067d7124b3fbe88e8a422212b645597a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539200
Reviewed-by: Arman Uguray <armansito@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
This includes using SkNoPixelsDevice for the GlyphTrackingDevice
in the SkChromeRemoteGlyphCache because it is the only thing that
uses SkGlyphRunPainter outside of GPU.
Change-Id: Id22e538b2c1aa0d65433878e428c9465fe222d12
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539558
Reviewed-by: Robert Phillips <robertphillips@google.com>
Commit-Queue: Herb Derby <herb@google.com>
GLSL/SkSL allow creation of a uniform scale matrix by constructing a
matrix with a single scalar argument. It will be splatted across the
matrix's diagonal.
Change-Id: If3410909f8136f3f4c39f7d7208d5bd6066e4e53
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539047
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
Looks like really old drivers struggled with weird no-op blocks.
Change-Id: Ie32754a9c221eb7c20924ee27e5facca7e9701f0
Bug: skia:13309
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539561
Reviewed-by: Brian Osman <brianosman@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
The conceptual meat of this change is that instead of taking a local-to-
device scale factor and the local stroke width, CalcNumRadialSegments
now takes the approximated device-space stroke radius. The main benefit
of this is that hairlines can be defined exactly without needing to
invert the local-to-device matrix or otherwise estimate a local-space
radius that would transform to the 1px hairline. The actual math for
regular strokes remains the same (scale factor * radius), but is moved.
In addition, this CL fixes a bug I had introduced in
https://skia-review.googlesource.com/c/skia/+/502059/
where I had not realized the CPU code operated on stroke *width* and the
GPU code operated on stroke *radius*, so updating them to use the same
equations w/o changing their inputs actually caused us to overestimate
the number of radial segments required. With this change, both the CPU
code and GPU code operate on stroke radius.
Between that bug fix and simplifying the hairline stroke radius math,
I am expecting lots of slight AA changes around stroked paths.
Lastly, I entirely removed the StrokeTolerancesBuffer, which was a class
that buffered up calculations of CalcNumRadialSegments by looking at the
entire list of paths in an op and putting them into a 4-lane SIMD
function instead. This isn't currently available for Graphite and makes
it harder to consolidate the radial segment calculations. Locally I
found no noticeable difference in the patch writing microbenchmarks
(where it would be more significant), and somehow the motionmark SKPs
were actually a touch faster. I'm hopeful that we can drop this
complexity and not take a large performance hit.
Bug: skia:13056, skia:13012
Change-Id: If16103078ebe9e4e60470445c0868deb3904f7de
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/537239
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
The fuzzer found a subtle bug where our line vs. rect clipping could
produce points outside of the rect. Thanks to floating poing inaccuracy,
the first (horizontal) clip would produce a point just outside the
bounds. Then, we'd re-clip against the vertical segments - but go back
and start from the original points. Again, thanks to floating point,
we'd produce a point just outside the bounds.
The fix here is straightforward: After we've done the first clip
(reducing the magnitude of the values involved, and constraining the X
coordinates), use *that* as the new endpoints for the second clip.
Mathematically this is identical (the tmp values are new upper bounds on
the extents of the final endpoints after the first clip), but the math
is more stable this way.
Bug: chromium:1320467
Change-Id: I9c816110edb2944544243f2ad89665d5bc5c9c53
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539196
Reviewed-by: Michael Ludwig <michaelludwig@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>
Change-Id: I2a1fedbbaabd57e4546c2a628f6d297e11f4244b
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539318
Reviewed-by: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
Commit-Queue: Michael Ludwig <michaelludwig@google.com>
This CL define a const label string in GrDrawOpAtlas and have the
constructor accept it. This will help us to label atlas.
Bug: chromium:1164111
Change-Id: I6e194b9a59b940b359887736df27c1da130b1d8a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539136
Reviewed-by: Greg Daniel <egdaniel@google.com>
Commit-Queue: Greg Daniel <egdaniel@google.com>
This is just a cascade of changes from removing the (unused) 'flushTimeOpsTask' parameter from SurfaceDrawContext::Make.
Change-Id: Ia92dfada8df007761d9de3c16e6a9c8850c3d399
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539066
Commit-Queue: Robert Phillips <robertphillips@google.com>
Reviewed-by: Greg Daniel <egdaniel@google.com>
This made sense when literals were spread across multiple classes
(FloatLiteral/IntLiteral/BoolLiteral). Nowadays we can just call
Literal::Make and pass the Type that we want our literal to be.
Change-Id: I54ce5fcf7bcadac4be1fe10a358b9c86c234de8f
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539397
Auto-Submit: John Stiles <johnstiles@google.com>
Commit-Queue: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
We will continue to fold expressions like `10 + 0` or `5 * 1` or
`PI * 1` (assuming PI is a const float) because both sides are known,
but non-constant-expressions like `x + 0` (assuming x is not a constant)
or `foo *= 1` will be left as-is when the optimizer is off.
This improves the accuracy of the Viewer shader tab, because a runtime
effect expression like `color *= scale` will be preserved in the final
output when optimization is off, instead of being replaced with a Nop
when scale equals one.
Change-Id: I218b327cb0cd12654dca446dee8a5baa96f589b8
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/539197
Commit-Queue: John Stiles <johnstiles@google.com>
Auto-Submit: John Stiles <johnstiles@google.com>
Reviewed-by: Brian Osman <brianosman@google.com>
