66082 - add gc-cache to xlib port

Status: VERIFIED FIXED

(Core :: XUL, defect)

Description

[Tomi Leppikangas]

xlib doesnt have gc-cache yet, that slows down it a bit.
I have done most of this work already, some problems with alpha images
and tiling.

Comment 1

[Tomi Leppikangas]

Attachment: patch that adds gc-cache to xlib + removes white bg

Comment 2

[Tomi Leppikangas]

Attachment: nsGCCache.cpp stolen from pavlov's gfx2

Comment 3

[Tomi Leppikangas]

Attachment: nsGCCache.h stolen from pavlov's gfx2

Comment 4

[Peter Trudelle]

Chris/Pav - who should own this?

Comment 5

[Tomi Leppikangas]

Attachment: better patch, now images work (allmost all)

Comment 6

[Tomi Leppikangas]

Here is new patch that adds gc-cache, and some optimisations to
nsWidget and nsWindow. There is bug in nsImage that i didnt find,
it crashes in viewer test #10

Comment 7

[Peter Trudelle]

->xlib guys

Comment 8

[Tomi Leppikangas]

Attachment: patch #3, this works right way

Comment 9

[Tomi Leppikangas]

This #3 patch works ok, tor helped to find bug which crahed in test #10.

I debugged this quite a lot, gc-cache doesnt leak any gc's, but xmon
shows that some gc's aren't freed, looks like there is leak of nsImageXlib
objects. And when they leak, some gc's are leaked too. TnsImageXlib leak
cause xpixmaps to leak too. (but thats not this patchs problem)

Comment 10

[Frank Belew]

Attachment: updated patch to resolve rejects on current tree, now just segfaults

Comment 11

[Frank Belew]

ok, after investigating a little bit more
it looks like someone leaked a lot of GC refs, since I got a SIGABORT on line 66
of nsGCCache.h which is:
  PRInt32 AddRef(void) {
(66)  if(mRefCnt>100) abort();
      NS_PRECONDITION(PRInt32(mRefCnt) >= 0, "illegal refcnt");
      ++mRefCnt;
      return mRefCnt;
  }

Comment 12

[Frank Belew]

Attachment: patch v2, new and improved with --no-crash enabled

Comment 13

[Frank Belew]

Ok, I found out that nsDrawingSurfaceXlib really didn't like being cast to an
nsRenderingContextXlib, so I copied the GetGC accessor to nsDrawingSurfaceXlib
and called it.
This causes the mGC to be returned properly, and not some random adress that
makes the recount look like 2^31 or something close

All in all, this current patch causes mozilla to run, but alpha, and animated
gifs are black

This also fixes the bug where you get a refresh flash before loading new pages
(with xlib)

Discussion on the imaages should move to #76820
The copious redraw is #82322

Comment 14

[pocemit]

Attachment: patch to the last patch to enable alpha images and fix CJK fonts

Comment 15

[pocemit]

This patch copies some things from the GTK port related to 1 and 8 bit alpha
images, adds (not implemented) function to draw scaled images, updates
::DrawString to draw CJK (multibyte) fonts correctly.

Also imoT's patch about DEFAULT_XLIB_FONT breaks default font selection for
themes - draws the UI in adobe-helvetica. The original version of using whatever
font was given looked better (used ms-tahoma on my system).

Comment 16

[pocemit]

Ah, yes another one I missed with the fonts - very important

--- /tmp/orig/xlib-gfx-v2/nsFontMetricsXlib.cpp Tue May 29 04:57:49 2001
+++ ../xlib/nsFontMetricsXlib.cpp       Tue May 29 05:17:20 2001
@@ -1724,7 +1724,7 @@
   if ((mFont->min_byte1 == 0) && (mFont->max_byte1 == 0))
     return XTextWidth(mFont, (char*) buf, len);
   else
-    return XTextWidth16(mFont, buf, len);
+    return XTextWidth16(mFont, buf, len / 2);
 }
 
 int

Comment 17

[Roland Mainz]

