X11 alpha blending routines

VERIFIED FIXED

Status

()

Core
Internationalization
VERIFIED FIXED
16 years ago
16 years ago

People

(Reporter: kill this account, Assigned: Stuart Parmenter)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

16 years ago
Since the checkin for bug 90813 is ~4500 lines I have broken it down into
sub bugs to make the checking process more managable.

This bug covers the alpha blending (transparency) parts needed for
anti-aliased scaled bitmap fonts.
(Reporter)

Comment 1

16 years ago
Created attachment 55293 [details] [diff] [review]
alpha blending code (new file in gfx/src/gtk)
(Reporter)

Updated

16 years ago
Blocks: 90813

Comment 2

16 years ago
pavlov: please reassign this bug back to bstell once you finish the review. 
thanks
(Reporter)

Comment 3

16 years ago
I tested the following big/little endian combinations:

client             server              frame buffer bits per pixel
-----------        ------------------  -----------------------------
sheep (big)        sheep (big)         32 (0888)

sheep (big)        peregrine (little)  15 (555), 16 (565), 24 (888), 32 (0888)

peregrine (little) peregrine (little)  15 (555), 16 (565), 24 (888), 32 (0888)

peregrine (little) sheep (big)         32 (0888)
(Reporter)

Comment 4

16 years ago
if anyone can suggest a big endian server with a 15 (555), 16, or 24 (888) bit
per pixel server I'd be glad to test on that system.

Comment 5

16 years ago
Two things:

  * Blend calculations are shifting left by 8 for the final divide.  
    This will lead to rendering problems because you're dividing by 
    256 instead of 255.  The FAST_DIVIDE_BY_255 and MOZ_BLEND macros
    in nsColor.h are probably the sort of thing you want (but see the
    next point).

  * Much of this code is duplicating the functionality already existing
    in the DrawComposited* functions of gfx/src/gtk/nsImageGTK.cpp.
(Reporter)

Comment 6

16 years ago
FAST_DIVIDE_BY_255 does rounding which is a theoretically good thing. At one
point I did add code to address the fact that the shift by 8 could only
yield a 254 value.

However, to make the glyphs less blurry I spent a long time trying various
non-linear transforms of the values and was very surpised by the large amount 
of transform needed to affect the image. The net effect of the best transform 
I found is that values above 192 could be moved to 255 (or 254) without 
significant change to the image. Changes in the 64-192 range did affect the 
sharpness. As such the non-linear transform's affect overwhelms any rounding 
error. Hence I would prefer not to spend more CPU cycles where there is no 
visual gain.

The routines in this patch code are tuned to draw anti-aliased glyphs (grey 
scale images).  The DrawComposited* functions are tuned to blend 24 bit color 
images. Since anti-aliased glyphs are grey scale images it would unnecessarly 
increase the memory bloat by 3 times if the glyphs were cached as 24 bit 
images. 

Comment 7

16 years ago
FAST_DIVIDE_BY_255 is completely accurate for numbers in the range of alpha
blending calculations (0 -> 255*255).  This difference between dividing by
256 and 255 is visibly noticable - the first implementation of the
DrawComposited* functions had the same mistake and it was spotted by users.
(Reporter)

Comment 8

16 years ago
I would like to read the bug reports. 
Would you kindly give me the numbers?

Comment 9

16 years ago
Bug 51179 is what you're looking for.
(Reporter)

Comment 10

16 years ago
thanks
(Reporter)

Comment 11

16 years ago
from bug 51179:

> The important part is the bit of residue that appears when I overlap the files 
> (in this case, 10 times)... 

I only see a glyph being drawn one time. I cannot see any case where a glyph
would be overlayed on top of itself 10 times.

Does anyone see overlaying a glyph 10 times as a reasonable case?

I'm not adverse to changing the code if people see this as a reasonable case
but for glyphs I don't see this as a reasonable case (at present). If this is
not a reasonable case I would prefer not to waste CPU cycles.

 

Comment 12

