| Message ID | LyGUh-g--3-2@lynne.ee |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [FFmpeg-devel] tiffdec: support embedded ICC profiles | expand |
| Context | Check | Description |
|---|---|---|
| andriy/ffmpeg-patchwork | success | Make fate finished |
On 10/01/2020 22:06, Lynne wrote: > Patch attached. > > Very widespread, every NASA TIFF image has an ICC profile embedded, and its almost never sRGB, so this is really needed for proper color display. [...] > --- > libavcodec/tiff.c | 13 +++++++++++++ > libavcodec/tiff.h | 1 + > 2 files changed, 14 insertions(+) > + case TIFF_ICC_PROFILE: > + if (count >= INT_MAX || count <= 0) > + return AVERROR_INVALIDDATA; Should this be > instead of >=? Otherwise, LGTM. - Derek
Jan 13, 2020, 16:06 by derek.buitenhuis@gmail.com: > On 10/01/2020 22:06, Lynne wrote: > >> Patch attached. >> >> Very widespread, every NASA TIFF image has an ICC profile embedded, and its almost never sRGB, so this is really needed for proper color display. >> > > [...] > >> --- >> libavcodec/tiff.c | 13 +++++++++++++ >> libavcodec/tiff.h | 1 + >> 2 files changed, 14 insertions(+) >> >> + case TIFF_ICC_PROFILE: >> + if (count >= INT_MAX || count <= 0) >> + return AVERROR_INVALIDDATA; >> > > Should this be > instead of >=? > Actually the entire condition needs to be gone. count is uint32_t. The length is already checked below. Copied this from ff_tadd_shorts_metadata which has an int count. And the offset value isn't taken into account. TIFF allows the ICC profile to be placed pretty much anywhere within the file (the 32 bit offset points from the start of the file, not the field). So this would only work with files where the ICC profile immediately follows the tag. Attached a new patch.
On 13/01/2020 17:50, Lynne wrote: > Actually the entire condition needs to be gone. count is uint32_t. The length is already checked below. > Copied this from ff_tadd_shorts_metadata which has an int count. Ah. > And the offset value isn't taken into account. TIFF allows the ICC profile to be placed pretty much anywhere within the file (the 32 bit offset points from the start of the file, not the field). So this would only work with files where the ICC profile immediately follows the tag. Oh, that's a bit wild. I did a quick Google search for the ICC profile tag spec, and it wasn't obvious to me where it is defined, so I assumed it was directly after the tag. > Attached a new patch. > + gb_temp = s->gb; > + bytestream2_seek(&gb_temp, SEEK_SET, off); > + > + if (bytestream2_get_bytes_left(&gb_temp) < count) > + return AVERROR_INVALIDDATA; Is it worth checking the bytestream2_seek return value too, or will that be handled by bytestream2_get_bytes_left anyway? If it is handled, patch seems OK. - Derek
Jan 13, 2020, 20:20 by derek.buitenhuis@gmail.com: > On 13/01/2020 17:50, Lynne wrote: > >> Actually the entire condition needs to be gone. count is uint32_t. The length is already checked below. >> Copied this from ff_tadd_shorts_metadata which has an int count. >> > > Ah. > >> And the offset value isn't taken into account. TIFF allows the ICC profile to be placed pretty much anywhere within the file (the 32 bit offset points from the start of the file, not the field). So this would only work with files where the ICC profile immediately follows the tag. >> > > Oh, that's a bit wild. I did a quick Google search for the ICC profile tag > spec, and it wasn't obvious to me where it is defined, so I assumed it was > directly after the tag. > Yeah, I thought it was after the tag too with the offset being used to align to the nearest 16bits or maybe for some padding. But for the sample I have the position of the offset and the current byte in the bytestream reader match, and the ICC spec (which defines TIFF encapsulation) says the start of the file, so I can't argue with that. >> Attached a new patch. >> + gb_temp = s->gb; >> + bytestream2_seek(&gb_temp, SEEK_SET, off); >> + >> + if (bytestream2_get_bytes_left(&gb_temp) < count) >> + return AVERROR_INVALIDDATA; >> > > Is it worth checking the bytestream2_seek return value too, or will that > be handled by bytestream2_get_bytes_left anyway? If it is handled, patch > seems OK. > bytestream2_seek returns the amount of bytes since the start of the buffer after seeking. It clips to the size of the buffer so you can't seek past the end.
On 13/01/2020 22:05, Lynne wrote:
> bytestream2_seek returns the amount of bytes since the start of the buffer after seeking. It clips to the size of the buffer so you can't seek past the end.
LGTM then.
- Derek
Jan 13, 2020, 22:07 by derek.buitenhuis@gmail.com: > On 13/01/2020 22:05, Lynne wrote: > >> bytestream2_seek returns the amount of bytes since the start of the buffer after seeking. It clips to the size of the buffer so you can't seek past the end. >> > > LGTM then. > Pushed, thanks.
From 1a3cbad82c897110e8ef221aae9733b841a443fc Mon Sep 17 00:00:00 2001 From: Lynne <dev@lynne.ee> Date: Fri, 10 Jan 2020 21:55:19 +0000 Subject: [PATCH] tiffdec: support embedded ICC profiles --- libavcodec/tiff.c | 13 +++++++++++++ libavcodec/tiff.h | 1 + 2 files changed, 14 insertions(+) diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c index 636614aa28..257572b551 100644 --- a/libavcodec/tiff.c +++ b/libavcodec/tiff.c @@ -1218,6 +1218,7 @@ static void set_sar(TiffContext *s, unsigned tag, unsigned num, unsigned den) static int tiff_decode_tag(TiffContext *s, AVFrame *frame) { + AVFrameSideData *sd; unsigned tag, type, count, off, value = 0, value2 = 1; // value2 is a denominator so init. to 1 int i, start; int pos; @@ -1643,6 +1644,18 @@ static int tiff_decode_tag(TiffContext *s, AVFrame *frame) } } break; + case TIFF_ICC_PROFILE: + if (count >= INT_MAX || count <= 0) + return AVERROR_INVALIDDATA; + if (bytestream2_get_bytes_left(&s->gb) < count) + return AVERROR_INVALIDDATA; + + sd = av_frame_new_side_data(frame, AV_FRAME_DATA_ICC_PROFILE, count); + if (!sd) + return AVERROR(ENOMEM); + + bytestream2_get_bufferu(&s->gb, sd->data, count); + break; case TIFF_ARTIST: ADD_METADATA(count, "artist", NULL); break; diff --git a/libavcodec/tiff.h b/libavcodec/tiff.h index 2184c2c829..c07a5d4fa9 100644 --- a/libavcodec/tiff.h +++ b/libavcodec/tiff.h @@ -92,6 +92,7 @@ enum TiffTags { TIFF_MODEL_TIEPOINT = 0x8482, TIFF_MODEL_PIXEL_SCALE = 0x830E, TIFF_MODEL_TRANSFORMATION= 0x8480, + TIFF_ICC_PROFILE = 0x8773, TIFF_GEO_KEY_DIRECTORY = 0x87AF, TIFF_GEO_DOUBLE_PARAMS = 0x87B0, TIFF_GEO_ASCII_PARAMS = 0x87B1, -- 2.25.0.rc2