Re: gdk/win32 and cairo [was: Cairo now a dependency of HEAD GTK+]

On Mon, 2005-02-28 at 22:38 +0100, Hans Breuer wrote:
> On 25.02.2005 00:57, Owen Taylor wrote:
> > On Mon, 2005-02-07 at 23:55 -0500, Owen Taylor wrote:
> > 
> > 
> >>The GTK+ code currently is organized around one DC per
> >>GC. My thought is that we should be moving more towards
> >>one-DC-per-GdkDrawable. If we have a DC associated with
> >>the GdkPixmap, then we can use that DC both for passing
> >>to Cairo and for BitBlt from that surface to another.
> >>There is no need to select the bitmap into another DC.
> >>
> [a little more from your original mail]
> >>The obvious response here is "that's a lot of DCs".
> >>I don't think DC's are the critically scarce resource in 
> >>current Windows that they once were, but yes, it is.
> I'm one of these peoples: uhm that's *a lot* of resource.
> And as I see it in many cases wasted for no good reason,
> i.e. why would a pixmap - not to be drawn to - need a
> permanent DC ?
> But "moving more towards one-DC-per-GdkDrawable" sounds
> reasonable and required for concurrent drawing Gdk *and*
> Cairo on the same thing.

Sure, if you look at my patch, you'll see that DC's are not kept
around permanently but rather only when needed.

> So the second iteration of my Gdk/Cairo integration does
> exactly this :
> - add hdc_get(), hdc_release() to GdkDrawableImplWin32 vtable
> - make the two win32 drawable impls pixmap and window manage
>    *one* refcounted DC per drawable
> - replace the internal use of CreateCompatibleDC/SelectObject/
>    SelectObject/DeleteDC with gdk_pixmap_impl_win32_hdc_(get|release)
> For the non Cairo case this boils dowwn to one more indirection
> but *no* additional resource usage. For the Cairo case the DC
> will be created together with the surface and go away when
> destroying the surface. It simply is reused for drawing to
> another drawable, see blit_from_pixmap()


> How do you think this compares to your approach?
> > 
> It is obviously less hackish than my first approach
> but - as tried to outline above - has some issue which
> I'd like to avoid.
> Also I don't like the use of acquire/release [1] and the
> parallel API it introduces to the existing gdk_win32_hdc_get/
> gdk_win32_hdc_release.
> How to continue ?

In terms of the DC part of the two patches, I don't think there
is a significant difference between the two patches. There are are
naming differences ... hdc_count vs. hdc_refs, etc. There are some
organizational differences ... virtualization vs. simple if 
statements. But in the end they do the same thing. 

[ I did notice you don't check the return value of CreateCompatibleDC ]

In terms of the acquire()/release() vs. get()/release(), the GTK+
standard is that get() functions don't return a newly created object
count, so I thing the existing gdk_win32_hdc_get() is misnamed.
If I was maintaining the Win32 backend, I'd go ahead and rename
it. acquire/release() are conventionally paired. But you could 
use ref()/unref() perhaps. Or you could leave it get/release().
It's up to you and Tor.

The handling of cairo_surfaces in your patch, on the other hand,
doesn't make a lot of sense to me:

 - In my patch, once a cairo_surface is created for a drawable
   it is kept around until the drawable is destroyed (window)
   or finalized (pixmap)

 - In your patch, once a cairo_surface is created for a drawable,
   *some* cairo_surface is kept around until the drawable is
   finalized, but for some reason, we destroy and create a new
   cairo surface.

 - In my patch, since we keep the cairo surface around, we keep
   a reference count on the DC until the drawable is destroyed
   or finalized.

 - In your patch, you apparently leak a reference count to the
   DC on every call to gdk_win32_set_cairo_target().

As noted in the comment in my patch, once some pending changes 
to Cairo land, we'll be able to do better for the handling of
cairo_surface ... actually free the cairo_surface and release
the DC once it is no longer  in use:

The way you do that is basically in gdk_win32_set_cairo_target()

 if (drawable->cairo_surface)
   cairo_set_target_surface (cr, drawable->cairo_surface);
    dc = get/acquire_dc (drawable);
    surface = cairo_win32_surface_create (dc);
    cairo_surface_set_user_data (surface, &key,
                                 drawable, clear_surface);
    cairo_set_target_surface (cr, drawable->cairo_surface);
    cairo_surface_destroy (surface);

WIth clear_surface looking like:

 clear_surface (drawable)
    release_dc (drawable);
    drawable->cairo_surface = NULL;

I'll leave the final patch up to you; I really don't care a lot
about naming inside the Win32 backend. But I would suggest you
compare the handling of the Cairo surface in the two patches;
I think what I'm doing will work better in the short term, and
is more amenable to improvement with future additions to Cairo.


Attachment: signature.asc
Description: This is a digitally signed message part

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]