369930 - Enable PDF surface in thebes

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Stuart Parmenter]

Attachment: v0.75

build/enable/fix the pdf surface in cairo on windows.

this patch also has part of the code from 367907 we need as well until that is checked in.

Comment 1

[Stuart Parmenter]

Attachment: v0.80

Comment 2

[nick]

I have been experimenting with the Cairo PDF backend on Windows in the last weeks and found out that snapping to "PDF device pixels" of 1/72th of an inch produces all kinds of rendering errors. This issue is also discussed in patch v0.80:

    /* XXX roc should look at this
    21:04 < roc> another approach would be to make device units for printing be
             1/(96*60) of an inch, set appunitsperdevpixel to 1, and apply a
             scale everywhere
21:05 < roc> actually
21:05 <@stuart> that might be harder to do
21:05 < roc> if we have any code in layout that rounds things to device pixels,
             that approach would work better
21:06 < roc> with your current approach, layout is going to round things to
             1/72in boundaries
21:08 < roc> and of course, we do have code in layout that rounds things to
             device pixels, so that approach really is the way to go
 */

I came up with the following patch that keeps 72 dpi, but disables snapping to device pixels for PDF surfaces. It basically adds a boolean member variable mSnap to the device and rendering context, that governs the rounding of coords to dev pixels.

I also had to introduce a version of nsThebesImage::Draw that accepts floating point coords for the destination rect. Fonts (on Windows) were another problem. Because all metrics are in device pixels I use the font metrics of a 20 times larger font and scale them back down.

The only code in layout that I found that rounds to dev pixels are the macros NS_ROUND_BORDER_TO_PIXELS and NS_ROUND_OFFSET_TO_PIXELS in nsStyleStruct.h. This should be disabled for PDF output.

This approach results in floating point accuracy in the generated PDFs as opposed to the approach suggested by roc that effectively treats PDFs as a 5760 dpi device. I doubt you would see a difference, but the ability to disable snapping to device pixels might also be interesting for future sub-pixel addressable Cairo backends.

The patch also contains other fixes like handling errors from cairo_pdf_surface_create and stuff related to scaling of pages when printing.

Comment 3

[nick]

Attachment: This patch disables snapping to device pixels for PDF surfaces

Comment 4

[Stuart Parmenter]

so cairo by default has a 32x font metrics thing it does but we set the scale factor to 1 to avoid doing that because scaling up and back down like that obviously doesn't work for bitmap fonts.  Looking at your patch we'd have the same problem with bitmap fonts.  I'm not really sure what to do about them :/

Should the cairo changes in your tree be pushed upstream?  I haven't dug in to them  much so I'm not quite sure what they're fixing.

Comment 5

[Stuart Parmenter]

Attachment: v0.85

cleaned up a bit more and changed the PDF and PS surfaces over to writing to an nsIOutputStream (nsIFileOutputStream backed) so that they support unicode file paths cross-platform and provide a more flexable surface type.

Comment 6

[nick]

I have posted my Cairo changes to the Cairo mailing list.

http://marc.10east.com/?l=cairo&m=116932420120081&w=2
Fix for a trivial bug that hopefully makes it in the next release

http://marc.10east.com/?l=cairo&m=116932489216849&w=2
A performance issue. I didn't get any feedback.

http://marc.theaimsgroup.com/?l=cairo&m=116932444815338&w=2
A known issue that will be fixed in the next Cairo release

I didn't think about bitmap fonts yet. Basically, I'm interested in generating PDFs from HTML pages with Gecko in a non-interactive, off-screen, server-side way. I wouldn't need bitmap fonts for that at all.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I prefer treating PDF (or other logically unlimited resolution surfaces, such as SVG) as a 5760dpi device because it's a slightly simpler interface to layout and in practice I doubt we're going to see any visible difference. You'd need very good eyes or be zooming in to see some extraordinarily fine detail in that HTML page...

Comment 8

[nick]

It shouldn't make a difference for PDFs. But there are also some attempts to disable snapping to pixels when drawing to a scaled surface for print preview (all the matrix.HasNonTranslation stuff). I wonder if snapping shouldn't be disabled all the time in this case.

Comment 9

[Stuart Parmenter]

Attachment: v0.88

updated patch... i'm going to split this out in to core pieces and text as i'd like to get the basic hookup pieces landed so I stop having merge conflicts...

I've tried lots of various text changes with varying degrees of successful looking pdf files.  Still not quite sure what we should do here...

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You probably should land with 72dpi. But then we should figure out how to get 5760dpi working. It should be enough to have PDF scale everything down by 80; font sizes will be a huge number of "device pixels", but we can request all fonts with 1/80th of the "device pixel" size, and then apply an upscale of 80 when rendering them (which would temporarily set the CTM back to identity assuming no other scaling is going on).

Comment 11

[Stuart Parmenter]

Attachment: v0.88 - core bits

this is the core changes I want to land now.  I'll follow up later with some better text/scaling/etc patches to make it all work properly.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 256671 [details] [diff] [review]
v0.88 - core bits


