Last Comment Bug 691061 - Don't use EXTEND_PAD on printing surfaces
: Don't use EXTEND_PAD on printing surfaces
Status: REOPENED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86_64 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on: 704185 825987
Blocks: 612844
  Show dependency treegraph
 
Reported: 2011-10-01 09:00 PDT by Adrian Johnson
Modified: 2014-05-27 15:37 PDT (History)
9 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
disabled


Attachments
don't use EXTEND_PAD on printing surfaces (604 bytes, patch)
2011-10-01 09:00 PDT, Adrian Johnson
jmuizelaar: review+
Details | Diff | Splinter Review
don't use EXTEND_PAD on printing surfaces - with comment (1.41 KB, patch)
2011-10-29 02:08 PDT, Adrian Johnson
no flags Details | Diff | Splinter Review
Proposed fix (1.95 KB, patch)
2012-05-17 15:54 PDT, Julian Viereck
jmuizelaar: feedback-
Details | Diff | Splinter Review
Part 1: Check if EXTEND_PAD group can be painted with EXTEND_NONE (2.09 KB, patch)
2012-09-24 18:35 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 2: Don't use patterns with padded images (12.29 KB, patch)
2012-09-24 18:36 PDT, Anthony Jones (:kentuckyfriedtakahe, :k17e)
jmuizelaar: review+
Details | Diff | Splinter Review

Description Adrian Johnson 2011-10-01 09:00:16 PDT
Created attachment 563972 [details] [diff] [review]
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) 2011-10-02 15:40:30 PDT
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] 2011-10-03 08:36:01 PDT
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 Adrian Johnson 2011-10-04 04:10:06 PDT
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] 2011-10-28 03:03:12 PDT
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 Adrian Johnson 2011-10-29 02:08:40 PDT
Created attachment 570467 [details] [diff] [review]
don't use EXTEND_PAD on printing surfaces - with comment

Patch with author and comment
Comment 7 Matt Brubeck (:mbrubeck) 2011-10-31 11:21:36 PDT
https://hg.mozilla.org/mozilla-central/rev/e481b6ffc60b
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2012-04-13 09:08:35 PDT
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?) 2012-04-13 09:16:23 PDT
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?) 2012-04-13 09:31:40 PDT
Backed out:
https://hg.mozilla.org/mozilla-central/rev/aa3b4c2feb42
Comment 11 Julian Viereck 2012-05-14 03:26:47 PDT
I might have a patch for this - just doing some try builds to check.
Comment 12 Julian Viereck 2012-05-17 15:54:50 PDT
Created attachment 624934 [details] [diff] [review]
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] 2012-05-17 16:08:43 PDT
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 Adrian Johnson 2012-05-17 16:54:27 PDT
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 2012-05-18 16:35:34 PDT
(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 Adrian Johnson 2012-05-18 17:23:56 PDT
(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 2012-05-21 00:52:46 PDT
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) 2012-08-24 14:23:35 PDT
Anthony, all that needs to be done here is backport some cairo patches, looks like.
Comment 19 Mozilla RelEng Bot 2012-09-23 22:45:28 PDT
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/ajones@mozilla.com-6bf234bf04e7
Comment 20 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-09-24 18:35:48 PDT
Created attachment 664314 [details] [diff] [review]
Part 1: Check if EXTEND_PAD group can be painted with EXTEND_NONE
Comment 21 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-09-24 18:36:44 PDT
Created attachment 664315 [details] [diff] [review]
Part 2: Don't use patterns with padded images
Comment 22 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-09-27 14:48:47 PDT
Checkin needed for Part 1 and Part 2.
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-09-27 16:54:07 PDT
Comment on attachment 624934 [details] [diff] [review]
Proposed fix

Please mark obsolete patches as such.
Comment 26 Philip Chee 2012-09-28 21:39:44 PDT
The commit message is kind of naff.
Comment 27 Ryan VanderMeulen [:RyanVM] 2012-09-29 04:49:25 PDT
Yeah, unfortunately I noticed it after pushing.
Comment 28 Anthony Jones (:kentuckyfriedtakahe, :k17e) 2012-09-30 12:27:29 PDT
The broken commit message is my fault. Sorry.
Comment 29 Ryan VanderMeulen [:RyanVM] 2013-01-30 05:40:30 PST
Backed out in bug 825987.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6a638c16bd4
Comment 30 Ryan VanderMeulen [:RyanVM] 2013-01-31 13:14:53 PST
https://hg.mozilla.org/mozilla-central/rev/f6a638c16bd4

Note You need to log in before you can comment on or make changes to this bug.