[Ffmpeg-devel] tiff encoder (qualification task for GSoC)

Michael Niedermayer michaelni
Thu Mar 22 21:28:32 CET 2007


Hi

On Thu, Mar 22, 2007 at 03:23:46PM +0100, Bart?omiej Wo?owiec wrote:
> I send a solution of Qualification tasks "TIFF encode". I implemented 24bits 
> rgb tiff encoder. Of course, this solution is only to show my programming 
> abilities in C and knowledge about compression and ffmpeg project. I wrote 
> most parts of standard, which are included in decoder. In future I try to 
> develop more functions, especially including more parts of tiff standard.
> (more colors depths and more pictures in file).
> If you have any suggestions, please write.

[...]
> Index: libavcodec/tiff.c
> ===================================================================
> --- libavcodec/tiff.c    (wersja 8483)
> +++ libavcodec/tiff.c    (kopia robocza)

please put the encoder into its own file tiffenc.c for example


> @@ -78,6 +78,9 @@
>      int compr;
>      int invert;
>  
> +    int bps;
> +    int bpr;

tabs are forbidden in svn


> +
>      int strips, rps;
>      int sot;
>      uint8_t* stripdata;
> @@ -86,6 +89,55 @@
>      LZWState *lzw;
>  } TiffContext;
>  
> +static void tput_short(uint8_t **p, int x, int le){
> +    if(le){
> +        **p = x & 0xff;
> +        *(*p + 1) = x >> 8;
> +    }else{
> +        *(*p + 1) = x & 0xff;
> +        *(*p) = x >> 8;
> +    }
> +    *p += 2;
> +}
> +
> +static void tput_long(uint8_t **p, long x, int le){
> +    if(le){
> +        **p = x & 0xff;
> +        *(*p + 1) = (x >> 8) & 0xff;
> +        *(*p + 2) = (x >> 16) & 0xff;
> +        *(*p + 3) = (x >> 24);
> +    }else{
> +        *(*p + 3) = x & 0xff;
> +        *(*p + 2) = (x >> 8) & 0xff;
> +        *(*p + 1) = (x >> 16) & 0xff;
> +        *(*p) = (x >> 24);
> +    }
> +    *p += 4;
> +}

please use bytestream_put_le or be*() also is there any reason why be and le
is supported in the encoder, this seems unneeded complexity


> +
> +static void tput(uint8_t **p, int val, int type, int le){
> +    switch(type){
> +    case TIFF_BYTE : *(*p)++ = val; break;
> +    case TIFF_SHORT: tput_short(p, val, le); break;
> +    case TIFF_LONG : tput_long (p, val, le); break;
> +    default        : assert(0 && "not implemented");
> +    }
> +}
> +
> +static void tnput(uint8_t **p, int n, uint8_t *val, int type, int le){ 

trailing whitespace is forbidden in svn


[...]
> +        case TIFF_PACKBITS:
> +            from = src;
> +            val = *(src++);
> +            while(src < end){
> +                // max 128 bytes in direct copy
> +                if(src - from >= 128){
> +                    *(*dst)++ = src - from - 1;
> +                    for(; from != src; from++){
> +                        *(*dst)++ = *from;
> +                    }
> +                }
> +                if(val == *src){
> +                    // write bytes from buffer
> +                    if(src - from >= 2){
> +                        *(*dst)++ = src - from - 2;
> +                        for(; from != src - 1; from++){
> +                            *(*dst)++ = *from;
> +                        }
> +                    }
> +
> +                    // how long is run ? 
> +                    while(*src == val && src - from < 128 && src < end -1)
> +                        src++;
> +
> +                    *(*dst)++ = from - src + 1; //TODO-(src-from)+1;
> +                    *(*dst)++ = val;
> +                    from = src;
> +                }
> +                val = *src;
> +                src++; 
> +            }
> +            // write buffer
> +            if(src - from > 0){
> +                *(*dst)++ = src - from - 1;
> +                for(; from != src ; from++){
> +                    *(*dst)++ = *from;
> +                }
> +            }

i think this encodes 1 0 0 1 to
0  1 -1  0  0  1
while the following would be shorter
3  1  0  0  1

also this should rather use targa_encode_rle() (if needed with extending
targa_encode_rle() to support any differences tiff requires)

(or targaenc.c should be changed to use your code if you make it work
 optimally and if it is faster which would need benchmarking of targaenc.c
 with START/STOP_TIMER)


> +            break;
> +        default:
> +            av_log(s->avctx, AV_LOG_ERROR, "Chosen compression is not supported\n");
> +            return -1;
> +    }
> +    return 0;
> +}
> +
> +#define TIFF_MAX_ENTRY 32
> +
> +static void add_entry(dir_entry **dir, dir_entry *end, int tag, int type, int count, int off, void *val){
> +    if(*dir == end)
> +        return;
> +    (*dir)->tag = tag;
> +    (*dir)->type = type;
> +    (*dir)->count= count;
> +    (*dir)->off = off;
> +    (*dir)->val = val;
> +    (*dir)++;
> +}

