[MPlayer-dev-eng] [PATCH] vobsub custom colors rgb->yuv fix.

[Ulion]

2007/12/4, Ulion <ulion2002 at gmail.com>:
> 2007/12/4, Ulion <ulion2002 at gmail.com>:
> > 2007/12/3, Ulion <ulion2002 at gmail.com>:
> > > 2007/12/3, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> > > > Hello,
> > > > On Mon, Dec 03, 2007 at 03:43:25PM +0800, Ulion wrote:
> > > > > 2007/12/3, Reimar Döffinger <Reimar.Doeffinger at stud.uni-karlsruhe.de>:
> > > > > > On Mon, Dec 03, 2007 at 11:31:27AM +0800, Ulion wrote:
> > > > > > > 2007/12/2, Ulion <ulion2002 at gmail.com>:
> > > > > > > > Hello,
> > > > > > > >
> > > > > > > > the custom colors in .idx file, is same with palette in .idx, all in
> > > > > > > > rgb format, spudec all use them as yuv format, and in vobsub.c,
> > > > > > > > palette has rgb->yuv convert code, but custom palette was not
> > > > > > > > converted.
> > > > > > > > Although spudec only use the 'y' part of palette, this bug will still
> > > > > > > > make spudec use wrong 'y' value of custom palette. Here's the fix, if
> > > > > > > > there are no objections, I will commit it tomorrow.
> > > > > > >
> > > > > > > Done.
> > > > > > >
> > > > > > > Same problem also happens in demux_mkv.c, here's a patch for it, if
> > > > > > > there's no objections, I will commit it tomorrow.
> > > > > >
> > > > > > Are you really sure about this? I thought there was some reason it was
> > > > > > like this...
> > > > >
> > > > > I'm sure because spudec use the second byte of the cuspal, it thought
> > > > > it was 'Y'.
> > > >
> > > > Actually I meant if you have checked the specification or a different
> > > > implementation that the mkv file does not actually store the yuv values.
> > > > I had assumed it was intentional since the conversion was only missing
> > > > in that case.
> > >
> > > I checked mkv specification, it just put the .idx file content into
> > > codec private data (remove comments, timestamps, etc). So it should be
> > > totally same with vobsub's custom colors.
> > > And all vobsub and mkv stored vobsub all use rgb as palette and custom
> > > color's presentation.
> > > But spudec all use them as yuv values. (by now only y get used)
> > >
> > > >
> > > > > But just now I fount the formulae has problem to convert from rgb->yuv.
> > > > > Indeed I should use macro RGB_TO_Y_CCIR, RGB_TO_U_CCIR, RGB_TO_V_CCIR
> > > > > to get the yuv in correct yuv color space range, should I, Reimar?
> > > >
> > > > You can't use stuff from libavcodec unconditionally. Also we do not
> > > > really care about full or non-full range elsewhere so I'd consider the
> > > > current formula "good enough" for now.
> > >
> > > Even with libavcodec disabled, the file libavcodec/colorspace.h is
> > > still usable because it's just group of macroes, could it be
> > > acceptable?
> > > And the current formula is very strange with the matrix values, I
> > > never see these values any otherelse, and it even far away from the
> > > non-full range formula. Are you sure current formula is correct?
> >
> > After did more research I found the truth why the original formula like that:
> >
> > All these mad stuff started from the vobsub's colorspace convert bug:
> > Check the source code of vobsub, you will found when vobsub convert
> > from .ifo to .idx, it use codes like these: (
> > http://guliverkli2.svn.sourceforge.net/viewvc/guliverkli2/src/subtitles/VobSubFile.cpp
> > )
> >                                 ifo->Read(&tmp, 1);
> >                                 ifo->Read(&y, 1);
> >                                 ifo->Read(&u, 1);
> >                                 ifo->Read(&v, 1);
> >
> >                                 y = (y-16)*255/219;
> >                                 m_pgc[i].pal[j].rgbRed = (uchar)min(max(1.0*y + 1.4022*(u-128), 0), 255);
> >                                 m_pgc[i].pal[j].rgbGreen = (uchar)min(max(1.0*y - 0.3456*(u-128) -
> > 0.7145*(v-128), 0), 255);
> >                                 m_pgc[i].pal[j].rgbBlue = (uchar)min(max(1.0*y + 1.7710*(v-128), 0) , 255);
> >
> > In the first and third forumla, the red and blue were incorrectly
> > calculated because use v and u in reversed places. Currect convert
> > should be (according to http://www.fourcc.org/fccyvrgb.php ):
> > R = Y + 1.403V'
> > G = Y - 0.344U' - 0.714V'
> > B = Y + 1.770U'
> >
> > But the mistake already happens for so long time (certainly it's a
> > mistake of vobsub, check its code, it directly use palette as rgb, but
> > it's not rgb), although in palette it's not real rgb values of
> > palette, other apps using vobsub still have to follow the wrong
> > forumlas.
> >
> > So they use the reversed forumlas calcuated by the vobsub wrong
> > forumlas, that should be:
> > y = (0.1494 * r + 0.6061 * g + 0.2445 * b ) * 255 / 219 + 16
> > u = 0.6066 * r - 0.4322 * g - 0.1744 * b + 128
> > v = -0.08435 * r - 0.3422 * g + 0.4266 * b + 128
> >
> > Check vobsub.c in mplayer, you will found it missed the '*255 / 219
> > +16' part for Y,
> >
> > So, the correct behavior should be:
> > 1. When parsing palette colors, they are not real rgb values, we
> > should convert them use the reversed version of forumlas from vobsub
> > official source.
> > 2. When parsing custom colors, treat them as true rgb value, and use
> > standard forumlas convert them to yuv.
> >
> > So, here first patch is for correct the Y space to the same with its
> > in .ifo file.
> > Second patch is convert custom colors from rgb -> yuv
> > Third attachment is my test program for rgb <->yuv convert (code is
> > not clean, just for test print), if interested, you can check the
> > values of rgb/yuv before and after different convert. Indeed when
> > doing rgb->yuv->rgb, the final result normally has some distance with
> > the original value so we should avoid such convert if possible.

If there's no object, I will commit the palette y space fix in 2 days,
and the custom color rgb->yuv fix in 3 days. Is directly include
libavcodec/colorspace.h acceptable after my explanation, Reimar?

-- 
Ulion