1060609 - Add downscale-during-decode support for the PNG decoder

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Artem Sobolev [:asobolev]]

Just as in case of JPEG (see https://bugzilla.mozilla.org/show_bug.cgi?id=1045929), we'd like to have downscaling during decoding for PNG images.

There's a problem with APNG format: since animation is done by use of "image patches", we can't downscale image on-the-fly because we don't know neighborhood for boundary pixels. Keeping previous frames would not result in memory saving, so APNGs are currently unsupported.

Comment 1

[Artem Sobolev [:asobolev]]

Attachment: Preliminary patch

Comment 2

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8484676 [details] [diff] [review]
Preliminary patch

Since there have been major changes to the decoder infrastructure since this patch was written, it needs a rewrite.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: Add downscale-during-decode support for the PNG decoder

Here's the patch. Easy enough. (And much easier than the previous patch, since
we now detect animation during metadata decoding and refuse to enable
downscale-during-decode if it's detected.)

I pulled the invalidation code into separate methods to avoid a bunch of nested
conditionals.

Comment 4

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=86e1a42d262d

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: Add downscale-during-decode support for the PNG decoder

Updated to fix an assertion.

Comment 6

[Glenn Randers-Pehrson]

The current patch isn't working for me.  Upscaling (<ctrl-shift+> or <ctrl-scrollwheelup> is fine but downscaling (<ctrl-> or <ctrl-scrollwheeldown>) gives erratic behavior.  Sometimes it makes PNGs disappear once it reaches a scale less than 1:1, and other times nothing happens (the 1:1 or larger image remains on screen).

Comment 7

[Seth Fowler [:seth] [:s2h]]

(In reply to Glenn Randers-Pehrson from comment #6)
> The current patch isn't working for me.  Upscaling (<ctrl-shift+> or
> <ctrl-scrollwheelup> is fine but downscaling (<ctrl-> or
> <ctrl-scrollwheeldown>) gives erratic behavior.  Sometimes it makes PNGs
> disappear once it reaches a scale less than 1:1, and other times nothing
> happens (the 1:1 or larger image remains on screen).

Yeah, I see that too. It looks like it's a bug in the invalidation code. I'll debug it and upload a fix.

Comment 8

[Seth Fowler [:seth] [:s2h]]

Attachment: Add downscale-during-decode support for the PNG decoder

OK, found the problem. When cleaning up the invalidation stuff I managed to
delete the call to Downscaler::CommitRow(), which is kinda critical!

Comment 9

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=26227c210c29

Comment 10

[Glenn Randers-Pehrson]

Better, but something's still wrong.  Now it downscales, but the interlaced PNG is squashed to half the proper height, overlaid on the correct image.  See for example grammie.html attached to bug #75077, let it finish updating all passes, then <ctrl-minus>.  It restores the correct image if you hit <ctrl plus> to get back to 1:1 or larger.

Comment 11

[Seth Fowler [:seth] [:s2h]]

(In reply to Glenn Randers-Pehrson from comment #10)
> Better, but something's still wrong.  Now it downscales, but the interlaced
> PNG is squashed to half the proper height, overlaid on the correct image. 
> See for example grammie.html attached to bug #75077, let it finish updating
> all passes, then <ctrl-minus>.  It restores the correct image if you hit
> <ctrl plus> to get back to 1:1 or larger.

Thanks Glenn. Though there *are* failures in the try job above, I don't think any of them capture this problem, which suggests that we don't have test coverage for this case. I'll add a test.

Comment 12

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cb95be85a52

Comment 13

[Seth Fowler [:seth] [:s2h]]

So in the previous try job I added a patch that disables downscale-during-decode whenever HQ scaling is disabled. I thought it already worked this way, but apparently I was wrong! That resolves the remaining oranges in existing tests.

The only failures in the previous try job are for the new tests I mentioned in comment 11; there's still an issue with interlaced PNGs. I'm pretty sure I know the issue: we need to run the main block of code in row_callback even if |new_row| is null if we have a downscaler, because the downscaler doesn't keep untouched rows from the last pass around. I'll push a new try job to verify this theory.

Comment 14

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=653d0533e2dd

Comment 15

[Glenn Randers-Pehrson]

Commenting out the test on row_num at line 755 worked for me.

    /* if (new_row) */ {

Comment 16

[Glenn Randers-Pehrson]

(In reply to Glenn Randers-Pehrson from comment #15)
> Commenting out the test on row_num at line 755 worked for me.

On new_row

Comment 17

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 1) - Disable downscale-during-decode when HQ scaling is disabled

OK, try is solid green now, and thanks to Glenn the issues with interlaced PNGs
have been resolved, so I think this is ready to go.

I've split this into a few different parts. In part 1, I just disable DDD when
HQ scaling is disabled. This stops tests from breaking when we turn on DDD for
PNGs because the tests assume that HQ scaling won't happen (after all, DDD gives
the same results as HQ scaling).

Comment 18

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 2) - Add downscale-during-decode support for the PNG decoder

This part actually adds DDD support for the PNG decoder. It's pretty much the
same as the previous patch, except that it includes the change mentioned in
comment 13, which fixes the issue with interlaced PNGs.

Comment 19

[Seth Fowler [:seth] [:s2h]]

Attachment: (Part 3) - Add tests that interlaced and non-interlaced PNGs have the same downscaling behavior

This part adds tests that check that interlaced and non-interlaced versions of
the same PNG image look the same when downscaled. These tests would've caught
the issue that Glenn pointed out in comment 10.

Comment 20

[Glenn Randers-Pehrson]

The latest set of three patches (2015-08-28 17:13 through 17:21 EDT) applied to a fresh download from mozilla-central are working fine for me with interlaced PNG images.

Comment 21

[Glenn Randers-Pehrson]

I'm still seeing some weirdness with these patches applied. When viewing a Facebook album, the forward and back arrows (at the right and left sides of the image) are distorted.  When viewing Google maps, the zoom-in and zoom-out (+ and - icons) are distorted. Google Map tiles are also malformed. The distortions go away when viewing at 1:1 scale.

Comment 22

[Glenn Randers-Pehrson]

(In reply to Glenn Randers-Pehrson from comment #21)
> I'm still seeing some weirdness with these patches applied.

Sorry, pilot error; I was not testing the right version.

Comment 23

[Seth Fowler [:seth] [:s2h]]

Thanks for checking these patches so thoroughly, Glenn!

Comment 24

[Seth Fowler [:seth] [:s2h]]

Looks like the most recent try job, in comment 14, is extremely green. That's great.

Comment 25

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e177bf901397
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6ee72faf4fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/40101a28f532

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e177bf901397
https://hg.mozilla.org/mozilla-central/rev/d6ee72faf4fb
https://hg.mozilla.org/mozilla-central/rev/40101a28f532