[libcamera-devel] [PATCH 3/3] libcamera: qcam: Improve colour information in DNG files
David Plowman
david.plowman at raspberrypi.com
Fri Jul 24 09:24:47 CEST 2020
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 at 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 at 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
More information about the libcamera-devel
mailing list