quy, please take a look at bug 16688 ("nsFontMetricsGTK::GetWidth/DrawString
ignore beyound 512 chars"). Maybe worth to pull the change over to Xlib-toolkit,
too...

Comment 18

[pocemit]

Attachment: image fixing patch 2.0, some cleanups and animated images support

Comment 19

[pocemit]

I messed up pretty badly cut & pasting v1 patch, this one is better, also
includes that multibyte font fix, and cleanups.

What does nsImageXlib::DrawImageOffscreen do?
It works without it. Gtk version doesn't have it.

Comment 20

[pocemit]

Attachment: implemented image scaling. patch for nsImageXlib.cpp after the last patch

Comment 21

[Roland Mainz]

Can anyone check if Xlib-toolkit now renders pages like
http://people.netscape.com/ftang/demo/utf8form.html correctly (tons of languages
in one document)...

Comment 22

[pocemit]

Yes, with the CJK font patch from earlier that page loads fine (I have most of
those fonts installed, and I have tested it).
I can attach a screenshot if needed. :)
It does not however, print them.
I looked at some of the comments in the bug attached to this, and will probably
look over nsFontMetricsXlib to see what can be done.

Comment 23

[Roland Mainz]

timeless:
Screenshots are always good (large one >=1152x900, please)... :-)
Sounds I will pull over these changes to Xprint, too (at least because I killed
some locales with my latest code-copies Xlib-toolkit-->Xprint
(nsFontmetrics*.*)...

Is it possible to set a milestone for this bug, please (what about 0.9.2) ?

Comment 24

[pocemit]

Attachment: the screenshot as requested

Comment 25

[Roland Mainz]

I've integrated the CJK-fixes into fixes for bug 77210... timeless... are the
CJK fonts in Xlib-toolkit screen output mainly on the left side, leaving the
right side blank ? It looks like the fonts are only using the left half of the
page... ;-(

Comment 26

[pocemit]

Hmm, i am "pocemit" on mozilla.org,
Secondly, what do you mean on the left side?
The screenshot shows them all centered, like supposed to be?
I have some other Japanese test pages like www.goo.ne.jp and it renders them
correctly.  Did you also apply the 1-line fix that was printed in a comment?
That one is important, otherwise it returns the widths incorrectly and causes
things to look bad.

Comment 27

[Roland Mainz]

Beep... yes... I missed that one-line patch in this comment... BAD... ;-(
Going to fix that...

Comment 28

[pocemit]

Attachment: patch to the last patch, adding scaling using XIE + scale.c

Comment 29

[pocemit]

This removes the slow smooth scaling code and adds newly ported XIE.c and
scale.c from the GTK toolkit. Makefile changes aren't included so you'd have to
add in your Makefile.in:

diff -u -r1.27 Makefile.in
--- Makefile.in 2001/05/17 18:25:21     1.27
+++ Makefile.in 2001/05/30 00:03:35
@@ -44,9 +44,18 @@
                nsRenderingContextXlib.cpp \
                nsScreenXlib.cpp \
                nsScreenManagerXlib.cpp \
-               nsPrintOptionsXlib.cpp \
+               nsPrintOptionsXlib.cpp \
+               nsGCCache.cpp \
                $(NULL)
 
+ifdef HAVE_XIE
+CSRCS          += XIE.c
+endif
+
+CSRCS          += scale.c
+
+EXPORTS                = nsGCCache.h
+
 include $(topsrcdir)/config/rules.mk
 
 ifndef MOZ_MONOLITHIC_TOOLKIT
@@ -57,10 +66,16 @@
 CXXFLAGS       += $(TK_CFLAGS)
 endif
 
+ifdef HAVE_XIE
+DEFINES         += -DHAVE_XIE
+GFX_XIE_LIBS    += $(MOZ_XIE_LIBS)
+endif
+
 EXTRA_DSO_LDOPTS += \
                $(NSPR_LIBS)    \
-               -lxpcom                 \
+               -lxpcom         \
                -lgkgfx         \
+               $(GFX_XIE_LIBS) \
                $(NULL)
 
 DEFINES                += -D_IMPL_NS_GFXNONXP


Also would someone want to look this bug over and assign a milestone or
something for  this?

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Before adding XIE support to the Xlib port please read
news://news.mozilla.org:119/[e-mail]
and the thread following.

Comment 31

[Roland Mainz]

And please read my reply to that posting...

Comment 32

[pocemit]

He makes sense.
XIE scaler does a nice fallback to the software scaling if XIE was for some
strange reason compiled in, it is #ifdef'd for those who don't have XIE like
mentioned in that post, and gdkpixbuf makes things depend on Gdk. These comments
are about gfx/gtk implementation of XIE scaler, since it has been proven to work
so far.

Comment 33

[Roland Mainz]

tim copperfield:
1. Could you please file a all-in-one patch (gdiff -r -u -N)
2. Uhm... what about overtaking this bug (e.g. reassign to you) ?
3. After step [2] please set a milestone - I really like to see this in 0.9.2...

Comment 34

[pocemit]

Attachment: all the patches here rolled into one plus Xlib native scrollbar fixes

Comment 35

[pocemit]

This one includes the original gc-cache patch plus
Myth's fixes to the current tree plus
Myth's fixes to the gc cache segfault plus
pocemit's fixes to the images plus
pocemit's fixes to scaled images plus
pocemit's fixes to the native scrollbar in nswidget.

Comment 36

[Christopher Blizzard (:blizzard)]

If you end up building gtk and xlib in the same tree this is going to conflict:

+EXPORTS = nsGCCache.h

Also, are you just copying those files? Shouldn't you pull them out of the gtk
port if you can?

+ mSurface = nsnull;
+ if(mDefaultFont)
+ XFreeFont(mDisplay, mDefaultFont);
+

If the default font handle is the same for all instances of the device context
why not share it in between all of them instead of making it a member of each one?

+ /* GC gc = XCreateGC(mDisplay,
(Drawable) mWidget,
0,
- NULL);
-
+ NULL);*/

If you're going to remove code then please actually remove it instead of just
commenting it out.

+ {
+ char *fontName = (char *)NULL;
+ unsigned long pr = 0;
+
+ ::XGetFontProperty(mDefaultFont, XA_FULL_NAME, &pr);
+ if(pr)
+ {
+ fontName = XGetAtomName(mDisplay, pr);
+ aInfo->mFont->name.AssignWithConversion(fontName);
+ ::XFree(fontName);
+ }

All of this stuff needs to be cached. The XGet* are usually server round trips.

+ // if (mGC)
+ // XFreeGC(mDisplay, mGC);

Once again, please remove this code if it's commented out and not needed for
something.

+ //mGC = XCreateGC(mDisplay, mDrawable, 0, NULL);

Same thing.

- mGC = XCreateGC(mDisplay, mDrawable, 0, NULL);
+ //mGC = XCreateGC(mDisplay, mDrawable, 0, NULL);

Same thing.

+ xGC * GetGC() { mGC->AddRef(); return mGC; }

Are you sure that's going to work every time? Is mGC always non-null?

+ fprintf(stderr, "Loading: %s\n", mName);

Spam. Please remove it.

+#include "drawers.h"

Are you copying this, too?

+  PRInt32 j = aSX + aSWidth;
+  PRInt32 z;

Can you use some self-describing variables there?  It makes the code
later harder to read.

+                           0, /* x offset, XXX fix this */

+    // no, it's still MSB XXX check on this!!
+    //      x_image->byte_order = LSBFirst;

You've got comments like this.  What's the deal?

+    // Write into the pixemap that is underneath gdk's mAlphaPixmap
+    // the image we just created.

I thought this was xlib? :)

+  /* XXX Why? DrawImageOffscreen(validX, validY, validWidth, validHeight); */

?

-class nsDrawingSurfaceXlib;
+// class nsDrawingSurfaceXlib;

+  //PRUint8    *mConvertedBits;

You should remove that if you are commenting it out.

+#define MAX(a, b) (((a) > (b)) ? (a) : (b))
+#define MIN(a, b) (((a) < (b)) ? (a) : (b))

I'm pretty sure these are defined in system headers somewhere.

+  float                mXScale;
+  float		       mYScale;

Watch your tabs.

-  attr.background_pixel = mBackgroundPixel;
-  attr.border_pixel = mBorderPixel;
+  /*attr.background_pixel = mBackgroundPixel;*/
+  /*attr.border_pixel = mBorderPixel;*/

-  attr_mask = CWBitGravity | CWEventMask | CWBackPixel | CWBorderPixel;
+  attr_mask = CWBitGravity | CWEventMask /*| CWBackPixel | CWBorderPixel*/;

More comments that should just be removed.

-  attr.background_pixel = mBackgroundPixel;
-  attr.border_pixel = mBorderPixel;
+  /*attr.border_pixel = mBorderPixel; */

-  attr_mask = CWBitGravity | CWEventMask | CWBackPixel | CWBorderPixel ;
+  attr_mask = CWBitGravity | CWEventMask  /*| CWBorderPixel*/ ;

*ahem*

+  PRBool nNeedToShow = PR_FALSE;

What is the 'n' prefix?

+
+  /*  if (aRepaint)
+      Invalidate(PR_FALSE);*/

Is that supposed to work?

I didn't review the XIE or GC cache code since I'm assuming that's
copied directly.

Comment 37

[Frank Belew]

Attachment 1 [details] [diff]: cleaned up patch with all changes that actually applies

All printfs are now either removed, or put in ifdefs
nsGCCache is no longer exported

Attachment 2 [details] [diff]: is an adapted scale.c
Attachment 3 [details] [diff]: is an adapted drawers.h

Comment 38

[Frank Belew]

Attachment: full patch that should apply from mozilla/

Comment 39

[Frank Belew]

Attachment: adapted scaled image routines

Comment 40

[Frank Belew]

Attachment: header for scale.c

Comment 41

[Frank Belew]

Attachment: adapted XIE code for alternate routines

Comment 42

[Tomi Leppikangas]

Patches seams to work ok. Xmon shows that CreateGC/ChangeGC counts
are near gtk ones. I hope to get thease in and open new bugs for
remaining issues.

Comment 43

[pocemit]

Attachment: new patch, replaces the last patch for widget and gfx

Comment 44

[pocemit]

I am sorry that things are posted under this bug, but most of these changes more
or less depend on the gc-cache addition... Plus its harder to keep track of
things otherwise.

Anyway, highlights of this patch:

* add Xlocale setting in AppShell, so that currently selected locale is properly
found by PlatformCharsetService and friends
* window title code adapted from the GTK version - correctly handles multibyte
titles
* cleanups per blizzards comment
* Context menus now work correctly
* Mouse/Keyboard grab fixed for popup menus + autocomplete window
* nsWidget now supports nsIWeakReference (part of context menu fix)
* rearranged nsWidget + nsWindow a little bit

Comment 45

[pocemit]

Attachment: new patch, fixes problems in last patch (particularly native widgets not getting Show().

Comment 46

[Tomi Leppikangas]

Patch 37516 works ok. I hope we get this in soon so it doesn't rot anymore,
gc patch has been hangin here over 4 months..

Comment 47

[Tomi Leppikangas]

Here is some build info:

Get latest patch from attachment 37516 [details] [diff] [review] and apply it.
Get attachments and name them as follows, all in mozilla/gfx/src/xlib/
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23077 -> nsGCCache.cpp
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23078 -> nsGCCache.h
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36963 -> scale.c
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36964 -> drawers.h
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36965 -> XIE.c

configure  --enable-toolkit=xlib --with-xlib

Comment 48

[timeless]

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

Comment 49

[Roland Mainz]

The patch looks very good... :-)

timecop:
Wanna integrate bug 16688 ("nsFontMetricsGTK::GetWidth/DrawString ignore beyound
512 chars") and bug 34242 ("X font lookups on -*-*-*-*-*-*-*-*-*-*-*-*-*-*")
into that patch, too ?

Comment 50

[pocemit]

I would love to do that (the -* fix looks interesting), as well as the buffer
bug, but can this PLEASE get reviewed and checked in before it becomes
un-appliable again?

Thank you.

Comment 51

[pocemit]

Tomi: Can you change the target milestone on this to something like 0.9.2 or so?
Perhaps more people would be willing to look into this then.

Comment 52

[Roland Mainz]

Setting target milestone to 0.9.2 and added "want for 0.9.2" to
status_whiteboard.

Comment 53

[Christopher Blizzard (:blizzard)]

Just some quick comments:

+  if (eventType == NS_MOUSE_RIGHT_BUTTON_DOWN)
+    eventType = NS_CONTEXTMENU;
+

Move that up into the ButtonPress: case above.  You are just resetting
it after it's already been set once and this violates the principal of
least surprise.

-  // XXX fix this
   pevent.time = 0;
-  //pevent.time = PR_Now();
-    //pevent.region = nsnull;

Don't even bother.  What events use time, anyway?

+/* Create scrollbar widget */
+void nsScrollbar::CreateNative(Window aParent, nsRect aRect)
+{
+  XSetWindowAttributes attr;
+  unsigned long attr_mask;
+
+  // on a window resize, we don't want to window contents to
+  // be discarded...
+  attr.bit_gravity = SouthEastGravity;

Don't you want NorthWest gravity instead of SouthEast?


+#define XLIB_DEFAULT_FONT1 "-*-helvetica-medium-r-*--*-140-*-*-*-*-iso8859-1"

Aren't you missing a '*' there in between r-* and *-140?

+  XFontStruct          *mDefaultFont;

That font is used all over and you're loading it from every class.
Why don't you make it a static and just destroy it when you are about
to shut down?

I could have nickel and dimed this patch a lot more but it's mostly
infastructure so in the interests of expediency I'm not going to.

Fix those and you have an r=blizzard

Comment 54

[pocemit]

I'll look into the other comments but the mDefaultFont thing, I really dont get
it - the font is only created once during DeviceContext::Init, then a bunch of
metrics is extracted off it (only happens 2 or 3 times), and then its freed at
shutdown.
What other classes are using it?

Comment 55

[Roland Mainz]

pocemit:
DeviceContext::Init() may be called multiple times in Mozilla's life (uhm...
once per window... I don't know anymore... ;-( ) - and then it does that stuff
multiple times, too. BAD.

Comment 56

[Roland Mainz]

pocemit:
If you are going to file a new patch to match r=blizzard please wait for bug
80224 - blizzard will r= it soon. It may be a good idea to migrate both patches.

BTW:
bug 16688 ("nsFontMetricsGTK::GetWidth/DrawString ignore beyound 512 chars") got
r=/sr=, too (currently waiting for a=) - if you're going to touch
nsFontMetricsXlib.cpp: wanna integrate that patch, too ?

Comment 57

[pocemit]

Attachment: new patch per blizzards comments

Comment 58

[pocemit]

Attachment: s/NorthWest/NorthWestGravity other than that, same thing as previous patch

Comment 59

[Roland Mainz]

The new patch is _great_ !!

Requesting sr= ...

Comment 60

[kinmoz]

sr=[e-mail]

Comment 61

[Roland Mainz]

asa:
Requesting a=[e-mail]

The risk associated with this patch is low as it patches only Xlib-toolkit
sources - which are all _not_ part of the default build...

Comment 62

[Asa Dotzler [:asa]]

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

Comment 63

[pocemit]

Things to be checked in:

Get latest patch from attachment 38436 [details] [diff] [review] and apply it.
Get attachments and name them as follows, all in mozilla/gfx/src/xlib/
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23077 -> nsGCCache.cpp
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=23078 -> nsGCCache.h
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36963 -> scale.c
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36964 -> drawers.h
http://bugzilla.mozilla.org/showattachment.cgi?attach_id=36965 -> XIE.c

That's it.

Comment 64

[Mike Kaply [:mkaply]]

Fix checked in for timecop by [e-mail]

Glad to be of service.

Marking fixed.

Comment 65

[Tomi Leppikangas]

Thanks for all who helped with this.
All seems to work ok now, marking as verified.