| Message ID | 20200704095914.17344-4-david.plowman@raspberrypi.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for the patch. On Sat, Jul 04, 2020 at 10:59:14AM +0100, David Plowman wrote: > This patch improves the colour information recorded in DNG files using > the ColourCorrectionMatrix metadata for the image. Note that we are > not supplying a full calibration using two illuminants, nonetheless > the single matrix here appears to be respected by a number of tools. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > src/qcam/dng_writer.cpp | 93 +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 93 insertions(+) > > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp > index 61505d3..222df9f 100644 > --- a/src/qcam/dng_writer.cpp > +++ b/src/qcam/dng_writer.cpp > @@ -34,6 +34,63 @@ struct FormatInfo { > unsigned int stride); > }; > > +struct Matrix { I'd name this Matrix3d as it's a 3x3 matrix. > + Matrix(float m0, float m1, float m2, > + float m3, float m4, float m5, > + float m6, float m7, float m8) > + { > + m[0] = m0, m[1] = m1, m[2] = m2; > + m[3] = m3, m[4] = m4, m[5] = m5; > + m[6] = m6, m[7] = m7, m[8] = m8; > + } Please add blank lines between functions. > + Matrix(float diag0, float diag1, float diag2) > + : Matrix(diag0, 0, 0, 0, diag1, 0, 0, 0, diag2) {} And we tend to move the brackets to lines of their own after the member initializer list. I wonder if this should be a static function though, static Matrix diag(float diag0, float diag1, float diag2) { return { diag0, 0, 0, 0, diag1, 0, 0, 0, diag2 }; } as using a custom constructor is a bit confusing. When I was reading the code below: Matrix wbGain(1, 1, 1); I wondered how a 3x3 matrix would be constructed from 3 values only. Matrix wbGain = Matrix::diag(1, 1, 1); would be more explicit. Another useful function would be Matrix identity() { return { 1, 0, 0, 0, 1, 0, 0, 0, 1 }; } > + Matrix() {} We usually put the default constructor first. > + float m[9]; We usually put member data after member functions. > + Matrix transpose() const > + { > + return Matrix(m[0], m[3], m[6], m[1], m[4], m[7], m[2], m[5], m[8]); You can also write return { m[0], m[3], m[6], m[1], m[4], m[7], m[2], m[5], m[8] }; which will cause the compiler to warn in case of loss of precision due to implicit casts. Same for cofactors(). > + } > + Matrix cofactors() const > + { > + return Matrix(m[4] * m[8] - m[5] * m[7], > + -(m[3] * m[8] - m[5] * m[6]), > + m[3] * m[7] - m[4] * m[6], > + -(m[1] * m[8] - m[2] * m[7]), > + m[0] * m[8] - m[2] * m[6], > + -(m[0] * m[7] - m[1] * m[6]), > + m[1] * m[5] - m[2] * m[4], > + -(m[0] * m[5] - m[2] * m[3]), > + m[0] * m[4] - m[1] * m[3]); > + } > + Matrix adjugate() const { return cofactors().transpose(); } > + float determinant() const > + { > + return (m[0] * (m[4] * m[8] - m[5] * m[7]) - > + m[1] * (m[3] * m[8] - m[5] * m[6]) + > + m[2] * (m[3] * m[7] - m[4] * m[6])); The outer parentheses are not needed. > + } > + Matrix inverse() const { return adjugate() * (1.0 / determinant()); } Should we protect against determinant being 0 ? > + Matrix operator*(Matrix const &other) const > + { > + Matrix result; > + for (int i = 0; i < 3; i++) unsigned int i Even though not strictly necessary, we use curly braces when the inner statement is a compound statement. > + for (int j = 0; j < 3; j++) unsigned int j > + result.m[i * 3 + j] = > + m[i * 3 + 0] * other.m[0 + j] + > + m[i * 3 + 1] * other.m[3 + j] + > + m[i * 3 + 2] * other.m[6 + j]; > + return result; > + } > + Matrix operator*(float const &f) const I think "float f" would be fine, is there a need for a reference ? > + { > + Matrix result; > + for (int i = 0; i < 9; i++) unsigned int i > + result.m[i] = m[i] * f; > + return result; > + } > +}; > + > void packScanlineSBGGR10P(void *output, const void *input, unsigned int width) > { > const uint8_t *in = static_cast<const uint8_t *>(input); > @@ -315,6 +372,42 @@ int DNGWriter::write(const char *filename, const Camera *camera, > TIFFSetField(tif, TIFFTAG_PLANARCONFIG, PLANARCONFIG_CONTIG); > TIFFSetField(tif, TIFFTAG_SAMPLEFORMAT, SAMPLEFORMAT_UINT); > > + /* > + * Fill in some reasonable colour information in the DNG. We supply > + * the "neutral" colour values which determine the white balance, and the > + * "ColorMatrix1" which converts XYZ to (un-white-balanced) camera RGB. > + * Note that this is not a "proper" colour calibration for the DNG, > + * nonetheless, many tools should be able to render the colours better. > + */ > + float neutral[3] = { 1, 1, 1 }; > + Matrix wbGain(1, 1, 1); > + /* From http://www.brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html */ https would be better, but it seems that the server doesn't support it :-( > + Matrix rgb2xyz(0.4124564, 0.3575761, 0.1804375, > + 0.2126729, 0.7151522, 0.0721750, > + 0.0193339, 0.1191920, 0.9503041); This should be const. > + Matrix ccm(1, 1, 1); > + const double eps = 1e-2; Out of curiosity, why 1e-2 ? > + > + if (metadata.contains(controls::ColourGains)) { > + Span<const float> colour_gains = metadata.get(controls::ColourGains); s/colour_gains/colourGains/ or just s/colour_gains/gains/ > + if (colour_gains[0] > eps && colour_gains[1] > eps) { > + wbGain = Matrix(colour_gains[0], 1, colour_gains[1]); > + neutral[0] = 1.0 / colour_gains[0]; /* red */ > + neutral[2] = 1.0 / colour_gains[1]; /* blue */ > + } > + } > + if (metadata.contains(controls::ColourCorrectionMatrix)) { > + Span<const float> m = metadata.get(controls::ColourCorrectionMatrix); > + Matrix tmp = Matrix(m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8]); Would a Matrix(Span<const float> m) constructor make sense to simplify this line ? Up to you. We really try not to use "tmp" as a variable name. Possible alternatives are "matrix", or just "m" if you rename the existing "m" to "value". > + if (tmp.determinant() > eps) > + ccm = tmp; > + } Please add a blank line here. > + /* This is guaranteed to be invertible because all the bits in it are. */ I'd expand this a little: /* * This is guaranteed to be invertible because all the bits in it are * (rgb2xyz is hardcoded and invertible, the ccm determinant is checked * manually above, and wbGain is a diagonal matrix with diagonal * elements checked to be non-zero). */ > + Matrix colorMatrix1 = (rgb2xyz * ccm * wbGain).inverse(); > + > + TIFFSetField(tif, TIFFTAG_COLORMATRIX1, 9, colorMatrix1.m); > + TIFFSetField(tif, TIFFTAG_ASSHOTNEUTRAL, 3, neutral); > + > /* > * Reserve space for the SubIFD and ExifIFD tags, pointing to the IFD > * for the raw image and EXIF data respectively. The real offsets will
Hi Laurent Thanks for the review of the various patches in this set. Let me roll all that up into a v2 and I'll send those out shortly. Best regards David On Fri, 24 Jul 2020 at 02:17, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi David, > > Thank you for the patch. > > On Sat, Jul 04, 2020 at 10:59:14AM +0100, David Plowman wrote: > > This patch improves the colour information recorded in DNG files using > > the ColourCorrectionMatrix metadata for the image. Note that we are > > not supplying a full calibration using two illuminants, nonetheless > > the single matrix here appears to be respected by a number of tools. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > src/qcam/dng_writer.cpp | 93 +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 93 insertions(+) > > > > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp > > index 61505d3..222df9f 100644 > > --- a/src/qcam/dng_writer.cpp > > +++ b/src/qcam/dng_writer.cpp > > @@ -34,6 +34,63 @@ struct FormatInfo { > > unsigned int stride); > > }; > > > > +struct Matrix { > > I'd name this Matrix3d as it's a 3x3 matrix. > > > + Matrix(float m0, float m1, float m2, > > + float m3, float m4, float m5, > > + float m6, float m7, float m8) > > + { > > + m[0] = m0, m[1] = m1, m[2] = m2; > > + m[3] = m3, m[4] = m4, m[5] = m5; > > + m[6] = m6, m[7] = m7, m[8] = m8; > > + } > > Please add blank lines between functions. > > > + Matrix(float diag0, float diag1, float diag2) > > + : Matrix(diag0, 0, 0, 0, diag1, 0, 0, 0, diag2) {} > > And we tend to move the brackets to lines of their own after the member > initializer list. > > I wonder if this should be a static function though, > > static Matrix diag(float diag0, float diag1, float diag2) > { > return { diag0, 0, 0, 0, diag1, 0, 0, 0, diag2 }; > } > > as using a custom constructor is a bit confusing. When I was reading the > code below: > > Matrix wbGain(1, 1, 1); > > I wondered how a 3x3 matrix would be constructed from 3 values only. > > Matrix wbGain = Matrix::diag(1, 1, 1); > > would be more explicit. Another useful function would be > > Matrix identity() > { > return { 1, 0, 0, 0, 1, 0, 0, 0, 1 }; > } > > > + Matrix() {} > > We usually put the default constructor first. > > > + float m[9]; > > We usually put member data after member functions. > > > + Matrix transpose() const > > + { > > + return Matrix(m[0], m[3], m[6], m[1], m[4], m[7], m[2], m[5], m[8]); > > You can also write > > return { m[0], m[3], m[6], m[1], m[4], m[7], m[2], m[5], m[8] }; > > which will cause the compiler to warn in case of loss of precision due > to implicit casts. Same for cofactors(). > > > + } > > + Matrix cofactors() const > > + { > > + return Matrix(m[4] * m[8] - m[5] * m[7], > > + -(m[3] * m[8] - m[5] * m[6]), > > + m[3] * m[7] - m[4] * m[6], > > + -(m[1] * m[8] - m[2] * m[7]), > > + m[0] * m[8] - m[2] * m[6], > > + -(m[0] * m[7] - m[1] * m[6]), > > + m[1] * m[5] - m[2] * m[4], > > + -(m[0] * m[5] - m[2] * m[3]), > > + m[0] * m[4] - m[1] * m[3]); > > + } > > + Matrix adjugate() const { return cofactors().transpose(); } > > + float determinant() const > > + { > > + return (m[0] * (m[4] * m[8] - m[5] * m[7]) - > > + m[1] * (m[3] * m[8] - m[5] * m[6]) + > > + m[2] * (m[3] * m[7] - m[4] * m[6])); > > The outer parentheses are not needed. > > > + } > > + Matrix inverse() const { return adjugate() * (1.0 / determinant()); } > > Should we protect against determinant being 0 ? > > > + Matrix operator*(Matrix const &other) const > > + { > > + Matrix result; > > + for (int i = 0; i < 3; i++) > > unsigned int i > > Even though not strictly necessary, we use curly braces when the inner > statement is a compound statement. > > > + for (int j = 0; j < 3; j++) > > unsigned int j > > > + result.m[i * 3 + j] = > > + m[i * 3 + 0] * other.m[0 + j] + > > + m[i * 3 + 1] * other.m[3 + j] + > > + m[i * 3 + 2] * other.m[6 + j]; > > + return result; > > + } > > + Matrix operator*(float const &f) const > > I think "float f" would be fine, is there a need for a reference ? > > > + { > > + Matrix result; > > + for (int i = 0; i < 9; i++) > > unsigned int i > > > + result.m[i] = m[i] * f; > > + return result; > > + } > > +}; > > + > > void packScanlineSBGGR10P(void *output, const void *input, unsigned int width) > > { > > const uint8_t *in = static_cast<const uint8_t *>(input); > > @@ -315,6 +372,42 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > TIFFSetField(tif, TIFFTAG_PLANARCONFIG, PLANARCONFIG_CONTIG); > > TIFFSetField(tif, TIFFTAG_SAMPLEFORMAT, SAMPLEFORMAT_UINT); > > > > + /* > > + * Fill in some reasonable colour information in the DNG. We supply > > + * the "neutral" colour values which determine the white balance, and the > > + * "ColorMatrix1" which converts XYZ to (un-white-balanced) camera RGB. > > + * Note that this is not a "proper" colour calibration for the DNG, > > + * nonetheless, many tools should be able to render the colours better. > > + */ > > + float neutral[3] = { 1, 1, 1 }; > > + Matrix wbGain(1, 1, 1); > > + /* From http://www.brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html */ > > https would be better, but it seems that the server doesn't support it > :-( > > > + Matrix rgb2xyz(0.4124564, 0.3575761, 0.1804375, > > + 0.2126729, 0.7151522, 0.0721750, > > + 0.0193339, 0.1191920, 0.9503041); > > This should be const. > > > + Matrix ccm(1, 1, 1); > > + const double eps = 1e-2; > > Out of curiosity, why 1e-2 ? > > > + > > + if (metadata.contains(controls::ColourGains)) { > > + Span<const float> colour_gains = metadata.get(controls::ColourGains); > > s/colour_gains/colourGains/ or just s/colour_gains/gains/ > > > + if (colour_gains[0] > eps && colour_gains[1] > eps) { > > + wbGain = Matrix(colour_gains[0], 1, colour_gains[1]); > > + neutral[0] = 1.0 / colour_gains[0]; /* red */ > > + neutral[2] = 1.0 / colour_gains[1]; /* blue */ > > + } > > + } > > + if (metadata.contains(controls::ColourCorrectionMatrix)) { > > + Span<const float> m = metadata.get(controls::ColourCorrectionMatrix); > > + Matrix tmp = Matrix(m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8]); > > Would a > > Matrix(Span<const float> m) > > constructor make sense to simplify this line ? Up to you. > > We really try not to use "tmp" as a variable name. Possible alternatives > are "matrix", or just "m" if you rename the existing "m" to "value". > > > + if (tmp.determinant() > eps) > > + ccm = tmp; > > + } > > Please add a blank line here. > > > + /* This is guaranteed to be invertible because all the bits in it are. */ > > I'd expand this a little: > > /* > * This is guaranteed to be invertible because all the bits in it are > * (rgb2xyz is hardcoded and invertible, the ccm determinant is checked > * manually above, and wbGain is a diagonal matrix with diagonal > * elements checked to be non-zero). > */ > > > + Matrix colorMatrix1 = (rgb2xyz * ccm * wbGain).inverse(); > > + > > + TIFFSetField(tif, TIFFTAG_COLORMATRIX1, 9, colorMatrix1.m); > > + TIFFSetField(tif, TIFFTAG_ASSHOTNEUTRAL, 3, neutral); > > + > > /* > > * Reserve space for the SubIFD and ExifIFD tags, pointing to the IFD > > * for the raw image and EXIF data respectively. The real offsets will > > -- > Regards, > > Laurent Pinchart
diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp index 61505d3..222df9f 100644 --- a/src/qcam/dng_writer.cpp +++ b/src/qcam/dng_writer.cpp @@ -34,6 +34,63 @@ struct FormatInfo { unsigned int stride); }; +struct Matrix { + Matrix(float m0, float m1, float m2, + float m3, float m4, float m5, + float m6, float m7, float m8) + { + m[0] = m0, m[1] = m1, m[2] = m2; + m[3] = m3, m[4] = m4, m[5] = m5; + m[6] = m6, m[7] = m7, m[8] = m8; + } + Matrix(float diag0, float diag1, float diag2) + : Matrix(diag0, 0, 0, 0, diag1, 0, 0, 0, diag2) {} + Matrix() {} + float m[9]; + Matrix transpose() const + { + return Matrix(m[0], m[3], m[6], m[1], m[4], m[7], m[2], m[5], m[8]); + } + Matrix cofactors() const + { + return Matrix(m[4] * m[8] - m[5] * m[7], + -(m[3] * m[8] - m[5] * m[6]), + m[3] * m[7] - m[4] * m[6], + -(m[1] * m[8] - m[2] * m[7]), + m[0] * m[8] - m[2] * m[6], + -(m[0] * m[7] - m[1] * m[6]), + m[1] * m[5] - m[2] * m[4], + -(m[0] * m[5] - m[2] * m[3]), + m[0] * m[4] - m[1] * m[3]); + } + Matrix adjugate() const { return cofactors().transpose(); } + float determinant() const + { + return (m[0] * (m[4] * m[8] - m[5] * m[7]) - + m[1] * (m[3] * m[8] - m[5] * m[6]) + + m[2] * (m[3] * m[7] - m[4] * m[6])); + } + Matrix inverse() const { return adjugate() * (1.0 / determinant()); } + Matrix operator*(Matrix const &other) const + { + Matrix result; + for (int i = 0; i < 3; i++) + for (int j = 0; j < 3; j++) + result.m[i * 3 + j] = + m[i * 3 + 0] * other.m[0 + j] + + m[i * 3 + 1] * other.m[3 + j] + + m[i * 3 + 2] * other.m[6 + j]; + return result; + } + Matrix operator*(float const &f) const + { + Matrix result; + for (int i = 0; i < 9; i++) + result.m[i] = m[i] * f; + return result; + } +}; + void packScanlineSBGGR10P(void *output, const void *input, unsigned int width) { const uint8_t *in = static_cast<const uint8_t *>(input); @@ -315,6 +372,42 @@ int DNGWriter::write(const char *filename, const Camera *camera, TIFFSetField(tif, TIFFTAG_PLANARCONFIG, PLANARCONFIG_CONTIG); TIFFSetField(tif, TIFFTAG_SAMPLEFORMAT, SAMPLEFORMAT_UINT); + /* + * Fill in some reasonable colour information in the DNG. We supply + * the "neutral" colour values which determine the white balance, and the + * "ColorMatrix1" which converts XYZ to (un-white-balanced) camera RGB. + * Note that this is not a "proper" colour calibration for the DNG, + * nonetheless, many tools should be able to render the colours better. + */ + float neutral[3] = { 1, 1, 1 }; + Matrix wbGain(1, 1, 1); + /* From http://www.brucelindbloom.com/index.html?Eqn_RGB_XYZ_Matrix.html */ + Matrix rgb2xyz(0.4124564, 0.3575761, 0.1804375, + 0.2126729, 0.7151522, 0.0721750, + 0.0193339, 0.1191920, 0.9503041); + Matrix ccm(1, 1, 1); + const double eps = 1e-2; + + if (metadata.contains(controls::ColourGains)) { + Span<const float> colour_gains = metadata.get(controls::ColourGains); + if (colour_gains[0] > eps && colour_gains[1] > eps) { + wbGain = Matrix(colour_gains[0], 1, colour_gains[1]); + neutral[0] = 1.0 / colour_gains[0]; /* red */ + neutral[2] = 1.0 / colour_gains[1]; /* blue */ + } + } + if (metadata.contains(controls::ColourCorrectionMatrix)) { + Span<const float> m = metadata.get(controls::ColourCorrectionMatrix); + Matrix tmp = Matrix(m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8]); + if (tmp.determinant() > eps) + ccm = tmp; + } + /* This is guaranteed to be invertible because all the bits in it are. */ + Matrix colorMatrix1 = (rgb2xyz * ccm * wbGain).inverse(); + + TIFFSetField(tif, TIFFTAG_COLORMATRIX1, 9, colorMatrix1.m); + TIFFSetField(tif, TIFFTAG_ASSHOTNEUTRAL, 3, neutral); + /* * Reserve space for the SubIFD and ExifIFD tags, pointing to the IFD * for the raw image and EXIF data respectively. The real offsets will
This patch improves the colour information recorded in DNG files using the ColourCorrectionMatrix metadata for the image. Note that we are not supplying a full calibration using two illuminants, nonetheless the single matrix here appears to be respected by a number of tools. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- src/qcam/dng_writer.cpp | 93 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+)