respect srgb gamma when building mips

respect srgb gamma when building mips

Proposed policy:
- If the target is *legacy* (e.g. L32/PMColor) ignore gamma
- If the target is S32/F16 respect gamma

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

Committed: https://skia.googlesource.com/skia/+/6644d9353f3f0c09914385fd762e073f98d54205

[reed1, 2016-06-02 19:03:45]

Description was changed from

==========
respect srgb gamma when building mips

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

to

==========
respect srgb gamma when building mips

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

[reed1, 2016-06-02 21:19:50]

reed@google.com changed reviewers:
+ brianosman@google.com, herb@google.com

[reed1, 2016-06-02 21:19:51]

I'm pretty sure I got the mode wrong in the gpu case ...

[reed1, 2016-06-02 21:20:27]

Description was changed from

==========
respect srgb gamma when building mips

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

to

==========
respect srgb gamma when building mips

Deduce (from the render-target-type) if we should respect the src-image's gamma
or not when building the miptree.

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

[reed1, 2016-06-02 21:21:33]

TODO: need to report the "honest" ImageInfo/gamma in each layer (reflecting if
we were asked to ignore/respect the src gamma).

[reed1, 2016-06-02 21:21:36]

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

[commit-bot: I haz the power, 2016-06-02 21:21:46]

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

[commit-bot: I haz the power, 2016-06-02 21:40:10]

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

[commit-bot: I haz the power, 2016-06-02 21:40:11]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-06-03 15:55:15]

Need help with the change to SkGr.cpp

[reed1, 2016-06-03 15:55:29]

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

[reed1, 2016-06-03 15:55:37]

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

[commit-bot: I haz the power, 2016-06-03 15:55:41]

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

[commit-bot: I haz the power, 2016-06-03 16:14:20]

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

[commit-bot: I haz the power, 2016-06-03 16:14:21]

Dry run: This issue passed the CQ dry run.

[msarett, 2016-06-03 16:54:17]

Looks good to me, though I'm not too familiar with this code.

https://codereview.chromium.org/2029373004/diff/60001/src/core/SkLightingShad...
File src/core/SkLightingShader.cpp (right):

https://codereview.chromium.org/2029373004/diff/60001/src/core/SkLightingShad...
src/core/SkLightingShader.cpp:676: SkShader::kClamp_TileMode,
SkShader::kClamp_TileMode,
nit: style?

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

https://codereview.chromium.org/2029373004/diff/60001/src/core/SkMipMap.cpp#n...
src/core/SkMipMap.cpp:480: // a given level, we augment this pixmap with fCS
(which we do managed).
nit: *manage

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

https://codereview.chromium.org/2029373004/diff/60001/src/core/SkMipMap.h#new...
src/core/SkMipMap.h:33: return (SkShader::ContextRec::kPMColor_DstType ==
rec.fPreferredDstType) ?
We will never want to respect gamma with kPMColor_DstType?

https://codereview.chromium.org/2029373004/diff/60001/tests/SkResourceCacheTe...
File tests/SkResourceCacheTest.cpp (right):

https://codereview.chromium.org/2029373004/diff/60001/tests/SkResourceCacheTe...
tests/SkResourceCacheTest.cpp:143: const SkMipMap* mipmap =
SkMipMapCache::FindAndRef(SkBitmapCacheDesc::Make(src[i]), mode, cache);
nit: 100 chars

[reed1, 2016-06-07 16:00:24]

Description was changed from

==========
respect srgb gamma when building mips

Deduce (from the render-target-type) if we should respect the src-image's gamma
or not when building the miptree.

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

to

==========
respect srgb gamma when building mips

Proposed policy:
- If the target is *legacy* (e.g. L32/PMColor) ignore gamma
- If the target is S32/F16 respect gamma

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

[reed1, 2016-06-07 16:01:10]

PTAL

Still does not know when to enable "respectful" behavior when the GPU wants to
build a mipmap, but that at least is no different from now. Expect we will
address that in subsequent CL(s).

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

