691061 - Don't use EXTEND_PAD on printing surfaces

Status: REOPENED

(Core :: Graphics, defect)

Description

[u285178]

Attachment: don't use EXTEND_PAD on printing surfaces

Using EXTEND_PAD on printing surfaces (PDF, PS, and win32-printing) is inefficient. These printing surfaces don't natively support or need EXTEND_PAD. If EXTEND_PAD is used these surfaces will paint the pattern to a temporary surface the size of the fill then embed the temporary image in the output. This increases printing time and prevents the use of mime data from cairo_surface_set_mime_data().

The attached patch fixes this.

Comment 1

[Joe Drew (not getting mail)]

Comment on attachment 563972 [details] [diff] [review]
don't use EXTEND_PAD on printing surfaces

Jeff, what are your thoughts on this? I'm supportive in principle.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 563972 [details] [diff] [review]
don't use EXTEND_PAD on printing surfaces

I don't really like this. In theory these surfaces are supposed to detect that the image is not drawn beyond the bounds of the surface and thus don't need to PAD. However, I am a pragmatist and will take the patch provided it comes with a comment that explains this situation.

Comment 3

[u285178]

Here's a comment explaining the situation that you can paste into the code.

// The printing surfaces don't natively support or need EXTEND_PAD for padding
// the edges. Using EXTEND_PAD this way is suboptimal as it will result in the
// printing surface creating a new image for each fill operation. The pattern
// will be painted to the image to pad out the pattern, then the new image
// will be used as the source. This increases printing time and memory use,
// and prevents the use of mime data from cairo_surface_set_mime_data().
// Bug 691061.

Comment 4

[Ed Morley [:emorley]]

Please can you add the comment from comment 3 & add author/commit message along the lines of http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Removing checkin-needed for now - please re-add once this is done.

Thanks :-)

Comment 5

[u285178]

Attachment: don't use EXTEND_PAD on printing surfaces - with comment

Patch with author and comment

Comment 6

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/e481b6ffc60b

Comment 7

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/e481b6ffc60b

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

I can confirm that backing this patch out restores "Save as PDF" functionality. The PDF has images rendered correctly.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

I am likely to back this out since it broke Firefox Mobile functionality. If anyone has concerns about backing out, speak up!

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Backed out:
https://hg.mozilla.org/mozilla-central/rev/aa3b4c2feb42

Comment 11

[Julian Viereck]

I might have a patch for this - just doing some try builds to check.

Comment 12

[Julian Viereck]

Attachment: Proposed fix

This is the patch I came up with to fix a problem with rastering a Recording surface if it gets drawn to a PDFSurface.

To make things work on android, I prevented dropping from EXTEND_PAD -> EXTEND_NONE if the type of the pattern is a surface AND it's an image surface. That works for the broken page in 704185.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 624934 [details] [diff] [review]
Proposed fix

Doing this in SetPattern seems like the wrong place to do it. i.e. the user of SetPattern will not expect the extend mode to be changed by calling SetPattern. You should be doing this when the pattern is created.

Comment 14

[u285178]

I have fixed the cairo pdf surface to convert EXTEND_PAD to EXTEND_NONE if padding is not required to paint the extents of a fill in the pdf surface. The fixes are in cairo 1.12.2 + an extra patch in master.

This will likely expose bug 704185 (unless something else in 1.12 has fixed it) as the previous behavior was hiding that bug. Previously when using EXTEND_PAD a cairo_image_surface the size of the fill extents was created and filled with the source. For some reason the source image in bug 704185 is not correctly embedded in the pdf but if it is first painted to an image surface then it can be correctly embedded. I don't have the hardware to reproduce that bug so I have no idea what the cause of it is.

Comment 15

[Julian Viereck]

