| Message ID | 20190317073833.19516-1-velocityra@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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 >
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
Στις Κυρ, 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.
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 >
Στις Κυρ, 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?
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.
Στις Κυρ, 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.
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" },