[libcamera-devel,v2,2/3] qcam: Add DNGWriter
diff mbox series

Message ID 20200501152745.437777-3-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • qcam: Add RAW capture support
Related show

Commit Message

Niklas Söderlund May 1, 2020, 3:27 p.m. UTC 
Add an initial DNG file writer. The writer can only deal with a small
set of pixelformats. The generated file consumable by standard tools.
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.

Signed-off-by: Niklas Söderlund <niklas.soderlund@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

Comments

Laurent Pinchart May 1, 2020, 5:23 p.m. UTC | #1 
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@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.

> +
> +	/* \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
Niklas Söderlund May 1, 2020, 11:09 p.m. UTC | #2 
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@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

Patch
diff mbox series 

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)
+{
+	uint8_t *in = static_cast<uint8_t *>(input);
+	uint8_t *out = static_cast<uint8_t *>(output);
+
+	/* \todo: Can this be made more efficient? */
+	for (unsigned int i = 0; i < width; i += 4) {
+		*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;
+}
+
+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();
+
+	const FormatInfo *info = getFormatInfo(config.pixelFormat);
+	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;
+	}
+
+	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";
+	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 */
+	for (unsigned int row = 0; row < config.size.height; row++) {
+		uint8_t *start =
+			static_cast<uint8_t *>(data) + config.stride * row;
+
+		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;
+		}
+	}
+
+	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_;
+};
+
+#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)
+
     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],
                        cpp_args : qt5_cpp_args)
 endif