[Ffmpeg-devel] [PATCH/RFC] 1-7 and 9-15 bits per pixel PGM files

[Reimar Döffinger]

Hello,
On Fri, Apr 06, 2007 at 03:01:09PM +0200, Ivo wrote:
> PGM files have a maxval value in their headers defining the maximum value a 
> pixel can have. This can be anything from 1 to 65535. Currently pnm.c only 
> supports 255 and 65535. The attached patch fixes this (not yet for PPM and 
> PAM files which can also have any maxval from 1 to 65535 for the R, G and B 
> components), but I am not sure if this is the way to go.

Is this transformation lossless (ignoring the case of a wrong maxval)?

> The patch upgrades anything below 255 to GRAY8 and anything from 256 to 
> 65535 to GRAY16BE while "decoding" the image file. I could also add 
> fourteen new PIX_FMT_*'s and their conversion routines to and from GRAY8/16 
> to libswscale and assume all reasonable maxval's will be powers of two, 
> minus one. What do you think?

If you only care for these maxvals the code can be extremely simplified,
even doing SIMD without normal C constructs.
IMO simple enough that it doesn't need extra PIX_FMTs.

> I will run into the same problem if I wanted to add Cineon support. .cin 
> files can have up to 8 channels (as of version 4.5 the channels can only 
> contain R, G or B components) and anything from 1 up to 255 bits per pixel, 
> which can be different for each channel. Even the number of pixels per line 
> and lines per image can be different for each channel and there are no 
> further constraints (i.e. ch. 1-3 could be 1920x1080 12bits/component, and 
> ch. 4-5 could be 640x360 7bits/component). How can we fit such things into 
> the current PIX_FMT scheme?

Well, how are they stored? Do they actually use e.g. 12 bit per pixel,
or do they in reality use 16 bit per pixel with the highest 4 bits
unused? In that case I think they're idiots for needlessly complicating
a storage format, they should just have shifted the whole thing left.
If they actually use only 12 bit to store, then blowing it up is a small
thing (and by far not as costly as the arbitrary maxval).

> @@ -142,7 +143,8 @@
>          return -1;
>      if (avctx->pix_fmt != PIX_FMT_MONOWHITE) {
>          pnm_get(s, buf1, sizeof(buf1));
> -        if(atoi(buf1) == 65535 && avctx->pix_fmt == PIX_FMT_GRAY8)
> +        s->maxval = atoi(buf1);
> +        if(s->maxval >= 256 && avctx->pix_fmt == PIX_FMT_GRAY8)
>              avctx->pix_fmt = PIX_FMT_GRAY16BE;

Hmm... Isn't the current code here buggy anyway and should check for >=
256 instead of == 65535? Then that's a bugfix that should be applied
anyway and before the rest.

> @@ -259,6 +261,27 @@
>          }
>          break;
>      }
> +    if (avctx->pix_fmt == PIX_FMT_GRAY16BE && s->maxval != 65535) {
> +        ptr = p->data[0];
> +        for(i = 0; i < avctx->height; i++) {
> +            unsigned int j, val;
> +            for(j = 0;j < avctx->width; j++) {
> +                val = (ptr[j<<1]<<8) | ptr[(j<<1)+1];

AV_RB16, or maybe even better the bytestream stuff.

> +                val = ((val<<16) - val) / s->maxval;

Is ((val<<16) - val) really faster than 65535 * val? The later the
compiler might be able to optimize depending on whether shift or
multiply or some other code it comes up with is faster...

> +                ptr[j<<1]     = val>>8;
> +                ptr[(j<<1)+1] = val & 0xff;

AV_WB16 or bytestream

Greetings,
Reimar D?ffinger