Re: [PATCH] Fix links whose targets are on a page that is not created yet

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 
<[email protected]> 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

-- 
cairo mailing list
[email protected]
https://lists.cairographics.org/mailman/listinfo/cairo