16 years ago
Brian, because I'm not sure I understand the consequences of your "grayscale"
glyphs (isn't text colored in Mozilla?) here's a quick test if you're willing:

Create a solid background field of, say, green #00ff00
Use your code to Draw a bunch of text colored #00ff00 on that background
Screenshot
Load into say, Gimp, Photoshop or Paint Shop Pro
Crop to just the green area with "hidden" text
Auto stretch contrast (makes small color differences visible)

If the text (or parts of it) appear, you have a problem. If the text was already
visible before you took the screenshot then either you're very perceptive
(congratulations) or there is a very serious problem.

Assuming this test fails (ie your blend algorithm produces a quantitative
difference with just one blend) you have to decide whether it's worth using a
(more) accurate blend to fix this (rare) problem.

As to multiple blends, well I personally couldn't care less what happens when
people scribble multiple layers of text one over the other, but I'm not a web
designer.
(Reporter)

Comment 13

16 years ago
> (isn't text colored in Mozilla?) 

Yes, text is colored, but whether a particular pixel is on or off is not a 
color issue. Likewise whether a particular pixel is 50% or 0% or 100% 
solid (or transparent) is not a color issue. Thus the weights of each pixel 
in a glyph can be considered as being in a 0-255 grey scale and hence glyphs
are stored in a "grey scale" image. When drawn the weight of each pixel is 
then used as the alpha to blend the text color (foreground) with the 
background color.

> Create a solid background field of, say, green #00ff00
> Use your code to Draw a bunch of text colored #00ff00

I will agree that the blending function ((fg*alpha) + (bg*(255-alpha))>>8
where alpha varies from 0 to 255 will produce 254 instead of 255 (down by one).

ie:

   ((255*255) + (255*(255-255)))/256 = 254

(The correct function is ((fg*alpha) + (bg*(255-alpha))/255 but this involves
a division which may be expensive on some systems.)

I further agree that 
((((fg<<8)+fg+255)*alpha) + (((bg<<8)+bg+255)*(255-alpha))>>16
gives the correct result of 255 (actually 255.996 which truncates nicely
down to 255).

> Assuming this test fails (ie your blend algorithm produces a quantitative
> difference with just one blend) 

Lets assume it does produce 254 instead of 255.
 
> you have to decide whether it's worth using a (more) accurate blend to fix
> this (rare) problem.

Exactly, does the average case slow down a little so that a pathological case
can work?

I really don't have a strong opinion on this I just do not like to spend
CPU cycles if I cannot see a benefit.

> As to multiple blends, well I personally couldn't care less what happens when
> people scribble multiple layers of text one over the other, but I'm not a web
> designer.

In this case the inaccuracy will compound exactly as you predict. Is this a 
case mozilla needs to support?

Anyone care to express an opinion on this point?

If there is consensus that this is important I will gladly change the
code.
What about the effective code duplication tor mentioned under point 2?

/be
(Reporter)

Comment 15

16 years ago
> What about the effective code duplication tor mentioned under point 2?

The routines in this patch code are tuned to draw/blend 8 bit (grey 
scale) anti-aliased glyphs directly to color using a supplied text 
color whereas the DrawComposited* functions are tuned to directly
blend 24 bit color images. This code stores the anti-aliased glyphs 
as grey scale images since the same glyph may be drawn on the display in
several different colors. To use the DrawComposited* routines this code 
would need to convert the 8 bit grey images to 24 bit color images
then send the 24 bit color images to the DrawComposited* routines.

Because the text color is not generally 255/255/255, the conversion to 24
bit color would have to be done using 3 multiplications per pixel to convert 
the grey to color. The pixels in the resulting 24 bit color image would then 
again be multiplied (3 multiplies per pixel) in blending portion of the 
DrawComposited* routines.

Sharing the DrawComposited* routines is possible but it would mean doubling 
the multiplies and divides/shifts.

Comment 16

16 years ago
With revised patch of
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=57234,
I'm now seeing UMR in purify outputs on Solaris.

Comment 17

16 years ago
Created attachment 57577 [details]
UMR (Uninitialized memory read)
(Reporter)

Comment 18

16 years ago
attachment 57234 [details] [diff] [review] is currently out of date.

I will build a purify version using the latest version (see bug 90813)
http://bugzilla.mozilla.org/attachment.cgi?id=57544&action=view
and resolve the UMR.

(Assignee)

Comment 19

16 years ago
Comment on attachment 55293 [details] [diff] [review]
alpha blending code (new file in gfx/src/gtk)

fix the UMR and r=pavlov
Attachment #55293 - Flags: review+
(Reporter)

Comment 20

16 years ago
I just purified the latest version, bug 90813 attachment 57544 [details] [diff] [review], and
I did not get a UMR at nsBlendMonoImage0888. I did put in a printf at
that function to make sure it was being called. I checked
http://home.netscape.com/ja and http://www.china.com/zh_cn

Katakai: could you update your version and re-run purify?
Also, what URLs did you test?
(Reporter)

Comment 21

16 years ago
Katakai: you were running a 32 bit solaris client on a solaris X server, 
right?

Comment 22

16 years ago
Yes, I'm using 32 bit Solaris and 24 depth X server.

I visted www.yahoo.co.jp and change the glyph size to 200%,
also open mailnews and read some japanese messages.

For testing purpose, I use only 18 point ascii fonts and
24 point japanese fonts, described in bug 107020.

mkdir /tmp/a
cd /tmp/a
cp /usr/openwin/lib/X11/fonts/F3bitmaps/Lucida*18* .
cp /usr/openwin/lib/locale/ja/X11/fonts/TTbitmaps/HG-MinchoL-*24* .
/usr/openwin/bin/mkfontdir .
modify fonts.dir to change "lucida sans" to "katakai", "hg mincho l" to "katakai"
specify "katakai" for western and japanese fonts in prefs
Now ascii and japanese fonts of 18 and 24 points are scalled and used.

I tried the patch now and I'm still seeing the UMR.

Comment 23

16 years ago
****  Purify instrumented ../../xpfe/bootstrap/mozilla-bin.pure (pid 1047)  ****
UMR: Uninitialized memory read (1188 times):
  * This is occurring while in:
	void nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int) [nsX11AlphaBlend.cpp:501]
	void
nsXFontAAScaledBitmap::DrawText8or16(_GdkWindow*,_GdkGC*,int,int,void*,unsigned)
[nsXFontAAScaledBitmap.cpp:270]
	void nsXFontAAScaledBitmap::DrawText16(_GdkWindow*,_GdkGC*,int,int,const
XChar2b*,unsigned) [nsXFontAAScaledBitmap.cpp:134]
	int
nsFontGTKNormal::DrawString(nsRenderingContextGTK*,nsDrawingSurfaceGTK*,int,int,const
unsigned short*,unsigned) [nsFontMetricsGTK.cpp:2225]
	unsigned nsRenderingContextGTK::DrawString(const unsigned
short*,unsigned,int,int,int,const int*) [nsRenderingContextGTK.cpp:1629]
	void
nsTextFrame::PaintUnicodeText(nsIPresContext*,nsIRenderingContext&,nsIStyleContext*,nsTextFrame::TextStyle&,int,int)
[nsTextFrame.cpp]
  * Reading 4 bytes from 0xffbeb434 on the stack.
  * Address 0xffbeb434 is local variable "(void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int))#block 4::dst_pixel" in function void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int).

****  Purify instrumented ../../xpfe/bootstrap/mozilla-bin.pure (pid 1047)  ****
UMR: Uninitialized memory read (1188 times):
  * This is occurring while in:
	void nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int) [nsX11AlphaBlend.cpp:502]
	void
nsXFontAAScaledBitmap::DrawText8or16(_GdkWindow*,_GdkGC*,int,int,void*,unsigned)
[nsXFontAAScaledBitmap.cpp:270]
	void nsXFontAAScaledBitmap::DrawText16(_GdkWindow*,_GdkGC*,int,int,const
XChar2b*,unsigned) [nsXFontAAScaledBitmap.cpp:134]
	int
nsFontGTKNormal::DrawString(nsRenderingContextGTK*,nsDrawingSurfaceGTK*,int,int,const
unsigned short*,unsigned) [nsFontMetricsGTK.cpp:2225]
	unsigned nsRenderingContextGTK::DrawString(const unsigned
short*,unsigned,int,int,int,const int*) [nsRenderingContextGTK.cpp:1629]
	void
nsTextFrame::PaintUnicodeText(nsIPresContext*,nsIRenderingContext&,nsIStyleContext*,nsTextFrame::TextStyle&,int,int)
[nsTextFrame.cpp]
  * Reading 4 bytes from 0xffbeb434 on the stack.
  * Address 0xffbeb434 is local variable "(void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int))#block 4::dst_pixel" in function void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int).

****  Purify instrumented ../../xpfe/bootstrap/mozilla-bin.pure (pid 1047)  ****
UMR: Uninitialized memory read (1188 times):
  * This is occurring while in:
	void nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int) [nsX11AlphaBlend.cpp:503]
	void
