[cairo] OS/2 backend support files

[Carl Worth]

On Sun, 30 Jul 2006 13:49:17 +0200 (CEST), Doodle wrote:
> On Fri, 28 Jul 2006, Carl Worth wrote:
>
> Here on OS/2 we use Cairo for some screen saver modules (not counting
> Mozilla), and those screen saver modules are loaded into memory when
> needed, and unloaded when not needed, while the screen saver "master"
> application/process is always running in the background. Imagine, if
> neither Cairo nor Fontconfig would be cleaned up, we'd have a nice memory
> leak after every screen saver module running.

I didn't explain this very well. If you have a long-running process
that occasionally loads a fontconfig-using module, and you don't call
FcFini when unloading each module then you will not have any memory
leaks, (excepting bugs of course).

However, if you do call FcFini, then you will be removing a lot of
cached data and will be inflicting horrific performance
characteristics on your long-running process every time it starts
using fontconfig again.

So, again, the FcFini call is *not* designed to be called by running
code---it's only designed to be called when debugging memory
issues. (The naming convention we came up with for the analogous cairo
function more clearly hints at this usage: cairo_debug_reset_static_data).
The other naming bug is that the FcInit call is required, so the
API does give the appearance that a call to FcFini is required to
avoid leaks.

I'll take all the blame for that one.

> That's why I introduced cairo_os2_initialize() and
> cairo_os2_uninitialize(), so now Cairo is a nice player on OS/2, not
> leaving any dirt behind, but more about them later.

And I hope you're not calling cairo_debug_reset_static_data in your
uninitialize function. That would be the same mistake as currently
calling FcFini.

And I still prefer init/fini over initialize/uninitialize.

> I'm still not sure if we want that memset() or not.
> What happens in other backends, when they are created?
> Will they have a fully black surface by default?

Ah, good question. If the user is passing in object to surface_create
that might have some data on it, (say, passing an Xlib drawable to
cairo_xlib_surface_create, or calling cairo_image_surface_create_for_data),
then cairo should definitely leave that data alone, and respect the
contents that the user created.

However, if cairo is creating a new bugger from scratch on behalf of
the user, (say, cairo_image_surface_create), then cairo does clear the
memory to 0 in every channel.

> Another idea was that as it's a surface which is connected to an OS/2
> window, it should initialize its contents from the window itself.
> I dropped this idea because most of the time it's used to draw an image
> from scratch, and the whole window content will be redrawn, so copying
> over the window contents into the surface only for later being overwritten
> is stupid. If one really needs it, cairo's own dirty area API can be used
> to force such a copy.

So the assumption we've been making in all other backends is that the
initial state of the object being passed to surface_create is dirty
everywhere. I suppose you could avoid the performance penalty by being
lazy about the copy and avoiding it in the case of an initial SOURCE
paint() operation, (which would clear the dirty state everywhere).

> > Shouldn't there be error values returned in these cases?
>
> They are in void blabla() functions. I guess you meant to ask if those
> functions should be changed to return bool or something similar?
> Well, some of them are defined by Cairo itself (e.g.: release_source_image)
> and some might have no use (e.g. cairo_surface_repaint_window()) in
> returning success or failure, but I'm not against changing it.

If the internal cairo interfaces are defined as void, it's probably
because we have no possibility of failure in any other currently
implemented backend. But as you introduce new failure cases, please
feel free to adjust those interfaces.

But that should be a nice, small, independent patch, of course.

> Yes I know, Win32 has very similar things. One of the problems is with the
> mutexes, that they have to be initialized somewhere, without introducing
> any race condition. The best solution for that (imho) is a global
> init/fini function pair.

Yes, you're probably right. It's just too bad that the end result will
be so fragile. Basically, every cairo-using program will fail the
first time someone tries to static-link the program to cairo on win32
or OS/2. Or maybe that's more likely to just hit the cairo-using
toolkit, so it will be much less rare, and not as big a problem.

> So, it's more like an added feature. The HWND is not needed normally, but
> if somebody wants to separate the drawing from the windowing thread, then
> it can also be done now.

Thanks for the extra clarification, (for this and as well as the
timeout value).

> The "blit_as_changes" functions are only for performance reasons. By
> default, the OS/2 surface blits the changed part of the backbuffer into
> the window as soon as the change happens (at release_dest_image time).
> This way it always keeps the backbuffer and the real contents of the
> window in sync. However, it slows down things quite much, to show the
> changes "in realtime", so this feature can be turned off for those who
> know what they do. This way, one can draw the whole scene with Cairo
> without that being shown in the OS/2 Window (in other words, the surface
> can be pre-drawn, constructed in the background), and then call
> cairo_os2_surface_repaint_window() which will then blit the backbuffer
> into the window at once.

OK. That makes more sense to me now. I think that both the naming and
the documentation of these functions could be improved to better show
the connection between blit_as_changes and repaint_window.

Here's one suggestion for your consideration:

/* The contents of OS/2 surfaces are stored in a backing buffer from
 * which the actual window contents must be refreshed. By default,
 * cairo will automatically refresh the window after each drawing
 * operation. However, there is some performance cost to doing this
 * incremental refresh, which can be undesired, particularly if the
 * application is only interested in display the final result after
 * several drawing operations.
 *
 * The alternative approach is to put the surface into a manual
 * refresh mode by passing a true value to
 * cairo_os2_surface_set_manual_refresh and then calling
 * cairo_os2_surface_refresh whenever desired.
 */
cairo_public void
cairo_os2_surface_set_manual_refresh (cairo_surface_t *surface,
				      cairo_bool_t     blit_as_changes);

void
cairo_os2_surface_refresh (cairo_surface_t *surface,
			   HPS              hps_begin_paint,
			   PRECTL           prcl_begin_paint_rect)

You might come up with something different that suits you and OS/2
better than what I just wrote. But here are some of the important
features of the above that should be preserved:

1) A common verb, ("refresh" in my example), is used in both functions
   to indicate their connection.

2) The verb is obviously distinct from existing cairo functionality,
   (I didn't like the similarity of "repaint" and "paint" in this
   regard).

In general I'm not a fan of Boolean parameters to functions, but they
can be clear enough in the case of function that sets a Boolean
parameter as in the above set_manual_refresh. Another option would be
something that accepts an enum, (set_refresh_mode perhaps?). But the
problem I ran into there is that we don't yet have any precedent for
how to namespace a backend-specific enum. It would seem a bit awkward
to use something like:

	CAIRO_OS2_SURFACE_REFRESH_MODE_AUTOMATIC
	CAIRO_OS2_SURFACE_REFRESH_MODE_MANUAL

Though maybe just dropping "surface" would make that acceptable:

	CAIRO_OS2_REFRESH_MODE_AUTOMATIC
	CAIRO_OS2_REFRESH_MODE_MANUAL

Or you can just stick with the Boolean interface as I did it above.

Also, I think it would help if these 2 functions (or 3 with the getter
as well) were grouped together at the end of the header file, (since
they work together and introduce a more "advanced" mode of using the
OS/2 surface). At the very least, this stuff shouldn't be needed for
getting correct behavior, where something like set_size should be.

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.freedesktop.org/archives/cairo/attachments/20060801/f10bb8bf/attachment.pgp