https://codereview.chromium.org/2029373004/diff/60001/src/core/SkMipMap.cpp#n...
src/core/SkMipMap.cpp:480: // a given level, we augment this pixmap with fCS
(which we do managed).
On 2016/06/03 16:54:17, msarett wrote:
> nit: *manage

Done.

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

https://codereview.chromium.org/2029373004/diff/60001/src/core/SkMipMap.h#new...
src/core/SkMipMap.h:33: return (SkShader::ContextRec::kPMColor_DstType ==
rec.fPreferredDstType) ?
On 2016/06/03 16:54:17, msarett wrote:
> We will never want to respect gamma with kPMColor_DstType?

That is the policy I'm proposing:
- if the target is L32, we never respect gamma (or gamets)
- if the target is S32 or F16, we always respect gamma (etc.)

https://codereview.chromium.org/2029373004/diff/60001/tests/SkResourceCacheTe...
File tests/SkResourceCacheTest.cpp (right):

https://codereview.chromium.org/2029373004/diff/60001/tests/SkResourceCacheTe...
tests/SkResourceCacheTest.cpp:143: const SkMipMap* mipmap =
SkMipMapCache::FindAndRef(SkBitmapCacheDesc::Make(src[i]), mode, cache);
On 2016/06/03 16:54:17, msarett wrote:
> nit: 100 chars

Done.

[reed1, 2016-06-07 16:01:20]

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

[commit-bot: I haz the power, 2016-06-07 16:01:46]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029373004/100001

[Brian Osman, 2016-06-07 16:10:31]

https://codereview.chromium.org/2029373004/diff/100001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/2029373004/diff/100001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:349: *  TODO: determine when we want respect -vs- (compatible)
ignore
gammaTreatment is now passed into this function, so you can just use
gammaTreatment.

[commit-bot: I haz the power, 2016-06-07 16:22:46]

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

[commit-bot: I haz the power, 2016-06-07 16:22:48]

Dry run: Try jobs failed on following builders:
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)

[reed1, 2016-06-07 19:43:37]

https://codereview.chromium.org/2029373004/diff/100001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/2029373004/diff/100001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:349: *  TODO: determine when we want respect -vs- (compatible)
ignore
On 2016/06/07 16:10:30, Brian Osman wrote:
> gammaTreatment is now passed into this function, so you can just use
> gammaTreatment.

Doh!

[reed1, 2016-06-07 19:43:56]

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

[commit-bot: I haz the power, 2016-06-07 19:44:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029373004/120001

[commit-bot: I haz the power, 2016-06-07 21:22:28]

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

[commit-bot: I haz the power, 2016-06-07 21:22:30]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Build-Ubuntu-GCC-Arm64-Debug-Android-Trybot on client.skia.compile
(JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Arm7-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Arm...)
  Build-Ubuntu-GCC-Mips-Debug-Android-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-Mip...)

[reed1, 2016-06-08 18:23:20]

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

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029373004/140001

[commit-bot: I haz the power, 2016-06-08 18:26:34]

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

[commit-bot: I haz the power, 2016-06-08 18:26:35]

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...)

[reed1, 2016-06-08 19:05:32]

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

[commit-bot: I haz the power, 2016-06-08 19:05:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029373004/160001

[reed1, 2016-06-08 19:05:48]

ptal -- gamma GM now draws correctly for mipmaps!

[commit-bot: I haz the power, 2016-06-08 19:12:09]

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

[commit-bot: I haz the power, 2016-06-08 19:12:10]

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...)

[reed1, 2016-06-09 19:50:22]

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

[commit-bot: I haz the power, 2016-06-09 19:50:30]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029373004/180001

[commit-bot: I haz the power, 2016-06-09 20:17:25]

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

[commit-bot: I haz the power, 2016-06-09 20:17:26]

Dry run: This issue passed the CQ dry run.

[reed1, 2016-06-09 21:31:03]

