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

[Carl Eugen Hoyos]

Am Mo., 31. Okt. 2022 um 13:39 Uhr schrieb Anton Khirnov <anton at 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 at 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 at 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