Adding anisotropic mipmap levels to SkMipMap.

Adding anisotropic mipmap levels to SkMipMap.

Adding 1x2, 1x3, 2x1, 3x1 filters to SkMipMap and enabling SkMipMap to generate anisotropic mip levels.


BUG=590804
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1750303002

Committed: https://skia.googlesource.com/skia/+/5b9ad7620b36858f99fef0763d7fc04d024fd71d

[cblume, 2016-03-01 09:39:12]

Description was changed from

==========
Adding anisotropic mipmap levels to SkMipMap.

Adding 1x2, 1x3, 2x1, 3x1 filters to SkMipMap and enabling SkMipMap to generate
anisotropic mip levels.


BUG=590804
==========

to

==========
Adding anisotropic mipmap levels to SkMipMap.

Adding 1x2, 1x3, 2x1, 3x1 filters to SkMipMap and enabling SkMipMap to generate
anisotropic mip levels.


BUG=590804
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[cblume, 2016-03-01 09:40:24]

cblume@chromium.org changed reviewers:
+ bsalomon@google.com, mtklein@chromium.org

[cblume, 2016-03-01 09:40:46]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 09:40:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1750303002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1750303002/1

[commit-bot: I haz the power, 2016-03-01 09:40:55]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2016-03-01 15:40 UTC

[commit-bot: I haz the power, 2016-03-01 09:42:04]

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

[commit-bot: I haz the power, 2016-03-01 09:42:05]

Dry run: Try jobs failed on following builders:
  Build-Mac-Clang-Arm7-Debug-iOS-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-Arm7...)
  Build-Mac-Clang-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Mac-Clang-x86_...)

[cblume, 2016-03-01 09:47:07]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 09:47:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1750303002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1750303002/20001

[commit-bot: I haz the power, 2016-03-01 09:52:28]

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

[commit-bot: I haz the power, 2016-03-01 09:52:29]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[cblume, 2016-03-01 10:17:58]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 10:18:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1750303002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1750303002/40001

[mtklein, 2016-03-01 13:40:48]

mtklein@google.com changed reviewers:
+ mtklein@google.com

[mtklein, 2016-03-01 13:40:49]

https://codereview.chromium.org/1750303002/diff/40001/src/core/SkMipMap.cpp
File src/core/SkMipMap.cpp (left):

