| Message ID | 20240625080119.1905870-1-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2024-06-25 09:01:11) > In libtiff version 4.5.1 and later the CFA* tags were missing. This got > fixed in https://gitlab.com/libtiff/libtiff/-/commit/49856998c3d82e65444b47bb4fb11b7830a0c2be > Unfortunately the fix is not released yet, but the faulty libtiff is > contained in current buildroot. As a local fix is pretty easy and > without side effects, let's workaround that. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/apps/common/dng_writer.cpp | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > index 103e5c828c4d..7375577e920f 100644 > --- a/src/apps/common/dng_writer.cpp > +++ b/src/apps/common/dng_writer.cpp > @@ -541,6 +541,23 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > TIFFWriteDirectory(tif); > > + /* > + * Workaround for a bug introduced in libtiff version 4.5.1 and no fix > + * released. In these versions the CFA* tags were missing in the field > + * info. > + * Introduced by: https://gitlab.com/libtiff/libtiff/-/commit/738e04099b13192bb1f654e74e9b5829313f3161 > + * Fixed by: https://gitlab.com/libtiff/libtiff/-/commit/49856998c3d82e65444b47bb4fb11b7830a0c2be > + */ > + if (!TIFFFindField(tif, TIFFTAG_CFAREPEATPATTERNDIM, TIFF_ANY)) { > + static const TIFFFieldInfo infos[] = { > + { TIFFTAG_EP_CFAREPEATPATTERNDIM, 2, 2, TIFF_SHORT, FIELD_CUSTOM, > + 1, 0, const_cast<char *>("EP CFARepeatPatternDim") }, > + { TIFFTAG_EP_CFAPATTERN, -1, -1, TIFF_BYTE, FIELD_CUSTOM, > + 1, 1, const_cast<char *>("EP CFAPattern") }, > + }; As a workaround it sounds fine, but shouldn't there be something here which writes out the pattern based on the metadata/properties of the camera/raw stream? Or are these always fixed data entries? Aha, actually I think I see this is preparing the fields so that it can be set below! Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + TIFFMergeFieldInfo(tif, infos, 2); > + } > + > /* Create a new IFD for the RAW image. */ > const uint16_t cfaRepeatPatternDim[] = { 2, 2 }; > const uint8_t cfaPlaneColor[] = { > -- > 2.43.0 >
Hi Stefan, Thank you for the patch. On Tue, Jun 25, 2024 at 10:01:11AM +0200, Stefan Klug wrote: > In libtiff version 4.5.1 and later the CFA* tags were missing. This got > fixed in https://gitlab.com/libtiff/libtiff/-/commit/49856998c3d82e65444b47bb4fb11b7830a0c2be > Unfortunately the fix is not released yet, but the faulty libtiff is > contained in current buildroot. As a local fix is pretty easy and > without side effects, let's workaround that. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/apps/common/dng_writer.cpp | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > index 103e5c828c4d..7375577e920f 100644 > --- a/src/apps/common/dng_writer.cpp > +++ b/src/apps/common/dng_writer.cpp > @@ -541,6 +541,23 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > TIFFWriteDirectory(tif); > > + /* > + * Workaround for a bug introduced in libtiff version 4.5.1 and no fix > + * released. In these versions the CFA* tags were missing in the field > + * info. > + * Introduced by: https://gitlab.com/libtiff/libtiff/-/commit/738e04099b13192bb1f654e74e9b5829313f3161 > + * Fixed by: https://gitlab.com/libtiff/libtiff/-/commit/49856998c3d82e65444b47bb4fb11b7830a0c2be > + */ > + if (!TIFFFindField(tif, TIFFTAG_CFAREPEATPATTERNDIM, TIFF_ANY)) { > + static const TIFFFieldInfo infos[] = { > + { TIFFTAG_EP_CFAREPEATPATTERNDIM, 2, 2, TIFF_SHORT, FIELD_CUSTOM, > + 1, 0, const_cast<char *>("EP CFARepeatPatternDim") }, > + { TIFFTAG_EP_CFAPATTERN, -1, -1, TIFF_BYTE, FIELD_CUSTOM, > + 1, 1, const_cast<char *>("EP CFAPattern") }, This will fail to compile with libtiff versions older than v4.5.1, where the bug was introduced. TIFFTAG_CFAREPEATPATTERNDIM and TIFFTAG_CFAPATTERN became aliases for TIFFTAG_EP_CFAREPEATPATTERNDIM and TIFFTAG_EP_CFAPATTERN in that version, so I think you can switch to TIFFTAG_CFAREPEATPATTERNDIM and TIFFTAG_CFAPATTERN here. I would also change the description string to drop the "EP " prefix. With that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + }; > + TIFFMergeFieldInfo(tif, infos, 2); > + } > + > /* Create a new IFD for the RAW image. */ > const uint16_t cfaRepeatPatternDim[] = { 2, 2 }; > const uint8_t cfaPlaneColor[] = {
Quoting Laurent Pinchart (2024-06-25 09:43:48) > Hi Stefan, > > Thank you for the patch. > > On Tue, Jun 25, 2024 at 10:01:11AM +0200, Stefan Klug wrote: > > In libtiff version 4.5.1 and later the CFA* tags were missing. This got > > fixed in https://gitlab.com/libtiff/libtiff/-/commit/49856998c3d82e65444b47bb4fb11b7830a0c2be > > Unfortunately the fix is not released yet, but the faulty libtiff is > > contained in current buildroot. As a local fix is pretty easy and > > without side effects, let's workaround that. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/apps/common/dng_writer.cpp | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > > index 103e5c828c4d..7375577e920f 100644 > > --- a/src/apps/common/dng_writer.cpp > > +++ b/src/apps/common/dng_writer.cpp > > @@ -541,6 +541,23 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > > > TIFFWriteDirectory(tif); > > > > + /* > > + * Workaround for a bug introduced in libtiff version 4.5.1 and no fix > > + * released. In these versions the CFA* tags were missing in the field > > + * info. > > + * Introduced by: https://gitlab.com/libtiff/libtiff/-/commit/738e04099b13192bb1f654e74e9b5829313f3161 > > + * Fixed by: https://gitlab.com/libtiff/libtiff/-/commit/49856998c3d82e65444b47bb4fb11b7830a0c2be > > + */ > > + if (!TIFFFindField(tif, TIFFTAG_CFAREPEATPATTERNDIM, TIFF_ANY)) { > > + static const TIFFFieldInfo infos[] = { > > + { TIFFTAG_EP_CFAREPEATPATTERNDIM, 2, 2, TIFF_SHORT, FIELD_CUSTOM, > > + 1, 0, const_cast<char *>("EP CFARepeatPatternDim") }, > > + { TIFFTAG_EP_CFAPATTERN, -1, -1, TIFF_BYTE, FIELD_CUSTOM, > > + 1, 1, const_cast<char *>("EP CFAPattern") }, > > This will fail to compile with libtiff versions older than v4.5.1, where > the bug was introduced. TIFFTAG_CFAREPEATPATTERNDIM and > TIFFTAG_CFAPATTERN became aliases for TIFFTAG_EP_CFAREPEATPATTERNDIM and > TIFFTAG_EP_CFAPATTERN in that version, so I think you can switch to > TIFFTAG_CFAREPEATPATTERNDIM and TIFFTAG_CFAPATTERN here. I would also > change the description string to drop the "EP " prefix. https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1209546 shows failures. It could be the above - or something else, either way needs to be checked. -- Kieran > > With that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + }; > > + TIFFMergeFieldInfo(tif, infos, 2); > > + } > > + > > /* Create a new IFD for the RAW image. */ > > const uint16_t cfaRepeatPatternDim[] = { 2, 2 }; > > const uint8_t cfaPlaneColor[] = { > > -- > Regards, > > Laurent Pinchart
Hi Laurent, On Tue, Jun 25, 2024 at 11:43:48AM +0300, Laurent Pinchart wrote: > Hi Stefan, > > Thank you for the patch. > > On Tue, Jun 25, 2024 at 10:01:11AM +0200, Stefan Klug wrote: > > In libtiff version 4.5.1 and later the CFA* tags were missing. This got > > fixed in https://gitlab.com/libtiff/libtiff/-/commit/49856998c3d82e65444b47bb4fb11b7830a0c2be > > Unfortunately the fix is not released yet, but the faulty libtiff is > > contained in current buildroot. As a local fix is pretty easy and > > without side effects, let's workaround that. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/apps/common/dng_writer.cpp | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp > > index 103e5c828c4d..7375577e920f 100644 > > --- a/src/apps/common/dng_writer.cpp > > +++ b/src/apps/common/dng_writer.cpp > > @@ -541,6 +541,23 @@ int DNGWriter::write(const char *filename, const Camera *camera, > > > > TIFFWriteDirectory(tif); > > > > + /* > > + * Workaround for a bug introduced in libtiff version 4.5.1 and no fix > > + * released. In these versions the CFA* tags were missing in the field > > + * info. > > + * Introduced by: https://gitlab.com/libtiff/libtiff/-/commit/738e04099b13192bb1f654e74e9b5829313f3161 > > + * Fixed by: https://gitlab.com/libtiff/libtiff/-/commit/49856998c3d82e65444b47bb4fb11b7830a0c2be > > + */ > > + if (!TIFFFindField(tif, TIFFTAG_CFAREPEATPATTERNDIM, TIFF_ANY)) { > > + static const TIFFFieldInfo infos[] = { > > + { TIFFTAG_EP_CFAREPEATPATTERNDIM, 2, 2, TIFF_SHORT, FIELD_CUSTOM, > > + 1, 0, const_cast<char *>("EP CFARepeatPatternDim") }, > > + { TIFFTAG_EP_CFAPATTERN, -1, -1, TIFF_BYTE, FIELD_CUSTOM, > > + 1, 1, const_cast<char *>("EP CFAPattern") }, > > This will fail to compile with libtiff versions older than v4.5.1, where > the bug was introduced. TIFFTAG_CFAREPEATPATTERNDIM and > TIFFTAG_CFAPATTERN became aliases for TIFFTAG_EP_CFAREPEATPATTERNDIM and > TIFFTAG_EP_CFAPATTERN in that version, so I think you can switch to > TIFFTAG_CFAREPEATPATTERNDIM and TIFFTAG_CFAPATTERN here. I would also > change the description string to drop the "EP " prefix. Oh damn, how could I miss that. You are so right. Cheers, Stefan > > With that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + }; > > + TIFFMergeFieldInfo(tif, infos, 2); > > + } > > + > > /* Create a new IFD for the RAW image. */ > > const uint16_t cfaRepeatPatternDim[] = { 2, 2 }; > > const uint8_t cfaPlaneColor[] = { > > -- > Regards, > > Laurent Pinchart
diff --git a/src/apps/common/dng_writer.cpp b/src/apps/common/dng_writer.cpp index 103e5c828c4d..7375577e920f 100644 --- a/src/apps/common/dng_writer.cpp +++ b/src/apps/common/dng_writer.cpp @@ -541,6 +541,23 @@ int DNGWriter::write(const char *filename, const Camera *camera, TIFFWriteDirectory(tif); + /* + * Workaround for a bug introduced in libtiff version 4.5.1 and no fix + * released. In these versions the CFA* tags were missing in the field + * info. + * Introduced by: https://gitlab.com/libtiff/libtiff/-/commit/738e04099b13192bb1f654e74e9b5829313f3161 + * Fixed by: https://gitlab.com/libtiff/libtiff/-/commit/49856998c3d82e65444b47bb4fb11b7830a0c2be + */ + if (!TIFFFindField(tif, TIFFTAG_CFAREPEATPATTERNDIM, TIFF_ANY)) { + static const TIFFFieldInfo infos[] = { + { TIFFTAG_EP_CFAREPEATPATTERNDIM, 2, 2, TIFF_SHORT, FIELD_CUSTOM, + 1, 0, const_cast<char *>("EP CFARepeatPatternDim") }, + { TIFFTAG_EP_CFAPATTERN, -1, -1, TIFF_BYTE, FIELD_CUSTOM, + 1, 1, const_cast<char *>("EP CFAPattern") }, + }; + TIFFMergeFieldInfo(tif, infos, 2); + } + /* Create a new IFD for the RAW image. */ const uint16_t cfaRepeatPatternDim[] = { 2, 2 }; const uint8_t cfaPlaneColor[] = {
In libtiff version 4.5.1 and later the CFA* tags were missing. This got fixed in https://gitlab.com/libtiff/libtiff/-/commit/49856998c3d82e65444b47bb4fb11b7830a0c2be Unfortunately the fix is not released yet, but the faulty libtiff is contained in current buildroot. As a local fix is pretty easy and without side effects, let's workaround that. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/apps/common/dng_writer.cpp | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)