Bug 690572 - reference counting problem with devices & patterns
Summary: reference counting problem with devices & patterns
Status: RESOLVED FIXED
Alias: None
Product: Ghostscript
Classification: Unclassified
Component: Graphics Library (show other bugs)
Version: master
Hardware: PC Windows NT
: P2 major
Assignee: Tor Andersson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-06-24 09:25 UTC by Ken Sharp
Modified: 2010-05-21 10:22 UTC (History)
0 users

See Also:
Customer:
Word Size: ---


Attachments
patch to address reference counting (560 bytes, patch)
2010-05-20 14:39 UTC, Ken Sharp
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ken Sharp 2009-06-24 09:25:03 UTC
pdfwrite does not currently work with many of the XPS FTS files.

The problem seems to be due to patterns, files which contain some kinds of
patterns end up not closing the pdfwrite device, which relies upon the final
closure to write the xref table and a number of 'global' objects. The resulting
PDF file is so badly broken even Acrobat can't open it....

The problem is in an area of the code I'm not at all familiar with. It seems
that the pdfwrite device is reference counted in this situation, and the
de-initialisation of the 'universe' in the interpreter counts down the device.
Assuming the device has a reference count of 1 at this point it is properly closed.

The FTS files end up with the device having a reference count of several
thousand, which is why the device is not closed when the reference from the
universe counts down.

This 'seems' to be related to pattern fills. Starting at xps__parse_tiling_brush
we call xps_fill. Skipping over much of the call stack, we call gx_fill_path
which calls gdev_pdf_fill_path. 

The comment in gdev_pdf_fill_path says 'Fallback to the default implementation
for handling a shading with CompatibilityLevel<=1.2' presumably the 'shading'
isn't compatible with PDF.

Anyway, it looks like we then convert this into an image instead, by calling
gx_default_fill_path, which calls gx_general_fill_path. This seems to degenerate
the fill into a series of trapezoids and fills each individually.

Starting this time from gx_fill_trapezoid we call gx_dc_pattern_fill_rectangle,
this calls tile_fill_init which calls tile_clip_initialize which calls
gx_mask_clip_initialize. Now this routine calls gx_device_set_target, this is
important because it uses rc_assign to set the target (of gx_device_mask_clip),
this action increments the reference count of the target device, in this case
pdfwrite.

Note that although we have incremented the target device reference count we have
not incremented the reference count of the tile_clip device which remains at 0.
This is probably reasonable, I don't think we will use it again outside the
context of the gx_dc_pattern_fill_rectangle routine, it is a member of a
structure allocated locally on the heap in this routine.

Returning to gx_dc_pattern_fill_rectangle we call tile_by_steps. This duly calls
an appropriate fill procedure multiple times.

That takes us to the end of gx_dc_pattern_fill_rectangle and we exit (back to
the fill trapezoid code). 

At no point have we decremented the reference count of the 'target' forwarded
device, ie pdfwrite, so the reference count simply keeps on increasing with each
tile.


I can work around this by decrementing the target device in
gx_dc_pattern_fill_rectangle but this feels like a hack. There doesn't seem to
be any kind of finalize routine for these devices, so the fact that the
tile_clip device increments the reference count of the target device feels
wrong, there doesn't seem to be any way for it to decrement the reference count
again.

Even after working around this problem I still end up with a reference count of
26 for the pdfwrite device at the end of job, so there's obviously at least one
more problem of this nature.


To reproduce, you must run gxps, with the pdfwrite device:

gxps -sDEVICE=pdfwrite -sOutputFile=out.pdf fts_01xx.xps

I found it useful to set a breakpoint in gx_dc_pattern_fill_rectangle, and watch
the reference count in the function parameter 'dev', ie dev->rc.ref_count. Each
time you get here the count gets incremented.
Comment 1 Ken Sharp 2009-06-24 09:28:53 UTC
Created attachment 5157 [details]
fts_01xx.xps

A file which exhibits the problem (on page 2).
Comment 2 Ray Johnston 2009-06-25 10:26:42 UTC
How does this work with pattern fills and PS/PDF ?

Seems like whatever works there should work with other parsers IF they are
calling the graphics library similarly.
Comment 3 Henry Stiles 2009-06-25 10:58:05 UTC
> how does this work with pattern fills and PS/PDF ?
>
> Seems like whatever works there should work with other parsers IF they are
> calling the graphics library similarly.

postscript and pdf use the high level pattern manager, see bug #687303.  AFAIK
all the languages leak memory when using pdfwrite, so for now can we force the
reference count to 1 and print a warning before closing the device.
Comment 4 Ken Sharp 2009-06-26 00:19:26 UTC
I think the only place we can force the device reference count is in plmain.c,
pl_main_universe_dnit, line 579:

    if (universe->curr_device) {
	gs_unregister_root(universe->curr_device->memory, &device_root,
"pl_main_universe_select");
	/* ps allocator retain's the device, pl_alloc doesn't */
	gx_device_retain(universe->curr_device, false);
	universe->curr_device = NULL;
    }

We could force the reference count in gx_device_retain but I think it would be
better to force it before calling gx_device_retain. Eg:

    if (universe->curr_device) {
	gs_unregister_root(universe->curr_device->memory, &device_root,
"pl_main_universe_select");
        if(universe->curr_device.rc.ref_count > 1)
        {
            universe->curr_device.rc.ref_count = 1;
            if (err_str)
                sprintf(err_str, "Device reference count not 1 when closing.\n");
        }
	/* ps allocator retain's the device, pl_alloc doesn't */
	gx_device_retain(universe->curr_device, false);
	universe->curr_device = NULL;
    }

