1298652 - Images taller than 32767 pixels fail to be rendered on macOS

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Masatoshi Kimura [:emk]]

I found this when I was writing a reftest for bug 591822.

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

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

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

See bug 328258 which introduced the behaviour.

Comment 3

[Masatoshi Kimura [:emk]]

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.

Comment 4

[Masatoshi Kimura [:emk]]

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.

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Allow 64k width/height images to be rendered, v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=38351f3f8c01

Comment 6

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Allow 64k width/height images to be rendered, v2

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6896f229d3a3

Comment 8

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Large red png (65000x100)

Comment 10

[Masatoshi Kimura [:emk]]

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).

Comment 11

[Masatoshi Kimura [:emk]]

Bug 1274692 will removed the MOz2D limit soon.

Comment 12

[Masatoshi Kimura [:emk]]

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.

Comment 13

[Timothy Nikkel (:tnikkel)]

Andrew is waiting on Jeff's review here.

Comment 14

[Masatoshi Kimura [:emk]]

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

Jeff, please update the review flag if I am right.

Comment 15

[jscher2000]

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.

Comment 16

[Jeff Muizelaar [:jrmuizel]]

I think someone still needs to test whether printing images this big works with CoreGraphics.

Comment 17

[Masatoshi Kimura [:emk]]

Taking as pre-noticed.

Comment 18

[Masatoshi Kimura [:emk]]

Attachment: Bug 1298652 - Allow 64k width/height images to be rendered.

Review commit: https://reviewboard.mozilla.org/r/120552/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120552/

Comment 19

[Masatoshi Kimura [:emk]]

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.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

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

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bef45a9bb35e
Allow 64k width/height images to be rendered. r=jrmuizel

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Masatoshi Kimura [:emk]]

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.

Comment 24

[Masatoshi Kimura [:emk]]

(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 25

[Gerry Chang [:gchang]]

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+.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8c2bc928a44

Comment 27

[Carsten Book [:Tomcat]]

seems this hit a merge conflict on beta, Masatoshi could you take a look, thanks!

Comment 28

[Masatoshi Kimura [:emk]]

Attachment: patch for beta

Please use this.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f0331a214ae8

Comment 30

[Masatoshi Kimura [:emk]]

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.

Comment 31

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Masatoshi's assessment on manual testing needs (see Comment 23) and the fact that this fix has automated coverage.

Comment 32

[Julien Cristau [:jcristau]]

I'm confused here, Firefox 49 dropped support for OSX versions < 10.9, so I don't think they're supported in 52.

Comment 33

[Masatoshi Kimura [:emk]]

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

Oh, sorry, I was somehow confused about system requirements.