Re: [FFmpeg-devel] [RFC]lavc/tiff: Support dng cropping

[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.

-- 
Anton Khirnov
_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".