Add downscale-during-decode support for the PNG decoder

RESOLVED FIXED in Firefox 43

Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: asobolev, Assigned: seth)

Tracking

(Blocks 1 bug)

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

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

Updated

5 years ago
Blocks: ddd
Reporter

Comment 1

5 years ago
Posted patch Preliminary patch (obsolete) — Splinter Review
Assignee

Updated

4 years ago
Depends on: 1194059
Assignee

Updated

4 years ago
Blocks: 75077
Assignee

Comment 2

4 years ago
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.
Attachment #8484676 - Attachment is obsolete: true
Assignee

Comment 3

4 years ago
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.
Attachment #8653310 - Flags: review?(tnikkel)
Attachment #8653310 - Flags: feedback?(glennrp+bmo)
Assignee

Updated

4 years ago
Assignee: asobolev → seth
Status: NEW → ASSIGNED
Assignee

Comment 5

4 years ago
Updated to fix an assertion.
Attachment #8653328 - Flags: review?(tnikkel)
Attachment #8653328 - Flags: feedback?(glennrp+bmo)
Assignee

Updated

4 years ago
Attachment #8653310 - Attachment is obsolete: true
Attachment #8653310 - Flags: review?(tnikkel)
Attachment #8653310 - Flags: feedback?(glennrp+bmo)
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).
Assignee

Comment 7

4 years ago
(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.
Assignee

Updated

4 years ago
Attachment #8653328 - Flags: review?(tnikkel)
Attachment #8653328 - Flags: feedback?(glennrp+bmo)
Assignee

Comment 8

4 years ago
OK, found the problem. When cleaning up the invalidation stuff I managed to
delete the call to Downscaler::CommitRow(), which is kinda critical!
Attachment #8653770 - Flags: review?(tnikkel)
Attachment #8653770 - Flags: feedback?(glennrp+bmo)
Assignee

Updated

4 years ago
Attachment #8653328 - Attachment is obsolete: true
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.
Assignee

Comment 11

4 years ago
(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.
Assignee

Comment 13

4 years ago
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.
Commenting out the test on row_num at line 755 worked for me.

    /* if (new_row) */ {
Attachment #8653770 - Flags: feedback?(glennrp+bmo) → feedback+
(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
Assignee

Comment 17

4 years ago
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).
Attachment #8654346 - Flags: review?(tnikkel)
Assignee

Comment 18

4 years ago
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.
Attachment #8654349 - Flags: review?(tnikkel)
Assignee

Comment 19

4 years ago
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.
Attachment #8654352 - Flags: review?(tnikkel)
Assignee

Updated

4 years ago
Attachment #8653770 - Attachment is obsolete: true
Attachment #8653770 - Flags: review?(tnikkel)
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.
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.
(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.
Assignee

Comment 23

4 years ago
Thanks for checking these patches so thoroughly, Glenn!
Assignee

Comment 24

4 years ago
Looks like the most recent try job, in comment 14, is extremely green. That's great.
Attachment #8654346 - Flags: review?(tnikkel) → review+
Attachment #8654352 - Flags: review?(tnikkel) → review+
Attachment #8654349 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/e177bf901397
https://hg.mozilla.org/mozilla-central/rev/d6ee72faf4fb
https://hg.mozilla.org/mozilla-central/rev/40101a28f532
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Assignee

Updated

4 years ago
Depends on: 1204470
Assignee

Updated

4 years ago
No longer depends on: 1204470
Assignee

Updated

4 years ago
Depends on: 1210256
Assignee

Updated

4 years ago
Depends on: 1210142
You need to log in before you can comment on or make changes to this bug.