83920 - Remove XIE from gtk port

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[tor]

If we follow through on the threat to eliminate XIE and hand scaling from
the gtk port (see blizzard's post "Image rendering and XIE" in mozilla.unix),
here's the first part.  This just removes the XIE code.  XIE.c should
be removed when the patch is applied.

Comment 1

[tor]

Attachment: remove xie code

Comment 2

[Christopher Blizzard (:blizzard)]

r/sr=blizzard

Comment 3

[Stuart Parmenter]

i don't think we should remove this until we make 1bit alpha masks scale using 
gdkpixbuf

Comment 4

[Christopher Blizzard (:blizzard)]

what about the hand scaling functions?

Comment 5

[Stuart Parmenter]

that one works i think

Comment 6

[tor]

The following patch plus the two attached files (scale.cpp and scale.h) remove
XIE and gdkpixbuf.  The handscaling is faster than the previous version and
can handle 8-bit alpha in additional to the previously supported 1-bit alpha.

The following files are unneeded after this patch: XIE.c, drawers.h, scale.c.
You'll need to remove at least scale.c from your tree to try the patch (make
gets confused by having scale.cpp and scale.c).

Comment 7

[tor]

Attachment: wire in new scaler

Comment 8

[tor]

Attachment: scale.h

Comment 9

[tor]

Attachment: scale.cpp

Comment 10

[tor]

Attachment: updated scale.cpp - fix single-pixel dimension edge case

Comment 11

[Stuart Parmenter]

sweet.  you're my hero.  i'll give it an r= in a bit.

Comment 12

[tor]

There's a problem with clipping and 8-bit alpha scaling.  Investigating...

Comment 13

[tor]

Attachment: updated patch - fix clipping calc for 8-bit alpha scaling

Comment 14

[Chris Hiner]

I just froze a cvs build from this afternoon, with the 6/14 19:29 patch, while
trying to visit http://www.mozillazine.org.  I've been able to duplicate this
repeatedly.  Without the patch, it works fine.
I broke back to gdb, and I'll attach the backtrace.

If you need more details, tell me what you need.  Other than this hang, it's
been working quite well, although it feels a bit slower than the XIE version.

Comment 15

[Chris Hiner]

Attachment: Backtrace of http://www.mozillazine.org hang.

Comment 16

[tor]

Which version of scale.cpp did you use?  The first one had a hang problem.

Comment 17

[tor]

Attachment: odd corruption in the last patch - fresh version

Comment 18

[Chris Hiner]

I must have had the older version.  With the latest version of scale.cpp and the
patch on a new build today it's working great.  Sorry for the confusion.

Comment 19

[Stuart Parmenter]

ok, i'm good with this going in.  r=pavlov (and module owner approval).  lets 
get this sucker in to the tree.  blizzard:  can you sr= it?

Comment 20

[Roland Mainz]

Is it possible to keep XIE enabled for Solaris ? 
Xsun in Solaris has a working XIE implementation... I do not like to see code
removed which gives a noticeable speed enhancement just "because it's broken in
Xfree86 CVS"...

Comment 21

[tor]

Do you have performance measurements to back up the claim of "noticeable speed
enhancement"?

Comment 22

[Roland Mainz]

Sure. Run http://211.9.115.254/text.html on a Sun Ultra5/333MHz (U5 has a dumb
m64-based framebuffer).
CPU load with XIE enabled:  ~28%
CPU load with XIE disabled: ~96%

Uhm... I think this doesn't need futher comments... :-)

Comment 23

[tor]

Don't have that machine configuration lying around, but with this patch
on a 500MHz pentium-3 it runs at about 27% cpu.  There's a fairly obvious
optimization which I didn't bother complicating this patch with that
would improve that number somewhat for your example.

Comment 24

[Roland Mainz]

tor:
1. Did you test this on a Solaris machine (SPARC/x86) ? I was talking about XIE
in Xsun/Solaris...
2. This heavily depends on the type of framebuffer. If you have a _fast_ machine
with a framebuffer where many operations are accerlated (like ATI >= Rage128)
the performance improvement is lower than for the dumb framebuffers like m64 (or
SunRay[1]).

I see a huge advantage for things like Xterminals, Sun's SunRay and so on... -
that's why I want XIE in Mozilla5...

[1]= SunRay does not use X11 itself, instead a SunRay driver is plugged into
Xsun which talks to the SunRay terminals using it's own protocol. Most drawing
stuff is "done" by the CPU on the host machine...