ping -- need a review

[Brian Osman, 2016-06-10 13:37:14]

https://codereview.chromium.org/2029373004/diff/180001/bench/MipMapBench.cpp
File bench/MipMapBench.cpp (right):

https://codereview.chromium.org/2029373004/diff/180001/bench/MipMapBench.cpp#...
bench/MipMapBench.cpp:22: fName.printf("mipmap_build_%dx%d_%d_gamma", w, h,
static_cast<int>(treatment));
Not sure how much we care about preserving continuity of perf traces, but what
about:
fName.printf("mipmap_build_%dx%d%s", w, h, treatment == kRespect ? "_gamma" :
"");

https://codereview.chromium.org/2029373004/diff/180001/gm/all_bitmap_configs.cpp
File gm/all_bitmap_configs.cpp (right):

https://codereview.chromium.org/2029373004/diff/180001/gm/all_bitmap_configs....
gm/all_bitmap_configs.cpp:160: //    sk_tool_utils::draw_checkerboard(canvas,
SK_ColorLTGRAY, SK_ColorWHITE, 8);
Are the changes in this file meant to be submitted? Looks like things that were
temporarily removed for debugging?

https://codereview.chromium.org/2029373004/diff/180001/gm/mipmap.cpp
File gm/mipmap.cpp (right):

