Images taller than 32767 pixels fail to be rendered on macOS

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla55
Unspecified
Mac OS X
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

2 years ago
I found this when I was writing a reftest for bug 591822.
In the decoder setup path, we explicitly limit Mac OS X images to 32k height, where as other platforms are limited to 64k in either dimension. Apparently at the time, CoreGraphics could not render images taller than that.

https://dxr.mozilla.org/mozilla-central/rev/506facea63169a29e04eb140663da1730052db64/image/imgFrame.cpp#133
See bug 328258 which introduced the behaviour.
(Assignee)

Comment 3

2 years ago
It was a bug of the 10.4 era. According to the bug comment, it was fixed in 10.5.2. We are dropping support for 10.6-10.8. Is it still needed?

Even the 64k limit is lower than other browsers.
(Assignee)

Comment 4

2 years ago
Even if we need a limit, please do not enforce it in ImageLib. Otherwise Gfx has no way to display the image even when the image is scaled down.

Also, Gfx would have more knowledge about per-platform limits than cross-platform ImageLib.
Created attachment 8787172 [details] [diff] [review]
Allow 64k width/height images to be rendered, v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38351f3f8c01
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Attachment #8787172 - Flags: review?(jmuizelaar)
Comment on attachment 8787172 [details] [diff] [review]
Allow 64k width/height images to be rendered, v1

Review of attachment 8787172 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/cairo/cairo/src/cairo-image-surface.c
@@ +50,5 @@
>  
>  /* Limit on the width / height of an image surface in pixels.  This is
>   * mainly determined by coordinates of things sent to pixman at the
>   * moment being in 16.16 format. */
> +#define MAX_IMAGE_SIZE 65535

I don't think we can change this because this is a real limit that comes from pixman.
Attachment #8787172 - Flags: review?(jmuizelaar) → review-
Created attachment 8787250 [details] [diff] [review]
Allow 64k width/height images to be rendered, v2

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6896f229d3a3
Attachment #8787172 - Attachment is obsolete: true
Attachment #8787250 - Flags: review?(jmuizelaar)
Comment on attachment 8787250 [details] [diff] [review]
Allow 64k width/height images to be rendered, v2

Review of attachment 8787250 [details] [diff] [review]:
-----------------------------------------------------------------

If we do this we're going to have to test that printing works because that's what where we still use cairo-quartz-surface.c.
Created attachment 8787294 [details]
Large red png (65000x100)
(Assignee)

Comment 10

2 years ago
Cairo changes are not needed, by the way:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e368fd0badf7&selectedJob=26771837

The "failures" are unexpected pass because I'm marking the test with fails-if(OSX).
(Assignee)

Comment 11

2 years ago
Bug 1274692 will removed the MOz2D limit soon.
(Assignee)

Comment 12

2 years ago
Are you still working?

Note: I will not wait forever this time. If you do not respond within two weeks, I will take over this bug and update the patch with cairo changes dropped. We should not wait until printing surface migrate to skia.
Flags: needinfo?(aosmond)
Andrew is waiting on Jeff's review here.
(Assignee)

Comment 14

2 years ago
Jeff requests a unit test, so I thought it's Andrew's turn.

Jeff, please update the review flag if I am right.
Flags: needinfo?(jmuizelaar)

Comment 15

2 years ago
On Feb. 24th, Google started using this 33288px tall background image for emoji's on Gmail and Hangouts: https://ssl.gstatic.com/chat/emoji/png28-7f9d3a5045813584f828fe69a1fecb77.png

There have been numerous SuMo and Google forum threads about this not working in Firefox. I gave false hope to Mac users that Firefox 52 contained a fix that could help them; I now see bug 591822 only helps Windows users.

Maybe someone can press Google to do something on its end if this patch is going to take a long time to get to release.
I think someone still needs to test whether printing images this big works with CoreGraphics.
Flags: needinfo?(jmuizelaar)
(Assignee)

Comment 17

