[cairo] New PDF backend snapshot

[Kristian Høgsberg]

Carl Worth wrote:
> --[[text/plain]]
> Kristian,
> 
> Thanks so much for all your hard work on the PDF backend! I'm sorry
> I've been so slow to review it. I think this is good work, and I want
> it to land in CVS soon. I have a few comments, (most importantly with
> respect to the public API).
> 
> On Wed, 15 Dec 2004 12:52:38 -0500, Kristian Høgsberg wrote:
> 
>>I've pondering the cairo_document_t idea a bit and restructured the PDF 
>>backend so that it's split into a cairo_pdf_document_t object and a 
>>cairo_pdf_surface_t object.  I believe this makes the code easier to 
>>follow, but I'm no longer sure we need to expose a cairo_document_t in 
>>the user API.
> 
> 
> If it's easier to have two objects in the backend, that is fine.
> 
> 
>>public API.  2) It will be a nice way to set metadata and other PDF 
>>specific information for the document.
> 
> 
> Can't we just do that with cairo_pdf_surface functions? What advantage
> does the user get from having two different objects? So far, it seems
> the user just has to do more work before being able to use the PDF
> backend, (compared to the others).

Compared to the PS and PNG backends, yes.  However the Xlib, XCB and 
glitz backends all take backend specific objects in the surface 
constructor (Drawable, XCBDRAWABLE, and glitz_surface_t) that you have 
to create outside of cairo before you can create the surface.  All these 
objects can be used with the corresponding backend library to set 
backend specific "meta data" for the underlying resource.  The only 
difference with the PDF backend is that it doesn't rely on an external 
library.  It is certainly possible to remove cairo_pdf_document_t from 
the public API and manipulate document properties through the surface 
object.  An alternative route would be to split part of the backend into 
it's own library and namespace like glitz.

>>+void
>>+cairo_set_target_pdf (cairo_t  *cr,
>>+                     cairo_pdf_document_t *document);
> 
> 
> The notion behind the cairo_set_target family of functions is to allow
> the user to select a backend without having to create any new
> object. So the cairo_set_target_pdf function should accept the same
> arguments as cairo_pdf_surface_create.

I understand that I'm breaking the rule here, I'm just not sure how you 
would do it otherwise.  Of course, dropping cairo_pdf_document_t from 
the API would require both to take the same arguments as 
cairo_pdf_document_create(), which would solve the problem.  But if you 
did that, cairo_pdf_surface_create() could only be used for creating a 
surface to be used with cairo_set_target_surface() - you couldn't 
composite it into another PDF surface since it would belong to a 
different document.

>>There are two significant changes outside cairo_pdf_surface.c in this 
>>patch: I've added the set_clip_trapezoids surface method which is 
>>supposed to let cairo pass the absolute clipping path to the backends. 
> 
> 
> (Kristian and I discussed this off-list and he has now proposed a new
> patch without the set_clip_trapezoids method. See:
> 
> 	http://people.redhat.com/krh/cairo-pdf/cairo-pdf-4.patch
> )

Yes, I'm going to investigate using PDF soft masks for all clipping, 
which will connect nicely with the current cairo backend API and 
gradients with varying alpha should fall out this for free (more or 
less).  The downside is that it's one of the more advanced features of 
PDF 1.4 so it will be interesting to see what level of support the 
various viewers implement.

>>The other change is the addition of cairo_array_t implemented in 
>>cario_array.c.  This is an standard autogrowing array, doubling the 
>>allocation size when it runs out of space.  I was using chained lists 
>>before in the PDF backend, but I converted those to arrays.  I added the 
>>array as a general data type, since cairo uses the same type of array in 
>>other places and it might make sense to rewrite those places to use this 
>>array type.
> 
> Eliminating some code duplication seems like a good thing. But I don't
> like giving up type safety, and worse, I don't want to give up direct
> access of arrays using C [] notation. This kind of thing is always a
> tough call since C has no support for what we really want here.

I think there is also value in separating out boring container code from 
the interesting cairo code.  The code is easier to follow when you don't 
have to parse array growing logic intertwined with e.g. bounding box 
computations.

The cairo_array_index() was meant to give direct array access, for example:

	num_elements = cairo_array_num_elements (array);
	traps = cairo_array_index (array, index, num_elements);

	for (i = 0; i < num_elements; i++)
		traps[i].top = 100;

(the num_elements argument to cairo_array_index() isn't in the patch, 
but I'm thinking it should be there so the function can assert() that 
the elements the caller wants to access are within bounds).  As for type 
safety, that's a classic tradeoff with containers and we're already 
passing void pointers to surfaces to the backends.

> Regardless, if the code is to stay, then:
> 
> 
>>+    if (old_size == 0)
>>+	new_size = 1;
>>+    else
>>+	new_size = old_size * 2;
>>+
>>+    while (new_size < required_size)
>>+	new_size = old_size * 2;
> 
> 
> should be:
> 
> 	new_size = new_size * 2;
> 
> to eliminate the infinite loop.

Oh dear.  I never insert more than one element at the time in the PDF 
backend which explains why I never hit this bug.

> And these three functions:
> 
> 
>>+static cairo_status_t
>>+_cairo_pdf_surface_set_matrix (void             *abstract_surface,
>>+                               cairo_matrix_t   *matrix)
>>+
>>+static cairo_status_t
>>+_cairo_pdf_surface_set_filter (void             *abstract_surface,
>>+                               cairo_filter_t   filter)
>>+
>>+static cairo_status_t
>>+_cairo_pdf_surface_set_repeat (void             *abstract_surface,
>>+                               int              repeat)
> 
> 
> merit at least a comment stating they are unsupported.
> 
> This isn't *too* big a deal as I want to eliminate these functions
> from the surface interface anyway, (they really only belong on
> patterns).

I'll add a comment for now.

> Once the issues above are resolved I'd like to get this into CVS. I
> haven't actually played with the PDF output yet, but we can have fun
> playing with that after it lands in the tree.

Ok, sounds cool.  As mentioned above, I'm planning to rewrite it, so 
it'll be nice to just touch base with the current snapshot before I do that.

cheers,
Kristian