nsXFontAAScaledBitmap::DrawText8or16(_GdkWindow*,_GdkGC*,int,int,void*,unsigned)
[nsXFontAAScaledBitmap.cpp:270]
	void nsXFontAAScaledBitmap::DrawText16(_GdkWindow*,_GdkGC*,int,int,const
XChar2b*,unsigned) [nsXFontAAScaledBitmap.cpp:134]
	int
nsFontGTKNormal::DrawString(nsRenderingContextGTK*,nsDrawingSurfaceGTK*,int,int,const
unsigned short*,unsigned) [nsFontMetricsGTK.cpp:2225]
	unsigned nsRenderingContextGTK::DrawString(const unsigned
short*,unsigned,int,int,int,const int*) [nsRenderingContextGTK.cpp:1629]
	void
nsTextFrame::PaintUnicodeText(nsIPresContext*,nsIRenderingContext&,nsIStyleContext*,nsTextFrame::TextStyle&,int,int)
[nsTextFrame.cpp]
  * Reading 4 bytes from 0xffbeb434 on the stack.
  * Address 0xffbeb434 is local variable "(void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int))#block 4::dst_pixel" in function void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int).

****  Purify instrumented ../../xpfe/bootstrap/mozilla-bin.pure (pid 1047)  ****
UMR: Uninitialized memory read (519 times):
  * This is occurring while in:
	void nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int) [nsX11AlphaBlend.cpp:501]
	void
