[Ffmpeg-devel] [PATCH] TIFF encoder (Google SoC qualification task)

[Michael Niedermayer]

Hi

On Fri, Apr 06, 2007 at 02:17:32PM +0200, Kamil Nowosad wrote:
> Hi
> 
> On Thu, Apr 05, 2007 at 09:03:59PM +0200, Michael Niedermayer wrote:
> > so iam fine and would prefer if we would return to the original suggestion
> > from you guys that planar is converted to whatever weird packed format tiff
> > requires
> > sorry for the bad pix_fmt suggestion i thought tiff would be using one of
> > the standard packed yuv variants

[...]
> +static void pack_yuv(TiffEncoderContext * s, uint8_t * dst, int lnum)
> +{

> +    AVFrame *p = (AVFrame*) &s->picture;

senseless cast


> +    int i, j, k;
> +    int w = (s->width - 1) / s->subsampling[0] + 1;
> +    for (i = 0; i < w; i++){
> +        for (j = 0; j < s->subsampling[1]; j++)
> +            for (k = 0; k < s->subsampling[0]; k++)
> +                *dst++ = p->data[0][(lnum + j) * p->linesize[0] +
> +                                    i * s->subsampling[0] + k];
> +        *dst++ = p->data[1][lnum / s->subsampling[1] * p->linesize[1] + i];
> +        *dst++ = p->data[2][lnum / s->subsampling[1] * p->linesize[2] + i];
> +    }

uint8_t *pu= p->data[1] + lnum / s->subsampling[1] * p->linesize[1];
uint8_t *pv= p->data[2] + lnum / s->subsampling[1] * p->linesize[2];
before the loop would avoid / and * inside it



[...]
> @@ -232,12 +251,28 @@
>          s->bpp = 1;
>          s->invert = 0;
>          break;
> +    case PIX_FMT_YUV420P:
> +    case PIX_FMT_YUV422P:
> +    case PIX_FMT_YUV444P:
> +    case PIX_FMT_YUV410P:
> +    case PIX_FMT_YUV411P:

> +        s->invert = 6;

this conflicts with the photometric interpretation patch, that is
we cannot apply this and the photometric interpretation patch no matter
in which order, please send patches against svn + all previous approved
patches which touch the same code


> +        avcodec_get_chroma_sub_sample(avctx->pix_fmt,
> +                &shift_h, &shift_v);
> +        s->bpp = 8 + (16 >> (shift_h + shift_v));
> +        s->subsampling[0] = 1 << shift_h;
> +        s->subsampling[1] = 1 << shift_v;
> +        s->bpp_tab_size = 3;

> +        s->compr = TIFF_RAW;

silently overriding the compression selected by the user is not ok, 
if the combination isnt supported just fail (return -1)


[...]

> @@ -245,6 +280,9 @@
>      else
>          s->rps = FFMAX(8192 / (((s->width * s->bpp) >> 3) + 1), 1);     // suggest size of strip
>  
> +    if (is_yuv) /// round up the rps to multiple of s->subsampling[1]
> +        s->rps = ((s->rps -1) / s->subsampling[1] + 1) * s->subsampling[1];
> +
>      strips = (s->height - 1) / s->rps + 1;
>  
>      if (check_size(s, 8))
> @@ -260,7 +298,13 @@
>      strip_sizes = av_mallocz(sizeof(*strip_sizes) * strips);
>      strip_offsets = av_mallocz(sizeof(*strip_offsets) * strips);
>  
> -    bytes_per_row = (s->width * s->bpp + 7) >> 3;
> +    if (is_yuv){
> +        bytes_per_row = (((s->width - 1)/s->subsampling[0] + 1) * s->bpp
> +                        * s->subsampling[0] * s->subsampling[1]) >> 3;
> +        yuv_line = av_malloc(bytes_per_row);
> +    }
> +    else
> +        bytes_per_row = (s->width * s->bpp + 7) >> 3;

if s->subsampling[*] is 1 by default then this and the previous if can be
simplified


[...]
> +    if (is_yuv)
> +        av_free(yuv_line);

hmm, id rather set yuv_line to NULL and remove that if()

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

I have often repented speaking, but never of holding my tongue.
-- Xenocrates
-------------- 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/20070406/cd5d7f74/attachment.pgp>