Closed
Bug 557159
Opened 14 years ago
Closed 14 years ago
[OS/2] Optimize Cairo/Thebes surfaces
Categories
(Core :: Graphics, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: dragtext, Assigned: dragtext)
References
Details
Attachments
(4 files, 4 obsolete files)
360.78 KB,
image/png
|
Details | |
28.98 KB,
patch
|
mozilla
:
review+
|
Details | Diff | Splinter Review |
5.85 KB,
patch
|
mozilla
:
review+
|
Details | Diff | Splinter Review |
4.66 KB,
patch
|
mozilla
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•14 years ago
|
||
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.
Assignee | ||
Comment 2•14 years ago
|
||
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.
Comment 4•14 years ago
|
||
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?
Comment 5•14 years ago
|
||
(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.
Assignee | ||
Comment 6•14 years ago
|
||
(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.
Comment 7•14 years ago
|
||
(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.
Comment 8•14 years ago
|
||
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?]
Assignee | ||
Comment 9•14 years ago
|
||
(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.
Comment 10•14 years ago
|
||
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.)
Comment 11•14 years ago
|
||
(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
Assignee | ||
Comment 12•14 years ago
|
||
(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.
Comment 13•14 years ago
|
||
They are trying to update cairo to something newer than 1.8.2 in bug 542605, so that can mess up Rich's patch.
Assignee | ||
Comment 14•14 years ago
|
||
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
Comment 15•14 years ago
|
||
As requested, this is a screen capture of Firefox 3.6.3 install using Matrox G200 video adapter. No color, nothing is clickable.
Assignee | ||
Comment 16•14 years ago
|
||
(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.
Comment 17•14 years ago
|
||
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?
Comment 18•14 years ago
|
||
(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.
Comment 19•14 years ago
|
||
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.
Assignee | ||
Comment 20•14 years ago
|
||
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.
Assignee | ||
Comment 21•14 years ago
|
||
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)
Assignee | ||
Comment 22•14 years ago
|
||
These are Mozilla-only changes.
Attachment #463942 -
Flags: review?(mozilla)
Assignee | ||
Comment 23•14 years ago
|
||
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)
Updated•14 years ago
|
Attachment #463941 -
Flags: review?(mozilla) → review+
Comment 24•14 years ago
|
||
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 25•14 years ago
|
||
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+
Assignee | ||
Comment 26•14 years ago
|
||
(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.
Assignee | ||
Comment 27•14 years ago
|
||
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
Comment 29•14 years ago
|
||
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.
Updated•14 years ago
|
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.
Description
•