nsXFontAAScaledBitmap::DrawText8or16(_GdkWindow*,_GdkGC*,int,int,void*,unsigned)
[nsXFontAAScaledBitmap.cpp:270]
	void nsXFontAAScaledBitmap::DrawText16(_GdkWindow*,_GdkGC*,int,int,const
XChar2b*,unsigned) [nsXFontAAScaledBitmap.cpp:134]
	int
nsFontGTKNormal::DrawString(nsRenderingContextGTK*,nsDrawingSurfaceGTK*,int,int,const
unsigned short*,unsigned) [nsFontMetricsGTK.cpp:2225]
	unsigned nsRenderingContextGTK::DrawString(const unsigned
short*,unsigned,int,int,int,const int*) [nsRenderingContextGTK.cpp:1595]
	void
nsTextFrame::PaintUnicodeText(nsIPresContext*,nsIRenderingContext&,nsIStyleContext*,nsTextFrame::TextStyle&,int,int)
[nsTextFrame.cpp]
  * Reading 4 bytes from 0xffbea08c on the stack.
  * Address 0xffbea08c is local variable "(void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int))#block 4::dst_pixel" in function void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int).

****  Purify instrumented ../../xpfe/bootstrap/mozilla-bin.pure (pid 1047)  ****
UMR: Uninitialized memory read (519 times):
  * This is occurring while in:
	void nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int) [nsX11AlphaBlend.cpp:502]
	void
nsXFontAAScaledBitmap::DrawText8or16(_GdkWindow*,_GdkGC*,int,int,void*,unsigned)
[nsXFontAAScaledBitmap.cpp:270]
	void nsXFontAAScaledBitmap::DrawText16(_GdkWindow*,_GdkGC*,int,int,const
XChar2b*,unsigned) [nsXFontAAScaledBitmap.cpp:134]
	int
nsFontGTKNormal::DrawString(nsRenderingContextGTK*,nsDrawingSurfaceGTK*,int,int,const
unsigned short*,unsigned) [nsFontMetricsGTK.cpp:2225]
	unsigned nsRenderingContextGTK::DrawString(const unsigned
short*,unsigned,int,int,int,const int*) [nsRenderingContextGTK.cpp:1595]
	void
nsTextFrame::PaintUnicodeText(nsIPresContext*,nsIRenderingContext&,nsIStyleContext*,nsTextFrame::TextStyle&,int,int)
[nsTextFrame.cpp]
  * Reading 4 bytes from 0xffbea08c on the stack.
  * Address 0xffbea08c is local variable "(void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int))#block 4::dst_pixel" in function void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int).

****  Purify instrumented ../../xpfe/bootstrap/mozilla-bin.pure (pid 1047)  ****
UMR: Uninitialized memory read (519 times):
  * This is occurring while in:
	void nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int) [nsX11AlphaBlend.cpp:503]
	void
nsXFontAAScaledBitmap::DrawText8or16(_GdkWindow*,_GdkGC*,int,int,void*,unsigned)
[nsXFontAAScaledBitmap.cpp:270]
	void nsXFontAAScaledBitmap::DrawText16(_GdkWindow*,_GdkGC*,int,int,const
XChar2b*,unsigned) [nsXFontAAScaledBitmap.cpp:134]
	int