why do you build the thing in memory first and then
write it out instead of writing each entry immedeatly?


> +
> +// encode tiff
> +static int encode_frame(AVCodecContext *avctx, unsigned char *buf, int buf_size, void *data){

comments should be doxygen compatible, though in that case the comment
isnt really usefull so could also be removed


> +    TiffContext *s = avctx->priv_data;
> +    AVFrame *pict = data;
> +    AVFrame * const p= (AVFrame*)&s->picture;
> +    int i;
> +    uint8_t *ptr = (uint8_t *)buf;
> +    int le;
> +    dir_entry *dir_buf = NULL, *dir=NULL;
> +    uint8_t *offset;
> +    uint16_t bpp_tab[3] = {8,8,8};
> +    uint32_t *strip_offsets;
> +    uint32_t *strip_sizes;
> +    uint32_t strips;
> +    uint8_t *start_strip;
> +    uint32_t offset_dir;

is there some check that buf_size is large enough? if not please add one
something like buf_size < width*height*3 + MAX_HEADER_SIZE + MAX_OVERHEAD


> +
> +
> +    *p = *pict;
> +    p->pict_type = FF_I_TYPE;
> +    p->key_frame = 1;
> +
> +    if(avctx->pix_fmt != PIX_FMT_RGB24){
> +        av_log(s->avctx, AV_LOG_ERROR, "Only RGB24 is supported\n");
> +        return -1;
> +    }
> +
> +    if(avctx->compression_level == 0){

hmm, i think coder_type is more appropriate but thats not really important
compression_level is acceptable too


[...]
> +    strip_sizes = av_malloc(4 * strips);
> +    strip_offsets = av_malloc(4 * strips);
> +    bzero(strip_sizes, 4 * strips);

av_mallocz() = 2 in 1 av_malloc() + memset(0)


> +
> +    dir = av_malloc(sizeof(dir_entry) * TIFF_MAX_ENTRY);
> +    dir_buf = dir;
> +
> +    if(avctx->frame_number == 0){
> +        if(le){
> +            tput_short(&ptr, 0x4949, le);
> +        }else{
> +            tput_short(&ptr, 0x4D4D, le);
> +        }
> +        tput_short(&ptr, 42, le);
> +        offset = ptr;
> +        tput_long(&ptr, 0x0 , le);
> +    }

could you elaborate on the effect of the avctx->frame_number == 0 check?


[...]
> +        if(strip_sizes[i / (s->rps)] == 0){

the () is unneeded


[...]
> +    add_entry(&dir, dir_buf + TIFF_MAX_ENTRY, 0xfe, TIFF_LONG, 1, 0, NULL);
> +    add_entry(&dir, dir_buf + TIFF_MAX_ENTRY, TIFF_WIDTH, TIFF_LONG, 1, s->width, NULL);
> +    add_entry(&dir, dir_buf + TIFF_MAX_ENTRY, TIFF_HEIGHT, TIFF_LONG, 1, s->height, NULL);

these could be vertically aligned like:

add_entry(&dir, dir_buf + TIFF_MAX_ENTRY, 0xfe       , TIFF_LONG, 1, 0        , NULL);
add_entry(&dir, dir_buf + TIFF_MAX_ENTRY, TIFF_WIDTH , TIFF_LONG, 1, s->width , NULL);
add_entry(&dir, dir_buf + TIFF_MAX_ENTRY, TIFF_HEIGHT, TIFF_LONG, 1, s->height, NULL);

makes then slightly easier to read


[...]
> +#define SOFTWARE_NAME "FFmpeg"    
> +    add_entry(&dir, dir_buf + TIFF_MAX_ENTRY,0x131, TIFF_STRING, strlen(SOFTWARE_NAME), 0, SOFTWARE_NAME);

proper value is in LIBAVCODEC_IDENT


[...]
> +
> +AVCodec tiff_encoder = {
> +    "tiff",
> +    CODEC_TYPE_VIDEO,
> +    CODEC_ID_TIFF,
> +    sizeof(TiffContext),
> +    tiff_init,
> +    encode_frame,
> +    NULL,
> +    NULL

list of supported pix_fmts is missing


[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Concerning the gods, I have no means of knowing whether they exist or not
or of what sort they may be, because of the obscurity of the subject, and
the brevity of human life -- Protagoras
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20070322/44e14bcf/attachment.pgp>



More information about the ffmpeg-devel mailing list