Comment 25

[Christopher Blizzard (:blizzard)]

My concern with XIE is that if we allow the XIE code into the Mozilla tree Sun
can't kill it down the road.  It's unsupported by the X community which Sun does
share source with.

You have been saying that XFree's implementation is broken.  It's not broken in
XFree.  I use it via Mozilla and it's appeared to work fine.  That's not the
issue at all.  The issue is that no one uses it, no one supports it, and the X
maintainers want it dead.

It might be faster than what's here now but that's not the issue.  This isn't
entirely a technical issue.  The issue is that we are using a piece of code that
was stillborn.  It was DOA and we shouldn't encourage it's use and you shouldn't
either.

Comment 26

[Roland Mainz]

richb... wanna comment this ?

Comment 27

[Christopher Blizzard (:blizzard)]

+    PRUint8 *scaled = (PRUint8 *)nsMemory::Alloc(aDHeight*((aDWidth+7)>>3));
+    memset(scaled, 0, aDHeight*((aDWidth+7)>>3));
+    RectStretch(aSX, aSY, aSX+aSWidth-1, aSY+aSHeight-1,
+                0, 0, aDWidth-1, aDHeight-1,
+                mAlphaBits, mAlphaRowBytes, scaled, (aDWidth+7)>>3, 1);

Obfuscate me harder!  Please document those bit shifts with the addition.  I
know what they do but other people might not.  In fact, why don't you make that
a self describing macro since you use it in quite a few places?

Also, you don't handle the case where that Alloc() will fail.

+  PRUint8 *scaled = (PRUint8 *)nsMemory::Alloc(3*aDWidth*aDHeight);

Same with not handling the failed allocation case.

+    scaledImage = (PRUint8 *)nsMemory::Alloc(3*aDWidth*aDHeight);

Same.

+    scaledAlpha = (PRUint8 *)nsMemory::Alloc(aDWidth*aDHeight);

Same.

Fix that and you've got an sr=blizzard.

--Chris

Comment 28

[Rich Burridge]

I have no problem with this being removed. Blizzard and tor know what they
are doing.

Comment 29

[Roland Mainz]

Agreed.

Comment 30

[tor]

Attachment: same + less obfuscation and error checking

Comment 31

[tor]

Attachment: same + address blizzard's concerns

Comment 32

[z-aillon.c-obsolete]

I agree too with this being done because libXIE been removed from the XFree86
4.1.0 RPMs I've seen.  If you install XFree86 4.1.0 by RPM (which many users
will do), you will have NOT have libXIE.so.  It will remove it if it exists due
to upgrade.  This will cause Mozilla Not to Load at all. (temporarily move
libXIE out of /usr/X11R6/lib/  to see what happens).  I was going to post
earlier, but for some reason or other, I got distracted and forgot, but is it
too late to get a relnote for 0.9.1?  I've already fallen prey to this one as
have a few other people I know.

Comment 33

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 34

[tor]

Checked in.

Comment 35

[tor]

And closed...

Comment 36

[R.K.Aa.]

and problem. See bug 86529

Comment 37

[Jeremy M. Dolan]

*** Bug 86560 has been marked as a duplicate of this bug. ***

Comment 38

[Tom Mraz]

XIE isn't there anymore -> verified.

Comment 39

[Christopher Blizzard (:blizzard)]

*** Bug 87178 has been marked as a duplicate of this bug. ***

Comment 40

[dylang]

<random comment>
>+    PRUint8 *scaled = (PRUint8 *)nsMemory::Alloc(aDHeight*((aDWidth+7)>>3));
>+    memset(scaled, 0, aDHeight*((aDWidth+7)>>3));
>+    RectStretch(aSX, aSY, aSX+aSWidth-1, aSY+aSHeight-1,
>+                0, 0, aDWidth-1, aDHeight-1,
>+                mAlphaBits, mAlphaRowBytes, scaled, (aDWidth+7)>>3, 1);

>Obfuscate me harder!  Please document those bit shifts with the addition.  I
>know what they do but other people might not.  In fact, why don't you make that
>a self describing macro since you use it in quite a few places?


Wow.. didn't we learn anything from "Writing Solid Code"? Dump the shifts/etc
entirely, unless there is a specific reason for them!  div/muls should be
properly optimized into shifts if the compiler can do so safely, and you
preserve portability between endianess.  :)

</Random comment>

Comment 41

[Jim Dunn]

*** Bug 76699 has been marked as a duplicate of this bug. ***