[PATCH] cmov detection and draw_alpha fixes
Hi, since i added the box backgrounds in the osd menu i noticed a nasty bug with the draw alpha functions. They happily overflow leading to some nice distortions :/ So i went on to fix that, the mmx version are trivial to fix just change the final paddb to a paddusb. However for the generic i386 asm for rgb24/32 i didn't found a nice and quick solution w/o using cmov (their probably is or ?). So i used cmov and let the rest fallback on C. Sadly cmov is not present on all cpu, currently it is only used in lavc under a #if __CPU__ >= 686. However their are some 586 with cmov (for ex the AMD Geode which is a 586 with cmov and mmx). So i added a proper test to configure and cpudetect.c. Albeu
On Fri, Mar 31, 2006 at 03:53:21AM +0200, Alban Bedel wrote:
Hi,
since i added the box backgrounds in the osd menu i noticed a nasty bug with the draw alpha functions. They happily overflow leading to some nice distortions :/ So i went on to fix that, the mmx version are trivial to fix just change the final paddb to a paddusb. However for the generic i386 asm for rgb24/32 i didn't found a nice and quick solution w/o using cmov (their probably is or ?). So i used cmov and let the rest fallback on C.
Sadly cmov is not present on all cpu, currently it is only used in lavc under a #if __CPU__ >= 686. However their are some 586 with cmov (for ex the AMD Geode which is a 586 with cmov and mmx). So i added a proper test to configure and cpudetect.c.
This is all incorrect. There is no bug in the draw_alpha functions. Rather you are passing them incorrect data. MPlayer requires a special alpha format tailored to the optimizations used in the non-mmx code. Rich
2006/3/31, Rich Felker <dalias@aerifal.cx>:
On Fri, Mar 31, 2006 at 03:53:21AM +0200, Alban Bedel wrote:
Hi,
since i added the box backgrounds in the osd menu i noticed a nasty bug with the draw alpha functions. They happily overflow leading to some nice distortions :/ So i went on to fix that, the mmx version are trivial to fix just change the final paddb to a paddusb. However for the generic i386 asm for rgb24/32 i didn't found a nice and quick solution w/o using cmov (their probably is or ?). So i used cmov and let the rest fallback on C.
Sadly cmov is not present on all cpu, currently it is only used in lavc under a #if __CPU__ >= 686. However their are some 586 with cmov (for ex the AMD Geode which is a 586 with cmov and mmx). So i added a proper test to configure and cpudetect.c.
This is all incorrect. There is no bug in the draw_alpha functions. Rather you are passing them incorrect data. MPlayer requires a special alpha format tailored to the optimizations used in the non-mmx code.
Indeed. now let's say we have image pixles with variable "image" we have bitmap pixel with variable "bitmap" and alpha for the same bitmap called "alphamap", (all values are floats) so newimage = image*alphamap + bitmap*(1-alphamap). now, the (bitmap*(1-alphamap) depends only on the data from the bitmap, not the data from the image. This means it could be precalculated and stored. so: newimage = image*alphamap + calcmap. Now, if you have overflow you are most likely using "bitmap" instead of "calcmap" or there is some kind of rounding error (e.g. for speed reasons alpha 1.0 is 0xff not 0x100) Hope this helps
On Fri, 31 Mar 2006 10:53:53 +0300 "Ivan Kalvachev" <ikalvachev@gmail.com> wrote:
now let's say we have image pixles with variable "image" we have bitmap pixel with variable "bitmap" and alpha for the same bitmap called "alphamap", (all values are floats) so
newimage = image*alphamap + bitmap*(1-alphamap).
now, the (bitmap*(1-alphamap) depends only on the data from the bitmap, not the data from the image. This means it could be precalculated and stored. so:
newimage = image*alphamap + calcmap.
Now, if you have overflow you are most likely using "bitmap" instead of "calcmap" or there is some kind of rounding error (e.g. for speed reasons alpha 1.0 is 0xff not 0x100)
Seems so. However i still think their is a pb, at least that's not what is done for the subtitles. The code let the bitmap untouched and instead modify the alphamap such that image*alphamap + bitmap doesn't overflow. This will make sure the subs get over the image, but it's not really correct blending. I don't like that system much anyway, if you want to change the alphamap you need to recompute the whole bitmap. Plus such operation is loosy, so no way to get back :( It doesn't matter much for the subs, but for the osd menu it is quiet annoying. Their is another alternative to the formula you have shown: newimage = alphamap*(newimage-bitmap) + bitmap It only add one substraction compared to the current method and it doesn't need you to botch your bitmap or alphamap before rendring. So what about using that instead ? Albeu
2006/3/31, Alban Bedel <albeu@free.fr>:
On Fri, 31 Mar 2006 10:53:53 +0300 "Ivan Kalvachev" <ikalvachev@gmail.com> wrote:
now let's say we have image pixles with variable "image" we have bitmap pixel with variable "bitmap" and alpha for the same bitmap called "alphamap", (all values are floats) so
newimage = image*alphamap + bitmap*(1-alphamap).
now, the (bitmap*(1-alphamap) depends only on the data from the bitmap, not the data from the image. This means it could be precalculated and stored. so:
newimage = image*alphamap + calcmap.
Now, if you have overflow you are most likely using "bitmap" instead of "calcmap" or there is some kind of rounding error (e.g. for speed reasons alpha 1.0 is 0xff not 0x100)
Seems so. However i still think their is a pb, at least that's not what is done for the subtitles. The code let the bitmap untouched and instead modify the alphamap such that image*alphamap + bitmap doesn't overflow. This will make sure the subs get over the image, but it's not really correct blending.
I don't like that system much anyway, if you want to change the alphamap you need to recompute the whole bitmap. Plus such operation is loosy, so no way to get back :( It doesn't matter much for the subs, but for the osd menu it is quiet annoying.
Their is another alternative to the formula you have shown:
newimage = alphamap*(newimage-bitmap) + bitmap
It only add one substraction compared to the current method and it doesn't need you to botch your bitmap or alphamap before rendring. So what about using that instead ?
I'll think about it. However I want to reproduce the bug you are trying to fix. Please give me instructions. (consider that i've never been able to make -menu work ;) About cmov, you can commit it right away, i think lavc uses it.
participants (3)
-
Alban Bedel -
Ivan Kalvachev -
Rich Felker