[FFmpeg-devel] [PATCH]Allow setting TIFF image resolution

[Carl Eugen Hoyos]

Stefano Sabatini <stefano.sabatini-lala <at> poste.it> writes:

> > Please comment, I don't know much about options.
> > 
> > Carl Eugen
> 
> > diff --git a/libavcodec/tiffenc.c b/libavcodec/tiffenc.c
> > index d635e17..4f19a64 100644
> > --- a/libavcodec/tiffenc.c
> > +++ b/libavcodec/tiffenc.c
> > @@ -34,6 +34,7 @@
> >  #include "rle.h"
> >  #include "lzw.h"
> >  #include "put_bits.h"
> 
> > +#include "libavutil/opt.h"
> 
> Nit++: before the other headers, the standard headers order is
> EXTERNALS, INTERNALS_BUT_NOT_IN_LOCAL_DIR. LOCAL_DIR_HEADERS

Fixed locally.

> > +    uint32_t dpi;                       ///< image resolution
> 
> Nit: I don't like the use of the *measure unit* for referring the
> *measured thing*, it is like using "meters" for naming the lenght of a
> ship.

I agree, but everybody working with Tiff seems to call this "resolution".

> Thus I suggest:
> uint32_t res;  ///< image resolution in DPI (dots per inch?)

Changed locally to "image resolution in DPI"

> > +static const AVOption options[]={
> 
> > +{"dpi", "Sets the image resolution", offsetof(TiffEncoderContext, dpi),
FF_OPT_TYPE_INT, {.dbl =
> 72}, 1, 0x10000, AV_OPT_FLAG_VIDEO_PARAM|AV_OPT_FLAG_ENCODING_PARAM},
> 
> you can create an OFFSET macro, improves readability, especially when
> you have more options.

And it hurts readability if you have only one option...

> "Sets the image resolution" -> "set the image resolution (in dpi)"
> 
> impersonal form seems preferred (but this is highly inconsistent
> thorough the codebase).

Changed locally.

> Why 0x10000 as maximum value?

Because this is the maximum value that Gimp accepts.

> Also, which are the legal values for the resolution?

No idea.

Thank you, Carl Eugen