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

[Ulion]

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.

-- 
Ulion
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: vobsub_palette_y_space_fix.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071204/a71adffa/attachment.asc>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: vobsub_cuspal_fix2.diff
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071204/a71adffa/attachment.txt>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: yuvtest.c
URL: <http://lists.mplayerhq.hu/pipermail/mplayer-dev-eng/attachments/20071204/a71adffa/attachment-0001.asc>