Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Don't use EXTEND_PAD on printing surfaces

REOPENED
Unassigned

Status

()

Core
Graphics
REOPENED
6 years ago
3 years ago

People

(Reporter: Adrian Johnson, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox21 disabled)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Blocks: 612844
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.
Attachment #563972 - Flags: review?(jmuizelaar)
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.
Attachment #563972 - Flags: review?(jmuizelaar) → review+
(Reporter)

Comment 3

6 years ago
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.
(Reporter)

Updated

6 years ago
Keywords: checkin-needed

Updated

6 years ago
Assignee: nobody → ajohnson

Comment 4

6 years ago
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 :-)
Keywords: checkin-needed
(Reporter)

Comment 5

6 years ago
Created attachment 570467 [details] [diff] [review]
don't use EXTEND_PAD on printing surfaces - with comment

Patch with author and comment
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/e481b6ffc60b
Keywords: checkin-needed
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/e481b6ffc60b
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 704185
I can confirm that backing this patch out restores "Save as PDF" functionality. The PDF has images rendered correctly.
I am likely to back this out since it broke Firefox Mobile functionality. If anyone has concerns about backing out, speak up!
Backed out:
https://hg.mozilla.org/mozilla-central/rev/aa3b4c2feb42
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → NEW

Comment 11

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

Comment 12

5 years ago
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.
Attachment #624934 - Flags: feedback?(jmuizelaar)
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.
Attachment #624934 - Flags: feedback?(jmuizelaar) → feedback-
(Reporter)

Comment 14

5 years ago
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

5 years ago
(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!
(Reporter)

Comment 16

5 years ago
(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

5 years ago
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 :/
Anthony, all that needs to be done here is backport some cairo patches, looks like.
Assignee: ajohnson → ajones
Status: NEW → ASSIGNED

Comment 19

5 years ago
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
Created attachment 664314 [details] [diff] [review]
Part 1: Check if EXTEND_PAD group can be painted with EXTEND_NONE
Created attachment 664315 [details] [diff] [review]
Part 2: Don't use patterns with padded images
Attachment #664314 - Flags: review?(jmuizelaar)
Attachment #664315 - Flags: review?(jmuizelaar)
Attachment #664314 - Flags: review?(jmuizelaar) → review+
Attachment #664315 - Flags: review?(jmuizelaar) → review+
Checkin needed for Part 1 and Part 2.
Keywords: checkin-needed
Attachment #563972 - Attachment is obsolete: true
Attachment #570467 - Attachment is obsolete: true
Comment on attachment 624934 [details] [diff] [review]
Proposed fix

Please mark obsolete patches as such.
Attachment #624934 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b9a99a17afb
https://hg.mozilla.org/integration/mozilla-inbound/rev/74493ca34d5e
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: mozilla10 → ---
https://hg.mozilla.org/mozilla-central/rev/1b9a99a17afb
https://hg.mozilla.org/mozilla-central/rev/74493ca34d5e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Comment 26

5 years ago
The commit message is kind of naff.
Yeah, unfortunately I noticed it after pushing.
The broken commit message is my fault. Sorry.

Updated

5 years ago
Depends on: 825987
Backed out in bug 825987.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6a638c16bd4
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
status-firefox21: --- → disabled
Target Milestone: mozilla18 → ---
https://hg.mozilla.org/mozilla-central/rev/f6a638c16bd4
Assignee: ajones → nobody
You need to log in before you can comment on or make changes to this bug.