[MPlayer-dev-eng] [RFC][PATCH v2] Add support for 12-bit color mode.

[Janusz Krzysztofik]

Hi Reimar,
Thanks for your review time.

Friday 02 April 2010 20:32:13 Reimar Döffinger wrote:
> On Fri, Mar 19, 2010 at 01:52:42AM +0100, Janusz Krzysztofik wrote:
> > diff -upr trunk.orig/libmpcodecs/vf_tile.c trunk/libmpcodecs/vf_tile.c
> > --- trunk.orig/libmpcodecs/vf_tile.c	2010-02-26 02:39:46.000000000 +0100
> > +++ trunk/libmpcodecs/vf_tile.c	2010-03-19 01:01:29.000000000 +0100
> > @@ -202,12 +202,14 @@ static void uninit(struct vf_instance *v
> >  static int query_format(struct vf_instance *vf, unsigned int fmt)
> >  {
> >  	switch (fmt) {
> > -        /* rgb 15 -> 32 bit */
> > +        /* rgb 12 -> 32 bit */
> > +        case IMGFMT_RGB12:
> >          case IMGFMT_RGB15:
> >  	case IMGFMT_RGB16:
> >  	case IMGFMT_RGB24:
> >          case IMGFMT_RGB32:
> > -        /* bgr 15 -> 32 bit */
> > +        /* bgr 12 -> 32 bit */
> > +	case IMGFMT_BGR12:
>
> These two comments do not really make sense, I'd say the should
> be something like "bgr X -> 32 bit" instead of the specific value
> "12" or "15".

The broader view of the actual code code looks like this:

/* rgb/bgr 15->32 supported & some Yxxx */
static int query_format(struct vf_instance *vf, unsigned int fmt)
{
        switch (fmt) {
        /* rgb 15 -> 32 bit */
        case IMGFMT_RGB15:
        case IMGFMT_RGB16:
        case IMGFMT_RGB24:
        case IMGFMT_RGB32:
        /* bgr 15 -> 32 bit */
        case IMGFMT_BGR15:
        case IMGFMT_BGR16:
        case IMGFMT_BGR24:
        case IMGFMT_BGR32:
        /* Various Yxxx Formats */
        case IMGFMT_444P:
        ...

Then, replacing 15 with 12, also in the initial comment that I missed, seems 
to make sense if we replace those unfortunate -> arrows with dashes (or 
collons if preferred). What do you think?

BTW, this code snippet is a good example of mixed tabs and spaces that really 
requires cleanup. Whether to put spaces or a tab in front of my "case 
IMGFMT_BGR12:" placed between "<spaces>/* bgr 15 -> 32 bit */" and "<tab>case 
IMGFMT_BGR15:" is not an easy choice ;).

>
> > +		register unsigned int a=src[x]>>4;
> > +                dst[x]=(a<<8)|(a<<4)|a;
>
> Please keep the indentation consistent, you used tabs for the
> upper line and spaces for everything else (unless it contradicts
> the surrounding code I'm in favour of using spaces).

OK, I'll review all my modifications for unwanted tabs and replace them with 
spaces if not in conflict with existing code (not touching any tabs already 
present). What if I use ffmpeg provided tools/patcheck.pl for coding style 
evaluation?

> And for completeness I think the new formats should also be added to
> codec-cfg.c.

There were present in my initially submitted single patch covering both ffmpeg 
and mplayer changes. Then, while working on ffmpeg part, I was advised to 
split into much smaller patches. Only a part of them, those required for 
swscale 12-bit output format support, have been already worked on and 
applied. Then, 12-bit input format handling is still missing from libswscale.
I'm going to submit remaining swscale patches when ready with mplayer 12-bit 
output support, then remaining mplayer patch(es), including codec-cfg.c 
modifications. Would I ever fail with this, someone could pick up my initial 
submission as a starting point. Is this acceptable?

> Otherwise I think it is ok to be applied, very thorough work (even
> though it would be nicer if more was tested successfully, though
> it's very likely that e.g. SDL is just broken).

I can remove X and SDL support from this patch for now if preferred. Those 
could be added later, after I have some spare time to test with X and a more 
recent version of SDL.

Thanks,
Janusz