[FFmpeg-devel] avcodec/tiff: Add support for recognizing DNG files

Submitted by velocityra@gmail.com on March 17, 2019, 7:38 a.m.

Details

Message ID 20190317073833.19516-1-velocityra@gmail.com
State New
Headers show

Commit Message

velocityra@gmail.com March 17, 2019, 7:38 a.m.
From: Nick Renieris <velocityra@gmail.com>

Prints "DNG images are not supported" if it finds a TIFF image with
the 'DNGVersion' tag.  In DNG images with the .tiff extension it,
solves the issue where the TIFF thumbnail in IFD 0 was incorrectly
parsed (related confusion: [1]).  Also prints the DNG version of the
file on the debug channel.

Additionally:
 - Renamed TIFF_WHITE_LEVEL to DNG_WHITE_LEVEL since it is specified
   in the DNG spec.
 - Added/changed some comments to be more precise in differentiating
   between TIFF, TIFF/EP and DNG values.

Related to ticket: https://trac.ffmpeg.org/ticket/4364

---

[1]: https://superuser.com/questions/546879/creating-video-from-dng-images-with-ffmpeg

Signed-off-by: Nick Renieris <velocityra@gmail.com>
---
 libavcodec/tiff.c  | 25 +++++++++++++++++++++++--
 libavcodec/tiff.h  | 18 +++++++++++++-----
 libavformat/img2.c |  1 +
 3 files changed, 37 insertions(+), 7 deletions(-)

Comments

