Closed DavidBuchanan314 closed 3 years ago
I suppose the logic for implementing this could go inside filter_matrix
- which would have to be extended to allow saturation to be specified.
I wish I found this earlier - it gives a slightly cleaner definition of a saturation matrix, although its functionally equivalent to what I came up with (I think?): http://graficaobscura.com/matrix/index.html
That's very interesting, thanks for sharing. I'll take a look at it if you don't. I think it would best go into its own function that would initialize the 9*9 CSC matrix, around here:
std::array<float, 9> coeffs;
if (sat != 0)
coeffs = saturation_matrix(sat);
else
coeffs = filter_matrix(filter);
Also fwiw nvidia also has an ioctl called NVDISP_SET_CSC
, which should be able to set the saturation, hue and contrast: https://github.com/Project-Google-Tango/vendor_nvidia_jxd_src/blob/a036219a4b6b84253989a870a9c132ddc27612ed/tegra/core/include/nvdc.h#L410-L421. See also the implementation in the same repo.
This seems to be a feature of the TX1 separate from the CMU stuff, see 24.9.19 in the TRM, and seems to be programmable per hardware-window contrary to the CMU which is display-wide.
ams::Result Man::set_cmu(std::uint32_t fd, Temperature temp, ColorFilter filter, Gamma gamma, Luminance luma, ColorRange range) { auto &cmu = Man::cmu; cmu.reset();
// Calculate initial coefficients
auto coeffs = filter_matrix(filter);
// Apply temperature color correction
auto [krr, kgg, kbb] = whitepoint(temp);
krr = degamma(krr, 2.4f), kgg = degamma(kgg, 2.4f), kbb = degamma(kbb, 2.4f);
coeffs[0] *= krr, coeffs[4] *= kgg, coeffs[8] *= kbb;
float s = 0.25f;
float a = (1.0 - s) * krr + s;
float b = (1.0 - s) * krr;
float c = (1.0 - s) * krr;
float d = (1.0 - s) * kgg;
float e = (1.0 - s) * kgg + s;
float f = (1.0 - s) * kgg;
float g = (1.0 - s) * kbb;
float h = (1.0 - s) * kbb;
float i = (1.0 - s) * kbb + s;
float mat[16] = {
a, b, c, 0.0,
d, e, f, 0.0,
g, h, i, 0.0,
0.0, 0.0, 0.0, 1.0,
};
for (int m = 0; m < 16; m++)
{
coeffs[m] *= mat[m];
}
std::transform(coeffs.begin(), coeffs.end(), &cmu.krr, [](float c) -> QS18 { return c; });
// Calculate gamma ramps
degamma_ramp(cmu.lut_1.data(), cmu.lut_1.size(), DEFAULT_GAMMA, 12); // Set the LUT1 with a fixed gamma corresponding to the incoming data
regamma_ramp(cmu.lut_2.data(), 512, gamma, 8, 0.0f, 0.125f); // Set the first part of LUT2 (more precision in darker components)
regamma_ramp(cmu.lut_2.data() + 512, cmu.lut_2.size() - 512, gamma, 8, 0.125f, 1.0f); // Set the second part of LUT2 (less precision in brighter components)
// Apply luminance
apply_luma(cmu.lut_2.data(), cmu.lut_2.size(), luma);
// Apply color range
apply_range(cmu.lut_2.data(), cmu.lut_2.size(), range.lo, std::min(range.hi, cmu.lut_2.back() / 255.0f)); // Adjust max for luma
return nvioctlNvDisp_SetCmu(fd, cmu);
}
float s is saturation, don't raise too high
I'm a bit confused, isn't coeffs
a 3x3 matrix? Your multiplication loop would cause out-of-bounds writes. You can make mat
work as a 3x3 matrix just by chopping off the "extra" row and column.
Furthermore, is element-wise multiplication really the correct way to combine the colour correction matrix with the saturation matrix? Intuitively I'd have thought it needed a dot product, but I might be wrong.
Uhh I dunno what I'm doing but it works seemingly
I'm a bit confused, isn't
coeffs
a 3x3 matrix? Your multiplication loop would cause out-of-bounds writes. You can makemat
work as a 3x3 matrix just by chopping off the "extra" row and column.Furthermore, is element-wise multiplication really the correct way to combine the colour correction matrix with the saturation matrix? Intuitively I'd have thought it needed a dot product, but I might be wrong.
I think I'm doing it wrong but I've made the saturation slider and saving to config and stuff correctly if i should check that side in
EDIT: I'll try with a dot product now
@bscarell Very interesting, thanks for taking the time. Would you mind creating a pull request ? And I'll review your changes.
Furthermore, is element-wise multiplication really the correct way to combine the colour correction matrix with the saturation matrix? Intuitively I'd have thought it needed a dot product, but I might be wrong.
@DavidBuchanan314 Very good point, and I believe this is correct. Say you want to apply a saturation filter, then, separately, a color correction one, it amounts to
[cc1 0 0] ([s1 s2 s3] [r])
[0 cc2 0] * ([s4 s5 s6] * [g]) = (CC * S) * rgb
[0 0 cc3] ([s7 s8 s9] [b])
The current filter/color correction merge is only a multiplication because the CC matrix is diagonal. That means it's worth taking the time to think it which order we dot those matrices. I believe it should be filter color correction saturation. I also agree with the 4x4 oob issue.
@bscarell Very interesting, thanks for taking the time. Would you mind creating a pull request ? And I'll review your changes.
Furthermore, is element-wise multiplication really the correct way to combine the colour correction matrix with the saturation matrix? Intuitively I'd have thought it needed a dot product, but I might be wrong.
@DavidBuchanan314 Very good point, and I believe this is correct. Say you want to apply a saturation filter, then, separately, a color correction one, it amounts to
[cc1 0 0] ([s1 s2 s3] [r]) [0 cc2 0] * ([s4 s5 s6] * [g]) = (CC * S) * rgb [0 0 cc3] ([s7 s8 s9] [b])
The current filter/color correction merge is only a multiplication because the CC matrix is diagonal. That means it's worth taking the time to think it which order we dot those matrices. I believe it should be filter color correction saturation. I also agree with the 4x4 oob issue.
I'll get on the pull request, just remembering git and things I'm very rusty. I've tried a few methods and seem happy with my newest the most. I'll link you on gbatemp
I implemented saturation control with 293acc6. It seems to interact well with color filters and gamma/temperature correction. Waiting for some opinions before I make a new release, here are builds if needed: Fizeau-2.1.9-293acc6.zip Fizeau-chl-2.1.9-293acc6.zip
Also i gave the NVDISP_SET_CSC
ioctl a try but couldn't get it working. It's probably stubbed, though I haven't looked in nvdrv to confirm.
This is now part of the latest release.
I think it would be neat to have a slider/option to adjust the colour saturation. I might look into implementing this, at some point.
I believe this is possible by adjusting the CSC matrix.
As a quick PoC, I made this python script for adjusting the saturation of an image, based on a CSC matrix:
The variable
f
controls the saturation level. -1 is fully desaturated, 0 is normal, and above zero is increasingly saturated.Example input:
Example output (f=1.0):