>Index: gfx/cairo/cairo/src/Makefile.in
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/cairo/cairo/src/Makefile.in,v
>retrieving revision 1.28
>diff -u -6 -p -r1.28 Makefile.in
>--- gfx/cairo/cairo/src/Makefile.in	24 Jan 2007 23:53:03 -0000	1.28
>+++ gfx/cairo/cairo/src/Makefile.in	26 Feb 2007 23:57:14 -0000
>@@ -110,13 +110,19 @@ EXPORTS = cairo.h cairo-features.h cairo
> 
> 
> ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
> CSRCS   += cairo-win32-font.c \
>            cairo-win32-surface.c
> EXPORTS += cairo-win32.h
>+CSRCS   += cairo-base85-stream.c \
>+           cairo-pdf-surface.c \
>+	   cairo-type1-fallback.c \
>+	   cairo-truetype-subset.c \
>+	   cairo-cff-subset.c
>+EXPORTS += cairo-pdf.h
> endif


Any reason why we don't just always build PDF on all platforms?  It's fine as it is, but we may want to do that at some point in the future.

[nsThebesDeviceContext]
>+    } else {
>+        /* set mAppUnitsPerDevPixel so we're using exactly 72 dpi, even
>+         * though that means we have a non-integer number of device "pixels"
>+         * per CSS pixel
>+         */
>+        mAppUnitsPerDevPixel = (AppUnitsPerCSSPixel() * 96) / dpi;
>+        //mAppUnitsPerDevPixel = 1;
>+    }

nit: Get rid of that extra commented-out line

>Index: gfx/thebes/public/gfxPDFSurface.h

>RCS file: /cvsroot/mozilla/gfx/thebes/src/gfxPDFSurface.cpp,v
>retrieving revision 1.7
>diff -u -6 -p -r1.7 gfxPDFSurface.cpp
>--- gfx/thebes/src/gfxPDFSurface.cpp	8 Feb 2007 20:47:48 -0000	1.7
>+++ gfx/thebes/src/gfxPDFSurface.cpp	26 Feb 2007 23:57:29 -0000
>@@ -37,16 +37,25 @@
> 
> #include "gfxPDFSurface.h"
> 
> #include "cairo.h"
> #include "cairo-pdf.h"
> 
>-gfxPDFSurface::gfxPDFSurface(const char *filename, const gfxSize& aSizeInPoints)
>-    : mXDPI(-1), mYDPI(-1), mSize(aSizeInPoints)
>+static cairo_status_t
>+write_func(void *closure, const unsigned char *data, unsigned int length)
> {
>-    Init(cairo_pdf_surface_create(filename, mSize.width, mSize.height));
>+    PRUint32 written = 0;
>+    nsresult rv = reinterpret_cast<nsIOutputStream*>(closure)->Write((const char *)data, length, &written);
>+    NS_ASSERTION(written == length, "not everything was written to the file");

You can't just assert this... you have to call Write in a loop until it either returns an error or writes all the bytes.


>Index: gfx/thebes/src/gfxPSSurface.cpp

>+write_func(void *closure, const unsigned char *data, unsigned int length)
> {
>-    fwrite(data, 1, length, (FILE*)closure);
>+    PRUint32 written = 0;
>+    nsresult rv = reinterpret_cast<nsIOutputStream*>(closure)->Write((const char *)data, length, &written);
>+    NS_ASSERTION(written == length, "not everything was written to the file");

Same thing as above

>+    return CAIRO_STATUS_SUCCESS;
> }


Looks fine other than that.

Comment 13

[Stuart Parmenter]

Attachment: v0.90 - core bits

fixed the write loops and removed the comment..  don't want to build pdf surface on all platforms yet... not sure if it builds on the mac.  I may implement gfxPDFSurface using the quartz pdf surface on the mac at some point though...

Comment 14

[Stuart Parmenter]

Attachment: v0.91

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 256852 [details] [diff] [review]
v0.91

I think it would be best to use cairo PDF surface code on all platforms rather than have a separate Mac implementation. That'll make it easier for us to track down bugs etc, and it's less code to maintain (although I realize that the Quartz code would be small).

+    nsCOMPtr<nsIOutputStream> out = reinterpret_cast<nsIOutputStream*>(closure);

This could just be an nsIOutputStream *.

Have we changed the C++ guidelines so we don't need to use NS_*_CAST anymore?

You need to rev the UUID on nsIPrintSettings.

Is someone going to write code for Linux to use the right output format?

Comment 16

[Martijn Wargers (dead)]

Is this going to make Gecko1.9?

Comment 17

[Stuart Parmenter]

this is all mostly checked in.  It is possible to print to PDF using an extension I have.  Going to mark fixed..

Comment 18

[Martijn Wargers (dead)]

Where is that extension, can it be downloaded?
What does it mean for bug 162659? It means users won't be able to save as PDF by default, without using an extension, right?

Comment 19

[Worcester12345]

(In reply to comment #18)
> Where is that extension, can it be downloaded?
> What does it mean for bug 162659? It means users won't be able to save as PDF
> by default, without using an extension, right?
> 

Also looking for the same answers. Thanks.

Comment 20

[Worcester12345]

Disregard previous comment. Sorry.