[libcamera-devel] [PATCH v2 2/3] qcam: Add DNGWriter

Niklas Söderlund niklas.soderlund at ragnatech.se
Sat May 2 01:09:48 CEST 2020


Hi Laurent,

Thanks for your feedback. I have taken all comments except the one I 
disagree with and comment on bellow :-)

On 2020-05-01 20:23:30 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Fri, May 01, 2020 at 05:27:44PM +0200, Niklas Söderlund wrote:
> > Add an initial DNG file writer. The writer can only deal with a small
> > set of pixelformats. The generated file consumable by standard tools.
> 
> s/consumable/is consumable/ ?
> 
> > The writer needs to be extended to write more metadata to the generated
> > file.
> > 
> > This change also makes libtiff-4 mandatory for qcam. In the future it
> > could be made an optional dependency and only enable DNG support if it's
> > available.
> 
> I think it would be really nice to keep this optional. It shouldn't be
> difficult, and I can give it a go if you need help.
> 
> Edit: See below :-)
> 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/qcam/dng_writer.cpp | 152 ++++++++++++++++++++++++++++++++++++++++
> >  src/qcam/dng_writer.h   |  32 +++++++++
> >  src/qcam/meson.build    |   9 ++-
> >  3 files changed, 190 insertions(+), 3 deletions(-)
> >  create mode 100644 src/qcam/dng_writer.cpp
> >  create mode 100644 src/qcam/dng_writer.h
> > 
> > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp
> > new file mode 100644
> > index 0000000000000000..b58a6b9938992ed0
> > --- /dev/null
> > +++ b/src/qcam/dng_writer.cpp
> > @@ -0,0 +1,152 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> > + *
> > + * dng_writer.cpp - DNG writer
> > + */
> > +
> > +#include "dng_writer.h"
> > +
> > +#include <iomanip>
> > +#include <iostream>
> > +#include <map>
> > +#include <sstream>
> > +
> > +#include <tiffio.h>
> > +
> > +using namespace libcamera;
> > +
> > +struct FormatInfo {
> > +	uint8_t bitsPerSample;
> > +	uint8_t pattern[4]; /* R = 0, G = 1, B = 2 */
> > +	void (*packScanline)(void *, void *, unsigned int);
> > +};
> > +
> > +void packScanlineSBGGR10P(void *output, void *input, unsigned int width)
> 
> input should be a const pointer.
> 
> > +{
> > +	uint8_t *in = static_cast<uint8_t *>(input);
> > +	uint8_t *out = static_cast<uint8_t *>(output);
> 
> How about passing uint8_t * to the function ? Up to you.

I like to keep pass void * as I hope we will have more pack functions 
added in the future and would like to not impose an interpretation of 
the data from the arguments.

> 
> > +
> > +	/* \todo: Can this be made more efficient? */
> 
> s/todo:/todo/
> 
> And I'm sure it can, using SIMD for instance. Clearly something for
> later.
> 
> > +	for (unsigned int i = 0; i < width; i += 4) {
> 
> s/i/x/ ?
> 
> > +		*out++ = in[0];
> > +		*out++ = (in[4] & 0x03) << 6 | in[1] >> 2;
> > +		*out++ = (in[1] & 0x03) << 6 | (in[4] & 0x0c) << 2 | in[2] >> 4;
> > +		*out++ = (in[2] & 0x0f) << 4 | (in[4] & 0x30) >> 2 | in[3] >> 6;
> > +		*out++ = (in[3] & 0x3f) << 2 | (in[4] & 0xc0) >> 6;
> > +		in += 5;
> > +	}
> > +}
> > +
> > +static const std::map<PixelFormat, FormatInfo> formatInfo = {
> > +	{ PixelFormat(DRM_FORMAT_SBGGR10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 2, 1, 1, 0 }, packScanlineSBGGR10P } },
> > +	{ PixelFormat(DRM_FORMAT_SGBRG10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 1, 2, 0, 1 }, packScanlineSBGGR10P } },
> > +	{ PixelFormat(DRM_FORMAT_SGRBG10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 1, 0, 2, 1 }, packScanlineSBGGR10P } },
> > +	{ PixelFormat(DRM_FORMAT_SRGGB10, MIPI_FORMAT_MOD_CSI2_PACKED), { 10, { 0, 1, 1, 2 }, packScanlineSBGGR10P } },
> > +};
> > +
> > +const FormatInfo *getFormatInfo(const PixelFormat &format)
> > +{
> > +	const auto it = formatInfo.find(format);
> > +	if (it == formatInfo.cend())
> > +		return nullptr;
> > +
> > +	return &it->second;
> > +
> > +	return nullptr;
> 
> The last two lines can be removed.
> 
> > +}
> > +
> > +DNGWriter::DNGWriter(const std::string &pattern)
> > +	: pattern_(pattern)
> > +{
> > +}
> > +
> > +int DNGWriter::write(const Camera *camera, const Stream *stream,
> > +		     FrameBuffer *buffer, void *data)
> > +{
> > +	const StreamConfiguration &config = stream->configuration();
> 
> I wonder if the function shouldn't take the stream configuration instead
> of the stream.
> 
> > +
> > +	const FormatInfo *info = getFormatInfo(config.pixelFormat);
> 
> As you use this function in a single place and the implementation is
> very small, I would just inline it here.
> 
> > +	if (!info) {
> > +		std::cerr << "Unsupported pixel format" << std::endl;
> > +		return -EINVAL;
> > +	}
> > +
> > +	uint8_t *scanline = new uint8_t[config.size.width * info->bitsPerSample];
> > +	if (!scanline) {
> > +		std::cerr << "Can't allocate memory for scanline" << std::endl;
> > +		return -ENOMEM;
> > +	}
> 
> Does this really need to be allocated on the heap, can you allocate it
> on the stack ?
> 
> 	uint8_t scanline[config.size.width * info->bitsPerSample];
> 
> > +
> > +	std::string filename = genFilename(buffer->metadata());
> > +
> > +	TIFF *tif = TIFFOpen(filename.c_str(), "w");
> > +	if (!tif) {
> > +		std::cerr << "Failed to open tiff file" << std::endl;
> > +		delete[] scanline;
> > +		return -EINVAL;
> > +	}
> > +
> > +	const uint8_t version[] = "\01\02\00\00";
> 
> Would this read better as
> 
> 	const uint8_t version[] = { 1, 2, 0, 0 };
> 
> ? Or does TIFFSetField expect the version to be a null-terminated "binary
> string" ("\01\02\00\00" is really stored as { 1, 2, 0, 0, 0 } in memory) ?
> 
> > +	const uint16_t cfa_repeatpatterndim[] = { 2, 2 };
> > +	TIFFSetField(tif, TIFFTAG_DNGVERSION, version);
> > +	TIFFSetField(tif, TIFFTAG_DNGBACKWARDVERSION, version);
> > +	TIFFSetField(tif, TIFFTAG_SUBFILETYPE, 0);
> > +	TIFFSetField(tif, TIFFTAG_IMAGEWIDTH, config.size.width);
> > +	TIFFSetField(tif, TIFFTAG_IMAGELENGTH, config.size.height);
> > +	TIFFSetField(tif, TIFFTAG_BITSPERSAMPLE, info->bitsPerSample);
> > +	TIFFSetField(tif, TIFFTAG_COMPRESSION, COMPRESSION_NONE);
> > +	TIFFSetField(tif, TIFFTAG_PHOTOMETRIC, PHOTOMETRIC_CFA);
> > +	TIFFSetField(tif, TIFFTAG_FILLORDER, FILLORDER_MSB2LSB);
> > +	TIFFSetField(tif, TIFFTAG_MAKE, "libcamera");
> > +	TIFFSetField(tif, TIFFTAG_MODEL, camera->name().c_str());
> > +	TIFFSetField(tif, TIFFTAG_UNIQUECAMERAMODEL, camera->name().c_str());
> > +	TIFFSetField(tif, TIFFTAG_ORIENTATION, ORIENTATION_TOPLEFT);
> > +	TIFFSetField(tif, TIFFTAG_SAMPLESPERPIXEL, 1);
> > +	TIFFSetField(tif, TIFFTAG_PLANARCONFIG, PLANARCONFIG_CONTIG);
> > +	TIFFSetField(tif, TIFFTAG_SOFTWARE, "qcam");
> > +	TIFFSetField(tif, TIFFTAG_SAMPLEFORMAT, SAMPLEFORMAT_UINT);
> > +	TIFFSetField(tif, TIFFTAG_CFAREPEATPATTERNDIM, cfa_repeatpatterndim);
> > +	TIFFSetField(tif, TIFFTAG_CFAPATTERN, info->pattern);
> > +	TIFFSetField(tif, TIFFTAG_CFAPLANECOLOR, 3, "\00\01\02");
> > +	TIFFSetField(tif, TIFFTAG_CFALAYOUT, 1);
> > +
> > +	/* \todo: Add more EXIF fields to output. */
> > +
> > +	/* Write RAW content */
> 
> Would this be more efficient ?
> 
> 	uint8_t *row = static_cast<uint8_t *>(data);
> 
> > +	for (unsigned int row = 0; row < config.size.height; row++) {
> 
> With s/row/y/ here.
> 
> > +		uint8_t *start =
> > +			static_cast<uint8_t *>(data) + config.stride * row;
> 
> Delete this line.
> 
> > +
> > +		info->packScanline(scanline, start, config.size.width);
> > +
> > +		if (TIFFWriteScanline(tif, scanline, row, 0) != 1) {
> > +			std::cerr << "Failed to write scanline" << std::endl;
> > +			TIFFClose(tif);
> > +			delete[] scanline;
> > +			return -EINVAL;
> > +		}
> 
> 		row += config.stride;
> 
> > +	}
> > +
> > +	TIFFWriteDirectory(tif);
> > +
> > +	TIFFClose(tif);
> > +
> > +	delete[] scanline;
> > +
> > +	return 0;
> > +}
> > +
> > +std::string DNGWriter::genFilename(const FrameMetadata &metadata)
> > +{
> > +	std::string filename = pattern_;
> > +
> > +	size_t pos = filename.find_first_of('#');
> > +	if (pos != std::string::npos) {
> > +		std::stringstream ss;
> > +		ss << std::setw(6) << std::setfill('0') << metadata.sequence;
> > +		filename.replace(pos, 1, ss.str());
> > +	}
> > +
> > +	return filename;
> > +}
> > diff --git a/src/qcam/dng_writer.h b/src/qcam/dng_writer.h
> > new file mode 100644
> > index 0000000000000000..c7ccbffd5db69dbe
> > --- /dev/null
> > +++ b/src/qcam/dng_writer.h
> > @@ -0,0 +1,32 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.
> > + *
> > + * dng_writer.h - DNG writer
> > + */
> > +#ifndef __LIBCAMERA_DNG_WRITER_H__
> > +#define __LIBCAMERA_DNG_WRITER_H__
> > +
> > +#include <string>
> > +
> > +#include <libcamera/buffer.h>
> > +#include <libcamera/camera.h>
> > +#include <libcamera/stream.h>
> > +
> > +using namespace libcamera;
> > +
> > +class DNGWriter
> > +{
> > +public:
> > +	DNGWriter(const std::string &pattern = "frame-#.dng");
> > +
> > +	int write(const Camera *camera, const Stream *stream,
> > +		  FrameBuffer *buffer, void *data);
> > +
> > +private:
> > +	std::string genFilename(const FrameMetadata &metadata);
> > +
> > +	std::string pattern_;
> 
> I'd prefer keeping file name handling in the caller, as it's not
> specific to DNG, and the need is shared with JPG capture. I know there's
> a writer class in cam that handles file names, but I think it covers a
> different case, as it saves all frames. Furthermore, if we want to allow
> selection of a file name by the user (and I think it's useful, as I
> expect we'll want to save files with distinct names that will describe
> the capture environment for tuning purpose for instance), we have to
> pass the file name to the write() function.
> 
> The write() function could then become static, and you won't have to
> instantiate this class, you could just call DNGWrite::write(...).
> 
> > +};
> > +
> > +#endif /* __LIBCAMERA_DNG_WRITER_H__ */
> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build
> > index 895264be4a3388f4..8b9a35904facbce0 100644
> > --- a/src/qcam/meson.build
> > +++ b/src/qcam/meson.build
> > @@ -1,9 +1,10 @@
> >  qcam_sources = files([
> > +    '../cam/options.cpp',
> > +    '../cam/stream_options.cpp',
> > +    'dng_writer.cpp',
> >      'format_converter.cpp',
> >      'main.cpp',
> >      'main_window.cpp',
> > -    '../cam/options.cpp',
> > -    '../cam/stream_options.cpp',
> >      'viewfinder.cpp',
> >  ])
> >  
> > @@ -23,6 +24,8 @@ qt5_dep = dependency('qt5',
> >                       required : false)
> >  
> >  if qt5_dep.found()
> > +    tiff_dep = dependency('libtiff-4', required : true)
> > +
> 
>     qcam_deps = [
>         libcamera_dep,
>         qt5_dep,
>     ]
> 
>     tiff_dep = dependency('libtiff-4', required : false)
>     if tiff_dep.found()
>         qt5_cpp_args += [ '-DHAVE_TIFF' ]
>         qcam_deps += [ tiff_dep ]
>         qcam_sources += files([
>             'dng_writer.cpp',
>         ])
>     endif
> 
> >      qt5_cpp_args = [ '-DQT_NO_KEYWORDS' ]
> >  
> >      # gcc 9 introduced a deprecated-copy warning that is triggered by Qt until
> > @@ -40,6 +43,6 @@ if qt5_dep.found()
> >  
> >      qcam  = executable('qcam', qcam_sources, resources,
> >                         install : true,
> > -                       dependencies : [libcamera_dep, qt5_dep],
> > +                       dependencies : [libcamera_dep, qt5_dep, tiff_dep],
> 
>                        dependencies : qcam_deps,
> 
> >                         cpp_args : qt5_cpp_args)
> >  endif
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund


More information about the libcamera-devel mailing list