nsFontGTKNormal::DrawString(nsRenderingContextGTK*,nsDrawingSurfaceGTK*,int,int,const
unsigned short*,unsigned) [nsFontMetricsGTK.cpp:2225]
	unsigned nsRenderingContextGTK::DrawString(const unsigned
short*,unsigned,int,int,int,const int*) [nsRenderingContextGTK.cpp:1595]
	void
nsTextFrame::PaintUnicodeText(nsIPresContext*,nsIRenderingContext&,nsIStyleContext*,nsTextFrame::TextStyle&,int,int)
[nsTextFrame.cpp]
  * Reading 4 bytes from 0xffbea08c on the stack.
  * Address 0xffbea08c is local variable "(void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int))#block 4::dst_pixel" in function void
nsBlendMonoImage0888(_XImage*,nsAntiAliasedGlyph*,unsigned
char*,unsigned,int,int).
(Reporter)

Comment 24

16 years ago
Here is the code in nsX11AlphaBlend.cpp around line 501:

470  static void
471  nsBlendMonoImage0888(XImage *ximage, nsAntiAliasedGlyph * glyph,
472                       PRUint8 *aWeightTable, nscolor color, int xOff, int
yOff)
473  {
474    PRUint32 src_a, dst_a;
475
476    int xfer_width  = MIN((int)glyph->GetWidth(),  ximage->width-xOff);
477    int xfer_height = MIN((int)glyph->GetHeight(), ximage->height-yOff);
478    PRUint16 r = NS_GET_R(color);
479    PRUint16 g = NS_GET_G(color);
480    PRUint16 b = NS_GET_B(color);
481
482    PRUint8 *glyph_p = glyph->GetBuffer();
483    PRUint8 *imageLineStart = (PRUint8 *)ximage->data
484                               + 4*xOff + (yOff * ximage->bytes_per_line);
485
486    for (int row=0; row<xfer_height; row++) {
487      PRUint32 *image_p = (PRUint32 *)imageLineStart;
488      for (int j=0; j<xfer_width; j++,image_p++,glyph_p++) {
489        src_a = *glyph_p;
490        if (src_a == 0)
491          continue;
492        src_a = aWeightTable[src_a]; // weight the image
493        PRUint32 dst_pixel = *image_p;
494        PRUint32 hibits = dst_pixel & 0xFF000000;
495        if (src_a == 255) {
496          *image_p = hibits | (r << 16) | (g << 8) + b;
497          continue;
498        }
499        dst_a = 255 - src_a;
500
501        PRUint32 red   = ((r*src_a) + (((dst_pixel>>16)&0xFF) * dst_a)) >> 8;
502        PRUint32 green = ((g*src_a) + (((dst_pixel>>8) &0xFF) * dst_a)) >> 8;
503        PRUint32 blue  = ((b*src_a) + (( dst_pixel     &0xFF) * dst_a)) >> 8;
504        *image_p = hibits | (red << 16) | (green << 8) | blue;
505      }

On line 501 the follow variables are accessed:

  red       - being set
  r         - set on line 478
  src_a     - set on line src_a
  dst_pixel - set on line 493
  dst_a     - set on line dst_a

Purify says dst_pixel is unitialized but it was initialized on line 493.

Any suggestion how there could be a UMR here?

Comment 25

16 years ago
Comment on attachment 55293 [details] [diff] [review]
alpha blending code (new file in gfx/src/gtk)

sr=waterson
Attachment #55293 - Flags: superreview+

Comment 26

16 years ago
I'm stumped by the UMR. The rest looks good to me.
(Reporter)

Comment 27

16 years ago
chris: thanks for the speedy super-review

checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 28

16 years ago
The UMR might be ocurring because you're dereferencing
a (PRUint8 *) as a (PRUint32 *) and have thus wandered a
byte or two into the between-line padding (since
ximage->bytes_per_line can cheerfully be larger than the
useful pixel data span size) which no-one has ever
written meaningful data to since it was created.

Well, that's one explanation.

I have to admit that the whole recast of a (PRUint8 *) as
a (PRUint32 *) and then doing a quadword-fetch through it
worries me a little; some architectures (IIRC sparc, alpha)
fault every time a word-fetch is performed at a
non-word-aligned address so you'll have to be pretty sure
that those addresses are guaranteed to be aligned.
(Reporter)

Comment 29

16 years ago
> The UMR might be ocurring because you're dereferencing
> a (PRUint8 *) as a (PRUint32 *)

I thought of this but this could only be true if purify said the UMR 
was at line 493 where the pointer is used not 501 where the variable is used.

> some architectures (IIRC sparc, alpha)
> fault every time a word-fetch is performed at a
> non-word-aligned address so you'll have to be pretty sure
> that those addresses are guaranteed to be aligned

The address is initialized on line 483 to:

  ximage->data // a malloc'd address so it should be aligned
  + + 4*xOff   // the '4*' maintains alignment)
  + (yOff * ximage->bytes_per_line) // ximage->bytes_per_line should be a
                                    // multiple of 4 since the frame buffer has
                                    // a 32 bit depth

The address is changed by line 488:

    image_p++ // a post-increment of a PRUint32 pointer

and byt line 507:

507      imageLineStart += ximage->bytes_per_line; // again using
                                                   // ximage->bytes_per_line
bstell, how about some minimal assertions that quantities are 0 mod 4, to back
up your comment #29?

Adam's explanation of the UMR rings true.  Do we care?  We have "noise" UMRs in
purify output (e.g., due to bitfields, where purify lacks type info needed to
know that the UMR is benign).

/be

Comment 31

16 years ago
I did note that the UMR seemed to be upon the
use of dst_pixel instead of the explicit dereference
in the initialisation of dst_pixel, but supposed that
the compiler had just done something wacky like lazily
delayed the actual dereference until lines 496/501 (n.b.
hibits is only used once on each branch).

It's pretty academic anyway.  I'm generally with whatever
works and is fast (and is 'good enough', like the shift-as-
rough-divide!).

(Reporter)

Comment 32

16 years ago
> how about some minimal assertions that quantities are 0 mod 4

I will add asserts. Do I need r=/sr= for this?

> supposed that the compiler had just done something wacky like lazily
> delayed the actual dereference until lines 496/501 

I will setup the fonts as Katakai did and see if I can get the UMR to happen.
But lacking my getting the UMR to happen for me I don't know how
to proceed on it. (To do this I'll need to sit in at Pavlov's system
so I can control the X server / font path.)

Comment 33

16 years ago
> I will setup the fonts as Katakai did and see if I can get the UMR to happen.
> But lacking my getting the UMR to happen for me I don't know how
> to proceed on it. (To do this I'll need to sit in at Pavlov's system
> so I can control the X server / font path.)

I'm not your manager [:)] but if I had the power to control
other peoples' work-time then I'd say that in all liklihood
this is a genuinely-harmless UMR and not worth spending
quality time chasing unless/until there's evidence that it
really has fangs.

FWIW I can't see anything else obnoxious in the cited code.

bstell: you can add the asserts without review, I bet -- hard to mess up, DEBUG
only, etc.  Your call.

I agree with Adam, this UMR is not worth "fixing."

/be
(Reporter)

Comment 35

16 years ago
added these asserts:
 479   PRUint16 g = NS_GET_G(color);
 480   PRUint16 b = NS_GET_B(color);
 481 
+482   NS_ASSERTION(((ximage->data-(char*)0)&3)==0,"possible alignment error");
+483   NS_ASSERTION((ximage->bytes_per_line&3)==0,"possible alignment error");
 484   PRUint8 *glyph_p = glyph->GetBuffer();
 485   PRUint8 *imageLineStart = (PRUint8 *)ximage->data
 486                              + 4*xOff + (yOff * ximage->bytes_per_line);
 487 

Comment 36

16 years ago
Verified
Status: RESOLVED → VERIFIED

Comment 37

16 years ago
Re. the UMR: what if either xfer_width or xfer_height == 0?

*We* know that this won't happen - but the compiler doesn't. So it can't
assume that the for() loop bodies will always be executed.
(Reporter)

Comment 38

16 years ago
> Re. the UMR: what if either xfer_width or xfer_height == 0?
> 
> *We* know that this won't happen - but the compiler doesn't. So it can't
> assume that the for() loop bodies will always be executed.

Purify is a *runtime* tool not a compilier tool. It only reports what it
actually saw happening.

I was finally able to reproduce this UMR but for the life of me I cannot make
sense of it.
You need to log in before you can comment on or make changes to this bug.