[cairo] OS/2 backend support files

[Carl Worth]

On Sat, 22 Jul 2006 16:47:30 +0200, "Peter Weilbacher (Mozilla)" wrote:
> > Following my conversation with Carl in public, here is a zip
> > file containing the OS/2 specific files for Cairo v1.2.0.
> > 
> > http://scenergy.dfmk.hu/doodle/cairo_120_os2_files.zip
> > 
> > This contains the OS/2 surface support files, and some small
> > modifications in the common header files.
> 
> Thanks a lot for sending the files. As promised I have fiddled around
> with the build system and gotten it to work on OS/2 for the most part.
> >From Doodle's files I made two "git format-patch origin" patches from
> the current HEAD (or trunk or whatever that is called in cairo/git
> speak) which I attach to this email.

Thanks to you both. This is definitely the cleanest submission of this
backend that we have seen to date. (And Peter, you did the right thing
by pinging again on the list as a reply---there's a *lot* of cairo
backlog in my inbox by definition, so squeaky wheels do get the
grease, etc.)

Just a few comments below:

> - Doodle: I was wondering why you set (c) and initial developer as
>   RedHat in your files? Last time we met you were not working for them.

So that copyright statement at least does need to be fixed. I'm
definitely not going to accept code with an incorrect copyright
statement. (As for "initial developer", I'm not as concerned about
that one.)

> - OS/2 does not allow to set png_CFLAGS in the environment (every
>   variable is automatically uppercased). I don't know how to dead with
>   this, so the OS/2ers either have to set CFLAGS including PNG specific
>   flags

I don't know anything about this one. Where do we use png_CFLAGS as an
environment variable? (Which does look odd, if nothing else.)

> - Is it intentional that pixman.lib (lib for static linking) does not
>   get copied to $prefix/lib make install or am I doing something wrong
>   so that the cairo.lib does not contain the pixman functions?
> 
> - The biggest point so far is that I didn't find a version of libtool
>   for OS/2 that can actually compile a DLL, but I think that this has
>   nothing to do with these patches and has to be solved in libtool.

These two I also don't know anything about.

> OK, that's it for now. Let me know if I need to do anything more before
> this can get into the main repository. Any more requirements to get this
> into the 1.2 branch?

There are a few small comments on the patch itself which I will make
below. After those are addressed all we will need is someone who is
willing to maintain this code, (Doodle? Peter?), who should then
request a freedesktop account and then push the actual code into the
tree. Account request instructions are here:

	http://freedesktop.org/wiki/AccountRequests

(Ignore the fact that it calls it a "CVS" account. I'd fix the wiki to
say git/CVS, but it appears someone has restricted the permissions on
that page and left me out of the club.)

