Closed Bug 557159 Opened 14 years ago Closed 14 years ago

[OS/2] Optimize Cairo/Thebes surfaces

Categories

(Core :: Graphics, defect)

x86
OS/2
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: dragtext, Assigned: dragtext)

References

Details

Attachments

(4 files, 4 obsolete files)

This patch reduces system resources required by Thebes/Cairo surfaces.  It also provides small memory use & performance improvements, fixes a bug, and simplifies some existing code.
Attached patch Surface Part 1 - Cairo/Thebes (obsolete) — Splinter Review
Part 1:  For every new window, nsWindow creates a Thebes surface which in turn creates a Cairo surface.  As part of this process, Thebes allocates a presentation space (HPS) for the window.  This HPS is never used because any time one is needed, nsWindow::OnPaint() supplies one.

This patch eliminates creation of HPSs for windowing surfaces by modifying gfxOS2Surface.cpp and adding a new convenience function, cairo_os2_surface_create_for_window(), to cairo-os2-surface.c.  If either module needs an HPS and one isn't available, it will be created.

While working in cairo-os2-surface.c, I found & fixed a major bug but I don't know if the affected code is ever used.  If a video driver can't handle a 32-bit bitmap, Cairo-OS/2 creates a 24-bit version.  However, the rows of this bitmap aren't DWORD-aligned, causing the browser to crash as soon as it tries to display a window.

In the same file, I also rewrote bits of code that were either incorrect or redundant, and imported a function from a later version of Cairo-OS/2.
Attached patch Surface Part 2 - nsWindow (obsolete) — Splinter Review
Part 2:  For every window that you can see, there's another window of the same size behind it that you normally can't see.  Even though these hidden windows may never be painted, the current code still creates Thebes and Cairo surfaces for them.

This patch suppresses creation of surfaces for such windows.  Occassionally, these windows do become visible:  for example, if you open a link in a new tab and switch to it before the browser has connected to the website.  In cases like this, the patch fills the window with the default background color to eliminate artifacts of whatever had been on screen previously.

At first I thought this would produce major memory savings because Cairo allocates space for a 32-bpp bitmap the size of the window - this can be 2mb or more.  However, it doesn't because no physical memory is used if Cairo never has to paint the window.  Still, it produces modest reductions in memory usage & overhead.  Users with slow connections may benefit the most because they're the most likely to see these hidden windows.
Rich, I think the first patch should be split up between the Cairo bits and the gfxOS2Surface bits. The Cairo bits will have to be posted upstream.
I'll also see if I can wrestle git into downloading the latest Cairo to test this there and perhaps create the diff.
Please make sure to get someone running the Matrox driver to test the revised cairo code. That 24bit code path was entirely written for them and tested to work fine (but slow, of course).
The "Since: 1.6" in the new cairo APIs probably has to be 1.10 which I think is the next cairo version.