Paul B Mahol March 17, 2019, 8:34 a.m.
On 3/17/19, velocityra@gmail.com <velocityra@gmail.com> wrote:
> From: Nick Renieris <velocityra@gmail.com>
>
> Prints "DNG images are not supported" if it finds a TIFF image with
> the 'DNGVersion' tag.  In DNG images with the .tiff extension it,
> solves the issue where the TIFF thumbnail in IFD 0 was incorrectly
> parsed (related confusion: [1]).  Also prints the DNG version of the
> file on the debug channel.
>
> Additionally:
>  - Renamed TIFF_WHITE_LEVEL to DNG_WHITE_LEVEL since it is specified
>    in the DNG spec.
>  - Added/changed some comments to be more precise in differentiating
>    between TIFF, TIFF/EP and DNG values.
>
> Related to ticket: https://trac.ffmpeg.org/ticket/4364
>
> ---
>
> [1]:
> https://superuser.com/questions/546879/creating-video-from-dng-images-with-ffmpeg
>
> Signed-off-by: Nick Renieris <velocityra@gmail.com>
> ---
>  libavcodec/tiff.c  | 25 +++++++++++++++++++++++--
>  libavcodec/tiff.h  | 18 +++++++++++++-----
>  libavformat/img2.c |  1 +
>  3 files changed, 37 insertions(+), 7 deletions(-)
>
> diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
> index 112f5b52f4..95d00930ce 100644
> --- a/libavcodec/tiff.c
> +++ b/libavcodec/tiff.c
> @@ -70,6 +70,7 @@ typedef struct TiffContext {
>      int fill_order;
>      uint32_t res[4];
>
> +    int dng_mode; /** denotes that this is a DNG image */
>      int is_bayer;
>      uint8_t pattern[4];
>      unsigned white_level;
> @@ -1077,7 +1078,7 @@ static int tiff_decode_tag(TiffContext *s, AVFrame
> *frame)
>      case TIFF_SUB_IFDS:
>          s->sub_ifd = value;
>          break;
> -    case TIFF_WHITE_LEVEL:
> +    case DNG_WHITE_LEVEL:
>          s->white_level = value;
>          break;
>      case TIFF_CFA_PATTERN_DIM:
> @@ -1322,6 +1323,20 @@ static int tiff_decode_tag(TiffContext *s, AVFrame
> *frame)
>      case TIFF_SOFTWARE_NAME:
>          ADD_METADATA(count, "software", NULL);
>          break;
> +    case DNG_VERSION:
> +        if (count == 4) {
> +            unsigned int ver[4];
> +            ver[0] = ff_tget(&s->gb, type, s->le);
> +            ver[1] = ff_tget(&s->gb, type, s->le);
> +            ver[2] = ff_tget(&s->gb, type, s->le);
> +            ver[3] = ff_tget(&s->gb, type, s->le);
> +
> +            av_log(s->avctx, AV_LOG_DEBUG, "DNG file, version
> %u.%u.%u.%u\n",
> +                ver[0], ver[1], ver[2], ver[3]);
> +
> +            s->dng_mode = 1;
> +        }
> +        break;
>      default:
>          if (s->avctx->err_recognition & AV_EF_EXPLODE) {
>              av_log(s->avctx, AV_LOG_ERROR,
> @@ -1375,6 +1390,7 @@ again:
>      s->fill_order  = 0;
>      s->white_level = 0;
>      s->is_bayer    = 0;
> +    s->dng_mode    = 0;
>      free_geotags(s);
>
>      // Reset these offsets so we can tell if they were set this frame
> @@ -1389,6 +1405,11 @@ again:
>              return ret;
>      }
>
> +    if (s->dng_mode) {
> +        av_log(avctx, AV_LOG_ERROR, "DNG images are not supported\n");
> +        return AVERROR_PATCHWELCOME;

Are you sure that this does not break support with previously supported files.
Please remove return statement.

> +    }
> +
>      if (s->sub_ifd && s->get_subimage) {
>          off = s->sub_ifd;
>          if (off >= UINT_MAX - 14 || avpkt->size < off + 14) {
> @@ -1619,7 +1640,7 @@ static const AVClass tiff_decoder_class = {
>
>  AVCodec ff_tiff_decoder = {
>      .name           = "tiff",
> -    .long_name      = NULL_IF_CONFIG_SMALL("TIFF image"),
> +    .long_name      = NULL_IF_CONFIG_SMALL("TIFF / DNG image"),
>      .type           = AVMEDIA_TYPE_VIDEO,
>      .id             = AV_CODEC_ID_TIFF,
>      .priv_data_size = sizeof(TiffContext),
> diff --git a/libavcodec/tiff.h b/libavcodec/tiff.h
> index 4b08650108..3cede299f6 100644
> --- a/libavcodec/tiff.h
> +++ b/libavcodec/tiff.h
> @@ -20,7 +20,7 @@
>
>  /**
>   * @file
> - * TIFF tables
> + * TIFF constants & data structures
>   *
>   * For more information about the TIFF format, check the official docs at:
>   * http://partners.adobe.com/public/developer/tiff/index.html
> @@ -33,7 +33,7 @@
>  #include <stdint.h>
>  #include "tiff_common.h"
>
> -/** abridged list of TIFF tags */
> +/** abridged list of TIFF and TIFF/EP tags */
>  enum TiffTags {
>      TIFF_SUBFILE            = 0xfe,
>      TIFF_WIDTH              = 0x100,
> @@ -85,10 +85,17 @@ enum TiffTags {
>      TIFF_GEO_KEY_DIRECTORY  = 0x87AF,
>      TIFF_GEO_DOUBLE_PARAMS  = 0x87B0,
>      TIFF_GEO_ASCII_PARAMS   = 0x87B1,
> -    TIFF_WHITE_LEVEL        = 0xC61D,
>  };
>
> -/** list of TIFF compression types */
> +/** abridged list of DNG tags */
> +enum DngTags
> +{
> +    DNG_VERSION             = 0xC612,
> +    DNG_BACKWARD_VERSION    = 0xC613,
> +    DNG_WHITE_LEVEL         = 0xC61D,
> +};
> +
> +/** list of TIFF, TIFF/EP and DNG compression types */
>  enum TiffCompr {
>      TIFF_RAW = 1,
>      TIFF_CCITT_RLE,
> @@ -151,6 +158,7 @@ enum TiffGeoTagKey {
>      TIFF_VERTICAL_UNITS_GEOKEY               = 4099
>  };
>
> +/** list of TIFF, TIFF/AP and DNG PhotometricInterpretation
> (TIFF_PHOTOMETRIC) values */
>  enum TiffPhotometric {
>      TIFF_PHOTOMETRIC_NONE       = -1,
>      TIFF_PHOTOMETRIC_WHITE_IS_ZERO,      /* mono or grayscale, 0 is white
> */
> @@ -163,7 +171,7 @@ enum TiffPhotometric {
>      TIFF_PHOTOMETRIC_CIE_LAB    = 8,     /* 1976 CIE L*a*b* */
>      TIFF_PHOTOMETRIC_ICC_LAB,            /* ICC L*a*b* */
>      TIFF_PHOTOMETRIC_ITU_LAB,            /* ITU L*a*b* */
> -    TIFF_PHOTOMETRIC_CFA        = 32803, /* Color Filter Array (DNG) */
> +    TIFF_PHOTOMETRIC_CFA        = 32803, /* Color Filter Array (TIFF/AP and
> DNG) */
>      TIFF_PHOTOMETRIC_LOG_L      = 32844, /* CIE Log2(L) */
>      TIFF_PHOTOMETRIC_LOG_LUV,            /* CIE Log L*u*v* */
>      TIFF_PHOTOMETRIC_LINEAR_RAW = 34892, /* Linear Raw (DNG) */
> diff --git a/libavformat/img2.c b/libavformat/img2.c
> index 8432cc0955..16bc9d2abd 100644
> --- a/libavformat/img2.c
> +++ b/libavformat/img2.c
> @@ -51,6 +51,7 @@ const IdStrMap ff_img_tags[] = {
>      { AV_CODEC_ID_TARGA,      "tga"      },
>      { AV_CODEC_ID_TIFF,       "tiff"     },
>      { AV_CODEC_ID_TIFF,       "tif"      },
> +    { AV_CODEC_ID_TIFF,       "dng"      },
>      { AV_CODEC_ID_SGI,        "sgi"      },
>      { AV_CODEC_ID_PTX,        "ptx"      },
>      { AV_CODEC_ID_PCX,        "pcx"      },
> --
> 2.17.1.windows.2
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
Carl Eugen Hoyos March 17, 2019, 8:55 a.m.
Am So., 17. März 2019 um 08:46 Uhr schrieb <velocityra@gmail.com>:

> -    case TIFF_WHITE_LEVEL:
> +    case DNG_WHITE_LEVEL:

Why is this a good change?

> @@ -1389,6 +1405,11 @@ again:
>              return ret;
>      }
>
> +    if (s->dng_mode) {
> +        av_log(avctx, AV_LOG_ERROR, "DNG images are not supported\n");
> +        return AVERROR_PATCHWELCOME;
> +    }
> +

> @@ -1619,7 +1640,7 @@ static const AVClass tiff_decoder_class = {
>
>  AVCodec ff_tiff_decoder = {
>      .name           = "tiff",
> -    .long_name      = NULL_IF_CONFIG_SMALL("TIFF image"),
> +    .long_name      = NULL_IF_CONFIG_SMALL("TIFF / DNG image"),

I don't think above change (to error out for dng) justifies this
change.

Carl Eugen
velocityra@gmail.com March 17, 2019, 5:55 p.m.
Στις Κυρ, 17 Μαρ 2019 στις 10:43 π.μ., ο/η Paul B Mahol
<onemda@gmail.com> έγραψε:
> Are you sure that this does not break support with previously supported files.
> Please remove return statement.

By "support" you mean the thumbnail thing? Did you read the
StackExchange question? I would consider it more of a bug than
support...
I can't imagine any user wanting to convert their DNG files that would
actually want FFmpeg to use the low quality images that are normally
used for thumbnails.

Στις Κυρ, 17 Μαρ 2019 στις 10:56 π.μ., ο/η Carl Eugen Hoyos
<ceffmpeg@gmail.com> έγραψε:
>
> Am So., 17. März 2019 um 08:46 Uhr schrieb <velocityra@gmail.com>:
>
> > -    case TIFF_WHITE_LEVEL:
> > +    case DNG_WHITE_LEVEL:
>
> Why is this a good change?

Because I added a DngTags enum and this WhiteLevel tag is specified in
the DNG spec. If I removed it I would also have to name the two added
and all future DNG tags as TIFF_*, which I think is worse than having
a separate enum (since DNG images are quite different than TIFFs).

> I don't think above change (to error out for dng) justifies this
> change.

True, I didn't consider that. Will remove.
Paul B Mahol March 17, 2019, 6:33 p.m.
On 3/17/19, Nick Renieris <velocityra@gmail.com> wrote:
> Στις Κυρ, 17 Μαρ 2019 στις 10:43 π.μ., ο/η Paul B Mahol
> <onemda@gmail.com> έγραψε:
>> Are you sure that this does not break support with previously supported
>> files.
>> Please remove return statement.
>
> By "support" you mean the thumbnail thing? Did you read the
> StackExchange question? I would consider it more of a bug than
> support...
> I can't imagine any user wanting to convert their DNG files that would
> actually want FFmpeg to use the low quality images that are normally
> used for thumbnails.

Please try with option adding instead(-subimage). It will ignore
thumbnail and try to decode rest of image.
Anyhow the return statement is invalid, as it breaks support for files
that are already working just fine.

>
> Στις Κυρ, 17 Μαρ 2019 στις 10:56 π.μ., ο/η Carl Eugen Hoyos
> <ceffmpeg@gmail.com> έγραψε:
>>
>> Am So., 17. März 2019 um 08:46 Uhr schrieb <velocityra@gmail.com>:
>>
>> > -    case TIFF_WHITE_LEVEL:
>> > +    case DNG_WHITE_LEVEL:
>>
>> Why is this a good change?
>
> Because I added a DngTags enum and this WhiteLevel tag is specified in
> the DNG spec. If I removed it I would also have to name the two added
> and all future DNG tags as TIFF_*, which I think is worse than having
> a separate enum (since DNG images are quite different than TIFFs).
>
>> I don't think above change (to error out for dng) justifies this
>> change.
>
> True, I didn't consider that. Will remove.
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
velocityra@gmail.com March 17, 2019, 8:26 p.m.
Στις Κυρ, 17 Μαρ 2019 στις 8:34 μ.μ., ο/η Paul B Mahol
<onemda@gmail.com> έγραψε:
> Please try with option adding instead(-subimage). It will ignore
> thumbnail and try to decode rest of image.
> Anyhow the return statement is invalid, as it breaks support for files
> that are already working just fine.

Sorry, I still fail to see how this is already 'working just fine'.
I'm not sure if I was clear, but the images generated *are thumbnail
sized* (I tested it myself).
Why would anyone expect this to happen by default?

What I would propose - if you want to keep this thumbnail
functionality - would be to add a -dngthumbnail or -thumbnail option
and keep the default as is in the commit, which is, from a users
perspective (IMO) much more intuitive. Perhaps the -thumbnail option
could then be mentioned in the warning, should the user want to use
it.

How does that sound?
Paul B Mahol March 17, 2019, 8:41 p.m.
On 3/17/19, Nick Renieris <velocityra@gmail.com> wrote:
> Στις Κυρ, 17 Μαρ 2019 στις 8:34 μ.μ., ο/η Paul B Mahol
> <onemda@gmail.com> έγραψε:
>> Please try with option adding instead(-subimage). It will ignore
>> thumbnail and try to decode rest of image.
>> Anyhow the return statement is invalid, as it breaks support for files
>> that are already working just fine.
>
> Sorry, I still fail to see how this is already 'working just fine'.
> I'm not sure if I was clear, but the images generated *are thumbnail
> sized* (I tested it myself).

Give link to input you use.

> Why would anyone expect this to happen by default?

Yes, that is bad to display thumbnails. It still not ok to remove
present DNG support.

>
> What I would propose - if you want to keep this thumbnail
> functionality - would be to add a -dngthumbnail or -thumbnail option
> and keep the default as is in the commit, which is, from a users
> perspective (IMO) much more intuitive. Perhaps the -thumbnail option
> could then be mentioned in the warning, should the user want to use
> it.
>
> How does that sound?

Yes that is fine, thumbnails should be detected and only displayed
when option is set.
velocityra@gmail.com March 17, 2019, 9:12 p.m.
Στις Κυρ, 17 Μαρ 2019 στις 10:41 μ.μ., ο/η Paul B Mahol
<onemda@gmail.com> έγραψε:
> Give link to input you use.

All DNGs I've tried. An example is the samples linked in the ticket:
https://kenrockwell.com/leica/m9/sample-photos-3.htm
Running
ffmpeg -i L1004220.DNG out.jpg
with the 'return AVERROR_PATCHWELCOME;' commented, results in the
following image being generated: https://i.imgur.com/FjvWaDY.jpg

> > What I would propose - if you want to keep this thumbnail
> > functionality - would be to add a -dngthumbnail or -thumbnail option
> > and keep the default as is in the commit, which is, from a users
> > perspective (IMO) much more intuitive. Perhaps the -thumbnail option
> > could then be mentioned in the warning, should the user want to use
> > it.
> >
> > How does that sound?
>
> Yes that is fine, thumbnails should be detected and only displayed
> when option is set.

Ok, I'll post an updated patch here soon.

Patch hide | download patch | download mbox

diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c
index 112f5b52f4..95d00930ce 100644
--- a/libavcodec/tiff.c
+++ b/libavcodec/tiff.c
@@ -70,6 +70,7 @@  typedef struct TiffContext {
     int fill_order;
     uint32_t res[4];
 
+    int dng_mode; /** denotes that this is a DNG image */
     int is_bayer;
     uint8_t pattern[4];
     unsigned white_level;
@@ -1077,7 +1078,7 @@  static int tiff_decode_tag(TiffContext *s, AVFrame *frame)
     case TIFF_SUB_IFDS:
         s->sub_ifd = value;
         break;
-    case TIFF_WHITE_LEVEL:
+    case DNG_WHITE_LEVEL:
         s->white_level = value;
         break;
     case TIFF_CFA_PATTERN_DIM:
@@ -1322,6 +1323,20 @@  static int tiff_decode_tag(TiffContext *s, AVFrame *frame)
     case TIFF_SOFTWARE_NAME:
         ADD_METADATA(count, "software", NULL);
         break;
+    case DNG_VERSION:
+        if (count == 4) {
+            unsigned int ver[4];
+            ver[0] = ff_tget(&s->gb, type, s->le);
+            ver[1] = ff_tget(&s->gb, type, s->le);
+            ver[2] = ff_tget(&s->gb, type, s->le);
+            ver[3] = ff_tget(&s->gb, type, s->le);
+
+            av_log(s->avctx, AV_LOG_DEBUG, "DNG file, version %u.%u.%u.%u\n",
+                ver[0], ver[1], ver[2], ver[3]);
+
+            s->dng_mode = 1;
+        }
+        break;
     default:
         if (s->avctx->err_recognition & AV_EF_EXPLODE) {
             av_log(s->avctx, AV_LOG_ERROR,
@@ -1375,6 +1390,7 @@  again:
     s->fill_order  = 0;
     s->white_level = 0;
     s->is_bayer    = 0;
+    s->dng_mode    = 0;
     free_geotags(s);
 
     // Reset these offsets so we can tell if they were set this frame
@@ -1389,6 +1405,11 @@  again:
             return ret;
     }
 
+    if (s->dng_mode) {
+        av_log(avctx, AV_LOG_ERROR, "DNG images are not supported\n");
+        return AVERROR_PATCHWELCOME;
+    }
+
     if (s->sub_ifd && s->get_subimage) {
         off = s->sub_ifd;
         if (off >= UINT_MAX - 14 || avpkt->size < off + 14) {
@@ -1619,7 +1640,7 @@  static const AVClass tiff_decoder_class = {
 
 AVCodec ff_tiff_decoder = {
     .name           = "tiff",
-    .long_name      = NULL_IF_CONFIG_SMALL("TIFF image"),
+    .long_name      = NULL_IF_CONFIG_SMALL("TIFF / DNG image"),
     .type           = AVMEDIA_TYPE_VIDEO,
     .id             = AV_CODEC_ID_TIFF,
     .priv_data_size = sizeof(TiffContext),
diff --git a/libavcodec/tiff.h b/libavcodec/tiff.h
index 4b08650108..3cede299f6 100644
--- a/libavcodec/tiff.h
+++ b/libavcodec/tiff.h
@@ -20,7 +20,7 @@ 
 
 /**
  * @file
- * TIFF tables
+ * TIFF constants & data structures
  *
  * For more information about the TIFF format, check the official docs at:
  * http://partners.adobe.com/public/developer/tiff/index.html
@@ -33,7 +33,7 @@ 
 #include <stdint.h>
 #include "tiff_common.h"
 
-/** abridged list of TIFF tags */
+/** abridged list of TIFF and TIFF/EP tags */
 enum TiffTags {
     TIFF_SUBFILE            = 0xfe,
     TIFF_WIDTH              = 0x100,
@@ -85,10 +85,17 @@  enum TiffTags {
     TIFF_GEO_KEY_DIRECTORY  = 0x87AF,
     TIFF_GEO_DOUBLE_PARAMS  = 0x87B0,
     TIFF_GEO_ASCII_PARAMS   = 0x87B1,
-    TIFF_WHITE_LEVEL        = 0xC61D,
 };
 
-/** list of TIFF compression types */
+/** abridged list of DNG tags */
+enum DngTags
+{
+    DNG_VERSION             = 0xC612,
+    DNG_BACKWARD_VERSION    = 0xC613,
+    DNG_WHITE_LEVEL         = 0xC61D,
+};
+
+/** list of TIFF, TIFF/EP and DNG compression types */
 enum TiffCompr {
     TIFF_RAW = 1,
     TIFF_CCITT_RLE,
@@ -151,6 +158,7 @@  enum TiffGeoTagKey {
     TIFF_VERTICAL_UNITS_GEOKEY               = 4099
 };
 
+/** list of TIFF, TIFF/AP and DNG PhotometricInterpretation (TIFF_PHOTOMETRIC) values */
 enum TiffPhotometric {
     TIFF_PHOTOMETRIC_NONE       = -1,
     TIFF_PHOTOMETRIC_WHITE_IS_ZERO,      /* mono or grayscale, 0 is white */
@@ -163,7 +171,7 @@  enum TiffPhotometric {
     TIFF_PHOTOMETRIC_CIE_LAB    = 8,     /* 1976 CIE L*a*b* */
     TIFF_PHOTOMETRIC_ICC_LAB,            /* ICC L*a*b* */
     TIFF_PHOTOMETRIC_ITU_LAB,            /* ITU L*a*b* */
-    TIFF_PHOTOMETRIC_CFA        = 32803, /* Color Filter Array (DNG) */
+    TIFF_PHOTOMETRIC_CFA        = 32803, /* Color Filter Array (TIFF/AP and DNG) */
     TIFF_PHOTOMETRIC_LOG_L      = 32844, /* CIE Log2(L) */
     TIFF_PHOTOMETRIC_LOG_LUV,            /* CIE Log L*u*v* */
     TIFF_PHOTOMETRIC_LINEAR_RAW = 34892, /* Linear Raw (DNG) */
diff --git a/libavformat/img2.c b/libavformat/img2.c
index 8432cc0955..16bc9d2abd 100644
--- a/libavformat/img2.c
+++ b/libavformat/img2.c
@@ -51,6 +51,7 @@  const IdStrMap ff_img_tags[] = {
     { AV_CODEC_ID_TARGA,      "tga"      },
     { AV_CODEC_ID_TIFF,       "tiff"     },
     { AV_CODEC_ID_TIFF,       "tif"      },
+    { AV_CODEC_ID_TIFF,       "dng"      },
     { AV_CODEC_ID_SGI,        "sgi"      },
     { AV_CODEC_ID_PTX,        "ptx"      },
     { AV_CODEC_ID_PCX,        "pcx"      },