[{"id":4716,"web_url":"https://patchwork.libcamera.org/comment/4716/","msgid":"<20200502021956.GS5951@pendragon.ideasonboard.com>","date":"2020-05-02T02:19:56","subject":"Re: [libcamera-devel] [PATCH v3 2/3] qcam: Add DNGWriter","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Sat, May 02, 2020 at 04:10:44AM +0200, Niklas Söderlund wrote:\n> Add an initial DNG file writer. The writer can only deal with a small\n> set of pixelformats. The generated file is consumable by standard tools.\n\ns/pixelformats/pixel formats/ ?\n\n> The writer needs to be extended to write more metadata to the generated\n> file.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> * Changes since v2\n> - Mark input arg as const\n> - Fold getFormatInfo() into only caller\n> - Male DNGWriter::write() static and take filename as argument\n> - Make DNGWriter::write() take StreamConfiguraiton instead of Stream\n> - Optimize scanline loop\n> - Make libtiff dependecy optional\n> - Add 12 byte formats from Laurent\n> - Reformat table from Laurent\n> ---\n> src/qcam/dng_writer.cpp | 165 ++++++++++++++++++++++++++++++++++++++++\n> src/qcam/dng_writer.h | 24 ++++++\n> src/qcam/meson.build | 21 ++++-\n> 3 files changed, 207 insertions(+), 3 deletions(-)\n> create mode 100644 src/qcam/dng_writer.cpp\n> create mode 100644 src/qcam/dng_writer.h\n> \n> diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> new file mode 100644\n> index 0000000000000000..ed8f276741a954f9\n> --- /dev/null\n> +++ b/src/qcam/dng_writer.cpp\n> @@ -0,0 +1,165 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * dng_writer.cpp - DNG writer\n> + */\n> +\n> +#include \"dng_writer.h\"\n> +\n> +#include \n> +#include \n> +\n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +enum CFAPatternColour : uint8_t {\n> +\tCFAPatternRed = 0,\n> +\tCFAPatternGreen = 1,\n> +\tCFAPatternBlue = 2,\n> +};\n> +\n> +struct FormatInfo {\n> +\tuint8_t bitsPerSample;\n> +\tCFAPatternColour pattern[4];\n> +\tvoid (*packScanline)(void *, const void *, unsigned int);\n> +};\n> +\n> +void packScanlineSBGGR10P(void *output, const void *input, unsigned int width)\n> +{\n> +\tconst uint8_t *in = static_cast(input);\n> +\tuint8_t *out = static_cast(output);\n> +\n> +\t/* \\todo Can this be made more efficient? */\n> +\tfor (unsigned int x = 0; x < width; x += 4) {\n> +\t\t*out++ = in[0];\n> +\t\t*out++ = (in[4] & 0x03) << 6 | in[1] >> 2;\n> +\t\t*out++ = (in[1] & 0x03) << 6 | (in[4] & 0x0c) << 2 | in[2] >> 4;\n> +\t\t*out++ = (in[2] & 0x0f) << 4 | (in[4] & 0x30) >> 2 | in[3] >> 6;\n> +\t\t*out++ = (in[3] & 0x3f) << 2 | (in[4] & 0xc0) >> 6;\n> +\t\tin += 5;\n> +\t}\n> +}\n> +\n> +void packScanlineSBGGR12P(void *output, const void *input, unsigned int width)\n> +{\n> +\tconst uint8_t *in = static_cast(input);\n> +\tuint8_t *out = static_cast(output);\n> +\n> +\t/* \\todo: Can this be made more efficient? */\n> +\tfor (unsigned int i = 0; i < width; i += 2) {\n> +\t\t*out++ = in[0];\n> +\t\t*out++ = (in[2] & 0x0f) << 4 | in[1] >> 4;\n> +\t\t*out++ = (in[1] & 0x0f) << 4 | in[2] >> 4;\n> +\t\tin += 3;\n> +\t}\n> +}\n> +\n> +static const std::map formatInfo = {\n> +\t{ PixelFormat(DRM_FORMAT_SBGGR10, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> +\t\t.bitsPerSample = 10,\n> +\t\t.pattern = { CFAPatternBlue, CFAPatternGreen, CFAPatternGreen, CFAPatternRed },\n> +\t\t.packScanline = packScanlineSBGGR10P,\n> +\t} },\n> +\t{ PixelFormat(DRM_FORMAT_SGBRG10, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> +\t\t.bitsPerSample = 10,\n> +\t\t.pattern = { CFAPatternGreen, CFAPatternBlue, CFAPatternRed, CFAPatternGreen },\n> +\t\tpackScanlineSBGGR10P,\n> +\t} },\n> +\t{ PixelFormat(DRM_FORMAT_SGRBG10, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> +\t\t.bitsPerSample = 10,\n> +\t\t.pattern = { CFAPatternGreen, CFAPatternRed, CFAPatternBlue, CFAPatternGreen },\n> +\t\t.packScanline = packScanlineSBGGR10P,\n> +\t} },\n> +\t{ PixelFormat(DRM_FORMAT_SRGGB10, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> +\t\t.bitsPerSample = 10,\n> +\t\t.pattern = { CFAPatternRed, CFAPatternGreen, CFAPatternGreen, CFAPatternBlue },\n> +\t\t.packScanline = packScanlineSBGGR10P,\n> +\t} },\n> +\t{ PixelFormat(DRM_FORMAT_SBGGR12, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> +\t\t.bitsPerSample = 12,\n> +\t\t.pattern = { CFAPatternBlue, CFAPatternGreen, CFAPatternGreen, CFAPatternRed },\n> +\t\t.packScanline = packScanlineSBGGR12P,\n> +\t} },\n> +\t{ PixelFormat(DRM_FORMAT_SGBRG12, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> +\t\t.bitsPerSample = 12,\n> +\t\t.pattern = { CFAPatternGreen, CFAPatternBlue, CFAPatternRed, CFAPatternGreen },\n> +\t\t.packScanline = packScanlineSBGGR12P,\n> +\t} },\n> +\t{ PixelFormat(DRM_FORMAT_SGRBG12, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> +\t\t.bitsPerSample = 12,\n> +\t\t.pattern = { CFAPatternGreen, CFAPatternRed, CFAPatternBlue, CFAPatternGreen },\n> +\t\t.packScanline = packScanlineSBGGR12P,\n> +\t} },\n> +\t{ PixelFormat(DRM_FORMAT_SRGGB12, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> +\t\t.bitsPerSample = 12,\n> +\t\t.pattern = { CFAPatternRed, CFAPatternGreen, CFAPatternGreen, CFAPatternBlue },\n> +\t\t.packScanline = packScanlineSBGGR12P,\n> +\t} },\n> +};\n> +\n> +int DNGWriter::write(const char *filename, const Camera *camera,\n> +\t\t const StreamConfiguration &config,\n> +\t\t FrameBuffer *buffer, void *data)\n\nThe buffer argument isn't used, should it be removed ?\n\ndata should be const.\n\n> +{\n> +\tconst auto it = formatInfo.find(config.pixelFormat);\n> +\tif (it == formatInfo.cend()) {\n> +\t\tstd::cerr << \"Unsupported pixel format\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\tconst FormatInfo *info = &it->second;\n> +\n> +\tTIFF *tif = TIFFOpen(filename, \"w\");\n> +\tif (!tif) {\n> +\t\tstd::cerr << \"Failed to open tiff file\" << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\tconst uint8_t version[] = { 1, 2, 0, 0 };\n> +\tconst uint16_t cfa_repeatpatterndim[] = { 2, 2 };\n> +\tTIFFSetField(tif, TIFFTAG_DNGVERSION, version);\n> +\tTIFFSetField(tif, TIFFTAG_DNGBACKWARDVERSION, version);\n> +\tTIFFSetField(tif, TIFFTAG_SUBFILETYPE, 0);\n> +\tTIFFSetField(tif, TIFFTAG_IMAGEWIDTH, config.size.width);\n> +\tTIFFSetField(tif, TIFFTAG_IMAGELENGTH, config.size.height);\n> +\tTIFFSetField(tif, TIFFTAG_BITSPERSAMPLE, info->bitsPerSample);\n> +\tTIFFSetField(tif, TIFFTAG_COMPRESSION, COMPRESSION_NONE);\n> +\tTIFFSetField(tif, TIFFTAG_PHOTOMETRIC, PHOTOMETRIC_CFA);\n> +\tTIFFSetField(tif, TIFFTAG_FILLORDER, FILLORDER_MSB2LSB);\n> +\tTIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n> +\tTIFFSetField(tif, TIFFTAG_MODEL, camera->name().c_str());\n> +\tTIFFSetField(tif, TIFFTAG_UNIQUECAMERAMODEL, camera->name().c_str());\n> +\tTIFFSetField(tif, TIFFTAG_ORIENTATION, ORIENTATION_TOPLEFT);\n> +\tTIFFSetField(tif, TIFFTAG_SAMPLESPERPIXEL, 1);\n> +\tTIFFSetField(tif, TIFFTAG_PLANARCONFIG, PLANARCONFIG_CONTIG);\n> +\tTIFFSetField(tif, TIFFTAG_SOFTWARE, \"qcam\");\n> +\tTIFFSetField(tif, TIFFTAG_SAMPLEFORMAT, SAMPLEFORMAT_UINT);\n> +\tTIFFSetField(tif, TIFFTAG_CFAREPEATPATTERNDIM, cfa_repeatpatterndim);\n> +\tTIFFSetField(tif, TIFFTAG_CFAPATTERN, info->pattern);\n> +\tTIFFSetField(tif, TIFFTAG_CFAPLANECOLOR, 3, \"\\00\\01\\02\");\n> +\tTIFFSetField(tif, TIFFTAG_CFALAYOUT, 1);\n> +\n> +\t/* \\todo Add more EXIF fields to output. */\n> +\n> +\t/* Write RAW content */\n\ns/content/content./\n\n> +\tuint8_t *row = static_cast(data);\n> +\tuint8_t scanline[config.size.width * info->bitsPerSample];\n\nDid you mean (config.size.width * info->bitsPerSample + 7) / 8 ?\n\n> +\tfor (unsigned int y = 0; y < config.size.height; y++) {\n> +\t\tinfo->packScanline(&scanline, row, config.size.width);\n> +\n> +\t\tif (TIFFWriteScanline(tif, &scanline, y, 0) != 1) {\n> +\t\t\tstd::cerr << \"Failed to write scanline\" << std::endl;\n> +\t\t\tTIFFClose(tif);\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\trow += config.stride;\n> +\t}\n> +\n> +\tTIFFWriteDirectory(tif);\n> +\n> +\tTIFFClose(tif);\n> +\n> +\treturn 0;\n> +}\n> diff --git a/src/qcam/dng_writer.h b/src/qcam/dng_writer.h\n> new file mode 100644\n> index 0000000000000000..f0e69adb38c6f115\n> --- /dev/null\n> +++ b/src/qcam/dng_writer.h\n> @@ -0,0 +1,24 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> + *\n> + * dng_writer.h - DNG writer\n> + */\n> +#ifndef __LIBCAMERA_DNG_WRITER_H__\n> +#define __LIBCAMERA_DNG_WRITER_H__\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +using namespace libcamera;\n> +\n> +class DNGWriter\n> +{\n> +public:\n> +\tstatic int write(const char *filename, const Camera *camera,\n> +\t\t\t const StreamConfiguration &config, FrameBuffer *buffer,\n> +\t\t\t void *data);\n> +};\n> +\n> +#endif /* __LIBCAMERA_DNG_WRITER_H__ */\n> diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> index 895264be4a3388f4..426462ed5aebe964 100644\n> --- a/src/qcam/meson.build\n> +++ b/src/qcam/meson.build\n> @@ -1,9 +1,9 @@\n> qcam_sources = files([\n> + '../cam/options.cpp',\n> + '../cam/stream_options.cpp',\n> 'format_converter.cpp',\n> 'main.cpp',\n> 'main_window.cpp',\n> - '../cam/options.cpp',\n> - '../cam/stream_options.cpp',\n> 'viewfinder.cpp',\n> ])\n> \n> @@ -23,8 +23,23 @@ qt5_dep = dependency('qt5',\n> required : false)\n> \n> if qt5_dep.found()\n> + qcam_deps = [\n> + libcamera_dep,\n> + qt5_dep,\n> + ]\n> +\n> qt5_cpp_args = [ '-DQT_NO_KEYWORDS' ]\n> \n> + tiff_dep = dependency('libtiff-4', required : false)\n> + if tiff_dep.found()\n> + qt5_cpp_args += [ '-DHAVE_TIFF' ]\n> + qcam_deps += [ tiff_dep ]\n> + qcam_sources += files([\n> + 'dng_writer.cpp',\n> + ])\n> + endif\n> +\n> +\n\nOne blank line is enough.\n\nReviewed-by: Laurent Pinchart \n\n> # gcc 9 introduced a deprecated-copy warning that is triggered by Qt until\n> # Qt 5.13. clang 10 introduced the same warning, but detects more issues\n> # that are not fixed in Qt yet. Disable the warning manually in both cases.\n> @@ -40,6 +55,6 @@ if qt5_dep.found()\n> \n> qcam = executable('qcam', qcam_sources, resources,\n> install : true,\n> - dependencies : [libcamera_dep, qt5_dep],\n> + dependencies : qcam_deps,\n> cpp_args : qt5_cpp_args)\n> endif","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0D7C9603F2\n\tfor ;\n\tSat, 2 May 2020 04:20:00 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-216-236.bb.dnainternet.fi\n\t[81.175.216.236])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5B8CF521;\n\tSat, 2 May 2020 04:19:59 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"gNvXY9BX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1588385999;\n\tbh=jyxvS6TQlJDN2iHlbft5ZlChH3bXRpHKu9KCyenmoS8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gNvXY9BXNqq7WYBsDgVG0TATagDoNxVODyGjlNPaJFpiQndyLa+RRY+Htzbw009o+\n\tAQBNo2UzgNxwMX3JdRE5k9LDGuNPdjPRoPhZ495rjK0U2WrCJYEjrt3P+Jg8VYhY5k\n\t0+ejfV8uoPC9GX64kcC1PBTUz55vWy37e/CEEYLA=","Date":"Sat, 2 May 2020 05:19:56 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200502021956.GS5951@pendragon.ideasonboard.com>","References":"<20200502021045.785979-1-niklas.soderlund@ragnatech.se>\n\t<20200502021045.785979-3-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200502021045.785979-3-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] qcam: Add DNGWriter","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 02 May 2020 02:20:00 -0000"}},{"id":4718,"web_url":"https://patchwork.libcamera.org/comment/4718/","msgid":"<20200502113309.GA2730095@oden.dyn.berto.se>","date":"2020-05-02T11:33:09","subject":"Re: [libcamera-devel] [PATCH v3 2/3] qcam: Add DNGWriter","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2020-05-02 05:19:56 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Sat, May 02, 2020 at 04:10:44AM +0200, Niklas Söderlund wrote:\n> > Add an initial DNG file writer. The writer can only deal with a small\n> > set of pixelformats. The generated file is consumable by standard tools.\n> \n> s/pixelformats/pixel formats/ ?\n> \n> > The writer needs to be extended to write more metadata to the generated\n> > file.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > * Changes since v2\n> > - Mark input arg as const\n> > - Fold getFormatInfo() into only caller\n> > - Male DNGWriter::write() static and take filename as argument\n> > - Make DNGWriter::write() take StreamConfiguraiton instead of Stream\n> > - Optimize scanline loop\n> > - Make libtiff dependecy optional\n> > - Add 12 byte formats from Laurent\n> > - Reformat table from Laurent\n> > ---\n> > src/qcam/dng_writer.cpp | 165 ++++++++++++++++++++++++++++++++++++++++\n> > src/qcam/dng_writer.h | 24 ++++++\n> > src/qcam/meson.build | 21 ++++-\n> > 3 files changed, 207 insertions(+), 3 deletions(-)\n> > create mode 100644 src/qcam/dng_writer.cpp\n> > create mode 100644 src/qcam/dng_writer.h\n> > \n> > diff --git a/src/qcam/dng_writer.cpp b/src/qcam/dng_writer.cpp\n> > new file mode 100644\n> > index 0000000000000000..ed8f276741a954f9\n> > --- /dev/null\n> > +++ b/src/qcam/dng_writer.cpp\n> > @@ -0,0 +1,165 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * dng_writer.cpp - DNG writer\n> > + */\n> > +\n> > +#include \"dng_writer.h\"\n> > +\n> > +#include \n> > +#include \n> > +\n> > +#include \n> > +\n> > +using namespace libcamera;\n> > +\n> > +enum CFAPatternColour : uint8_t {\n> > +\tCFAPatternRed = 0,\n> > +\tCFAPatternGreen = 1,\n> > +\tCFAPatternBlue = 2,\n> > +};\n> > +\n> > +struct FormatInfo {\n> > +\tuint8_t bitsPerSample;\n> > +\tCFAPatternColour pattern[4];\n> > +\tvoid (*packScanline)(void *, const void *, unsigned int);\n> > +};\n> > +\n> > +void packScanlineSBGGR10P(void *output, const void *input, unsigned int width)\n> > +{\n> > +\tconst uint8_t *in = static_cast(input);\n> > +\tuint8_t *out = static_cast(output);\n> > +\n> > +\t/* \\todo Can this be made more efficient? */\n> > +\tfor (unsigned int x = 0; x < width; x += 4) {\n> > +\t\t*out++ = in[0];\n> > +\t\t*out++ = (in[4] & 0x03) << 6 | in[1] >> 2;\n> > +\t\t*out++ = (in[1] & 0x03) << 6 | (in[4] & 0x0c) << 2 | in[2] >> 4;\n> > +\t\t*out++ = (in[2] & 0x0f) << 4 | (in[4] & 0x30) >> 2 | in[3] >> 6;\n> > +\t\t*out++ = (in[3] & 0x3f) << 2 | (in[4] & 0xc0) >> 6;\n> > +\t\tin += 5;\n> > +\t}\n> > +}\n> > +\n> > +void packScanlineSBGGR12P(void *output, const void *input, unsigned int width)\n> > +{\n> > +\tconst uint8_t *in = static_cast(input);\n> > +\tuint8_t *out = static_cast(output);\n> > +\n> > +\t/* \\todo: Can this be made more efficient? */\n> > +\tfor (unsigned int i = 0; i < width; i += 2) {\n> > +\t\t*out++ = in[0];\n> > +\t\t*out++ = (in[2] & 0x0f) << 4 | in[1] >> 4;\n> > +\t\t*out++ = (in[1] & 0x0f) << 4 | in[2] >> 4;\n> > +\t\tin += 3;\n> > +\t}\n> > +}\n> > +\n> > +static const std::map formatInfo = {\n> > +\t{ PixelFormat(DRM_FORMAT_SBGGR10, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> > +\t\t.bitsPerSample = 10,\n> > +\t\t.pattern = { CFAPatternBlue, CFAPatternGreen, CFAPatternGreen, CFAPatternRed },\n> > +\t\t.packScanline = packScanlineSBGGR10P,\n> > +\t} },\n> > +\t{ PixelFormat(DRM_FORMAT_SGBRG10, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> > +\t\t.bitsPerSample = 10,\n> > +\t\t.pattern = { CFAPatternGreen, CFAPatternBlue, CFAPatternRed, CFAPatternGreen },\n> > +\t\tpackScanlineSBGGR10P,\n> > +\t} },\n> > +\t{ PixelFormat(DRM_FORMAT_SGRBG10, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> > +\t\t.bitsPerSample = 10,\n> > +\t\t.pattern = { CFAPatternGreen, CFAPatternRed, CFAPatternBlue, CFAPatternGreen },\n> > +\t\t.packScanline = packScanlineSBGGR10P,\n> > +\t} },\n> > +\t{ PixelFormat(DRM_FORMAT_SRGGB10, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> > +\t\t.bitsPerSample = 10,\n> > +\t\t.pattern = { CFAPatternRed, CFAPatternGreen, CFAPatternGreen, CFAPatternBlue },\n> > +\t\t.packScanline = packScanlineSBGGR10P,\n> > +\t} },\n> > +\t{ PixelFormat(DRM_FORMAT_SBGGR12, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> > +\t\t.bitsPerSample = 12,\n> > +\t\t.pattern = { CFAPatternBlue, CFAPatternGreen, CFAPatternGreen, CFAPatternRed },\n> > +\t\t.packScanline = packScanlineSBGGR12P,\n> > +\t} },\n> > +\t{ PixelFormat(DRM_FORMAT_SGBRG12, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> > +\t\t.bitsPerSample = 12,\n> > +\t\t.pattern = { CFAPatternGreen, CFAPatternBlue, CFAPatternRed, CFAPatternGreen },\n> > +\t\t.packScanline = packScanlineSBGGR12P,\n> > +\t} },\n> > +\t{ PixelFormat(DRM_FORMAT_SGRBG12, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> > +\t\t.bitsPerSample = 12,\n> > +\t\t.pattern = { CFAPatternGreen, CFAPatternRed, CFAPatternBlue, CFAPatternGreen },\n> > +\t\t.packScanline = packScanlineSBGGR12P,\n> > +\t} },\n> > +\t{ PixelFormat(DRM_FORMAT_SRGGB12, MIPI_FORMAT_MOD_CSI2_PACKED), {\n> > +\t\t.bitsPerSample = 12,\n> > +\t\t.pattern = { CFAPatternRed, CFAPatternGreen, CFAPatternGreen, CFAPatternBlue },\n> > +\t\t.packScanline = packScanlineSBGGR12P,\n> > +\t} },\n> > +};\n> > +\n> > +int DNGWriter::write(const char *filename, const Camera *camera,\n> > +\t\t const StreamConfiguration &config,\n> > +\t\t FrameBuffer *buffer, void *data)\n> \n> The buffer argument isn't used, should it be removed ?\n\nNo I think it should be kept as we are going to extract the metadata to \nput in the DNG file from the buffer's metadata.\n\n> \n> data should be const.\n\nIt should, I also made the buffer const.\n\n> \n> > +{\n> > +\tconst auto it = formatInfo.find(config.pixelFormat);\n> > +\tif (it == formatInfo.cend()) {\n> > +\t\tstd::cerr << \"Unsupported pixel format\" << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\tconst FormatInfo *info = &it->second;\n> > +\n> > +\tTIFF *tif = TIFFOpen(filename, \"w\");\n> > +\tif (!tif) {\n> > +\t\tstd::cerr << \"Failed to open tiff file\" << std::endl;\n> > +\t\treturn -EINVAL;\n> > +\t}\n> > +\n> > +\tconst uint8_t version[] = { 1, 2, 0, 0 };\n> > +\tconst uint16_t cfa_repeatpatterndim[] = { 2, 2 };\n> > +\tTIFFSetField(tif, TIFFTAG_DNGVERSION, version);\n> > +\tTIFFSetField(tif, TIFFTAG_DNGBACKWARDVERSION, version);\n> > +\tTIFFSetField(tif, TIFFTAG_SUBFILETYPE, 0);\n> > +\tTIFFSetField(tif, TIFFTAG_IMAGEWIDTH, config.size.width);\n> > +\tTIFFSetField(tif, TIFFTAG_IMAGELENGTH, config.size.height);\n> > +\tTIFFSetField(tif, TIFFTAG_BITSPERSAMPLE, info->bitsPerSample);\n> > +\tTIFFSetField(tif, TIFFTAG_COMPRESSION, COMPRESSION_NONE);\n> > +\tTIFFSetField(tif, TIFFTAG_PHOTOMETRIC, PHOTOMETRIC_CFA);\n> > +\tTIFFSetField(tif, TIFFTAG_FILLORDER, FILLORDER_MSB2LSB);\n> > +\tTIFFSetField(tif, TIFFTAG_MAKE, \"libcamera\");\n> > +\tTIFFSetField(tif, TIFFTAG_MODEL, camera->name().c_str());\n> > +\tTIFFSetField(tif, TIFFTAG_UNIQUECAMERAMODEL, camera->name().c_str());\n> > +\tTIFFSetField(tif, TIFFTAG_ORIENTATION, ORIENTATION_TOPLEFT);\n> > +\tTIFFSetField(tif, TIFFTAG_SAMPLESPERPIXEL, 1);\n> > +\tTIFFSetField(tif, TIFFTAG_PLANARCONFIG, PLANARCONFIG_CONTIG);\n> > +\tTIFFSetField(tif, TIFFTAG_SOFTWARE, \"qcam\");\n> > +\tTIFFSetField(tif, TIFFTAG_SAMPLEFORMAT, SAMPLEFORMAT_UINT);\n> > +\tTIFFSetField(tif, TIFFTAG_CFAREPEATPATTERNDIM, cfa_repeatpatterndim);\n> > +\tTIFFSetField(tif, TIFFTAG_CFAPATTERN, info->pattern);\n> > +\tTIFFSetField(tif, TIFFTAG_CFAPLANECOLOR, 3, \"\\00\\01\\02\");\n> > +\tTIFFSetField(tif, TIFFTAG_CFALAYOUT, 1);\n> > +\n> > +\t/* \\todo Add more EXIF fields to output. */\n> > +\n> > +\t/* Write RAW content */\n> \n> s/content/content./\n> \n> > +\tuint8_t *row = static_cast(data);\n> > +\tuint8_t scanline[config.size.width * info->bitsPerSample];\n> \n> Did you mean (config.size.width * info->bitsPerSample + 7) / 8 ?\n\nGood catch, thanks.\n\n> \n> > +\tfor (unsigned int y = 0; y < config.size.height; y++) {\n> > +\t\tinfo->packScanline(&scanline, row, config.size.width);\n> > +\n> > +\t\tif (TIFFWriteScanline(tif, &scanline, y, 0) != 1) {\n> > +\t\t\tstd::cerr << \"Failed to write scanline\" << std::endl;\n> > +\t\t\tTIFFClose(tif);\n> > +\t\t\treturn -EINVAL;\n> > +\t\t}\n> > +\n> > +\t\trow += config.stride;\n> > +\t}\n> > +\n> > +\tTIFFWriteDirectory(tif);\n> > +\n> > +\tTIFFClose(tif);\n> > +\n> > +\treturn 0;\n> > +}\n> > diff --git a/src/qcam/dng_writer.h b/src/qcam/dng_writer.h\n> > new file mode 100644\n> > index 0000000000000000..f0e69adb38c6f115\n> > --- /dev/null\n> > +++ b/src/qcam/dng_writer.h\n> > @@ -0,0 +1,24 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2020, Raspberry Pi (Trading) Ltd.\n> > + *\n> > + * dng_writer.h - DNG writer\n> > + */\n> > +#ifndef __LIBCAMERA_DNG_WRITER_H__\n> > +#define __LIBCAMERA_DNG_WRITER_H__\n> > +\n> > +#include \n> > +#include \n> > +#include \n> > +\n> > +using namespace libcamera;\n> > +\n> > +class DNGWriter\n> > +{\n> > +public:\n> > +\tstatic int write(const char *filename, const Camera *camera,\n> > +\t\t\t const StreamConfiguration &config, FrameBuffer *buffer,\n> > +\t\t\t void *data);\n> > +};\n> > +\n> > +#endif /* __LIBCAMERA_DNG_WRITER_H__ */\n> > diff --git a/src/qcam/meson.build b/src/qcam/meson.build\n> > index 895264be4a3388f4..426462ed5aebe964 100644\n> > --- a/src/qcam/meson.build\n> > +++ b/src/qcam/meson.build\n> > @@ -1,9 +1,9 @@\n> > qcam_sources = files([\n> > + '../cam/options.cpp',\n> > + '../cam/stream_options.cpp',\n> > 'format_converter.cpp',\n> > 'main.cpp',\n> > 'main_window.cpp',\n> > - '../cam/options.cpp',\n> > - '../cam/stream_options.cpp',\n> > 'viewfinder.cpp',\n> > ])\n> > \n> > @@ -23,8 +23,23 @@ qt5_dep = dependency('qt5',\n> > required : false)\n> > \n> > if qt5_dep.found()\n> > + qcam_deps = [\n> > + libcamera_dep,\n> > + qt5_dep,\n> > + ]\n> > +\n> > qt5_cpp_args = [ '-DQT_NO_KEYWORDS' ]\n> > \n> > + tiff_dep = dependency('libtiff-4', required : false)\n> > + if tiff_dep.found()\n> > + qt5_cpp_args += [ '-DHAVE_TIFF' ]\n> > + qcam_deps += [ tiff_dep ]\n> > + qcam_sources += files([\n> > + 'dng_writer.cpp',\n> > + ])\n> > + endif\n> > +\n> > +\n> \n> One blank line is enough.\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > # gcc 9 introduced a deprecated-copy warning that is triggered by Qt until\n> > # Qt 5.13. clang 10 introduced the same warning, but detects more issues\n> > # that are not fixed in Qt yet. Disable the warning manually in both cases.\n> > @@ -40,6 +55,6 @@ if qt5_dep.found()\n> > \n> > qcam = executable('qcam', qcam_sources, resources,\n> > install : true,\n> > - dependencies : [libcamera_dep, qt5_dep],\n> > + dependencies : qcam_deps,\n> > cpp_args : qt5_cpp_args)\n> > endif\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lf1-x143.google.com (mail-lf1-x143.google.com\n\t[IPv6:2a00:1450:4864:20::143])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 83EE4603F4\n\tfor ;\n\tSat, 2 May 2020 13:33:12 +0200 (CEST)","by mail-lf1-x143.google.com with SMTP id s9so591332lfp.1\n\tfor ;\n\tSat, 02 May 2020 04:33:12 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tp2sm3749413lji.40.2020.05.02.04.33.10\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 02 May 2020 04:33:10 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com header.b=\"UJxN9e50\"; \n\tdkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=WNXO1lPOcz/WKpGqi58vRSCn8fex3JeJI9Yfl43hTog=;\n\tb=UJxN9e50CTGH6jFQpzMaQYR/9Bvu1jOm+2PwWJCs8il0QL135sbv5dUk6AZC5qcHFD\n\tqebYA3ei4y65gqRiR4yXtuz7Nxj348FgC6pbcA7esOM/8kvulCA1Ea/kva0BE04f7e74\n\tAHRZoWjQljUoa/oD8ZSw98jR7JcDUNglymFhB5If6OEkTvRqEUJj1ZMsBlmi7zhaHVxR\n\tQa6HNq674cNzlwbxXQDaBaH6d5qmOOx1gblg/AILJz9wPzU50FPPgNToEhpnGZkRx7ts\n\tiYvoVoDQa6iamqYqwZ7Vj04swYYI6Sk+++mPB93NopRhjS66pF6xRXiZ6eb6KaybTzqw\n\tPfKA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=WNXO1lPOcz/WKpGqi58vRSCn8fex3JeJI9Yfl43hTog=;\n\tb=NUrXPckJju/y0b6PnwfTosMBfi+hx+oeYc6Uy3oRlQFG/UkHnJkYCglZ/MACvo7g6o\n\tpQDJv3KlIm3Qp+wB0ZXeBGZupqCWfajpuhnRcAB5yGqEEydvwDfqFuIzjfhKvMlT9Y08\n\tTbmDExnk2pZAaBENll8/Ii4KyNoMTuXdZgBmxZeu559MfVjXxrMLTieUyXposUYQZJH7\n\t1wcC6toA3uhggwp26qb4koW3cCYdpGZb5FT231asT7hvlFs4gmCS9Kfs3e8p71uwcA2D\n\tnzqEuw+sg4aIYZj0vv9fVb8xSsRqeXG07HrM3uwUmqvvqqBpHBqK0nUhjxbCLmVmdmdo\n\toW2A==","X-Gm-Message-State":"AGi0PubGztfXTDyc2YPa1RyWBjzmkxiaAadhYrUZr75hv63gshXt9jYV\n\t1/kgTTOpeZtsZM77zPmOV01piuJg6iM=","X-Google-Smtp-Source":"APiQypKdQ+Pr6anTRsGapd+PMtFj+qm6JMojA5fRWDPDDtRsNWN3uuNTYbPB1phhFLhrNP75Fz0jeQ==","X-Received":"by 2002:a19:8809:: with SMTP id k9mr5337861lfd.151.1588419191541;\n\tSat, 02 May 2020 04:33:11 -0700 (PDT)","Date":"Sat, 2 May 2020 13:33:09 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20200502113309.GA2730095@oden.dyn.berto.se>","References":"<20200502021045.785979-1-niklas.soderlund@ragnatech.se>\n\t<20200502021045.785979-3-niklas.soderlund@ragnatech.se>\n\t<20200502021956.GS5951@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20200502021956.GS5951@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/3] qcam: Add DNGWriter","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 02 May 2020 11:33:12 -0000"}}]