[cairo] [PATCH] Fix links whose targets are on a page that is not created yet
Guillaume Ayoub
guillaume.ayoub at kozea.fr
Tue Aug 28 22:56:31 UTC 2018
Thanks for the review.
I've created an issue on GitLab
(https://gitlab.freedesktop.org/cairo/cairo/issues/336) with a new
patch and some explanation.
I'm not sure that I can easily add tests for now. I don't understand
(yet) how it works and how to launch tests, I feel far from adding one.
Maybe Adrian could help?
--
Guillaume
Le ven. 24 août 2018 à 20:24, Bryce Harrington
<bryce at bryceharrington.org> a écrit :
> [Adrian - would be great to get your feedback on this patch.]
>
> On Tue, Aug 21, 2018 at 03:55:35PM +0200, Guillaume Ayoub wrote:
>> As explained on the mailing list[1], links are broken when their
>> targets are on
>> a page that has not been created yet.
>>
>> This commit fixes the problem by using indirect references for
>> /Dest objects,
>> and by creating the references when the file has been totally drawn.
>>
>> I'm not sure whether the "links" property should be in "surface" or
>> in
>> "interchange". I may also have added some memory leaks even if I
>> did my best to
>> avoid them, as I'm not a regular C developer.
>>
>> [1]
>> https://lists.cairographics.org/archives/cairo/2018-August/028715.html
>>
>> ---
>> src/cairo-pdf-interchange.c | 100
>> +++++++++++++++++++++++---------
>> src/cairo-pdf-surface-private.h | 9 +++
>> src/cairo-pdf-surface.c | 2 +
>> 3 files changed, 84 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/cairo-pdf-interchange.c
>> b/src/cairo-pdf-interchange.c
>> index 9aa6934dc..d9b1e19fc 100644
>> --- a/src/cairo-pdf-interchange.c
>> +++ b/src/cairo-pdf-interchange.c
>> @@ -327,29 +327,34 @@ _named_dest_pluck (void *entry, void *closure)
>>
>> static cairo_int_status_t
>> cairo_pdf_interchange_write_explicit_dest (cairo_pdf_surface_t
>> *surface,
>> - int
>> page,
>> - cairo_bool_t
>> has_pos,
>> - double x,
>> - double y)
>> + cairo_pdf_link_t *link)
>> {
>> cairo_pdf_resource_t res;
>> double height;
>>
>> - if (page < 1 || page > (int)_cairo_array_num_elements
>> (&surface->pages))
>> + if (link->page < 1 || link->page >
>> (int)_cairo_array_num_elements (&surface->pages))
>> return CAIRO_INT_STATUS_TAG_ERROR;
>>
>> - _cairo_array_copy_element (&surface->page_heights, page - 1,
>> &height);
>> - _cairo_array_copy_element (&surface->pages, page - 1, &res);
>> - if (has_pos) {
>> - _cairo_output_stream_printf (surface->output,
>> - " /Dest [%d 0 R /XYZ %f %f
>> 0]\n",
>> - res.id,
>> - x,
>> - height - y);
>> + _cairo_pdf_surface_update_object (surface, link->res);
>> +
>> + _cairo_array_copy_element (&surface->page_heights, link->page
>> - 1, &height);
>> + _cairo_array_copy_element (&surface->pages, link->page - 1,
>> &res);
>> + if (link->has_pos) {
>> + _cairo_output_stream_printf (surface->output,
>> + "%d 0 obj\n"
>> + " [%d 0 R /XYZ %f %f 0]\n"
>> + "endobj",
>> + link->res.id,
>> + res.id,
>> + link->x,
>> + height - link->y);
>> } else {
>> - _cairo_output_stream_printf (surface->output,
>> - " /Dest [%d 0 R /XYZ null
>> null 0]\n",
>> - res.id);
>> + _cairo_output_stream_printf (surface->output,
>> + "%d 0 obj\n"
>> + " [%d 0 R /XYZ null null 0]\n"
>> + "endobj",
>> + link->res.id,
>> + res.id);
>> }
>>
>> return CAIRO_STATUS_SUCCESS;
>> @@ -376,6 +381,10 @@ cairo_pdf_interchange_write_dest
>> (cairo_pdf_surface_t *surface,
>> * reference instead of the name */
>> double x = 0;
>> double y = 0;
>> + cairo_pdf_resource_t res;
>> + cairo_pdf_link_t *link;
>> +
>> + link = _cairo_malloc (sizeof (cairo_pdf_link_t));
>
> link can be NULL here if malloc fails, so error handling for that
> might
> be appropriate here.
>
>> if (named_dest->extents.valid) {
>> x = named_dest->extents.extents.x;
>> @@ -388,10 +397,16 @@ cairo_pdf_interchange_write_dest
>> (cairo_pdf_surface_t *surface,
>> if (named_dest->attrs.y_valid)
>> y = named_dest->attrs.y;
>>
>> - status = cairo_pdf_interchange_write_explicit_dest (surface,
>> - named_dest->page,
>> - TRUE,
>> - x, y);
>> + res = _cairo_pdf_surface_new_object (surface);
>> + _cairo_output_stream_printf (surface->output,
>> + " /Dest %d 0 R\n",
>> + res.id);
>> + link->res = res;
>> + link->page = named_dest->page;
>> + link->has_pos = TRUE;
>> + link->x = x;
>> + link->y = y;
>> + status = _cairo_array_append (&surface->links, link);
>
> I'm not sure I follow the reason for the above change, can you
> explain?
>
>> return status;
>> }
>> }
>> @@ -406,11 +421,24 @@ cairo_pdf_interchange_write_dest
>> (cairo_pdf_surface_t *surface,
>> dest);
>> free (dest);
>> } else {
>> - status = cairo_pdf_interchange_write_explicit_dest (surface,
>> - link_attrs->page,
>> - link_attrs->has_pos,
>> - link_attrs->pos.x,
>> - link_attrs->pos.y);
>> + cairo_pdf_resource_t res;
>> + cairo_pdf_link_t *link;
>> +
>> + link = _cairo_malloc (sizeof (cairo_pdf_link_t));
>> + res = _cairo_pdf_surface_new_object (surface);
>> + _cairo_output_stream_printf (surface->output,
>> + " /Dest %d 0 R\n",
>> + res.id);
>> + link->res = res;
>> + link->page = link_attrs->page;
>> + link->has_pos = link_attrs->has_pos;
>> + if (link->has_pos) {
>> + link->x = link_attrs->pos.x;
>> + link->y = link_attrs->pos.y;
>> + }
>> + status = _cairo_array_append (&surface->links, link);
>> + if (unlikely (status))
>> + return status;
>
> An explanation of the above would be appreciated as well. Make sure
> to
> also check link for NULL, and _cairo_pdf_surface_new_object() also has
> an error condition to check for, which can be detected here if res.id
> == 0
>
>> }
>>
>> return CAIRO_STATUS_SUCCESS;
>> @@ -855,6 +883,20 @@ strcmp_null (const char *s1, const char *s2)
>> return FALSE;
>> }
>>
>> +static cairo_int_status_t
>> +cairo_pdf_interchange_write_links (cairo_pdf_surface_t *surface)
>> +{
>> + int num_elems, i;
>> + cairo_pdf_link_t link;
>> +
>> + num_elems = _cairo_array_num_elements (&surface->links);
>> + for (i = 0; i < num_elems; i++) {
>> + _cairo_array_copy_element (&surface->links, i, &link);
>> + cairo_pdf_interchange_write_explicit_dest (surface, &link);
>> + }
>> + return CAIRO_STATUS_SUCCESS;
>> +}
>
> I assume if there are zero links (i.e. num_elems == 0) that calling
> this
> should still result in success; if it shouldn't then might want to
> check
> and return a different status in that case.
>
>> +
>> static cairo_int_status_t
>> cairo_pdf_interchange_write_page_labels (cairo_pdf_surface_t
>> *surface)
>> {
>> @@ -1357,10 +1399,10 @@ _cairo_pdf_interchange_write_page_objects
>> (cairo_pdf_surface_t *surface)
>> cairo_int_status_t status;
>>
>> status = cairo_pdf_interchange_write_page_annots (surface);
>> - if (unlikely (status))
>> + if (unlikely (status))
>> return status;
>>
>> - cairo_pdf_interchange_clear_annotations (surface);
>> + cairo_pdf_interchange_clear_annotations (surface);
>>
>> return cairo_pdf_interchange_write_page_parent_elems (surface);
>> }
>> @@ -1395,6 +1437,10 @@
>> _cairo_pdf_interchange_write_document_objects (cairo_pdf_surface_t
>> *surface)
>> if (unlikely (status))
>> return status;
>>
>> + status = cairo_pdf_interchange_write_links (surface);
>> + if (unlikely (status))
>> + return status;
>> +
>> status = cairo_pdf_interchange_write_names_dict (surface);
>> if (unlikely (status))
>> return status;
>> diff --git a/src/cairo-pdf-surface-private.h
>> b/src/cairo-pdf-surface-private.h
>> index 3793332ce..64dd300f6 100644
>> --- a/src/cairo-pdf-surface-private.h
>> +++ b/src/cairo-pdf-surface-private.h
>> @@ -208,6 +208,14 @@ typedef struct _cairo_pdf_outline_entry {
>> int count;
>> } cairo_pdf_outline_entry_t;
>>
>> +typedef struct _cairo_pdf_link {
>> + cairo_pdf_resource_t res;
>> + int page;
>> + cairo_bool_t has_pos;
>> + double x;
>> + double y;
>> +} cairo_pdf_link_t;
>> +
>> struct docinfo {
>> char *title;
>> char *author;
>> @@ -327,6 +335,7 @@ struct _cairo_pdf_surface {
>> cairo_pdf_interchange_t interchange;
>> int page_parent_tree; /* -1 if not used */
>> cairo_array_t page_annots;
>> + cairo_array_t links;
>> cairo_bool_t tagged;
>> char *current_page_label;
>> cairo_array_t page_labels;
>> diff --git a/src/cairo-pdf-surface.c b/src/cairo-pdf-surface.c
>> index ab6781340..9fd48265d 100644
>> --- a/src/cairo-pdf-surface.c
>> +++ b/src/cairo-pdf-surface.c
>> @@ -493,6 +493,7 @@ _cairo_pdf_surface_create_for_stream_internal
>> (cairo_output_stream_t *output,
>>
>> surface->page_parent_tree = -1;
>> _cairo_array_init (&surface->page_annots, sizeof
>> (cairo_pdf_resource_t));
>> + _cairo_array_init (&surface->links, sizeof (cairo_pdf_link_t));
>> surface->tagged = FALSE;
>> surface->current_page_label = NULL;
>> _cairo_array_init (&surface->page_labels, sizeof (char *));
>> @@ -2296,6 +2297,7 @@ _cairo_pdf_surface_finish (void
>> *abstract_surface)
>> _cairo_array_fini (&surface->fonts);
>> _cairo_array_fini (&surface->knockout_group);
>> _cairo_array_fini (&surface->page_annots);
>> + _cairo_array_fini (&surface->links);
>>
>> if (surface->font_subsets) {
>> _cairo_scaled_font_subsets_destroy (surface->font_subsets);
>
> Thanks for working on this. You might want to file a ticket in
> Bugzilla
> if there's not one already, so it can be referenced from this patch.
> (This may be handy with doing bugfix backporting, for ex.)
>
> Make sure to also look at how to get some test coverage on this.
> Would
> something squeezed into create-for-stream.c make sense? Or, maybe
> better to craft a new test case that checks for this particular
> situation?
>
> Beyond that, I'd love if Adrian can jump in with comments on it as
> well,
> before we look at landing it.
>
> Bryce
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.cairographics.org/archives/cairo/attachments/20180829/afc2c4d5/attachment.html>
More information about the cairo
mailing list