https://codereview.chromium.org/1750303002/diff/40001/src/core/SkMipMap.cpp#o...
src/core/SkMipMap.cpp:105: template <typename F> void downsample_3_2(void* dst,
const void* src, size_t srcRB, int count) {
As far as I can tell, the old functions follow the convention of downsample_x_y,
where x is the number of pixels taken from a given row horizontally, and y the
number of rows averaged vertically.  E.g.

_2_2:  a b
       c d 

_2_3:  a b
       c d
       e f

_3_2:  a b c
       d e f

I think you have adopted the opposite convention for these new filters:

_1_3: a b c

_2_1: a
      b

This seems at best confusing, probably buggy?

[commit-bot: I haz the power, 2016-03-01 15:40:01]

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

[commit-bot: I haz the power, 2016-03-01 15:40:02]

Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer

[cblume, 2016-03-01 19:28:36]

https://codereview.chromium.org/1750303002/diff/40001/src/core/SkMipMap.cpp
File src/core/SkMipMap.cpp (left):

https://codereview.chromium.org/1750303002/diff/40001/src/core/SkMipMap.cpp#o...
src/core/SkMipMap.cpp:105: template <typename F> void downsample_3_2(void* dst,
const void* src, size_t srcRB, int count) {
On 2016/03/01 13:40:49, mtklein wrote:
> As far as I can tell, the old functions follow the convention of
downsample_x_y,
> where x is the number of pixels taken from a given row horizontally, and y the
> number of rows averaged vertically.  E.g.
> 
> _2_2:  a b
>        c d 
> 
> _2_3:  a b
>        c d
>        e f
> 
> _3_2:  a b c
>        d e f
> 
> I think you have adopted the opposite convention for these new filters:
> 
> _1_3: a b c
> 
> _2_1: a
>       b
> 
> This seems at best confusing, probably buggy?

You are right.
This should be fixed now (plus adding some extra SkASSERT(count > 0); calls).

[cblume, 2016-03-01 19:28:44]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 19:28:55]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1750303002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1750303002/60001

[commit-bot: I haz the power, 2016-03-01 19:28:56]

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

[commit-bot: I haz the power, 2016-03-01 19:28:57]

Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer

[mtklein, 2016-03-01 19:35:35]

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

[mtklein, 2016-03-01 19:35:35]

+reed

[reed1, 2016-03-01 20:17:46]

https://codereview.chromium.org/1750303002/diff/60001/src/core/SkMipMap.cpp
File src/core/SkMipMap.cpp (right):

https://codereview.chromium.org/1750303002/diff/60001/src/core/SkMipMap.cpp#n...
src/core/SkMipMap.cpp:143: SkASSERT(count > 0);
why remove this assert?

https://codereview.chromium.org/1750303002/diff/60001/src/core/SkMipMap.cpp#n...
src/core/SkMipMap.cpp:225: SkASSERT(count > 0);
curious why we remove this assert?

https://codereview.chromium.org/1750303002/diff/60001/src/core/SkMipMap.cpp#n...
src/core/SkMipMap.cpp:340: width = SkTMax(1, width);
nit: width = SkTMax(1, width >> 1) ?

https://codereview.chromium.org/1750303002/diff/60001/src/core/SkMipMap.cpp#n...
src/core/SkMipMap.cpp:413: width = SkTMax(1, width);
same nit: width = SkTMax(1, width >> 1);

[cblume, 2016-03-01 20:39:29]

https://codereview.chromium.org/1750303002/diff/60001/src/core/SkMipMap.cpp
File src/core/SkMipMap.cpp (right):

https://codereview.chromium.org/1750303002/diff/60001/src/core/SkMipMap.cpp#n...
src/core/SkMipMap.cpp:143: SkASSERT(count > 0);
On 2016/03/01 20:17:46, reed1 wrote:
> why remove this assert?

I'm adding it.
The existing code has the assert sometimes but not all the time:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/skia/s...

https://codereview.chromium.org/1750303002/diff/60001/src/core/SkMipMap.cpp#n...
src/core/SkMipMap.cpp:340: width = SkTMax(1, width);
On 2016/03/01 20:17:46, reed1 wrote:
> nit: width = SkTMax(1, width >> 1) ?

Done.

https://codereview.chromium.org/1750303002/diff/60001/src/core/SkMipMap.cpp#n...
src/core/SkMipMap.cpp:413: width = SkTMax(1, width);
On 2016/03/01 20:17:46, reed1 wrote:
> same nit: width = SkTMax(1, width >> 1);

Done.

[cblume, 2016-03-01 20:40:48]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 20:40:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1750303002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1750303002/80001

[commit-bot: I haz the power, 2016-03-01 20:40:52]

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

[commit-bot: I haz the power, 2016-03-01 20:40:54]

Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer

[cblume, 2016-03-01 20:56:00]

The CQ bit was checked by cblume@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 20:56:03]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1750303002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1750303002/80001

[commit-bot: I haz the power, 2016-03-01 20:56:05]

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

[commit-bot: I haz the power, 2016-03-01 20:56:06]

Dry run: No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer

[reed1, 2016-03-01 20:56:42]

lgtm

[cblume, 2016-03-01 21:39:39]

The CQ bit was checked by cblume@chromium.org

[commit-bot: I haz the power, 2016-03-01 21:39:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1750303002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1750303002/80001

[commit-bot: I haz the power, 2016-03-01 21:54:32]

Description was changed from

==========
Adding anisotropic mipmap levels to SkMipMap.

Adding 1x2, 1x3, 2x1, 3x1 filters to SkMipMap and enabling SkMipMap to generate
anisotropic mip levels.


BUG=590804
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Adding anisotropic mipmap levels to SkMipMap.

Adding 1x2, 1x3, 2x1, 3x1 filters to SkMipMap and enabling SkMipMap to generate
anisotropic mip levels.


BUG=590804
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/5b9ad7620b36858f99fef0763d7fc04d024fd71d
==========

[commit-bot: I haz the power, 2016-03-01 21:54:33]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/5b9ad7620b36858f99fef0763d7fc04d024fd71d