Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2190573002:
Add SkGammaColorFilter (Closed)
|
Created:
4 years, 4 months ago by robertphillips Modified:
4 years, 4 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionAdd SkGammaColorFilter
WDYT about this as a means of replacing GrContext::applyGamma with a normal SkCanvas::drawImage?
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190573002
Committed: https://skia.googlesource.com/skia/+/a408c8fb6d367d494437b8709fdfa8c822267e6e
Patch Set 1 #Patch Set 2 : Clean up #
Total comments: 2
Patch Set 3 : Add assert & comment #Patch Set 4 : Make gammacolorfilter GM GPU-only #Patch Set 5 : Add comment #Patch Set 6 : Add src/gpu/effects to private include dirs in gn file #Patch Set 7 : ?? #
Messages
Total messages: 36 (22 generated)
Description was changed from ========== Add SkGammaColorFilter ========== to ========== Add SkGammaColorFilter GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190573002 ==========
Description was changed from ========== Add SkGammaColorFilter GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190573002 ========== to ========== Add SkGammaColorFilter GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190573002 ==========
robertphillips@google.com changed reviewers: + brianosman@google.com
Description was changed from ========== Add SkGammaColorFilter GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190573002 ========== to ========== Add SkGammaColorFilter WDYT about this as a means of replacing GrContext::applyGamma with a normal SkCanvas::drawImage? GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190573002 ==========
mtklein@google.com changed reviewers: + mtklein@google.com
https://codereview.chromium.org/2190573002/diff/20001/src/effects/SkGammaColo... File src/effects/SkGammaColorFilter.cpp (right): https://codereview.chromium.org/2190573002/diff/20001/src/effects/SkGammaColo... src/effects/SkGammaColorFilter.cpp:18: SkPMColor dst[]) const { Can we assert here? Gamma-correcting bytes to bytes seems pretty questionable... if the cpu code tries to call in something like this, it's likely taking a bad approach.
https://codereview.chromium.org/2190573002/diff/20001/src/effects/SkGammaColo... File src/effects/SkGammaColorFilter.cpp (right): https://codereview.chromium.org/2190573002/diff/20001/src/effects/SkGammaColo... src/effects/SkGammaColorFilter.cpp:18: SkPMColor dst[]) const { On 2016/07/27 12:06:22, mtklein wrote: > Can we assert here? Gamma-correcting bytes to bytes seems pretty > questionable... if the cpu code tries to call in something like this, it's > likely taking a bad approach. Done.
reed@google.com changed reviewers: + reed@google.com
Given klein's comments, why does this need to exist? Why need it be public? side-note: can tablecolorfilter do this as well?
On 2016/07/27 12:27:42, reed1 wrote: > Given klein's comments, why does this need to exist? Why need it be public? > side-note: can tablecolorfilter do this as well? GrContext::applyGamma needs to go - replaced with something that doesn't take a GrRenderTarget. Given what it does, a ColorFilter seems the best match. The SampleApp & the viewer use this call to present gamma-corrected 30-bit & F16 raster results, so it seems unlikely we would want to keep it private. I don't believe the tablecolorfilter will suffice since the GrGammaEffect is actually doing the full gamma correction math - but Brian O. would know better.
brianosman@google.com changed reviewers: + bsalomon@google.com
I'm pretty sure bsalomon@ was changing the applyGamma API to eliminate the render target references (as of Monday)?
On 2016/07/27 13:10:45, Brian Osman wrote: > I'm pretty sure bsalomon@ was changing the applyGamma API to eliminate the > render target references (as of Monday)? See also: https://codereview.chromium.org/2176383002/
On 2016/07/27 13:11:53, Brian Osman wrote: > On 2016/07/27 13:10:45, Brian Osman wrote: > > I'm pretty sure bsalomon@ was changing the applyGamma API to eliminate the > > render target references (as of Monday)? > > See also: https://codereview.chromium.org/2176383002/ I ran into two issues while working on that 1) I don't think it is possible to trigger the applyGamma calling code in viewer so I couldn't verify it using viewer. 2) Removing GrRenderTarget and having a image instead in SampleApp is non-trivial and spans a variety of classes. I'm putting more weight behind improving viewer as a replacement and addressing 1 so that SampleApp need not be fixed. Note that the CL Brian linked to would be followed by a CL to reimplement GrContext::applyGamma as a GrDrawContext function, was just starting from the public interface direction. I'm also not opposed to a color filter approach.
On 2016/07/27 12:44:24, robertphillips wrote: > On 2016/07/27 12:27:42, reed1 wrote: > > Given klein's comments, why does this need to exist? Why need it be public? > > side-note: can tablecolorfilter do this as well? > > GrContext::applyGamma needs to go - replaced with something that doesn't take a > GrRenderTarget. Given what it does, a ColorFilter seems the best match. The > SampleApp & the viewer use this call to present gamma-corrected 30-bit & F16 > raster results, so it seems unlikely we would want to keep it private. > > I don't believe the tablecolorfilter will suffice since the GrGammaEffect is > actually doing the full gamma correction math - but Brian O. would know better. Looking at this more carefully - tablecolorfilter would definitely be less precise. With a texture-based lookup table, we could - at best - get a piecewise linear approximation to the correct curve. As it stands, we don't filter the texture in ColorTableEffect, so it would be much worse than that. (The canonical use-case for GrGammaEffect is FP16 input, so we have more precision on our inputs than the table can represent).
OK, maybe I'm starting to understand more. 1. I missed the TODO on the raster-side. As spec'd, the raster version can't be more precise than a table, since its byte-in --> byte-out. 2. I didn't dig into the asFragment part -- I guess its trickier (does sRGB math?) 3. I was somewhat joking about Table, only wanted to ferret out what was going on. Since I see now that gpu will use it for 10bit or F16 -> 8bit, it makes sense to have a custom version (should be not-hard to steal code from matt to impl the raster side). lgtm
Lets add some dox to the public part... like how is gamma interpreted! - is it only really intended for linear->srgb? - does it only need 1 exp, and no other parts (ICC Profiles have several equation variations)
I've added a comment. Brian O. please let me know if I've got all the details right.
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by robertphillips@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by robertphillips@google.com
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/2190573002/#ps120001 (title: "??")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add SkGammaColorFilter WDYT about this as a means of replacing GrContext::applyGamma with a normal SkCanvas::drawImage? GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190573002 ========== to ========== Add SkGammaColorFilter WDYT about this as a means of replacing GrContext::applyGamma with a normal SkCanvas::drawImage? GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2190573002 Committed: https://skia.googlesource.com/skia/+/a408c8fb6d367d494437b8709fdfa8c822267e6e ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://skia.googlesource.com/skia/+/a408c8fb6d367d494437b8709fdfa8c822267e6e |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
