[RFC]lavc/tiff: Support dng cropping
Hi! I tried to implement dng cropping, it unfortunately can only work with -flags +unaligned, is there an alternative to simply print a warning if the flag was not supplied? Thank you, Carl Eugen
Am So., 23. Okt. 2022 um 16:35 Uhr schrieb Carl Eugen Hoyos <ceffmpeg@gmail.com>:
Hi!
I tried to implement dng cropping, it unfortunately can only work with -flags +unaligned, is there an alternative to simply print a warning if the flag was not supplied?
New patch with more parentheses attached. Please comment, Carl Eugen
Quoting Carl Eugen Hoyos (2022-10-23 20:46:57)
Am So., 23. Okt. 2022 um 16:35 Uhr schrieb Carl Eugen Hoyos <ceffmpeg@gmail.com>:
Hi!
I tried to implement dng cropping, it unfortunately can only work with -flags +unaligned, is there an alternative to simply print a warning if the flag was not supplied?
New patch with more parentheses attached.
Please comment, Carl Eugen
From 1bfe065564604659b7703e75b1bb750c031fdc81 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Sun, 23 Oct 2022 16:31:53 +0200 Subject: [PATCH] lavc/tiff: Support dng cropping,
A FATE test would be nice.
needs -flags +unaligned
AFAICT this is not entirely correct. Applying left cropping in libavcodec might need AV_CODEC_FLAG_UNALIGNED, but not always. Users may also set apply_cropping=0 and apply cropping themselves. The decoder should not care about it in any case, since it's handled in the generic code.
Fixes samples mentioned in ticket #4364. --- libavcodec/tiff.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ libavcodec/tiff.h | 3 ++ 2 files changed, 86 insertions(+)
diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c index fd9db18c0b..33edff8213 100644 --- a/libavcodec/tiff.c +++ b/libavcodec/tiff.c @@ -1492,6 +1492,89 @@ static int tiff_decode_tag(TiffContext *s, AVFrame *frame) case DNG_WHITE_LEVEL: s->white_level = value; break; + case DNG_CROP_ORIGIN: + if (count != 2 || type != TIFF_SHORT && type != TIFF_LONG && type != TIFF_RATIONAL) {
This condition could definitely use more parentheses. Same in two checks below.
+ av_log(s->avctx, AV_LOG_WARNING, "Invalid crop origin (count: %d, type: %d)\n", count, type); + break; + } + if (type == TIFF_RATIONAL) { + unsigned denum1, denum2; + value = ff_tget(&s->gb, TIFF_LONG, s->le); + denum1 = ff_tget(&s->gb, TIFF_LONG, s->le); + value2 = ff_tget(&s->gb, TIFF_LONG, s->le); + denum2 = ff_tget(&s->gb, TIFF_LONG, s->le); + if (denum1 != 1 || denum2 != 1) { + av_log(s->avctx, AV_LOG_WARNING, "Unsupported crop origin\n"); + break; + } + } else { + value = ff_tget(&s->gb, type, s->le); + value2 = ff_tget(&s->gb, type, s->le); + }
This entire block is duplicated for DNG_CROP_ORIGIN and DNG_CROP_SIZE, you could split it into a function.
+ av_log(s->avctx, AV_LOG_DEBUG, "dng crop origin: %d/%d\n", value, value2); + if (value >= s->width || value2 >= s->height) { + av_log(s->avctx, AV_LOG_WARNING, "Invalid crop origin (%d/%d)\n", value, value2); + break; + } + if ((value || value2) && !(s->avctx->flags & AV_CODEC_FLAG_UNALIGNED)) { + av_log(s->avctx, AV_LOG_WARNING,"Correct DNG cropping needs -flags +unaligned\n"); + } else { + frame->crop_left = value; + frame->crop_top = value2; + } + break; + case DNG_CROP_SIZE: + if (count != 2 || type != TIFF_SHORT && type != TIFF_LONG && type != TIFF_RATIONAL) { + av_log(s->avctx, AV_LOG_WARNING, "Invalid crop size (count: %d, type: %d)\n", count, type); + break; + } + if (type == TIFF_RATIONAL) { + unsigned denum1, denum2; + value = ff_tget(&s->gb, TIFF_LONG, s->le); + denum1 = ff_tget(&s->gb, TIFF_LONG, s->le); + value2 = ff_tget(&s->gb, TIFF_LONG, s->le); + denum2 = ff_tget(&s->gb, TIFF_LONG, s->le); + if (denum1 != 1 || denum2 != 1) { + av_log(s->avctx, AV_LOG_WARNING, "Unsupported crop size\n"); + break; + } + } else { + value = ff_tget(&s->gb, type, s->le); + value2 = ff_tget(&s->gb, type, s->le); + } + av_log(s->avctx, AV_LOG_DEBUG, "dng crop size %d x %d\n", value, value2); + if (value + frame->crop_left >= s->width || value2 + frame->crop_top >= s->height) {
value/value2 can be arbitrary 32bit integers, so the addition can overflow. Move crop_left/top to the other side of the comparison, since they are known to be smaller than width/height. Analogously for DNG_ACTIVE_AREA. -- Anton Khirnov
Am Mo., 31. Okt. 2022 um 13:39 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
Quoting Carl Eugen Hoyos (2022-10-23 20:46:57)
Am So., 23. Okt. 2022 um 16:35 Uhr schrieb Carl Eugen Hoyos <ceffmpeg@gmail.com>:
Hi!
I tried to implement dng cropping, it unfortunately can only work with -flags +unaligned, is there an alternative to simply print a warning if the flag was not supplied?
New patch with more parentheses attached.
Please comment, Carl Eugen
From 1bfe065564604659b7703e75b1bb750c031fdc81 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Sun, 23 Oct 2022 16:31:53 +0200 Subject: [PATCH] lavc/tiff: Support dng cropping,
A FATE test would be nice.
The samples are big but will look into this once the patch is ok.
needs -flags +unaligned
AFAICT this is not entirely correct. Applying left cropping in libavcodec might need AV_CODEC_FLAG_UNALIGNED, but not always.
Of course, but evidence indicates that in nearly all cases, it is needed.
Users may also set apply_cropping=0 and apply cropping themselves.
It should be possible to check this.
The decoder should not care about it in any case, since it's handled in the generic code.
Could you elaborate? The reason I added this check (and I would love to see that it is not necessary) is that there is no (working) generic code afaict and that decoding just breaks without a useful hint if the check is removed.
Fixes samples mentioned in ticket #4364. --- libavcodec/tiff.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ libavcodec/tiff.h | 3 ++ 2 files changed, 86 insertions(+)
diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c index fd9db18c0b..33edff8213 100644 --- a/libavcodec/tiff.c +++ b/libavcodec/tiff.c @@ -1492,6 +1492,89 @@ static int tiff_decode_tag(TiffContext *s, AVFrame *frame) case DNG_WHITE_LEVEL: s->white_level = value; break; + case DNG_CROP_ORIGIN: + if (count != 2 || type != TIFF_SHORT && type != TIFF_LONG && type != TIFF_RATIONAL) {
This condition could definitely use more parentheses. Same in two checks below.
Sorry, I disagree.
+ av_log(s->avctx, AV_LOG_WARNING, "Invalid crop origin (count: %d, type: %d)\n", count, type); + break; + } + if (type == TIFF_RATIONAL) { + unsigned denum1, denum2; + value = ff_tget(&s->gb, TIFF_LONG, s->le); + denum1 = ff_tget(&s->gb, TIFF_LONG, s->le); + value2 = ff_tget(&s->gb, TIFF_LONG, s->le); + denum2 = ff_tget(&s->gb, TIFF_LONG, s->le); + if (denum1 != 1 || denum2 != 1) { + av_log(s->avctx, AV_LOG_WARNING, "Unsupported crop origin\n"); + break; + } + } else { + value = ff_tget(&s->gb, type, s->le); + value2 = ff_tget(&s->gb, type, s->le); + }
This entire block is duplicated for DNG_CROP_ORIGIN and DNG_CROP_SIZE, you could split it into a function.
Will do, but I expect this to look ugly.
+ av_log(s->avctx, AV_LOG_DEBUG, "dng crop origin: %d/%d\n", value, value2); + if (value >= s->width || value2 >= s->height) { + av_log(s->avctx, AV_LOG_WARNING, "Invalid crop origin (%d/%d)\n", value, value2); + break; + } + if ((value || value2) && !(s->avctx->flags & AV_CODEC_FLAG_UNALIGNED)) { + av_log(s->avctx, AV_LOG_WARNING,"Correct DNG cropping needs -flags +unaligned\n"); + } else { + frame->crop_left = value; + frame->crop_top = value2; + } + break; + case DNG_CROP_SIZE: + if (count != 2 || type != TIFF_SHORT && type != TIFF_LONG && type != TIFF_RATIONAL) { + av_log(s->avctx, AV_LOG_WARNING, "Invalid crop size (count: %d, type: %d)\n", count, type); + break; + } + if (type == TIFF_RATIONAL) { + unsigned denum1, denum2; + value = ff_tget(&s->gb, TIFF_LONG, s->le); + denum1 = ff_tget(&s->gb, TIFF_LONG, s->le); + value2 = ff_tget(&s->gb, TIFF_LONG, s->le); + denum2 = ff_tget(&s->gb, TIFF_LONG, s->le); + if (denum1 != 1 || denum2 != 1) { + av_log(s->avctx, AV_LOG_WARNING, "Unsupported crop size\n"); + break; + } + } else { + value = ff_tget(&s->gb, type, s->le); + value2 = ff_tget(&s->gb, type, s->le); + } + av_log(s->avctx, AV_LOG_DEBUG, "dng crop size %d x %d\n", value, value2); + if (value + frame->crop_left >= s->width || value2 + frame->crop_top >= s->height) {
value/value2 can be arbitrary 32bit integers, so the addition can overflow. Move crop_left/top to the other side of the comparison, since they are known to be smaller than width/height. Analogously for DNG_ACTIVE_AREA.
Definitely, will do. Thank you, Carl Eugen
Quoting Carl Eugen Hoyos (2022-11-01 03:08:36)
Am Mo., 31. Okt. 2022 um 13:39 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
Quoting Carl Eugen Hoyos (2022-10-23 20:46:57)
Am So., 23. Okt. 2022 um 16:35 Uhr schrieb Carl Eugen Hoyos <ceffmpeg@gmail.com>:
Hi!
I tried to implement dng cropping, it unfortunately can only work with -flags +unaligned, is there an alternative to simply print a warning if the flag was not supplied?
New patch with more parentheses attached.
Please comment, Carl Eugen
From 1bfe065564604659b7703e75b1bb750c031fdc81 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Sun, 23 Oct 2022 16:31:53 +0200 Subject: [PATCH] lavc/tiff: Support dng cropping,
A FATE test would be nice.
The samples are big but will look into this once the patch is ok.
needs -flags +unaligned
AFAICT this is not entirely correct. Applying left cropping in libavcodec might need AV_CODEC_FLAG_UNALIGNED, but not always.
Of course, but evidence indicates that in nearly all cases, it is needed.
Users may also set apply_cropping=0 and apply cropping themselves.
It should be possible to check this.
The decoder should not care about it in any case, since it's handled in the generic code.
Could you elaborate? The reason I added this check (and I would love to see that it is not necessary) is that there is no (working) generic code afaict and that decoding just breaks without a useful hint if the check is removed.
The generic code I'm talking about is apply_cropping() in libavcodec/decode.c. If you want to add a warning, it should live there and be added in a separate patch. Note how no other decoder has such a warning internally, even though they set crop_left. What will be slightly nontrivial is detecting when the warning is appropriate, i.e. when av_frame_apply_croppying() is unable to apply the cropping fully. What I'd do is change av_frame_apply_croppying() to return 1 in such a case (it is currently documented to return >= 0, so this is backwards compatible). -- Anton Khirnov
Am Mo., 31. Okt. 2022 um 13:39 Uhr schrieb Anton Khirnov <anton@khirnov.net>:
Quoting Carl Eugen Hoyos (2022-10-23 20:46:57)
Am So., 23. Okt. 2022 um 16:35 Uhr schrieb Carl Eugen Hoyos <ceffmpeg@gmail.com>:
Hi!
I tried to implement dng cropping, it unfortunately can only work with -flags +unaligned, is there an alternative to simply print a warning if the flag was not supplied?
New patch with more parentheses attached.
Please comment, Carl Eugen
From 1bfe065564604659b7703e75b1bb750c031fdc81 Mon Sep 17 00:00:00 2001 From: Carl Eugen Hoyos <ceffmpeg@gmail.com> Date: Sun, 23 Oct 2022 16:31:53 +0200 Subject: [PATCH] lavc/tiff: Support dng cropping,
A FATE test would be nice.
needs -flags +unaligned
AFAICT this is not entirely correct. Applying left cropping in libavcodec might need AV_CODEC_FLAG_UNALIGNED, but not always. Users may also set apply_cropping=0 and apply cropping themselves.
The decoder should not care about it in any case, since it's handled in the generic code.
Fixes samples mentioned in ticket #4364. --- libavcodec/tiff.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++ libavcodec/tiff.h | 3 ++ 2 files changed, 86 insertions(+)
diff --git a/libavcodec/tiff.c b/libavcodec/tiff.c index fd9db18c0b..33edff8213 100644 --- a/libavcodec/tiff.c +++ b/libavcodec/tiff.c @@ -1492,6 +1492,89 @@ static int tiff_decode_tag(TiffContext *s, AVFrame *frame) case DNG_WHITE_LEVEL: s->white_level = value; break; + case DNG_CROP_ORIGIN: + if (count != 2 || type != TIFF_SHORT && type != TIFF_LONG && type != TIFF_RATIONAL) {
This condition could definitely use more parentheses. Same in two checks below.
+ av_log(s->avctx, AV_LOG_WARNING, "Invalid crop origin (count: %d, type: %d)\n", count, type); + break; + } + if (type == TIFF_RATIONAL) { + unsigned denum1, denum2; + value = ff_tget(&s->gb, TIFF_LONG, s->le); + denum1 = ff_tget(&s->gb, TIFF_LONG, s->le); + value2 = ff_tget(&s->gb, TIFF_LONG, s->le); + denum2 = ff_tget(&s->gb, TIFF_LONG, s->le); + if (denum1 != 1 || denum2 != 1) { + av_log(s->avctx, AV_LOG_WARNING, "Unsupported crop origin\n"); + break; + } + } else { + value = ff_tget(&s->gb, type, s->le); + value2 = ff_tget(&s->gb, type, s->le); + }
This entire block is duplicated for DNG_CROP_ORIGIN and DNG_CROP_SIZE, you could split it into a function.
+ av_log(s->avctx, AV_LOG_DEBUG, "dng crop origin: %d/%d\n", value, value2); + if (value >= s->width || value2 >= s->height) { + av_log(s->avctx, AV_LOG_WARNING, "Invalid crop origin (%d/%d)\n", value, value2); + break; + } + if ((value || value2) && !(s->avctx->flags & AV_CODEC_FLAG_UNALIGNED)) { + av_log(s->avctx, AV_LOG_WARNING,"Correct DNG cropping needs -flags +unaligned\n"); + } else { + frame->crop_left = value; + frame->crop_top = value2; + } + break; + case DNG_CROP_SIZE: + if (count != 2 || type != TIFF_SHORT && type != TIFF_LONG && type != TIFF_RATIONAL) { + av_log(s->avctx, AV_LOG_WARNING, "Invalid crop size (count: %d, type: %d)\n", count, type); + break; + } + if (type == TIFF_RATIONAL) { + unsigned denum1, denum2; + value = ff_tget(&s->gb, TIFF_LONG, s->le); + denum1 = ff_tget(&s->gb, TIFF_LONG, s->le); + value2 = ff_tget(&s->gb, TIFF_LONG, s->le); + denum2 = ff_tget(&s->gb, TIFF_LONG, s->le); + if (denum1 != 1 || denum2 != 1) { + av_log(s->avctx, AV_LOG_WARNING, "Unsupported crop size\n"); + break; + } + } else { + value = ff_tget(&s->gb, type, s->le); + value2 = ff_tget(&s->gb, type, s->le); + } + av_log(s->avctx, AV_LOG_DEBUG, "dng crop size %d x %d\n", value, value2); + if (value + frame->crop_left >= s->width || value2 + frame->crop_top >= s->height) {
value/value2 can be arbitrary 32bit integers, so the addition can overflow. Move crop_left/top to the other side of the comparison, since they are known to be smaller than width/height. Analogously for DNG_ACTIVE_AREA.
New patch attached. Please comment, Carl Eugen
participants (2)
-
Anton Khirnov -
Carl Eugen Hoyos