[Ffmpeg-devel] [RFC] RGB48 support

[Ivo]

Hi,

On Tuesday 10 April 2007 23:13, Michael Niedermayer wrote:
> On Tue, Apr 10, 2007 at 06:15:35PM +0200, Ivo wrote:
> > I noticed several lines of:
> >
> > src2= formatConvBuffer+2048;
> >
> > in swscale_template.c. If I understand the code correctly, this
> > constant is arbitrarily chosen and swscale will fail if you feed it
> > very large images (like an 8k 70mm film scan). Is that correct?
>
> yes, the 2048 stuff should be changed to a #defined constant (or variable
> but that will be more difficult and likely somewhat slower, so constant
> is better)

I think a variable would be preferable, unless the speed penalty is really 
noticable. Otherwise, what constant should we choose? Somebody might feed 
it a 21k IMAX scan in the future, but a constant that large would be a 
waste of space if you are merely processing VCD's or DVD's.

> did you test the yuv2rgb.c code ?

Yes, I converted several yuv sources to rgb48, outputted the raw data, 
prepended a PPM P6 header with dd and viewed the result :)  Do you see 
something wrong with the code?

> > +    PIX_FMT_RGB48BE,   ///< Packed RGB 16:16:16, 48bpp, big-endian
> > +    PIX_FMT_RGB48LE,   ///< Packed RGB 16:16:16, 48bpp, little-endian
>
> iam fine with this but i think the comments could be clearer

I added a note that the endianness refers to the components and not to the 
component order. 

> > +static inline void RENAME(rgb48to48)(const uint8_t *src, uint8_t *dst,
> > long src_size) +{
> > +    uint8_t *d = dst, *s = (uint8_t *) src;
> > +    const uint8_t *end = s + src_size;
> > +
> > +    for (; s<end; s+=2, d+=2) {
> > +        *d     = *(s+1);
> > +        *(d+1) = *s;
> > +    }
> > +}
>
> hmm try:
> unsigned int a= *(uint32_t*)s;
> *(uint32_t*)d= ((a>>8)&0x00FF00FF) + ((a<<8)&0xFF00FF00);

I suppose that can overread the buffer by 2 bytes or are the buffers 
required to be over-malloced and have enough room at the end?

> > +static inline void RENAME(rgb48beto24)(const uint8_t *src, uint8_t
> > *dst, long src_size) +{
> > [..]
> > +static inline void RENAME(rgb48leto24)(const uint8_t *src, uint8_t
> > *dst, long src_size) +{
> > [..]
>
> hmmmmm, isnt this the same as the gray conversation stuff? if yes please
> avoid the code duplication / use it for both gray and rgb

Yes. I changed the gray16togray code to reuse the functions above. As an 
added bonus, gray16 downsampling will now be MMX enhanced :)

> > +	"	punpcklbw	%%mm0,  %%mm6	\n"
> > +	"	punpcklbw	%%mm1,  %%mm7	\n"
>
> hmm try:
> punpcklbw	%%mm0,  %%mm0

Done.

> > +    d += 48;
> > +    s += 24;
> > +    }
>
> please write the whole loop in asm, i hate c-loop + asm code as gcc tends
> to make a quite suboptimal mess out of it most of the time ...

Done. It actually runs an itty-bitty faster now.

> > +    STOP_TIMER("rgb24to48")}
>
> iam happy that you benchmark your code but this shouldnt be in the
> patches you send :)

Yes, I'll remove them in the final patch. I just thought it wouldn't matter 
for an intermediate review :)

> > +static inline void RENAME(rgb48leToY)(uint8_t *dst, uint8_t *src, int
> > width) +{
> > [..]
>
> code duplication rgb48beToY(+1) will work too (of course alternative
> high accuracy code which scales the 16bits rgb to internal yuv is welcome
> too ...)

I think I'll stick to 8-bit yuv for the moment until the rgb48 colorspace is 
fully supported and accepted. After that, I'll look into internal yuv, as 
it seems non-trivial to me now. But it would be a desirable feature to 
have, as it would be useless if you rescale a 4k 16-bits/component source 
down to 2k 16-bits/component (which seems a frequent operation in the film 
industry) and loose a lot more data than just the 3/4 of pixels.

--Ivo