Refactoring of CPU NormalMap handling out into its own class

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/+/790a70118327a129cb6b48fabe80f4e184c1e67c
Committed: https://skia.googlesource.com/skia/+/12c4fc2579017a162668db077e8067512fd968ca

[dvonbeck, 2016-06-08 17:38:55]

Description was changed from

==========
LightingShader normal vector CPU computation refactor. DOES NOT RUN.

DO NOT LAND. CPU RENDERING OF LIGHTING SHADER SEGFAULTS.

BUG=skia:
==========

to

==========
LightingShader normal vector CPU computation refactor. DOES NOT RUN.

DO NOT LAND. CPU RENDERING OF LIGHTING SHADER SEGFAULTS.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2050773002
==========

[dvonbeck, 2016-06-08 17:44:08]

Description was changed from

==========
LightingShader normal vector CPU computation refactor. DOES NOT RUN.

DO NOT LAND. CPU RENDERING OF LIGHTING SHADER SEGFAULTS.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2050773002
==========

to

==========
SkLightingShader normal vector CPU computation refactor. DOES NOT RUN.

DO NOT LAND. CPU RENDERING OF LIGHTING SHADER SEGFAULTS. Implementation's
structure is clunky, lacking comments, and does not comply with style guide.

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 is 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
==========

[dvonbeck, 2016-06-08 17:44:08]

dvonbeck@google.com changed reviewers:
+ egdaniel@google.com

[dvonbeck, 2016-06-08 19:44:06]

Description was changed from

==========
SkLightingShader normal vector CPU computation refactor. DOES NOT RUN.

DO NOT LAND. CPU RENDERING OF LIGHTING SHADER SEGFAULTS. Implementation's
structure is clunky, lacking comments, and does not comply with style guide.

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 is 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
==========

to

==========
SkLightingShader normal vector CPU computation refactor.

DO NOT LAND. Implementation's structure is clunky, lacking comments, and does
not comply with style guide. Execution should be unaffected though.

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 is 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
==========

[dvonbeck, 2016-06-13 16:52:33]

Description was changed from

==========
SkLightingShader normal vector CPU computation refactor.

DO NOT LAND. Implementation's structure is clunky, lacking comments, and does
not comply with style guide. Execution should be unaffected though.

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 is 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
==========

to

==========
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 is 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
==========

[egdaniel, 2016-06-16 13:48:10]

+Mike to check all the needed scan line stuff is added correctly

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
File src/core/SkLightingShader.cpp (right):

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
src/core/SkLightingShader.cpp:443: size_t n = count;
why the change to size_t here? Quick scan seems to be using it as int it most
cases like in normalProvider->fillScanLine(). This may cause a compiler to give
a warning about loss of data going from size_t to int.

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
File src/core/SkLightingShader.h (right):

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
src/core/SkLightingShader.h:41: virtual ~Provider(){};
nit: space between () and {}

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
File src/core/SkLightingShader_NormalSource.cpp (right):

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
src/core/SkLightingShader_NormalSource.cpp:67: const NormalMapSourceImpl&
fSource;
Generally we store pointers here if you just need to this as a const pointer
back to its owner (NormalMapSourceImpl)

[egdaniel, 2016-06-16 13:48:34]

egdaniel@google.com changed reviewers:
+ reed@google.com

[egdaniel, 2016-06-16 13:48:34]

actually adding reed this time

[dvonbeck, 2016-06-16 21:51:53]

Description was changed from

==========
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 is 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
==========

to

==========
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
==========

[dvonbeck, 2016-06-16 22:05:40]

I just uploaded a BitmapShader-backed NormalMapSource. It still has deeply
nested classes, but there's less weird coupling going on.

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
File src/core/SkLightingShader.cpp (right):

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
src/core/SkLightingShader.cpp:443: size_t n = count;
On 2016/06/16 13:48:10, egdaniel wrote:
> why the change to size_t here? Quick scan seems to be using it as int it most
> cases like in normalProvider->fillScanLine(). This may cause a compiler to
give
> a warning about loss of data going from size_t to int.

I was avoiding a compiler error "comparison between unsigned and signed", since
BUFFER_MAX was unsigned. I circumvented it some other way, so it is back to
being an int.

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
File src/core/SkLightingShader.h (right):

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
src/core/SkLightingShader.h:41: virtual ~Provider(){};
On 2016/06/16 13:48:10, egdaniel wrote:
> nit: space between () and {}

Done.

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
File src/core/SkLightingShader_NormalSource.cpp (right):

https://codereview.chromium.org/2050773002/diff/100001/src/core/SkLightingSha...
src/core/SkLightingShader_NormalSource.cpp:67: const NormalMapSourceImpl&
fSource;
On 2016/06/16 13:48:10, egdaniel wrote:
> Generally we store pointers here if you just need to this as a const pointer
> back to its owner (NormalMapSourceImpl)

I did this because it's what LightingShader context does. Should I change this?
Should I change LightingShaderContext?

[dvonbeck, 2016-06-20 14:23:20]

I refactored NormalSource out into it's own header file. There's no 3 levels of
scope now, and it's ready for review again.

[reed1, 2016-06-20 15:00:39]

https://codereview.chromium.org/2050773002/diff/160001/src/core/SkLightingSha...
File src/core/SkLightingShader.h (left):

