Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1954863002:
Make GrGammaEffect have explicit sRGB modes, plus exponential mode. (Closed)
|
Created:
4 years, 7 months ago by Brian Osman Modified:
4 years, 7 months ago CC:
reviews_skia.org Base URL:
https://skia.googlesource.com/skia.git@master Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
DescriptionMake GrGammaEffect have explicit sRGB modes, plus exponential mode.
Convert it to a "standard" FP that just transforms the input color.
For now, we still infer the sRGB transfer curves from the exponent,
but I'm hoping that eventually SkGammas will provide us with the
exact curve we're supposed to be applying. In any case, this adds
support for doing the inverse transformation, as well, which will
be needed in an upcoming Vulkan YUV change, among other things.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1954863002
Committed: https://skia.googlesource.com/skia/+/2139303e4c0a9cbcfac695977a80eb026a9296ab
Committed: https://skia.googlesource.com/skia/+/fe4d5d3af53e2c6903abf9bdac2503e3e69ceffe
Patch Set 1 #Patch Set 2 : Go back to inferring sRGB from exponent (but leave mode intact for future use) #Patch Set 3 : Remove spurious temporary #
Total comments: 3
Patch Set 4 : enum class #Patch Set 5 : powf -> pow in shader code #
Messages
Total messages: 27 (15 generated)
Description was changed from ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. BUG=skia: ========== to ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was checked by brianosman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1954863002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1954863002/20001
Description was changed from ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. For now, we still infer the sRGB transfer curves from the exponent, but I'm hoping that eventually SkGammas will provide us with the exact curve we're supposed to be applying. In any case, this adds support for doing the inverse transformation, as well, which will be needed in an upcoming Vulkan YUV change, among other things. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. For now, we still infer the sRGB transfer curves from the exponent, but I'm hoping that eventually SkGammas will provide us with the exact curve we're supposed to be applying. In any case, this adds support for doing the inverse transformation, as well, which will be needed in an upcoming Vulkan YUV change, among other things. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. Convert it to a "standard" FP that just transforms the input color. For now, we still infer the sRGB transfer curves from the exponent, but I'm hoping that eventually SkGammas will provide us with the exact curve we're supposed to be applying. In any case, this adds support for doing the inverse transformation, as well, which will be needed in an upcoming Vulkan YUV change, among other things. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
brianosman@google.com changed reviewers: + bsalomon@google.com, robertphillips@google.com
lgtm w/ some comments inline https://codereview.chromium.org/1954863002/diff/40001/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): https://codereview.chromium.org/1954863002/diff/40001/src/gpu/GrContext.cpp#n... src/gpu/GrContext.cpp:553: paint.addColorTextureProcessor(src, GrCoordTransform::MakeDivByTextureWHMatrix(src)); I may have asked this before, but I'll ask again anyway just in case :) Should we check if the src texture happens to be sRGB and the dst happens to be linear and the gamma happens to be 2.2? (Or the reverse transformation). https://codereview.chromium.org/1954863002/diff/40001/src/gpu/effects/GrGamma... File src/gpu/effects/GrGammaEffect.h (right): https://codereview.chromium.org/1954863002/diff/40001/src/gpu/effects/GrGamma... src/gpu/effects/GrGammaEffect.h:15: enum Mode { I hear enum class is all the rage.
Switched over to enum class. https://codereview.chromium.org/1954863002/diff/40001/src/gpu/GrContext.cpp File src/gpu/GrContext.cpp (right): https://codereview.chromium.org/1954863002/diff/40001/src/gpu/GrContext.cpp#n... src/gpu/GrContext.cpp:553: paint.addColorTextureProcessor(src, GrCoordTransform::MakeDivByTextureWHMatrix(src)); On 2016/05/11 02:15:01, bsalomon wrote: > I may have asked this before, but I'll ask again anyway just in case :) Should > we check if the src texture happens to be sRGB and the dst happens to be linear > and the gamma happens to be 2.2? (Or the reverse transformation). Perhaps, although right now I'm picturing that this is only going to be used when we know that we need it (because the automatic behavior wouldn't be correct). I don't picture external code ever injecting these, and I think that we're going to want a higher level check? Maybe not - perhaps we end up always inserting a "conversion" effect after texture read that's actually a no-op in those cases...
The CQ bit was checked by brianosman@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1954863002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1954863002/60001
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 brianosman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1954863002/#ps60001 (title: "enum class")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1954863002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1954863002/60001
Message was sent while issue was closed.
Description was changed from ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. Convert it to a "standard" FP that just transforms the input color. For now, we still infer the sRGB transfer curves from the exponent, but I'm hoping that eventually SkGammas will provide us with the exact curve we're supposed to be applying. In any case, this adds support for doing the inverse transformation, as well, which will be needed in an upcoming Vulkan YUV change, among other things. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. Convert it to a "standard" FP that just transforms the input color. For now, we still infer the sRGB transfer curves from the exponent, but I'm hoping that eventually SkGammas will provide us with the exact curve we're supposed to be applying. In any case, this adds support for doing the inverse transformation, as well, which will be needed in an upcoming Vulkan YUV change, among other things. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/2139303e4c0a9cbcfac695977a80eb026a9296ab ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://skia.googlesource.com/skia/+/2139303e4c0a9cbcfac695977a80eb026a9296ab
Message was sent while issue was closed.
A revert of this CL (patchset #4 id:60001) has been created in https://codereview.chromium.org/1964943003/ by robertphillips@google.com. The reason for reverting is: Shader compilation appears to be failing.
Message was sent while issue was closed.
Description was changed from ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. Convert it to a "standard" FP that just transforms the input color. For now, we still infer the sRGB transfer curves from the exponent, but I'm hoping that eventually SkGammas will provide us with the exact curve we're supposed to be applying. In any case, this adds support for doing the inverse transformation, as well, which will be needed in an upcoming Vulkan YUV change, among other things. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/2139303e4c0a9cbcfac695977a80eb026a9296ab ========== to ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. Convert it to a "standard" FP that just transforms the input color. For now, we still infer the sRGB transfer curves from the exponent, but I'm hoping that eventually SkGammas will provide us with the exact curve we're supposed to be applying. In any case, this adds support for doing the inverse transformation, as well, which will be needed in an upcoming Vulkan YUV change, among other things. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/2139303e4c0a9cbcfac695977a80eb026a9296ab ==========
The CQ bit was checked by brianosman@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/1954863002/#ps80001 (title: "powf -> pow in shader code")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1954863002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1954863002/80001
Message was sent while issue was closed.
Description was changed from ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. Convert it to a "standard" FP that just transforms the input color. For now, we still infer the sRGB transfer curves from the exponent, but I'm hoping that eventually SkGammas will provide us with the exact curve we're supposed to be applying. In any case, this adds support for doing the inverse transformation, as well, which will be needed in an upcoming Vulkan YUV change, among other things. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/2139303e4c0a9cbcfac695977a80eb026a9296ab ========== to ========== Make GrGammaEffect have explicit sRGB modes, plus exponential mode. Convert it to a "standard" FP that just transforms the input color. For now, we still infer the sRGB transfer curves from the exponent, but I'm hoping that eventually SkGammas will provide us with the exact curve we're supposed to be applying. In any case, this adds support for doing the inverse transformation, as well, which will be needed in an upcoming Vulkan YUV change, among other things. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... Committed: https://skia.googlesource.com/skia/+/2139303e4c0a9cbcfac695977a80eb026a9296ab Committed: https://skia.googlesource.com/skia/+/fe4d5d3af53e2c6903abf9bdac2503e3e69ceffe ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://skia.googlesource.com/skia/+/fe4d5d3af53e2c6903abf9bdac2503e3e69ceffe |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
