Images taller than 32767 pixels fail to be rendered on macOS

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: emk, Assigned: emk)

Tracking

unspecified
mozilla55
Unspecified
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
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.
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.
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-
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.
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).
Bug 1274692 will removed the MOz2D limit soon.
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.
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)
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)
Taking as pre-noticed.
Assignee: aosmond → VYV03354
Flags: needinfo?(aosmond)
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.
Attachment #8787250 - Attachment is obsolete: true
Attachment #8787250 - Flags: review?(jmuizelaar)
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+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/bef45a9bb35e
Allow 64k width/height images to be rendered. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/bef45a9bb35e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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?
(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).
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+
seems this hit a merge conflict on beta, Masatoshi could you take a look, thanks!
Flags: needinfo?(VYV03354)
Please use this.
Flags: needinfo?(VYV03354)
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)
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.