https://codereview.chromium.org/2050773002/diff/160001/src/core/SkLightingSha...
src/core/SkLightingShader.h:59: static sk_sp<NormalSource> MakeMap(const
SkBitmap& normal, const SkVector& invNormRotation,
0. Should we have a variant that takes SkImage? SkPixmap?
1. MakeFromBitmap ?
2. More dox on normal rotation... 
- does that vector have to be normalized (i.e. length == 1)?
- why is 'inv' in the name?
- what is a default value? (... 1,0 ?)
3. Is normLocalM forward (my guess) or inverse?
- can I pass nullptr?

[dvonbeck, 2016-06-20 20:03:58]

https://codereview.chromium.org/2050773002/diff/160001/src/core/SkLightingSha...
File src/core/SkLightingShader.h (left):

https://codereview.chromium.org/2050773002/diff/160001/src/core/SkLightingSha...
src/core/SkLightingShader.h:59: static sk_sp<NormalSource> MakeMap(const
SkBitmap& normal, const SkVector& invNormRotation,
On 2016/06/20 15:00:39, reed1 wrote:
> 0. Should we have a variant that takes SkImage? SkPixmap?
> 1. MakeFromBitmap ?
> 2. More dox on normal rotation... 
> - does that vector have to be normalized (i.e. length == 1)?
> - why is 'inv' in the name?
> - what is a default value? (... 1,0 ?)
> 3. Is normLocalM forward (my guess) or inverse?
> - can I pass nullptr?

0. I could make it take in a shader instead of a bitmap, and make it
"format-agnostic". I think that sounds like the best option, instead of having
different factories for each format.
1. I called MakeMap from NormalMap, not Bitmap. If I end up writing the API I
described  just now, I'll call it MakeFromNormalMap and take in a Shader
instead.
2. The vector does have to be normalized. Greg and I will talk to Rob tomorrow
and discuss what the best approach for this specific parameter is once he gets
back on Wednesday, since he wrote that part. Inv is supposed to stand for
inverted. I'll give it a default value that results in no rotation.
3. It is a transformation from texture coordinates to the shape's source space.
I assume this is forward? It is the same format as the local matrices for the
bitmap shader. Nullptr is accepted, gets interpreted as an identity matrix. I
will add this details to the comments.

[dvonbeck, 2016-06-24 15:25:41]

Review comments were addressed.

[egdaniel, 2016-06-27 13:47:16]

This looks fine to me, Mike is there anything else you want to change/add here?

[reed1, 2016-06-27 14:28:20]

lgtm

https://codereview.chromium.org/2050773002/diff/220001/src/core/SkNormalSource.h
File src/core/SkNormalSource.h (right):

https://codereview.chromium.org/2050773002/diff/220001/src/core/SkNormalSourc...
src/core/SkNormalSource.h:18: virtual ~SkNormalSource();
nit: ~SkNormalSource() override;

[dvonbeck, 2016-06-27 15:18:09]

https://codereview.chromium.org/2050773002/diff/220001/src/core/SkNormalSource.h
File src/core/SkNormalSource.h (right):

https://codereview.chromium.org/2050773002/diff/220001/src/core/SkNormalSourc...
src/core/SkNormalSource.h:18: virtual ~SkNormalSource();
On 2016/06/27 14:28:20, reed1 wrote:
> nit: ~SkNormalSource() override;

Done.

[dvonbeck, 2016-06-27 15:19:08]

The CQ bit was checked by dvonbeck@google.com

[dvonbeck, 2016-06-27 15:19:09]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/2050773002/#ps240001
(title: "Added override keyword to virtual overriding destructors")

[commit-bot: I haz the power, 2016-06-27 15:19:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 15:44:01]

Description was changed from

==========
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
==========

to

==========
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/+/790a70118327a129cb6b48fabe80f4e184c1e67c
==========

[commit-bot: I haz the power, 2016-06-27 15:44:02]

Committed patchset #13 (id:240001) as
https://skia.googlesource.com/skia/+/790a70118327a129cb6b48fabe80f4e184c1e67c

[dvonbeck, 2016-06-27 16:30:10]

A revert of this CL (patchset #13 id:240001) has been created in
https://codereview.chromium.org/2101653002/ by dvonbeck@google.com.

The reason for reverting is: GMs are crashing on Windows Test trybots.

[dvonbeck, 2016-06-27 16:31:20]

Description was changed from

==========
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/+/790a70118327a129cb6b48fabe80f4e184c1e67c
==========

to

==========
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/+/790a70118327a129cb6b48fabe80f4e184c1e67c
==========

[dvonbeck, 2016-06-27 17:36:04]

The CQ bit was checked by dvonbeck@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-27 17:36:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 18:08:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-27 18:08:23]

Dry run: This issue passed the CQ dry run.

[dvonbeck, 2016-06-27 18:39:45]

The CQ bit was checked by dvonbeck@google.com

[dvonbeck, 2016-06-27 18:39:45]

The patchset sent to the CQ was uploaded after l-g-t-m from reed@google.com
Link to the patchset: https://codereview.chromium.org/2050773002/#ps260001
(title: "SkLS Context dependencies now allocated on the heap")

[commit-bot: I haz the power, 2016-06-27 18:39:49]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-27 18:40:48]

Description was changed from

==========
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/+/790a70118327a129cb6b48fabe80f4e184c1e67c
==========

to

==========
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/+/790a70118327a129cb6b48fabe80f4e184c1e67c
Committed:
https://skia.googlesource.com/skia/+/12c4fc2579017a162668db077e8067512fd968ca
==========

[commit-bot: I haz the power, 2016-06-27 18:40:51]

Committed patchset #14 (id:260001) as
https://skia.googlesource.com/skia/+/12c4fc2579017a162668db077e8067512fd968ca