About 9x faster than Murmur3 for long inputs.
Most of this is a mechanical change from SkChecksum::Murmur3(...) to SkOpts::hash(...).
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2208903002
CQ_INCLUDE_TRYBOTS=master.client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot;master.client.skia.compile:Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot,Build-Mac-Clang-x86_64-Release-CMake-Trybot
Review-Url: https://codereview.chromium.org/2208903002
SkLiteRecorder, a new SkCanvas, fills out SkLiteDL, a new SkDrawable.
This SkDrawable is a display list similar to SkRecord and SkBigPicture / SkRecordedDrawable, but with a few new design points inspired by Android and slimming paint:
1) SkLiteDL is structured as one big contiguous array rather than the two layer structure of SkRecord. This trades away flexibility and large-op-count performance for better data locality for small to medium size pictures.
2) We keep a global freelist of SkLiteDLs, both reusing the SkLiteDL struct itself and its contiguous byte array. This keeps the expected number of mallocs per display list allocation <1 (really, ~0) for cyclical use cases.
These two together mean recording is faster. Measuring against the code we use at head, SkLiteRecorder trends about ~3x faster across various size pictures, matching speed at 0 draws and beating the special-case 1-draw pictures we have today. (I.e. we won't need those special case implementations anymore, because they're slower than this new generic code.) This new strategy records 10 drawRects() in about the same time the old strategy took for 2.
This strategy stays the winner until at least 500 drawRect()s on my laptop, where I stopped checking.
A simpler alternative to freelisting is also possible (but not implemented here), where we allow the client to manually reset() an SkLiteDL for reuse when its refcnt is 1. That's essentially what we're doing with the freelist, except tracking what's available for reuse globally instead of making the client do it.
This code is not fully capable yet, but most of the key design points are there. The internal structure of SkLiteDL is the area I expect to be most volatile (anything involving Op), but its interface and the whole of SkLiteRecorder ought to be just about done.
You can run nanobench --match picture_overhead as a demo. Everything it exercises is fully fleshed out, so what it tests is an apples-to-apples comparison as far as recording costs go. I have not yet compared playback performance.
It should be simple to wrap this into an SkPicture subclass if we want.
I won't start proposing we replace anything old with anything new quite yet until I have more ducks in a row, but this does look pretty promising (similar to the SkRecord over old SkPicture change a couple years ago) and I'd like to land, experiment, iterate, especially with an eye toward Android.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2213333002
Review-Url: https://codereview.chromium.org/2213333002
Reason for revert:
UBSAN says we're reading a bad bool here:
bool usesDistanceVectorField() const { return fUsesDistanceVectorField; }
../../../include/gpu/GrPaint.h:83:51: runtime error: load of value 239, which is not a valid value for type 'bool'
SUMMARY: AddressSanitizer: undefined-behavior ../../../include/gpu/GrPaint.h:83:51 in
Seems likely also the root of Valgrind failure:
https://luci-milo.appspot.com/swarming/task/30522e4f2241cb10
Original issue's description:
> GrFP can express distance vector field req., program builder declares variable for it
>
> This update allows fragment processors to require a field of vectors to the nearest edge. This requirement propagates:
>
> - from child FPs to their parent
> - from parent FPs to the GrPaint
> - from GrPaint through the PipelineBuilder into GrPipeline
> - acessed from GrPipeline by GrGLSLProgramBuilder
>
> GrGLSL generates a variable for the distance vector and passes it down to the GeometryProcessor->emitCode() method.
>
> This CL's base is the CL for adding the BevelNormalSource API: https://codereview.chromium.org/2080993002
>
> BUG=skia:
> GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2114993002
>
> Committed: https://skia.googlesource.com/skia/+/4ef6dfa7089c092c67b0d5ec34e89c1e319af196TBR=egdaniel@google.com,robertphillips@google.com,bsalomon@google.com,dvonbeck@google.com
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG=skia:
Review-Url: https://codereview.chromium.org/2201613002
This update allows fragment processors to require a field of vectors to the nearest edge. This requirement propagates:
- from child FPs to their parent
- from parent FPs to the GrPaint
- from GrPaint through the PipelineBuilder into GrPipeline
- acessed from GrPipeline by GrGLSLProgramBuilder
GrGLSL generates a variable for the distance vector and passes it down to the GeometryProcessor->emitCode() method.
This CL's base is the CL for adding the BevelNormalSource API: https://codereview.chromium.org/2080993002
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2114993002
Review-Url: https://codereview.chromium.org/2114993002
Reason for revert:
Monotonicity assert is failing on ARM. (Different rsqrt() and invert() precision?) Will investigate a bit tomorrow... might reland with the test TODO.
Original issue's description:
> Move sRGB <-> linear conversion components to their own files.
>
> This makes them a little easier to use outside SkColorXform code.
>
> I've added some notes about how best to use them and their eccentricities, and added a test.
>
> Ultimately any software sRGB <-> linear conversion should funnel somehow through here.
>
> BUG=skia:
> GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128893002
> CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
>
> Committed: https://skia.googlesource.com/skia/+/45e58c8807179638980aae8503573b950b844e4cTBR=reed@google.com,msarett@google.com,mtklein@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:
Review-Url: https://codereview.chromium.org/2131793002
This makes them a little easier to use outside SkColorXform code.
I've added some notes about how best to use them and their eccentricities, and added a test.
Ultimately any software sRGB <-> linear conversion should funnel somehow through here.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2128893002
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
Review-Url: https://codereview.chromium.org/2128893002
Reason for revert:
GMs are crashing on Windows Test trybots
Original issue's description:
> SkLightingShader normal vector CPU computation refactor.
>
> The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources.
>
> This CL's base was the CL for GPU handling: https://codereview.chromium.org/2043393002/
>
> What this CL includes:
>
> - A refactor of the SkLightingShader context's code that deals with reading normals off of a normal map. This is now abstracted out into a NormalSource::Provider class that the context uses.
>
> BUG=skia:
> GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2050773002
>
> Committed: https://skia.googlesource.com/skia/+/790a70118327a129cb6b48fabe80f4e184c1e67cTBR=egdaniel@google.com,reed@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:
Review-Url: https://codereview.chromium.org/2101653002
The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources.
This CL's base was the CL for GPU handling: https://codereview.chromium.org/2043393002/
What this CL includes:
- A refactor of the SkLightingShader context's code that deals with reading normals off of a normal map. This is now abstracted out into a NormalSource::Provider class that the context uses.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2050773002
Review-Url: https://codereview.chromium.org/2050773002
Reason for revert:
break deps roll
Original issue's description:
> Refactoring of GPU NormalMap handling out into its own class.
>
> The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources.
>
> What this CL includes:
>
> - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP.
>
> - Encapsulates this new fragment processor on a new class NormalMapSource.
>
> - Created a NormalSource abstraction that will interface with SkLightingShader.
>
> - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet.
>
> BUG=skia:
> GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002
>
> Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da
> Committed: https://skia.googlesource.com/skia/+/a7d1e2a57aef2aa4913d4380646d60bbab761318TBR=reed@google.com,dvonbeck@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:
Review-Url: https://codereview.chromium.org/2068983005
The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources.
What this CL includes:
- Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP.
- Encapsulates this new fragment processor on a new class NormalMapSource.
- Created a NormalSource abstraction that will interface with SkLightingShader.
- Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002
Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08da
Review-Url: https://codereview.chromium.org/2043393002
Reason for revert:
Breaking build and deps roll. Need to move include of SkBitmapProcShader in SkLightingShader.cpp from gpu include list to general list.
Original issue's description:
> Refactoring of GPU NormalMap handling out into its own class.
>
> The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources.
>
> What this CL includes:
>
> - Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP.
>
> - Encapsulates this new fragment processor on a new class NormalMapSource.
>
> - Created a NormalSource abstraction that will interface with SkLightingShader.
>
> - Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet.
>
> BUG=skia:
> GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002
>
> Committed: https://skia.googlesource.com/skia/+/87b0dd00cf9409c5fc990f5d0bb7c0df837f08daTBR=reed@google.com,dvonbeck@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:
Review-Url: https://codereview.chromium.org/2062133004
The purpose of this change is to refactor the handling of normal maps out of SkLightingShader, laying the groundwork to eventually allow for multiple normal sources.
What this CL includes:
- Created a new 'NormalMapFP', out of the existing normal map reading behavior in LightingFP.
- Encapsulates this new fragment processor on a new class NormalMapSource.
- Created a NormalSource abstraction that will interface with SkLightingShader.
- Adapted SkLightingShader to use the normals from its NormalSource field ON THE GPU SIDE. No changes done to the CPU side yet.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2043393002
Review-Url: https://codereview.chromium.org/2043393002
- remove dead code
- rewrite float -> int converters
The strategy for the new converters is:
- convert input to double
- floor/ceil/round in double space
- pin that double to [SK_MinS32, SK_MaxS32]
- truncate that double to int32_t
This simpler strategy does not work:
- floor/ceil/round in float space
- pin that float to [SK_MinS32, SK_MaxS32]
- truncate that float to int32_t
SK_MinS32 and SK_MaxS32 are not representable as floats:
they round to the nearest float, ±2^31, which makes the
pin insufficient for floats near SK_MinS32 (-2^31+1) or
SK_MaxS32 (+2^31-1).
float only has 24 bits of precision, and we need 31.
double can represent all integers up to 50-something bits.
An alternative is to pin in float to ±2147483520, the last
exactly representable float before SK_MaxS32 (127 too small).
Our tests test that we round as floor(x+0.5), which can
return different numbers than round(x) for negative x.
So this CL explicitly uses floor(x+0.5).
I've updated the tests with ±inf and ±NaN, and tried to
make them a little clearer, especially using SK_MinS32
instead of -SK_MaxS32.
I have not timed anything here. I have never seen any of these
methods in a profile.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2012333003
Review-Url: https://codereview.chromium.org/2012333003
It's always nice to kill off a synchronization primitive.
And while less terse, I think this new code reads more clearly.
... and, SkOncePtr's tests were the only thing now using sk_num_cores()
outside of SkTaskGroup, so I've hidden it as static inside SkTaskGroup.cpp.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1953533002
CQ_EXTRA_TRYBOTS=client.skia:Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-SKNX_NO_SIMD-Trybot
Review-Url: https://codereview.chromium.org/1953533002
Implemented as a different subclass for SkColorShader (which is also private) partly to make the CL clearer/simpler, and partly for flatten/unflatten compatibility. Later I'm sure we could combine these if that proves useful.
Lots of TODOs at the moment, but still valuable to get reviewed.
Note: this ignores the question (for the moment) about how to interpret SkColor in the larger world. That needs to happen, but this CL is more focused on what *else* to do besides handle the old-style input (and old-style pipeline).
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1934313002
Review-Url: https://codereview.chromium.org/1934313002