2 years ago
Taking as pre-noticed.
Assignee: aosmond → VYV03354
Flags: needinfo?(aosmond)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

2 years ago
mozreview-review
Comment on attachment 8847594 [details]
Bug 1298652 - Allow 64k width/height images to be rendered.

https://reviewboard.mozilla.org/r/120552/#review122498

This patch does not contain cairo changes. I will not fix macOS printing in this bug.
(Assignee)

Updated

2 years ago
Attachment #8787250 - Attachment is obsolete: true
Attachment #8787250 - Flags: review?(jmuizelaar)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8847594 [details]
Bug 1298652 - Allow 64k width/height images to be rendered.

https://reviewboard.mozilla.org/r/120552/#review122508
Attachment #8847594 - Flags: review?(jmuizelaar) → review+

Comment 21

2 years ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/bef45a9bb35e
Allow 64k width/height images to be rendered. r=jrmuizel

Comment 22

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bef45a9bb35e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 23

2 years ago
Comment on attachment 8847594 [details]
Bug 1298652 - Allow 64k width/height images to be rendered.

Approval Request Comment
[Feature/Bug causing the regression]: bug 328258
[User impact if declined]: Emojis on Gmail will not work on macOS, see bug 1342697.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]:  No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This will only remove a limit for obsolete (no longer supported) macOS versions.
[String changes made/needed]: None.
Attachment #8847594 - Flags: approval-mozilla-beta?
Attachment #8847594 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 24

2 years ago
(In reply to Masatoshi Kimura [:emk] from comment #23)
> [List of other uplifts needed for the feature/fix]: None.

Sorry, this is wrong. Bug 591822 test is needed for beta (aurora already includes it).

Updated

2 years ago
status-firefox53: --- → affected
status-firefox54: --- → affected
Comment on attachment 8847594 [details]
Bug 1298652 - Allow 64k width/height images to be rendered.

Fix an image issue so that 64k width/height images can be rendered. Aurora54+ & Beta53+.
Attachment #8847594 - Flags: approval-mozilla-beta?
Attachment #8847594 - Flags: approval-mozilla-beta+
Attachment #8847594 - Flags: approval-mozilla-aurora?
Attachment #8847594 - Flags: approval-mozilla-aurora+

Comment 26

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/f8c2bc928a44
status-firefox54: affected → fixed
seems this hit a merge conflict on beta, Masatoshi could you take a look, thanks!
Flags: needinfo?(VYV03354)
(Assignee)

Comment 28

2 years ago
Created attachment 8849107 [details] [diff] [review]
patch for beta

Please use this.
Flags: needinfo?(VYV03354)

Comment 29

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/f0331a214ae8
status-firefox53: affected → fixed
(Assignee)

Comment 30

2 years ago
Comment on attachment 8849107 [details] [diff] [review]
patch for beta

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
ESR 52 is the last version that supports OS X 10.6 to 10.8 and this patch will fix a bug on those versions of OS X.
User impact if declined: Emojis on Gmail will not work on macOS, see bug 1342697.
Fix Landed on Version: 53
Risk to taking this patch (and alternatives if risky): Low. This will only remove a limit for obsolete (no longer supported) macOS versions.
String or UUID changes made by this patch: None.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8849107 - Flags: approval-mozilla-esr52?
Setting qe-verify- based on Masatoshi's assessment on manual testing needs (see Comment 23) and the fact that this fix has automated coverage.
Flags: qe-verify-
I'm confused here, Firefox 49 dropped support for OSX versions < 10.9, so I don't think they're supported in 52.
Flags: needinfo?(VYV03354)
(Assignee)

Comment 33

2 years ago
Comment on attachment 8849107 [details] [diff] [review]
patch for beta

Oh, sorry, I was somehow confused about system requirements.
Flags: needinfo?(VYV03354)
Attachment #8849107 - Flags: approval-mozilla-esr52?
You need to log in before you can comment on or make changes to this bug.