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()
do:
if (drawable->cairo_surface)
cairo_set_target_surface (cr, drawable->cairo_surface);
else
{
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:
void
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.
Regards,
Owen
Attachment:
signature.asc
Description: This is a digitally signed message part