> +CAIRO_BACKEND_ENABLE(os2, OS/2, os2, OS2_SURFACE, auto, [

The general rule is that we use "no" instead of "auto" for a backend
while it is still experimental, (that is, until it is integrated and
passes the test suite). That's so that users will get the warning
about the experimental backend unless they explicitly ask to compile
the backend with --enable-os2.

> +#if defined(__OS2__) && !defined(CAIRO_MUTEX_DECLARE)
> +
> +#define INCL_BASE
> +#define INCL_PM
> +#include <os2.h>
> +
> +# define CAIRO_MUTEX_DECLARE(name) extern HMTX name
> +# define CAIRO_MUTEX_DECLARE_GLOBAL(name) extern HMTX name
> +# define CAIRO_MUTEX_LOCK(name) DosRequestMutexSem(name, SEM_INDEFINITE_WAIT)
> +# define CAIRO_MUTEX_UNLOCK(name) DosReleaseMutexSem(name)
> +#endif

There's inconsistent spacing there, (missing space after '#'
character).

Other than those little things, the changes to non-os2-specific files
look fine. It's very nice to see that nothing more invasive is needed
anymore.

>  src/cairo-os2-dll.c     |   78 ++++
>  src/cairo-os2-extra.c   |  161 ++++++++
>  src/cairo-os2-private.h |   76 ++++
>  src/cairo-os2-surface.c |  995 +++++++++++++++++++++++++++++++++++++++++++++++

I don't really like seeing the os2 implementation spread out over four
files like this. None of the other backends do this for example.

In particular, the contents of cairo-os2-private.h should be part of
cairo-os2-surface.c since it is the only file that includes it. Then,
I think you might as well just pull the -dll.c and -extra.c files in
there as well, (they're not that big).

If that seems really objectionable, perhaps we can come up with
something else, (an additional directory for backends that want more
than one .c file?).

> +++ b/src/cairo-os2-dll.c
> @@ -0,0 +1,78 @@
> +/* Cairo - a vector graphics library with display and print output
> + *
> + * Copyright © 2005 Red Hat, Inc.
...
> +++ b/src/cairo-os2-extra.c
> @@ -0,0 +1,161 @@
> +/* Cairo - a vector graphics library with display and print output
> + *
> + * Copyright © 2005 Red Hat, Inc.

As mentioned earlier, these Copyright statements need to be changed to
the actual entity with copyright interest in this code, (and in
subsequent files).

I'm not reviewing the actual implementation, but I will comment on
some style issues that should be fixed so that the code looks like
"cairo code". See the cairo/CODING_STYLE file for lots of details on
this. I'm pointing out an initial occurrence of each issue, but the
issues appear throughout the implementation.

> +static int iCairoIsInitialized = 0;

The cairo coding style does not use StudlyCaps for identifiers, but
underscore_separators instead. And what is that initial 'i' ?

Personally, when I see a name like "is_initialized" it reads to me
like a Boolean value. So I would have expected something like:

	static cairo_bool_t cairo_os2_is_initialized = FALSE;

But I see in the code that you are actually using a counter
here. Perhaps cairo_os2_initialization_count or something like
that---which would look less like a Boolean.

> +static void inline DisableFPUException(void)
> +{
> +  unsigned short usCW;

The indentation should use a 4-character indent.

> +  // Some OS/2 PM API calls modify the FPU Control Word,
> +  // but forget to restore it.

We also don't use this C99 (or "C++") style of single-line comment in
the cairo source code, (though it looks like I forgot to mention that
in CODING_STYLE). Please use the good old /* ... */ style instead.

> +  _control87(usCW, MCW_EM | 0x80);

The cairo coding style puts a space between the function name and the
left parenthesis.

> +  if (iCairoIsInitialized>1) return;

This should be written with a whitespace around the '>' operator.

> +  /* Uninitialize FontConfig */
> +  FcFini();

As it turns out, there's no good reason to call FcFini in "production"
code. It's only there so that one can cause fontconfig to free up all
of its static data, (compare to cairo_debug_reset_static_data). That's
nice when doing testing with valgrind, but not actually helpful in
general, (and could in fact slow things down).

> +    } else
> +    {

The CODING_STYLE guidelines allow either:

	} else {
or:
	}
	else
	}

but not the above.

> +  /*
> +  pOS2Surface->bmi2BitmapInfo.ulCompression = BCA_UNCOMP;
> +  pOS2Surface->bmi2BitmapInfo.cbImage = 0;
> +  pOS2Surface->bmi2BitmapInfo.cxResolution = 70;
> +  pOS2Surface->bmi2BitmapInfo.cyResolution = 70;
> +  pOS2Surface->bmi2BitmapInfo.cclrUsed = 0;
> +  pOS2Surface->bmi2BitmapInfo.cclrImportant = 0;
> +  pOS2Surface->bmi2BitmapInfo.usUnits = BRU_METRIC;
> +  pOS2Surface->bmi2BitmapInfo.usReserved = 0;
> +  pOS2Surface->bmi2BitmapInfo.usRecording = BRA_BOTTOMUP;
> +  pOS2Surface->bmi2BitmapInfo.usRendering = BRH_NOTHALFTONED;
> +  pOS2Surface->bmi2BitmapInfo.cSize1 = 0;
> +  pOS2Surface->bmi2BitmapInfo.cSize2 = 0;
> +  pOS2Surface->bmi2BitmapInfo.ulColorEncoding = BCE_RGB;
> +  pOS2Surface->bmi2BitmapInfo.ulIdentifier = 0;
> +  */

Why is all this dead code included? It should be removed, unless there
is a very good reason for it to stay, (in which case that reason needs
to be present as a comment).

> +  /* This is possibly not needed, malloc'd space is
> +   * usually zero'd out!
> +   */
> +  /*
> +   memset(pOS2Surface->pchPixels, 0x00, swpTemp.cx * swpTemp.cy * 4);
> +   */

That's not a good assumption to make in general. Use calloc instead of
malloc if you want to ensure that.

> +    /* Invalid parameter (wrong surface)! */
> +    return;
..
> +    /* Could not get mutex! */
> +    return;

Shouldn't there be error values returned in these cases?

> +  /* Done! */
> +  DosReleaseMutexSem(pOS2Surface->hmtxUsePrivateFields);

A comment like that isn't adding anything, so it should be removed,
(same for several other non-substantive comments).