https://codereview.chromium.org/2029373004/diff/180001/gm/mipmap.cpp#newcode14
gm/mipmap.cpp:14: static sk_sp<SkImage> make_image(bool useSRGB) {
Want to make sure I understand the changes to this GM: The image returned by
make_image now contains the "same" data, encoded as either sRGB or Linear. (But
representing the same absolute brightness in both cases). Then we draw them
side-by-side, so that (in S32 mode) the only real different should be the
filtering and mip-mapping of the two sources? But in L32 mode, they will look
very different, because one of them contains linear values that are never going
to be corrected for display?

Assuming that's the intention, it might be nice to add a comment explaining
what's happening.

https://codereview.chromium.org/2029373004/diff/180001/src/core/SkPM4fPriv.h
File src/core/SkPM4fPriv.h (right):

https://codereview.chromium.org/2029373004/diff/180001/src/core/SkPM4fPriv.h#...
src/core/SkPM4fPriv.h:62: if (x <= 0.04044824) {
Where did this constant come from? I've always seen it published as 0.04045
(although that's obviously not the exact value of linear_to_srgb(0.0031308))

https://codereview.chromium.org/2029373004/diff/180001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/2029373004/diff/180001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:388: return
ctx->textureProvider()->createMipMappedTexture(desc, SkBudgeted::kYes,
texels.get(),
Ganesh is going to think the mips are not gamma-correct, but I can fix that in
the follow-up change. (We need a setter on GrTexture for fMipMapsAreSRGBCorrect,
and we ought to just set that on the result of createMipMappedTexture here,
before returning it).

Save this for a future change, though - I want to switch that boolean member to
SkSourceGammaTreatment while I'm in there.

[reed1, 2016-06-10 14:26:04]

https://codereview.chromium.org/2029373004/diff/180001/gm/all_bitmap_configs.cpp
File gm/all_bitmap_configs.cpp (right):

https://codereview.chromium.org/2029373004/diff/180001/gm/all_bitmap_configs....
gm/all_bitmap_configs.cpp:160: //    sk_tool_utils::draw_checkerboard(canvas,
SK_ColorLTGRAY, SK_ColorWHITE, 8);
On 2016/06/10 13:37:14, Brian Osman wrote:
> Are the changes in this file meant to be submitted? Looks like things that
were
> temporarily removed for debugging?

reverted

https://codereview.chromium.org/2029373004/diff/180001/gm/mipmap.cpp
File gm/mipmap.cpp (right):

https://codereview.chromium.org/2029373004/diff/180001/gm/mipmap.cpp#newcode14
gm/mipmap.cpp:14: static sk_sp<SkImage> make_image(bool useSRGB) {
On 2016/06/10 13:37:14, Brian Osman wrote:
> Want to make sure I understand the changes to this GM: The image returned by
> make_image now contains the "same" data, encoded as either sRGB or Linear.
(But
> representing the same absolute brightness in both cases). Then we draw them
> side-by-side, so that (in S32 mode) the only real different should be the
> filtering and mip-mapping of the two sources? But in L32 mode, they will look
> very different, because one of them contains linear values that are never
going
> to be corrected for display?
> 
> Assuming that's the intention, it might be nice to add a comment explaining
> what's happening.

reverted

https://codereview.chromium.org/2029373004/diff/180001/src/core/SkPM4fPriv.h
File src/core/SkPM4fPriv.h (right):

https://codereview.chromium.org/2029373004/diff/180001/src/core/SkPM4fPriv.h#...
src/core/SkPM4fPriv.h:62: if (x <= 0.04044824) {
On 2016/06/10 13:37:14, Brian Osman wrote:
> Where did this constant come from? I've always seen it published as 0.04045
> (although that's obviously not the exact value of linear_to_srgb(0.0031308))

Done.

https://codereview.chromium.org/2029373004/diff/180001/src/gpu/SkGr.cpp
File src/gpu/SkGr.cpp (right):

https://codereview.chromium.org/2029373004/diff/180001/src/gpu/SkGr.cpp#newco...
src/gpu/SkGr.cpp:388: return
ctx->textureProvider()->createMipMappedTexture(desc, SkBudgeted::kYes,
texels.get(),
On 2016/06/10 13:37:14, Brian Osman wrote:
> Ganesh is going to think the mips are not gamma-correct, but I can fix that in
> the follow-up change. (We need a setter on GrTexture for
fMipMapsAreSRGBCorrect,
> and we ought to just set that on the result of createMipMappedTexture here,
> before returning it).
> 
> Save this for a future change, though - I want to switch that boolean member
to
> SkSourceGammaTreatment while I'm in there.

Acknowledged.

[reed1, 2016-06-10 14:26:14]

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

[commit-bot: I haz the power, 2016-06-10 14:26:26]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029373004/220001

[commit-bot: I haz the power, 2016-06-10 14:39:51]

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

[commit-bot: I haz the power, 2016-06-10 14:39:52]

Dry run: This issue passed the CQ dry run.

[Brian Osman, 2016-06-10 14:40:49]

lgtm

[reed1, 2016-06-10 15:09:26]

added GM

[reed1, 2016-06-10 15:09:39]

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

[commit-bot: I haz the power, 2016-06-10 15:09:51]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029373004/240001

[commit-bot: I haz the power, 2016-06-10 15:24:20]

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

[commit-bot: I haz the power, 2016-06-10 15:24:21]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86-D...)
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[reed1, 2016-06-10 18:26:58]

The CQ bit was checked by reed@google.com

[reed1, 2016-06-10 18:26:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
brianosman@google.com
Link to the patchset: https://codereview.chromium.org/2029373004/#ps260001
(title: "fix warning")

[commit-bot: I haz the power, 2016-06-10 18:27:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029373004/260001

[commit-bot: I haz the power, 2016-06-10 18:41:50]

Description was changed from

==========
respect srgb gamma when building mips

Proposed policy:
- If the target is *legacy* (e.g. L32/PMColor) ignore gamma
- If the target is S32/F16 respect gamma

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

to

==========
respect srgb gamma when building mips

Proposed policy:
- If the target is *legacy* (e.g. L32/PMColor) ignore gamma
- If the target is S32/F16 respect gamma

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

Committed:
https://skia.googlesource.com/skia/+/6644d9353f3f0c09914385fd762e073f98d54205
==========

[commit-bot: I haz the power, 2016-06-10 18:41:52]

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

[commit-bot: I haz the power, 2016-06-10 18:41:55]

CQ bit was unchecked