(In reply to Adrian Johnson from comment #14)
> I have fixed the cairo pdf surface to convert EXTEND_PAD to EXTEND_NONE if
> padding is not required to paint the extents of a fill in the pdf surface.
> The fixes are in cairo 1.12.2 + an extra patch in master.

That sounds awesome. Do you have a link to that patch?

> This will likely expose bug 704185 (unless something else in 1.12 has fixed
> it) as the previous behavior was hiding that bug. Previously when using
> EXTEND_PAD a cairo_image_surface the size of the fill extents was created
> and filled with the source. For some reason the source image in bug 704185
> is not correctly embedded in the pdf but if it is first painted to an image
> surface then it can be correctly embedded. 

As a work around, could this creation of an extra image still happen, if the pattern is based on an surface an the surface type is an CairoImageSurface? That's basically what I did in my patch and that worked for Android.

>I don't have the hardware to reproduce that bug so I have no idea what the cause of it is.

It seems possible to run Firefox on Android in the Android emulator, but I haven't tried it out :/

If you have some patches you want me to try out and give you some feedback how it looks, let me know!

Comment 16

[u285178]

(In reply to Julian Viereck from comment #15)
> That sounds awesome. Do you have a link to that patch?

These two look like they should fix the problem. I don't know if they will apply to 1.10 or if there were any other fixes in other patches. 1.10 is no longer supported.

http://cgit.freedesktop.org/cairo/commit/?id=c7ce1b68d5370f6e804a6edbf5be4bca3a5b7c57
http://cgit.freedesktop.org/cairo/commit/?id=50f08352f463d86022a0d7544d461fe2e5ac9076

> 
> > This will likely expose bug 704185 (unless something else in 1.12 has fixed
> > it) as the previous behavior was hiding that bug. Previously when using
> > EXTEND_PAD a cairo_image_surface the size of the fill extents was created
> > and filled with the source. For some reason the source image in bug 704185
> > is not correctly embedded in the pdf but if it is first painted to an image
> > surface then it can be correctly embedded. 
> 
> As a work around, could this creation of an extra image still happen, if the
> pattern is based on an surface an the surface type is an CairoImageSurface?
> That's basically what I did in my patch and that worked for Android.

It won't happen in cairo since it would break embedding of mime surfaces like jpeg as well as waste cpu and memory. But you can do what you like with the cairo version in mozilla. If you just use the 50f08352 patch it should do this but I have not tested this patch with 1.10.

It would be better to figure out the proper fix for bug 704185. If you have an Android it should not be hard to set a breakpoint in _cairo_pdf_surface_emit_image and find out what sort of image is being passed in. You could see if this patch fixes it:

http://cgit.freedesktop.org/cairo/commit/?id=33f9e433eef13a2b39a8213c6997399f3a5896a8

Comment 17

[Julian Viereck]

Adrian, thanks so much for these links.

I looked at 

   http://cgit.freedesktop.org/cairo/commit/?id=c7ce1b68d5370f6e804a6edbf5be4bca3a5b7c57

and it won't apply directly as some other stuff in the function changed. Not sure if I can try to merge the patch in any time soon, as I have to make sure some other stuff lands first :/

Comment 18

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

Anthony, all that needs to be done here is backport some cairo patches, looks like.

Comment 19

[Mozilla RelEng Bot]

Try run for 6bf234bf04e7 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6bf234bf04e7
Results (out of 285 total builds):
    exception: 2
    success: 260
    warnings: 22
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6bf234bf04e7

Comment 20

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 1: Check if EXTEND_PAD group can be painted with EXTEND_NONE

Comment 21

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Attachment: Part 2: Don't use patterns with padded images

Comment 22

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

Checkin needed for Part 1 and Part 2.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 624934 [details] [diff] [review]
Proposed fix

Please mark obsolete patches as such.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9a99a17afb
https://hg.mozilla.org/integration/mozilla-inbound/rev/74493ca34d5e

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1b9a99a17afb
https://hg.mozilla.org/mozilla-central/rev/74493ca34d5e

Comment 26

[Philip Chee]

The commit message is kind of naff.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Yeah, unfortunately I noticed it after pushing.

Comment 28

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

The broken commit message is my fault. Sorry.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Backed out in bug 825987.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6a638c16bd4

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6a638c16bd4