Nice to see the Thebes changes, I had something similar in my tree for a long time, but because that didn't fix the printing stuff I didn't pursue it further (and it was never as well thought out as your changes). Can you not also get rid of the mBitmap stuff, too? cairo creates one internally so the one in gfxOS2Surface never gets used, right?
(In reply to comment #3)
> Rich, I think the first patch should be split up between the Cairo bits and the
> gfxOS2Surface bits. The Cairo bits will have to be posted upstream.

Yes, that should also give Doodle a chance to comment.

> I'll also see if I can wrestle git into downloading the latest Cairo to test
> this there and perhaps create the diff.

Wasn't SmartGIT announced on OS2World to also run with Java on OS/2? Wanted to test that for a long time, but never got the chance.
(In reply to comment #4)
> Please make sure to get someone running the Matrox driver to test the revised
> cairo code. That 24bit code path was entirely written for them and tested to
> work fine (but slow, of course).

I don't know how that could be.  Bitmap rows must be DWORD-aligned with padding at the end of each row, and these aren't.  My tests crashed because the raw bitmap data area was at least 1kb shorter than PM thought it should be, so it accessed unallocated memory.  In other circumstances, there might not have been a crash but the displayed bitmap would be askew & have garbage at the top(?).

> Nice to see the Thebes changes, I had something similar in my tree for a long
> time, but because that didn't fix the printing stuff I didn't pursue it further
> (and it was never as well thought out as your changes). Can you not also get
> rid of the mBitmap stuff, too? cairo creates one internally so the one in
> gfxOS2Surface never gets used, right?

No bitmap is created for windowed surfaces - and that's all I was focused on.
(In reply to comment #6)
>> Please make sure to get someone running the Matrox driver to test the revised
>> cairo code. That 24bit code path was entirely written for them and tested to
>> work fine (but slow, of course).
> 
> bitmap data area was at least 1kb shorter than PM thought it should be, so it
> accessed unallocated memory.  In other circumstances, there might not have 
> been a crash but the displayed bitmap would be askew & have garbage at the 
> top(?).

The Matrox drivers implemented a lot of stuff of their own, since they were based on the old, pre-GRADD model.  You can expect them to have their own set of bugs and way of working.  :-/

For the longest time, they really were the best choice for OS/2 display drivers, and they were certainly popular.  It's probably getting difficult to find people still using them now, though.
I don't remember details of why the cairo 24bit codepath ended this way and I have never pretended to understand OS/2/PM bitmaps. But what is there was about the 50th iteration of what finally tested OK (after about two months of sending tests and results back and forth) on Matrox drivers with an external cairo test program (I think Doodle's clock program, not a browser).

[If you don't have a Matrox driver, how did you actually manage to get the first GpiDrawBits() to fail?]
(In reply to comment #8)
> [If you don't have a Matrox driver, how did you actually manage to get the
> first GpiDrawBits() to fail?]

I didn't.  I just commented it out & plugged GPI_ERROR into rc.

BTW... what was the first Firefox version to use Cairo?  I'll need this info to solicit testers in the newsgroups.
Ah! FF 3.0 was the first.

(Any cairo-based Mozilla app will probably have been too slow to get Matrox driver users, that's why I used the clock test program at the time. But if cairo is changed one still has to keep them in mind because they may be using it through stuff like DSSaver.)
(In reply to comment #3)
> Rich, I think the first patch should be split up between the Cairo bits and the
> gfxOS2Surface bits. The Cairo bits will have to be posted upstream.
> I'll also see if I can wrestle git into downloading the latest Cairo to test
> this there and perhaps create the diff.

Unluckily I'm not sure how to apply the last part of the diff,
From the reject
*** 1320,1326 ****
      NULL, /* check_span_renderer */
      NULL, /* copy_page */
      NULL, /* show_page */
-     NULL, /* set_clip_region */
      NULL, /* intersect_clip_path */
      _cairo_os2_surface_get_extents,
      NULL, /* old_show_glyphs */
--- 1427,1433 ----
      NULL, /* check_span_renderer */
      NULL, /* copy_page */
      NULL, /* show_page */
+     _cairo_os2_surface_set_clip_region,
      NULL, /* intersect_clip_path */
      _cairo_os2_surface_get_extents,
      NULL, /* old_show_glyphs */

current trunk,

static const cairo_surface_backend_t cairo_os2_surface_backend = {
    CAIRO_SURFACE_TYPE_OS2,
    NULL, /* create_similar */
    _cairo_os2_surface_finish,
    _cairo_os2_surface_acquire_source_image,
    _cairo_os2_surface_release_source_image,
    _cairo_os2_surface_acquire_dest_image,
    _cairo_os2_surface_release_dest_image,
    NULL, /* clone_similar */
    NULL, /* composite */
    NULL, /* fill_rectangles */
    NULL, /* composite_trapezoids */
    NULL, /* create_span_renderer */
    NULL, /* check_span_renderer */
    NULL, /* copy_page */
    NULL, /* show_page */
    _cairo_os2_surface_get_extents,
    NULL, /* old_show_glyphs */
    NULL, /* get_font_options */
    NULL, /* flush */
    _cairo_os2_surface_mark_dirty_rectangle,
    NULL, /* scaled_font_fini */
    NULL, /* scaled_glyph_fini */
    NULL, /* paint */
    NULL, /* mask */
    NULL, /* stroke */
    NULL, /* fill */
    NULL, /* show_glyphs */
    NULL, /* snapshot */
    NULL, /* is_similar */
    NULL, /* reset */
    NULL, /* fill_stroke */
    NULL, /* create_solid_pattern_surface */
    NULL, /* can_repaint_solid_pattern_surface */
    NULL, /* has_show_text_glyphs */
    NULL  /* show_text_glyphs */
};

The good news is that unpatched trunk compiles with only a couple of #include <sys/wait.h> added to the boilerplate library (needed for make test) and the test suite runs to completion though with a lot of errors, 105 passed, 172 failed, 0 crashed (8 expected), 4 skipped.
A lot of these failures are probably due to wrong fonts, wrong version of ghostscript etc.
Also I couldn't link to mozfntcfg due to a missing symbol,
cairo-ft-font.c:2594 (.libs/cairo-ft-font.o): Undefined symbol _FcPatternDuplicate referenced from text segment
cairo-ft-font.c:2367 (.libs/cairo-ft-font.o): Undefined symbol _FcPatternDuplicate referenced from text segment
(In reply to comment #11)

Mozilla claims to be using cairo v1.8.2 but it isn't.  The v1.8.x backend contains 'set_clip_region' & 'intersect_clip_path' but the 1.9.x version doesn't.  OTOH, the Mozilla code includes 'create_span_renderer' & 'check_span_renderer' which weren't introduced until v1.9.x.  (Note that v1.9.x, which you appear to be using, is a development version; v 1.10.x will be the release version.)

Incorporating applicable changes into the cairo repository to ensure they don't get wiped out if/when Mozilla updates its codebase is certainly a good idea.  However, my only interest in Cairo is how it affects Mozilla.  If we can find someone who's still using Matrox drivers, we can test my 24-bit bitmap fix.  If not, we can remove it since it otherwise has no affect on Mozilla.

BTW... _FcPatternDuplicate is in mzfntcfgft\src\fontconfig.c.  Perhaps the leading underscore is the problem.
They are trying to update cairo to something newer than 1.8.2 in bug 542605, so that can mess up Rich's patch.
Attached patch Surface Part1 v2 - Cairo/Thebes (obsolete) — Splinter Review
This version removes |_cairo_os2_surface_set_clip_region()| so it should be compatible with the changes in Bug 542605 (and Dave's build of 1.9.x).
Attachment #436961 - Attachment is obsolete: true
As requested, this is a screen capture of Firefox 3.6.3 install using Matrox G200 video adapter.

No color, nothing is clickable.
(In reply to comment #15)
> As requested, this is a screen capture of Firefox 3.6.3 install using Matrox
> G200 video adapter.  No color, nothing is clickable.

Barbara and others have confirmed that a build with this patch fixes the problems with Matrox video drivers.
The new cairo snapshot has landed yesterday. Now the browser crashes with or without your patches. Mozilla devs added a bunch of fixes for clipping, that's now supposed to be handled by the backends. However, there appear to be no special cairo-os2 functions for clipping implemented, could that be the reason for the crashes?
(In reply to comment #12)
> (In reply to comment #11)
> 
> Mozilla claims to be using cairo v1.8.2 but it isn't.  The v1.8.x backend
> contains 'set_clip_region' & 'intersect_clip_path' but the 1.9.x version
> doesn't.  OTOH, the Mozilla code includes 'create_span_renderer' &
> 'check_span_renderer' which weren't introduced until v1.9.x.  (Note that
> v1.9.x, which you appear to be using, is a development version; v 1.10.x will
> be the release version.)

I'm actually using the tip of trunk. Git is working to keep the tree up to date but diff doesn't work :(

> 
> Incorporating applicable changes into the cairo repository to ensure they don't
> get wiped out if/when Mozilla updates its codebase is certainly a good idea.

Should I be pursuing that?
 
> However, my only interest in Cairo is how it affects Mozilla.  If we can find
> someone who's still using Matrox drivers, we can test my 24-bit bitmap fix.  If
> not, we can remove it since it otherwise has no affect on Mozilla.

You never know what ever ports to OS/2 will use Cairo, as well as things like Doodles screensaver. The better they work, including working with Matrox drivers, the better for the community.

> 
> BTW... _FcPatternDuplicate is in mzfntcfgft\src\fontconfig.c.  Perhaps the
> leading underscore is the problem.

I hadn't rebuild my aout libraries since Peter added FcPatternDuplicate. Now the compile creates cairo.dll fine. It ends while compiling cairo-script with the unresolved symbol _FcNameParse. It's hard to imagine Mozilla using cairo-script so shouldn't matter.
Attached patch Proposed patch to send upstream (obsolete) — Splinter Review
Rich, can you double check I didn't screw up the patch. This compiles (after applying the fix from https://bugzilla.mozilla.org/show_bug.cgi?id=562898) and testing with clock works. At that before this patch resizing clock painted garbage until it redrew. After the patch there is a black screen until redrawing.
I do see these warnings when compiling

  CC    cairo-os2-surface.o
cairo-os2-surface.c:172: warning: no previous prototype for '_buffer_alloc'
cairo-os2-surface.c:210: warning: no previous prototype for '_buffer_free'
cairo-os2-surface.c: In function '_cairo_os2_surface_get_pixels_from_screen':
cairo-os2-surface.c:455: warning: pointer targets in passing argument 3 of 'DevOpenDC' differ in signedness
I:/usr/include/os2emx.h:9977: note: expected 'PCSZ' but argument is of type 'const char *'
cairo-os2-surface.c: In function 'cairo_os2_surface_create':
cairo-os2-surface.c:756: warning: unused variable 'rc'
cairo-os2-surface.c: At top level:
cairo-os2-surface.c:1420: warning: excess elements in struct initializer
cairo-os2-surface.c:1420: warning: (near initialization for 'cairo_os2_surface_backend')

with the warnings after line# 756 showing up after applying your patch.
Depends on: 562898
Here are revised patches based on what I expect will be committed to the Cairo trunk.  I've split the cairo-related changes into two parts:  those which will become part of cairo and those which are moz-only.  The nsWindow patch is unchanged.
This patch is identical to the patches sent upstream with one exception (a symbolic constant that is not yet defined).  This exception will resolve itself the next time Mozilla updates its version of Cairo.
Attachment #438402 - Attachment is obsolete: true
Attachment #463941 - Flags: review?(mozilla)
These are Mozilla-only changes.
Attachment #463942 - Flags: review?(mozilla)
Stand-alone changes to nsWindow that leverage changes to Cairo/Thebes.
Attachment #436963 - Attachment is obsolete: true
Attachment #443032 - Attachment is obsolete: true
Attachment #463943 - Flags: review?(mozilla)
Attachment #463941 - Flags: review?(mozilla) → review+
Comment on attachment 463942 [details] [diff] [review]
Surface Part 2 - MozCairo and Thebes

You could use NULLHANDLE for HWNDs instead of 0, but otherwise this looks fine to me.
Attachment #463942 - Flags: review?(mozilla) → review+
Comment on attachment 463943 [details] [diff] [review]
Surface Part 3 - nsWindow

I don't fully grasp the first if and why e.g. |mWindowType == eWindowType_toplevel| signifies a window always covered by something else. Other than that, this looks like a nice optimization.
Attachment #463943 - Flags: review?(mozilla) → review+
(In reply to comment #25)
> I don't fully grasp the first if and why e.g. |mWindowType ==
> eWindowType_toplevel| signifies a window always covered by something else.

A top-level window consists of a frame and its child, FID_CLIENT.  This client window is never used by Mozilla.  Instead, Moz creates a mozilla-child window with the same dimensions that it uses to display chrome and content.  See comment #2 for more details.
All changes contained in "Surface Part 1 - Cairo" (attachment 463941 [details] [diff] [review]) were committed to the Cairo trunk on 2010-08-08.
Keywords: checkin-needed
Whiteboard: m-c a=NPOTB - OS/2 only
http://hg.mozilla.org/mozilla-central/rev/b970fa3804ee
http://hg.mozilla.org/mozilla-central/rev/8f1a1bad511e
http://hg.mozilla.org/mozilla-central/rev/77a52b34ea6c

In the future, please follow these suggestions for using checkin-needed:
http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

It's a pain to have to manually set the user and checkin comment when landing
your patches.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Whiteboard: m-c a=NPOTB - OS/2 only
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: