[PATCH] vobsub custom colors rgb->yuv fix.
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. -- Ulion
2007/12/2, Ulion <ulion2002@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. -- Ulion
Hello, On Mon, Dec 03, 2007 at 11:31:27AM +0800, Ulion wrote:
2007/12/2, Ulion <ulion2002@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... Greetings, Reimar Döffinger
2007/12/3, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Mon, Dec 03, 2007 at 11:31:27AM +0800, Ulion wrote:
2007/12/2, Ulion <ulion2002@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'. 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? -- Ulion
2007/12/3, Ulion <ulion2002@gmail.com>:
2007/12/3, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Mon, Dec 03, 2007 at 11:31:27AM +0800, Ulion wrote:
2007/12/2, Ulion <ulion2002@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'.
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?
Here's formulae fix for rgb->yuv, use RGB_TO_Y_CCIR... in libavcodec/colorspace.h, is here any can tell me whether directly include libavcodec/colorspace.h in vobsub.c and libmpdemux/demux_mkv.c is safe and acceptable? Second attachment is the custom palette rgb->yuv fix for demux_mkv, updated to use correct formulae. If there's no object, I will commit these two patch in 2 days. -- Ulion
Hello, On Mon, Dec 03, 2007 at 03:43:25PM +0800, Ulion wrote:
2007/12/3, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
On Mon, Dec 03, 2007 at 11:31:27AM +0800, Ulion wrote:
2007/12/2, Ulion <ulion2002@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.
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. But as I can see this would be the fourth case or so where that exactly same code is use, I'd say it is high time to make it an extra function. Greetings, Reimar Döffinger
2007/12/3, Reimar Döffinger <Reimar.Doeffinger@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@stud.uni-karlsruhe.de>:
On Mon, Dec 03, 2007 at 11:31:27AM +0800, Ulion wrote:
2007/12/2, Ulion <ulion2002@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?
But as I can see this would be the fourth case or so where that exactly same code is use, I'd say it is high time to make it an extra function.
Problem is where to put it, any advice? I still prefer use libavcodec macroes if it could. -- Ulion
2007/12/3, Ulion <ulion2002@gmail.com>:
2007/12/3, Reimar Döffinger <Reimar.Doeffinger@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@stud.uni-karlsruhe.de>:
On Mon, Dec 03, 2007 at 11:31:27AM +0800, Ulion wrote:
2007/12/2, Ulion <ulion2002@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/VobS... ) 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
2007/12/4, Ulion <ulion2002@gmail.com>:
2007/12/3, Ulion <ulion2002@gmail.com>:
2007/12/3, Reimar Döffinger <Reimar.Doeffinger@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@stud.uni-karlsruhe.de>:
On Mon, Dec 03, 2007 at 11:31:27AM +0800, Ulion wrote:
2007/12/2, Ulion <ulion2002@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/VobS... ) 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.
typo fix for second patch in last post. -- Ulion
2007/12/4, Ulion <ulion2002@gmail.com>:
2007/12/4, Ulion <ulion2002@gmail.com>:
2007/12/3, Ulion <ulion2002@gmail.com>:
2007/12/3, Reimar Döffinger <Reimar.Doeffinger@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@stud.uni-karlsruhe.de>:
On Mon, Dec 03, 2007 at 11:31:27AM +0800, Ulion wrote: > 2007/12/2, Ulion <ulion2002@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/VobS... ) 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
2007/12/6, Ulion <ulion2002@gmail.com>:
2007/12/4, Ulion <ulion2002@gmail.com>:
2007/12/4, Ulion <ulion2002@gmail.com>:
2007/12/3, Ulion <ulion2002@gmail.com>:
2007/12/3, Reimar Döffinger <Reimar.Doeffinger@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@stud.uni-karlsruhe.de>: > On Mon, Dec 03, 2007 at 11:31:27AM +0800, Ulion wrote: > > 2007/12/2, Ulion <ulion2002@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/VobS... ) 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?
y space fix done. Tomorrow I will apply the custom color rgb->yuv fix. Is it ok to use lavcodec/colorspace.h, Reimar? It's just a file with macroes, and will always be there within mplayer directory, isn't it? -- Ulion
Hello, On Sat, Dec 08, 2007 at 10:41:09AM +0800, Ulion wrote: [...]
y space fix done. Tomorrow I will apply the custom color rgb->yuv fix. Is it ok to use lavcodec/colorspace.h, Reimar? It's just a file with macroes, and will always be there within mplayer directory, isn't it?
We shouldn't depend on it regardless. IMO add a uint32_t vobsub_rgb_to_yuv(uint32_t) function to vobsub.c that does this conversion and reuse it. Greetings, Reimar Döffinger
2007/12/8, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Sat, Dec 08, 2007 at 10:41:09AM +0800, Ulion wrote: [...]
y space fix done. Tomorrow I will apply the custom color rgb->yuv fix. Is it ok to use lavcodec/colorspace.h, Reimar? It's just a file with macroes, and will always be there within mplayer directory, isn't it?
We shouldn't depend on it regardless. IMO add a uint32_t vobsub_rgb_to_yuv(uint32_t) function to vobsub.c that does this conversion and reuse it.
A function is good, but now the one used to custom colors are in vobsub.c and libmpdemux/demux_mkv.c, does that mean let demux_mkv.c include vobsub.h? -- Ulion
Hello, On Sat, Dec 08, 2007 at 06:29:23PM +0800, Ulion wrote:
2007/12/8, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
On Sat, Dec 08, 2007 at 10:41:09AM +0800, Ulion wrote: [...]
y space fix done. Tomorrow I will apply the custom color rgb->yuv fix. Is it ok to use lavcodec/colorspace.h, Reimar? It's just a file with macroes, and will always be there within mplayer directory, isn't it?
We shouldn't depend on it regardless. IMO add a uint32_t vobsub_rgb_to_yuv(uint32_t) function to vobsub.c that does this conversion and reuse it.
A function is good, but now the one used to custom colors are in vobsub.c and libmpdemux/demux_mkv.c, does that mean let demux_mkv.c include vobsub.h?
I think that's okay, yes. Greetings, Reimar Döffinger
2007/12/8, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Sat, Dec 08, 2007 at 06:29:23PM +0800, Ulion wrote:
2007/12/8, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
On Sat, Dec 08, 2007 at 10:41:09AM +0800, Ulion wrote: [...]
y space fix done. Tomorrow I will apply the custom color rgb->yuv fix. Is it ok to use lavcodec/colorspace.h, Reimar? It's just a file with macroes, and will always be there within mplayer directory, isn't it?
We shouldn't depend on it regardless. IMO add a uint32_t vobsub_rgb_to_yuv(uint32_t) function to vobsub.c that does this conversion and reuse it.
A function is good, but now the one used to custom colors are in vobsub.c and libmpdemux/demux_mkv.c, does that mean let demux_mkv.c include vobsub.h?
I think that's okay, yes.
OK, then there should be two functions. Now I moved the palette->yuv code into a common function and commited. Here's the patch for custom color rgb->yuv use common function. If there's no object, I will commit it tomorrow. -- Ulion
Hello, On Sat, Dec 08, 2007 at 08:37:51PM +0800, Ulion wrote: [...]
+unsigned int vobsub_rgb_to_yuv(unsigned int rgb) +{ + int r, g, b, y, u, v; + r = rgb >> 16 & 0xff; + g = rgb >> 8 & 0xff; + b = rgb & 0xff; + y = round(( 0.299 * r + 0.587 * g + 0.114 * b) * 219 / 255 + 16); + u = round((-0.16874 * r - 0.33126 * g + 0.5 * b) * 224 / 255 + 128); + v = round(( 0.5 * r - 0.41869 * g - 0.08131 * b) * 224 / 255 + 128); + return y << 16 | u << 8 | v; +}
The *219/255 etc. is a really bad idea IMO, since it means the subtitles can at best become a muddy gray and never truly white. This may be acceptable or even good for videos, but not for subtitles. Also my man pages say round is C99 only, if you really think it is worth the bother just add + 0.5 as the last step (though I admit it is not equivalent for u and v). Greetings, Reimar Döffinger
2007/12/8, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Sat, Dec 08, 2007 at 08:37:51PM +0800, Ulion wrote: [...]
+unsigned int vobsub_rgb_to_yuv(unsigned int rgb) +{ + int r, g, b, y, u, v; + r = rgb >> 16 & 0xff; + g = rgb >> 8 & 0xff; + b = rgb & 0xff; + y = round(( 0.299 * r + 0.587 * g + 0.114 * b) * 219 / 255 + 16); + u = round((-0.16874 * r - 0.33126 * g + 0.5 * b) * 224 / 255 + 128); + v = round(( 0.5 * r - 0.41869 * g - 0.08131 * b) * 224 / 255 + 128); + return y << 16 | u << 8 | v; +}
The *219/255 etc. is a really bad idea IMO, since it means the subtitles can at best become a muddy gray and never truly white. This may be acceptable or even good for videos, but not for subtitles.
I don't think so. Value 235 of Y is truly white, values above 235 is lighter than white. Or value 255 of Y is truly white? Anyone make it clear?
Also my man pages say round is C99 only, if you really think it is worth the bother just add + 0.5 as the last step (though I admit it is not equivalent for u and v).
OK, remove round, change to add 0.5. -- Ulion
Hello, On Sat, Dec 08, 2007 at 10:32:05PM +0800, Ulion wrote:
2007/12/8, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
On Sat, Dec 08, 2007 at 08:37:51PM +0800, Ulion wrote: [...]
+unsigned int vobsub_rgb_to_yuv(unsigned int rgb) +{ + int r, g, b, y, u, v; + r = rgb >> 16 & 0xff; + g = rgb >> 8 & 0xff; + b = rgb & 0xff; + y = round(( 0.299 * r + 0.587 * g + 0.114 * b) * 219 / 255 + 16); + u = round((-0.16874 * r - 0.33126 * g + 0.5 * b) * 224 / 255 + 128); + v = round(( 0.5 * r - 0.41869 * g - 0.08131 * b) * 224 / 255 + 128); + return y << 16 | u << 8 | v; +}
The *219/255 etc. is a really bad idea IMO, since it means the subtitles can at best become a muddy gray and never truly white. This may be acceptable or even good for videos, but not for subtitles.
I don't think so. Value 235 of Y is truly white, values above 235 is lighter than white. Or value 255 of Y is truly white? Anyone make it clear?
That depends on the video that is playing, MPlayer does not correctly distinguish it yet. Well, it is easily changed if it ever bothers anyone, and like this we don't have to bother with overflow checking, so apply it as is if you want.
static int vobsub_parse_custom_colors (sh_sub_t *sh, const char *start) { - int use_custom_colors, i; + int use_custom_colors, i, tmp;
use_custom_colors = 0; start += 14; @@ -361,8 +361,9 @@ start++; for (i = 0; i < 4; i++) { - if (sscanf(start, "%06x", &sh->colors[i]) != 1) + if (sscanf(start, "%06x", &tmp) != 1) break; + sh->colors[i] = vobsub_rgb_to_yuv(tmp);
I think it would be better to declare tmp in this block, since it is not used outside of it. Also for consistency making it "unsigned" instead of "int" might be better. Greetings, Reimar Döffinger
2007/12/8, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Sat, Dec 08, 2007 at 10:32:05PM +0800, Ulion wrote:
2007/12/8, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
On Sat, Dec 08, 2007 at 08:37:51PM +0800, Ulion wrote: [...]
+unsigned int vobsub_rgb_to_yuv(unsigned int rgb) +{ + int r, g, b, y, u, v; + r = rgb >> 16 & 0xff; + g = rgb >> 8 & 0xff; + b = rgb & 0xff; + y = round(( 0.299 * r + 0.587 * g + 0.114 * b) * 219 / 255 + 16); + u = round((-0.16874 * r - 0.33126 * g + 0.5 * b) * 224 / 255 + 128); + v = round(( 0.5 * r - 0.41869 * g - 0.08131 * b) * 224 / 255 + 128); + return y << 16 | u << 8 | v; +}
The *219/255 etc. is a really bad idea IMO, since it means the subtitles can at best become a muddy gray and never truly white. This may be acceptable or even good for videos, but not for subtitles.
I don't think so. Value 235 of Y is truly white, values above 235 is lighter than white. Or value 255 of Y is truly white? Anyone make it clear?
That depends on the video that is playing, MPlayer does not correctly distinguish it yet. Well, it is easily changed if it ever bothers anyone, and like this we don't have to bother with overflow checking, so apply it as is if you want.
static int vobsub_parse_custom_colors (sh_sub_t *sh, const char *start) { - int use_custom_colors, i; + int use_custom_colors, i, tmp;
use_custom_colors = 0; start += 14; @@ -361,8 +361,9 @@ start++; for (i = 0; i < 4; i++) { - if (sscanf(start, "%06x", &sh->colors[i]) != 1) + if (sscanf(start, "%06x", &tmp) != 1) break; + sh->colors[i] = vobsub_rgb_to_yuv(tmp);
I think it would be better to declare tmp in this block, since it is not used outside of it. Also for consistency making it "unsigned" instead of "int" might be better.
Sure, then I will commit it in tomorrow. -- Ulion
2007/12/9, Ulion <ulion2002@gmail.com>:
2007/12/8, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
Hello, On Sat, Dec 08, 2007 at 10:32:05PM +0800, Ulion wrote:
2007/12/8, Reimar Döffinger <Reimar.Doeffinger@stud.uni-karlsruhe.de>:
On Sat, Dec 08, 2007 at 08:37:51PM +0800, Ulion wrote: [...]
+unsigned int vobsub_rgb_to_yuv(unsigned int rgb) +{ + int r, g, b, y, u, v; + r = rgb >> 16 & 0xff; + g = rgb >> 8 & 0xff; + b = rgb & 0xff; + y = round(( 0.299 * r + 0.587 * g + 0.114 * b) * 219 / 255 + 16); + u = round((-0.16874 * r - 0.33126 * g + 0.5 * b) * 224 / 255 + 128); + v = round(( 0.5 * r - 0.41869 * g - 0.08131 * b) * 224 / 255 + 128); + return y << 16 | u << 8 | v; +}
The *219/255 etc. is a really bad idea IMO, since it means the subtitles can at best become a muddy gray and never truly white. This may be acceptable or even good for videos, but not for subtitles.
I don't think so. Value 235 of Y is truly white, values above 235 is lighter than white. Or value 255 of Y is truly white? Anyone make it clear?
That depends on the video that is playing, MPlayer does not correctly distinguish it yet. Well, it is easily changed if it ever bothers anyone, and like this we don't have to bother with overflow checking, so apply it as is if you want.
static int vobsub_parse_custom_colors (sh_sub_t *sh, const char *start) { - int use_custom_colors, i; + int use_custom_colors, i, tmp;
use_custom_colors = 0; start += 14; @@ -361,8 +361,9 @@ start++; for (i = 0; i < 4; i++) { - if (sscanf(start, "%06x", &sh->colors[i]) != 1) + if (sscanf(start, "%06x", &tmp) != 1) break; + sh->colors[i] = vobsub_rgb_to_yuv(tmp);
I think it would be better to declare tmp in this block, since it is not used outside of it. Also for consistency making it "unsigned" instead of "int" might be better.
Sure, then I will commit it in tomorrow.
Done. -- Ulion
participants (2)
-
Reimar Döffinger -
Ulion