> +/* cairo_os2_initialize() :                                       */
> +/*                                                                */
> +/* Initializes the Cairo library. This function is automatically  */
> +/* called if Cairo was compiled to be a DLL (however it's not a   */
> +/* problem if it's called multiple times), but if you link to     */
> +/* Cairo statically, you have to call it once to set up Cairo's   */
> +/* internal structures and mutexes.                               */
> +
> +cairo_public void
> +cairo_os2_initialize(void);
> +
> +/* cairo_os2_uninitialize() :                                     */
> +/*                                                                */
> +/* Uninitializes the Cairo library. This function is automatically*/
> +/* called if Cairo was compiled to be a DLL (however it's not a   */
> +/* problem if it's called multiple times), but if you link to     */
> +/* Cairo statically, you have to call it once to shut down Cairo, */
> +/* to let it free all the resources it has allocated.             */
> +
> +cairo_public void
> +cairo_os2_uninitialize(void);

Hmmm... I'm not sure about this. We currently have a similar
static-linking problem with the win32 backend. If we do decide to use
extra functions like this, then we should also add them to the win32
backend and use consistent naming, (I'd vote for cairo_<foo>_init and
cairo_<foo>_fini myself).

But the problem is that this is an inherently fragile approach. The
extra functions are not needed when dynamically linking, (nor are they
needed for many backend), so we expect many users to not put them
in. This means that programs that work fine with dynamic linking will
fail with static linking.

An alternate idea would be to identify the minimal necessary entry
points and sprinkle them with the initialization calls internally. I
think those entry points are surface_create, (which is an easy one,
since it's already backend-specific), and font_face_create, (which is
a bit trickier, since it's not backend-specific---but maybe we just
define a new CAIRO_MUTEX_INITIALIZE macro next to CAIRO_MUTEX_DECLARE
and the font_face_create and surface_create functions can call it if
it is not NULL). Thoughts?

> +cairo_public cairo_surface_t *
> +cairo_os2_surface_create (HPS hpsClientWindow,
> +                          int iWidth, int iHeight);

I don't know much about what an HPS, but that function looks
reasonable enough as an interface, (though I would use parameter names
of hps (or maybe hps_client_window), width, and height).

> +/* It's also a solution, but experience shows that if this happens*/
> +/* from a non-PM thread, then it can screw up PM internals.       */
> +/*                                                                */
> +/* So, best solution is to set the HWND for the surface after the */
> +/* surface creation, so every blit will be done from application's*/
> +/* message processing loop, which is the safest way to do.        */
> +
> +cairo_public void
> +cairo_os2_surface_set_HWND (cairo_surface_t *pSurface,
> +                            HWND hwndClientWindow);

The description here is confusing me. Is it saying that calling
cairo_os2_surface_create without also calling
cairo_os2_surface_set_HWND is fragile and likely not to work? In that
case, should the HWND parameter simply be a required parameter to
cairo_os2_surface_create and we can drop this second API?

> +cairo_public void
> +cairo_os2_surface_set_blit_as_changes (cairo_surface_t *pSurface,
> +                                       int bBlitAsChanges);

The 'p' and 'b' prefixes here are ugly and inconsistent with cairo
style. I would suggest the following instead:

> +cairo_public void
> +cairo_os2_surface_set_blit_as_changes (cairo_surface_t *surface,
> +                                       cairo_bool_t	   blit_as_changes);

> +/* cairo_os2_surface_window_resized() :                           */
> +/*                                                                */
> +/* When the client window is resized, call this API so the

Similar calls in the other backends, (such as xlib), use "set_size"
rather than "window_resized". I would recommend that for consistency.

> +/* The timeout value is in milliseconds, and tells how much the   */
> +/* function should wait on other parts of the program to release  */
> +/* the buffers. It is necessary, because it can be that Cairo is  */
> +/* just drawing something into the surface while we want to       */
> +/* destroy and recreate it.

Couldn't one instead just call cairo_surface_flush() and avoid the
need for a timeout value here?

> +/* cairo_os2_surface_repaint_window() :                           */
> +/*                                                                */
> +/* This function can be used to force a repaint of a given area   */
> +/* of the client window. Most of the time it is called from the   */
> +/* WM_PAINT processing of the window proc. However, it can be     */
> +/* called anytime if a given part of the window has to be

I don't understand why this function needs to exist. We certainly
don't have anything like it in the interface of cairo to any other
window system. (I also may not be understanding why blit_as_changes is
needed.)

Can the user not "manually" do what repaint_window is doing by using
window-system specific calls on the HPS and HWND? If the user can,
then I don't think these API calls belong in cairo's interface.

-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/20060728/be0d8e3d/attachment.pgp