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

Stefano Sabatini stefano.sabatini-lala at poste.it
Thu Aug 18 00:24:05 CEST 2011


On date Wednesday 2011-08-17 23:58:24 +0200, Carl Eugen Hoyos encoded:
> Hi!
> 
> 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

does it matter? maybe

>  
>  #define TIFF_MAX_ENTRY 32
>  
> @@ -61,6 +62,7 @@ typedef struct TiffEncoderContext {
>      int buf_size;                       ///< buffer size
>      uint16_t subsampling[2];            ///< YUV subsampling factors
>      struct LZWEncodeState *lzws;        ///< LZW Encode state

> +    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. Thus I suggest:
uint32_t res;  ///< image resolution in DPI (dots per inch?)

>  } TiffEncoderContext;
>  
>  
> @@ -210,7 +212,7 @@ static int encode_frame(AVCodecContext * avctx, unsigned char *buf,
>      uint32_t *strip_sizes = NULL;
>      uint32_t *strip_offsets = NULL;
>      int bytes_per_row;
> -    uint32_t res[2] = { 72, 1 };        // image resolution (72/1)
> +    uint32_t res[2] = { s->dpi, 1 };        // image resolution (72/1)
>      uint16_t bpp_tab[] = { 8, 8, 8, 8 };
>      int ret = -1;
>      int is_yuv = 0;
> @@ -441,6 +443,12 @@ fail:
>      return ret;
>  }
>  
> +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.

"Sets the image resolution" -> "set the image resolution (in dpi)"

impersonal form seems preferred (but this is highly inconsistent
thorough the codebase).

Why 0x10000 as maximum value? Also, which are the legal values for the
resolution?
-- 
FFmpeg = Furious Foolish Mind-dumbing Puristic Embarassing Gospel


More information about the ffmpeg-devel mailing list