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

[Ivo]

On Friday 06 April 2007 16:17, Reimar D?ffinger wrote:
> 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)?

Yes, though I think I should add checks for maxval being between 0 and 
65536, not including the boundaries.

> 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.

OK. I think it is reasonable to assume only maxvals of (2^n)-1 for 0<n<=16 
are used in the wild. I have never seen anything other than 255, 1023, 
2047, 4095, 16383 and 65535.

>>[Cineon weird formats]
> 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).

It seems that for legacy Cineon, there are only 10-bit log files in the wild 
(which is that the Kodak scanner produces) with matching dimension for all 
channels. For DPX files, it looks like 1, 8, 10, 12 and 16 bits per 
component are common. There's also 32 bits (float) but I haven't seen an 
implementation of that yet.

Pixels can be stored in a variety of ways. They define cells being 8, 16 or 
32 bits wide and fields being packed inside those cells. They can be padded 
with zeroes, or they can have as much fields per cell as possible (i.e. 6 
bits per field, ch1[6] ch2[6] ch3[6] ch1[6] ch2[6] pad[2] etc...). Well, 
supporting these funky schemes is madness, so they should be torn appart 
and stored as 1 or 2 bytes per pixel IMHO. The question is, should we add 
PIX_FMT's for 10, 12 and 16 bits/component RGB(A) and do not support 
anything else or should we only add PIX_FMT_RGB48/64 and blow everything 
else up in the image decoder? (including proper scaling so white stays 
white, i.e. 0x3fff -> 0xffff instead of 0xfffc).

> > @@ -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.

Checking for >= 256 in the old code will result in very dark images for, 
say, 12-bit grayscale images. But, that's arguably better than the garbage 
image it produces now. 

> > +                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...

OK. I will change that back and leave it to the compiler, unless I'll change 
the whole conversion to simply blowing up to 8 or 16 bits, in which case 
there will be no multiplication or division necessary.

> AV_WB16 or bytestream

I'll use the _WB16 and _RB16 macro's in whichever solution will be final :)

--Ivo