If that looks OK I'll make the change, it'll save me chasing the other reference
counting problem as well.
Comment 5 Michael Vrhel 2009-06-26 05:18:14 UTC
That seems reasonable to me for now, but are the visual brushes of XPS not 
being mapped into pattern fills for PDF write?   I have not had a chance to 
look at this yet, but it would seem that is what should be done eventually.
Comment 6 Ken Sharp 2009-06-26 05:37:33 UTC
Visual brushes are indeed converted to pattern fills by a complex process.
However pdfwrite cannot handle some of the visual brushes that are being used.

I checked the first case that fails to write out as a fill using a /Pattern
colour space (this is what leads to the reference counting), and the reason
pdfwrite cannot handle it is because the pattern matrix is not orthogonal to the
axes. In gdevpdfv.c, pdf_pattern, line around line 126:

    /*
     * We currently can't handle Patterns whose X/Y step isn't parallel
     * to the coordinate axes.
     */

Matrix in this case is [75 0 -37.5 0 3525 5730]

Its possible (perhaps even likely) that this fallback isn't working either,
since the pages cause errors in Acrobat even when the reference counting is
'fixed'. But that's a different bug, #689484, which is what I was looking at
when the reference counting issue came up. I had hoped the fix I'd done for
#690056 might have been the same issue, so I ran the FTS file to check, only to
encounter a different problem :-(


I could concentrate on fixing this problem instead, but I believe there are
shadings in XPS which can't readily be handled as PDF shadings, in which case I
think we would need to use this path anyway, so it needs to work.
Comment 7 Michael Vrhel 2009-06-26 05:49:11 UTC
I see that the matrix you mention [75 0 -37.5 0 3525 5730] is singular.   I 
don't see why in this XPS file we would be getting such a matrix.   
Comment 8 Ken Sharp 2009-06-26 06:05:17 UTC
Well, XPS isn't my area of expertise, so I'm not exactly sure. However this is
the step matrix, not the pattern matrix. Its how to transform the unit square to
get to the next pattern tile (I think). PDF Patterns have an XStep and a YStep
which is not independent of the Pattern Matrix, as they are measured in the
pattern co-ordinate system. I think its possible to deal with this matrix and
the general case, its just not done at the moment in pdfwrite.

There are two areas on page 2 of the XPS file which look possible candidates for
causing the issue; in the top section the image + blue hatching at the bottom
right is sheared in the x-direction, in the lower section the lowest set of
rectangles is sheared in the y-direction.

To be honest I've concentrated rather more on the reference counting issue than
any other aspects of the file. There are several patterns which work well
including those on pages 3, 5 and 6.
Comment 9 Ken Sharp 2009-06-26 06:07:08 UTC
Ooops, forgot to mention also that some of the patterns are 'flipped' or
mirrored in either the x or y direction, which might also be causing difficulty.
Comment 10 Michael Vrhel 2009-06-26 06:57:06 UTC
OK.  When I get done with this pattern transparency stuff, I am going to take 
a quick look at this example for myself.  Anytime I see a matrix that is 
singular, I become a bit suspicious.  Especially since I don't see where there 
would be any singular matrices for creating the shear in that example for an 
XPS visual brush or for a PDF pattern.   XPS has the viewport and viewbox 
(along with the tile mode to do the mirroring and flipping) to define the 
tiling.  I am pretty sure all of these should map straightforward into well 
behaved forms for PDF patterns.   But I need to sit down and go through the 
math before I can be certain of that.
Comment 11 Henry Stiles 2009-06-26 08:30:15 UTC
Tor should be responsible for investigating the singular matrix and he can
assign back to one of us once that part of the problem is analyzed.
Comment 12 Henry Stiles 2009-07-29 07:39:36 UTC
Raising priority since this would be a serious issue for any user.
Comment 13 Tor Andersson 2009-07-29 08:39:07 UTC
For whatever it's worth I can't find any singular matrices at that location of the code when running 
fts_01xx.xps. The closest matrix to the one Ken mentioned that I've found is [75 0 -37.5 75 3525 5730]. 
Could it be a typo that made us believe it was singular?
Comment 14 Ken Sharp 2009-07-29 08:43:27 UTC
Probably a typo on my part. I was *much* more concerned with the reference
counting being wrong than the matrix contents.

Who is the owner for problems involving devices ? ;-)
Comment 15 Ray Johnston 2009-07-29 10:34:49 UTC
Adding Ralph in since he should be aware of reference counting issues.
Comment 16 Ken Sharp 2010-05-20 14:39:50 UTC
Created attachment 6307 [details]
patch to address reference counting

The attached patch addresses the problem for me. In gx_dc_pattern_fill_rectangle, before calling the initialisation routine, it initialises the clip device finalize routine to NULL. 

If the initialisation decides that we need to clip the pattern fill, then it will create and open a clip device, storing the device in 'state.cdev'. If we do not need to clip the pattern fill then the clip device remains untounched.

At the end of the routine the clip device finalize routine will either be NULL (not clipping) or will have been set by the open device call. If the routine is not NULL then we call the finalize routine. This then counts down the reference to the forwarded device (pdfwrite in this case).

This works for me, and a cluster test appears to be OK, the only differences being more or less expected pdfwrite ones. I believe this is safe, but I'd very much appreciate someone else (Henry ?) looking at this as well.

For me this prevents pdfwrite emitting unfinished PDF files for:
fts_01xx.xps
fts_31xx.xps
fts_32xx.xps
fts_46xx.xps

Which isn't to say that they are without problems, but they do partially work which is an improvement.
Comment 17 Ken Sharp 2010-05-21 10:22:10 UTC
revision 11297:
http://ghostscript.com/pipermail/gs-cvs/2010-May/011089.html

resolves this issue for me. The other device reference counting issues noted above seem to have already been resolved by Henry's work in this area. The file now runs to completion, though there are other problems still to address.