Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1950183006:
Approximate common gamma tables as single values (Closed)
DescriptionApproximate common gamma tables as single values
This should improve performance of gamma conversion in
software (we can use vector math instead of table look
ups).
Additionally this allows us to quickly and easily
identify sRGB-like gammas.
On gpu, identifying sRGB gamma improves performance/memory,
because the hardware may support gamma conversion.
This will help us identify situations where gamma
conversion is not necessary.
Ex: sRGB input -> sRGB display
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1950183006
Committed: https://skia.googlesource.com/skia/+/2c3eca38c9f74be9d91f467540feb586a0e941ca
Patch Set 1 : #
Total comments: 5
Patch Set 2 : Move comments #Patch Set 3 : Fix tests #
Messages
Total messages: 26 (14 generated)
Description was changed from ========== Approximate common gamma tables as single values BUG=skia: ========== to ========== Approximate common gamma tables as single values BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Patchset #1 (id:1) has been deleted
msarett@google.com changed reviewers: + brianosman@google.com, reed@google.com, scroggo@google.com
I've been testing on a set of 97 jpegs with ICC profiles. It's an arbitrary set from the 10k skps (small sample size is so it's more manageable for me). 56 of these images had 1024 entry gamma tables for all three components. All of these tables were identical and a close approximation of 2.2f. No other images had 1024 entry tables. 26 of these images had 26 entry gamma tables for all three components. All of these tables were identical and a close approximation of 2.2f. No other images had 26 entry tables. If you're interested, you can see plots of these curves by clicking around the tabs of this sheet. https://docs.google.com/a/google.com/spreadsheets/d/1M4F-4eF_ITgadiRtvcohqDwy... Also, FWIW in the rest of the gammas, there were also 9 single value gammas, 4 parametric gammas (we don't handle these yet - but they tend to match 2.2f pretty closely), and 2 very wacky 256 entry tables. https://codereview.chromium.org/1950183006/diff/20001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1950183006/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:400: if (i != numGammas - 1) { Bug fix :)
msarett@google.com changed reviewers: + herb@google.com
Any thoughts on this? This will also be useful for us to be able to recognize color profiles as sRGB.
On 2016/05/09 15:09:40, msarett wrote: > Any thoughts on this? > > This will also be useful for us to be able to recognize color profiles as sRGB. What's the end result of this CL? - To be faster (since we have specialized for sRGB)? - To be more correct (since these are approximations of sRGB)? - To be correct at all (maybe we don't honor these curves as they are)? https://codereview.chromium.org/1950183006/diff/20001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1950183006/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:332: // robust solution would be to compare each value in this curve against nit: It seems like these comments actually apply to the section below. I would put this outside the first if statement. https://codereview.chromium.org/1950183006/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:400: if (i != numGammas - 1) { On 2016/05/05 20:11:48, msarett wrote: > Bug fix :) Might be worth a separate change?
On 2016/05/09 17:39:58, scroggo wrote: > On 2016/05/09 15:09:40, msarett wrote: > > Any thoughts on this? > > > > This will also be useful for us to be able to recognize color profiles as > sRGB. > > What's the end result of this CL? > - To be faster (since we have specialized for sRGB)? > - To be more correct (since these are approximations of sRGB)? > - To be correct at all (maybe we don't honor these curves as they are)? From a GPU perspective, being able to identify that an image has a gamma function that is very similar to sRGB (or 2.2, if we consider that close enough) is very useful for performance and memory. The HW can apply the sRGB gamma for free, so even if there is a gamut mismatch, we can choose to treat it as sRGB, then just do a color matrix in the shader to get the correct color. The alternative is to ask for the linear values (decoded as F16).
On 2016/05/09 17:47:01, Brian Osman wrote: > On 2016/05/09 17:39:58, scroggo wrote: > > On 2016/05/09 15:09:40, msarett wrote: > > > Any thoughts on this? > > > > > > This will also be useful for us to be able to recognize color profiles as > > sRGB. > > > > What's the end result of this CL? > > - To be faster (since we have specialized for sRGB)? > > - To be more correct (since these are approximations of sRGB)? > > - To be correct at all (maybe we don't honor these curves as they are)? > > From a GPU perspective, being able to identify that an image has a gamma > function that is very similar to sRGB (or 2.2, if we consider that close > enough) is very useful for performance and memory. The HW can apply the sRGB > gamma for free, so even if there is a gamut mismatch, we can choose to treat > it as sRGB, then just do a color matrix in the shader to get the correct > color. The alternative is to ask for the linear values (decoded as F16). Thanks for the explanation! Might be worth an explanation in the commit message, but otherwise lgtm
Description was changed from ========== Approximate common gamma tables as single values BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Approximate common gamma tables as single values From a GPU perspective, being able to identify that an image has a gamma function that is very similar to sRGB (or 2.2, if we consider that close enough) is very useful for performance and memory. The HW can apply the sRGB gamma for free, so even if there is a gamut mismatch, we can choose to treat it as sRGB, then just do a color matrix in the shader to get the correct color. The alternative is to ask for the linear values (decoded as F16). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Approximate common gamma tables as single values From a GPU perspective, being able to identify that an image has a gamma function that is very similar to sRGB (or 2.2, if we consider that close enough) is very useful for performance and memory. The HW can apply the sRGB gamma for free, so even if there is a gamut mismatch, we can choose to treat it as sRGB, then just do a color matrix in the shader to get the correct color. The alternative is to ask for the linear values (decoded as F16). BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Approximate common gamma tables as single values This should improve performance of gamma conversion in software. Additionally this allows us to quickly and easily identify sRGB like gammas. On gpu, identifying sRGB gamma improves performance/memory, where the hardware may support gamma conversion. Finally this will help us identify situations where gamma conversion is not necessary. Ex: sRGB input -> sRGB display BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
Description was changed from ========== Approximate common gamma tables as single values This should improve performance of gamma conversion in software. Additionally this allows us to quickly and easily identify sRGB like gammas. On gpu, identifying sRGB gamma improves performance/memory, where the hardware may support gamma conversion. Finally this will help us identify situations where gamma conversion is not necessary. Ex: sRGB input -> sRGB display BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Approximate common gamma tables as single values This should improve performance of gamma conversion in software (we can use vector math instead of table look ups). Additionally this allows us to quickly and easily identify sRGB-like gammas. On gpu, identifying sRGB gamma improves performance/memory, because the hardware may support gamma conversion. This will help us identify situations where gamma conversion is not necessary. Ex: sRGB input -> sRGB display BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ==========
> > > > Any thoughts on this? > > > > > > > > This will also be useful for us to be able to recognize color profiles as > > > sRGB. > > > > > > What's the end result of this CL? > > > - To be faster (since we have specialized for sRGB)? Yes, I believe so. Brian gave a good case for the gpu, and I also expect vector math to win vs. table-lookups in software. > > > - To be more correct (since these are approximations of sRGB)? I think we ought to consider "correct" as exactly what the table specifies. So I think we'll be less correct. But the difference is small. Right now, we are willing to make this tradeoff and approximate the sRGB curve, but this is worth mentioning. > > > - To be correct at all (maybe we don't honor these curves as they are)? > > We don't support curves or table right now. So I guess no matter what, right now, we are wrong. I expect that we will end up supporting both. But we will probably work very hard to optimize 2.2f and leave table-lookups as a rare fallback. > > From a GPU perspective, being able to identify that an image has a gamma > > function that is very similar to sRGB (or 2.2, if we consider that close > > enough) is very useful for performance and memory. The HW can apply the sRGB > > gamma for free, so even if there is a gamut mismatch, we can choose to treat > > it as sRGB, then just do a color matrix in the shader to get the correct > > color. The alternative is to ask for the linear values (decoded as F16). > Thanks Brian! > Thanks for the explanation! Might be worth an explanation in the commit message, > but otherwise lgtm Updated the commit message :) https://codereview.chromium.org/1950183006/diff/20001/src/core/SkColorSpace.cpp File src/core/SkColorSpace.cpp (right): https://codereview.chromium.org/1950183006/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:332: // robust solution would be to compare each value in this curve against On 2016/05/09 17:39:58, scroggo wrote: > nit: It seems like these comments actually apply to the section below. I would > put this outside the first if statement. Done. https://codereview.chromium.org/1950183006/diff/20001/src/core/SkColorSpace.c... src/core/SkColorSpace.cpp:400: if (i != numGammas - 1) { On 2016/05/09 17:39:58, scroggo wrote: > On 2016/05/05 20:11:48, msarett wrote: > > Bug fix :) > > Might be worth a separate change? You're right, I should have uploaded separately... But I think I'll leave it here since this is about to land.
The CQ bit was checked by msarett@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/1950183006/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950183006/40001
The CQ bit was unchecked by commit-bot@chromium.org
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...)
The CQ bit was checked by msarett@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/1950183006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950183006/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 msarett@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com Link to the patchset: https://codereview.chromium.org/1950183006/#ps60001 (title: "Fix tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1950183006/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1950183006/60001
Message was sent while issue was closed.
Description was changed from ========== Approximate common gamma tables as single values This should improve performance of gamma conversion in software (we can use vector math instead of table look ups). Additionally this allows us to quickly and easily identify sRGB-like gammas. On gpu, identifying sRGB gamma improves performance/memory, because the hardware may support gamma conversion. This will help us identify situations where gamma conversion is not necessary. Ex: sRGB input -> sRGB display BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is... ========== to ========== Approximate common gamma tables as single values This should improve performance of gamma conversion in software (we can use vector math instead of table look ups). Additionally this allows us to quickly and easily identify sRGB-like gammas. On gpu, identifying sRGB gamma improves performance/memory, because the hardware may support gamma conversion. This will help us identify situations where gamma conversion is not necessary. Ex: sRGB input -> sRGB display 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/+/2c3eca38c9f74be9d91f467540feb586a0e941ca ==========
Message was sent while issue was closed.
Committed patchset #3 (id:60001) as https://skia.googlesource.com/skia/+/2c3eca38c9f74be9d91f467540feb586a0e941ca |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
