Load progressive PNG images -> incomplete rendering

VERIFIED FIXED in M17

Status

Core Graveyard
GFX
P2
normal
VERIFIED FIXED
19 years ago
9 years ago

People

(Reporter: dbaron, Assigned: pnunn)

Tracking

Trunk
Other
Other

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [nsbeta2-]PLEASE reconsider for nsbeta2+., URL)

Attachments

(5 attachments)

(Reporter)

Description

19 years ago
The large snake image a few pages down in the above URL has strange progressive
display.  (You have to scroll down while it's still loading).   The progressive
display works fine on the right 20% or so of the image, but for the rest of the
image you only seem to be showing the lines you already have rather than
filling in the other areas.

Updated

19 years ago
Assignee: michaelp → pnunn
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED

Updated

19 years ago
QA Contact: 4110 → 2792

Comment 1

19 years ago
[QA Assigning to self.]
(Assignee)

Updated

19 years ago
Target Milestone: M5

Comment 2

19 years ago
This is due to imagelib modifying one of libpng's buffers (scale.cpp,
il_emit_row(), is_interleaved_alpha section).  Imagelib needs to allocate its
own buffers for an RGB row and an alpha row, copying from the libpng (RGBA) row
buffer appropriately.  Now that imagelib 2.0 is checked in and almost working,
we should be able to fix this relatively quickly.
(Assignee)

Comment 3

19 years ago
I think this has more to do with the
progressive display passes not rescaling.
Not the dimensions of each pass.
-pn
(Assignee)

Updated

19 years ago
Target Milestone: M5 → M6
(Reporter)

Comment 4

19 years ago
The images at

http://www.fas.harvard.edu/~dbaron/css/fonts/sizes/nn45linux

are also a little messed up.  The text comes out a bit rough.  There are some
pixels that are wrong every other row (looks like leftover from interlacing).
It's easily visible on the text.

Updated

19 years ago
QA Contact: 2792 → 1698
(Assignee)

Updated

19 years ago
Target Milestone: M6 → M9

Comment 5

19 years ago
This is two separate bugs.  The first, which applies only to transparent,
interlaced images, is due to the buffer problem I identified on 24 April; I've
fixed this in my code (temporary fix; final fix awaits other alpha-transparency
fixes).

The second bug, reported by dbaron, results in the even rows (if top = row 1 =
odd) of an interlaced image, either transparent or opaque, being shifted to the
left by one pixel in the final display.  I don't know what causes this, but I
suspect it's partly due to the fact that all PNG passes *except* the final one
have "holes" in the row data, and there is no per-pass mask being applied.  (Of
course, one would then expect that the *odd* rows would be screwed up...)  This
seems to be a fundamental limitation of the current imagelib implementation,
i.e., that masks correspond to transparency only.  Fixing that requires
structural changes to il_emit_row() in scale.cpp.

Comment 6

19 years ago
adding self to cc list
(Assignee)

Updated

19 years ago
Assignee: pnunn → newt
Status: ASSIGNED → NEW

Updated

19 years ago
Target Milestone: M9 → M11

Comment 7

19 years ago
m11

Updated

19 years ago
Status: NEW → ASSIGNED

Comment 8

19 years ago
I did some further investigation of the second bug, using the transparency mask
for both transparency and masking of partial interlace passes and calling
ImgDCBFlushImage() for every row.  Either ImgDCBFlushImage() isn't flushing, or
else something upstream isn't treating the mask correctly.  My best guess at the
moment is that the image mask is not being used to selectively lay down new
pixels but rather to selectively replace old ones with the background.  The two
are subtly different--critically so for PNG--though their behavior happens to be
identical in the case of GIF.

If my guess is correct, it's going to cause big problems for interlaced,
NON-transparent PNGs.  Masking is required for these images in order to avoid
replacing valid, previous-pass pixels with garbage from later, sparse passes
(e.g., in pass 6, only every other pixel is valid; what's in between is
undefined).  But Mozilla should never resolve the mask "transparency" in such
images by replacing it with background pixels--the mask in this case is
ephemeral and becomes totally opaque by the final interlace pass.

Eli, I don't know who's responsible for gfx/src, but I believe this all takes
place in nsImageRenderer.cpp (ImageRendererImpl::UpdatePixmap()) and through
that in the platform-specific nsImage[FE]::ImageUpdated() methods.  Could you
please cc somebody relevant who can discuss masks with me?

Thanks,
  Greg

Comment 9

19 years ago
[not sure who optimal person was, so pinged Greg by e-mail to suggest a few
folks.]

Comment 10

19 years ago
Pam checked in the other half of my pngcom revisions/fixes on 9 September, and
that fixes the first bug (i.e., the one where interlaced passes on transparent
images get squished/"echoed" to the left).

No progress yet on the second one.

Greg

Comment 11

19 years ago
*** Bug 8127 has been marked as a duplicate of this bug. ***

Comment 12

19 years ago
*** Bug 16820 has been marked as a duplicate of this bug. ***

Comment 13

19 years ago
[note to self: upon verification, please be sure to double-check that all of the
various interlaced PNGs that have been marked as duplicates do in fact display
correctly.]

Updated

19 years ago
Target Milestone: M11 → M12

Comment 14

19 years ago
m12.  let me know if there are more changes ready for this in
in the next couple of days and we can see about getting them into m11

Comment 15

19 years ago
Nothing is going to happen here for M11. Geoff, what's the deal with getting PNG
support up to scratch? Do you need any help from this end?

Comment 16

19 years ago
I'm Greg, and yes, I could use some help with the concept of masks as practiced
by gfx/src (see 09/03/99 entry above).  At Eli's suggestion, I sent an offline
e-mail to Patrick Beard asking for insights, but I never got any response.

Recap of basic problem:  using the transparency mask for both transparency and
interlacing purposes results in holes in the opaque parts of the image--i.e.,
the background shows through.  This SHOULD NEVER HAPPEN since previous passes
have paved over the entire area with opaque pixels, and I've flushed all pending
data.  Ergo, something funny is happening with the handling of masks, and I'm
pretty sure it has to do with old assumptions based on GIF transparency and
interlacing.  But I haven't had time to dig through the code under gfx for many
weeks.  (For that matter, it's been two months since I even had a
halfway-working build.)

Greg

Comment 17

19 years ago
*** Bug 18474 has been marked as a duplicate of this bug. ***

Updated

19 years ago
Summary: problems with progressive display of interlaced PNG → Progressive load of PNG images leave incomplete rendering

Comment 18

19 years ago
[updated to bug description from 18645 which is IMHO clearer.]

Comment 19

19 years ago
BTW, if this bug contains two separate issues (rather than just the incomplete
rendering on progressively loaded PNG images), could they be split into two
separate bugs, please?

[I can take a stab, but this is more white-box.]

Comment 20

19 years ago
*** Bug 18645 has been marked as a duplicate of this bug. ***

Comment 21

19 years ago
elig@netscape.com wrote:

> BTW, if this bug contains two separate issues (rather than just the incomplete
> rendering on progressively loaded PNG images), could they be split into two
> separate bugs, please?

Well, one is fixed, so I would argue that 3195 now contains only one (open)
issue. :-)  There are also a lot of links (including mental ones) to 3195 that
would break if the open part got moved.  Is it really that important?  If so,
I'd suggest moving the (fixed) transparency/interlacing bug somewhere else.  But
that seems a little silly to me...

Greg

Comment 22

19 years ago
Hrmf...as long as the summary accurately describes the current bug report, it's
all fine with me. (Please amend it if it's inaccurate!) Thanks!

Comment 23

19 years ago
The summary is correct for the outstanding bug--the transparency-related one
only showed up for the intermediate passes.

I haven't heard doodly from anyone who might know about how masks are used (in
the philosophical sense) in Mozilla, but if it would help, I think I could dig
out a patch I did that combines transparency-masking plus interlace-masking in
the same mask.  This results in final images that look OK except that they have
holes in them (every other line, every other pixel--i.e., 25% holes).  This is
the case that should never happen if the masking code did what (I believe) it
should--that is, all of the holes were opaque in previous interlace passes.  But
now that I think about it some more, it looks like Mozilla is saving that final,
combined mask for subsequent rendering tasks, when one really wants only part of
the mask to be saved.  This relates to Pam's comment (somewhere, once upon a
time) about needing two different types of masks--one for PNG/GIF transparency
and one for PNG interlacing.  I think she's right about that.

Greg

Updated

19 years ago
Target Milestone: M12 → M14

Comment 24

19 years ago
m14.  let me know if there are more changes ready for this in
in the next couple of days and we can see about getting them into m12

Comment 25

19 years ago
*** Bug 22729 has been marked as a duplicate of this bug. ***

Comment 26

19 years ago
*** Bug 24046 has been marked as a duplicate of this bug. ***

Comment 27

19 years ago
Adding self to cc list

Comment 28

19 years ago
Per e-mail from Greg, he has relinquished PNG component ownership; thus, 
reassigning assigned bugs to ImageLib owner.
Assignee: newt → pnunn
Status: ASSIGNED → NEW

Comment 29

19 years ago
*** Bug 25221 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

19 years ago
Status: NEW → ASSIGNED
Target Milestone: M14 → M17
(Assignee)

Comment 30

18 years ago
*** Bug 29428 has been marked as a duplicate of this bug. ***

Comment 31

18 years ago
*** Bug 31795 has been marked as a duplicate of this bug. ***

Comment 32

18 years ago
*** Bug 33324 has been marked as a duplicate of this bug. ***

Comment 33

18 years ago
Altered subject to add 'transparent' keyword.
Summary: Progressive load of PNG images leave incomplete rendering → Load progressive or transparent PNG images -> incomplete rendering

Comment 34

18 years ago
Why has "transparent" been added back to the summary? All transparency-related
bugs were reported fixed earlier in this report.
If you want to report a regression please file a New P2-major bug to reflect the
seriousness of degraded graphics support in a graphical browser.

Comment 35

18 years ago
Doh, didn't read past the first two pages...
Summary: Load progressive or transparent PNG images -> incomplete rendering → Load progressive PNG images -> incomplete rendering

Comment 36

18 years ago
*** Bug 34750 has been marked as a duplicate of this bug. ***

Comment 37

18 years ago
*** Bug 35943 has been marked as a duplicate of this bug. ***

Comment 38

18 years ago
I'll attach a patch to correctly render interlaced PNG files.  

The problem is that PNG interlacing needs to selectively mask writes to pixels 
within a scanline because in addition to horizontal strips, vertical strips are 
used during decoding. Images appear corrupted on every other line because the 
6th and final pass (horizontal) covers every other line; the lines that aren't 
covered on the 6th pass are incorrect because previous in previous vertical 
passes all the pixels in each line are written when only certain pixels should 
be.  The bug doesn't have anything to do with transparency.

My patch allocates a memory buffer for the entire image if the PNG is 
interlaced.  This is somewhat wasteful of memory, but the intermediate results 
must be stored somewhere and I certainly don't want to touch il_emit_row.  I 
don't know if it would even be possible.  If it is, there would be major work 
involved.  I suppose il_emit row could be called up to 100 times per row, but 
that would be silly.  Progressive decoding requires read access to previous 
scanlines that have not been transformed in any way.  I used the 
png_progressive_combine_row function to do the actual combining. This function 
was mentioned in the row_callback, but not called.  I think the comment was 
copied from somewhere else. 

I don't know ImgDCBHaveRow "pass" parameter should be.  What happens with that 
value? Does it understand PNG pass numbers? If it doesn't will that cause 
problems?  Perhaps 0 should be used? I continute to pass the pass value for 
now.  

I followed the pattern in the existing code and freed the buffer in 
png_il_abort, I don't know if it can/should be freed at an earlier point.  I 
just mention it because we don't want a potentially large buffer (it is height 
times larger than the existing row buffer) hanging around for no reason once the 
image is decoded.

Comment 39

18 years ago
Created attachment 7850 [details] [diff] [review]
Patch to handle interlaced PNG files

Comment 40

18 years ago
Created attachment 7853 [details] [diff] [review]
Please disregard the last patch, it had a bug.  Here's the real one!

Updated

18 years ago
Keywords: patch

Updated

18 years ago
Keywords: nsbeta2

Comment 41

18 years ago
Putting on [nsbeta2-] radar. Not critical to beta2.  Putting on nsbeta3 for a 
fix for PR2.
Keywords: nsbeta3
Whiteboard: [nsbeta2-]
Chris Hill - is seems this patch has been let slip through the net. Could you 
possibly have a look at it to see if it still applies correctly, and then we'll 
try and get it in?

Gerv

Comment 43

18 years ago
The files that the patch modifies (ipng.cpp, ipng.h) are still at the same 
revision levels (1.22, 1.6) so it should apply correctly.  It would be great to 
get this patch applied.
Chris - in the end, that's up to pnunn@netscape.com - and seeing as they are 
about to feature freeze, he's probably a bit busy. And this bug is marked 
nsbeta3, so it will happen eventually.

I just wanted to check that dev. wasn't going on and leaving this patch 
behind. If he's not about to do anything to either of the PNG files, I suppose 
it can wait. The patch won't rot. Still, it would be nice... <wistful sigh> 

Gerv
Sorry, Pam - your real name isn't associated with your Bugzilla ID. That should 
read: "...she's a bit busy...". Thanks to tor for the tip-off. ;-)

Gerv

Comment 46

18 years ago
I sent mail to Pam last week.  She was involved with another beta critical 
issue, but said she would test the patch when she could.  I had hoped that this 
patch would be applied sooner, and I still hope it will happen soon.  This is a 
really bad PNG bug.  

I'm still waiting for my first mozilla patch to be applied, even though I have 
submitted several.  I'll feel better when some of the fixes I have in my local 
copy are available for everyone.  I got so fed up with this bug (and a few 
others) I decided to download the source for the first time and fix it myself.
*** Bug 39701 has been marked as a duplicate of this bug. ***

Comment 48

18 years ago
*** Bug 39701 has been marked as a duplicate of this bug. ***
I've remerged this patch since it doesn't apply cleanly anymore.

Pam, what's you status?  Have you had a change to look at this?
(Assignee)

Comment 51

18 years ago
Chris:
Nope. I've not had time to look at it. I have some other bugs
that affect all images, not just one format...
Will get to as soon as I can.

-P
*SPAM* - adding mostfreq keyword to bugs with loads of DUPEs. Please aid this 
effort by adding this keyword to any bugs with more than 15 DUPEs.

Gerv
Keywords: mostfreq
(Assignee)

Comment 53

18 years ago
Chris H:
Thanks for the patch and the *remerged* patch. (CB)
This looks great. 

I'll start begging the PDT team to tag this an nsbeta2+,
so I can check it in.
...and sorry about the delay. Its been a zoo here.
-pn

and...

Gervase:
no problem on the "he's abit busy". In cyberspace,
no one knows you're a goddess. ;)
I don't take offense if none is intended.
-pn
(Assignee)

Updated

18 years ago
Whiteboard: [nsbeta2-] → [nsbeta2-]PLEASE reconsider for nsbeta2+.
(Assignee)

Comment 54

18 years ago
PDT: I've replaced 'nsBeta2-' with 'nsBeta2' in the
hopes this change will bring this bug back to your
attention.
-pnunn
Whiteboard: [nsbeta2-]PLEASE reconsider for nsbeta2+. → [nsbeta2]PLEASE reconsider for nsbeta2+.

Comment 55

18 years ago
Looking at my patch again, it looks like one line got mixed up with some stuff I 
was trying with bug 19283.

-        if(png_ptr->color_type || PNG_COLOR_MASK_ALPHA )
+        if(png_ptr->color_type & PNG_COLOR_MASK_ALPHA )

This line shouldn't be in the patch, it doesn't have anything to do with 
interlacing (and it is incorrect because palette based images can use 8-bit 
alpha).  There is a big discussion in 19283 about this.  I'm glad this patch is 
closer to getting checked in, I don't want to break anything.  Once this bug and 
19283 (aka 36694) are fixed (there is a patch waiting for that as well), PNG 
support in mozilla will be in much better shape.
(Assignee)

Comment 56

18 years ago
Not to worry, CH.
-P
(Assignee)

Comment 57

18 years ago
Created attachment 9078 [details] [diff] [review]
final patch for prog png checkin
(Assignee)

Comment 58

18 years ago
Created attachment 9082 [details] [diff] [review]
the final final patch.
(Assignee)

Comment 59

18 years ago
Looks like I lost some of my comments when I attached
the last patch.

I made a few modifications. Mainly I removed code needed
when we were doing 8bit mask development and some, but not all,
platforms could handle it. 

I've got necessary approvals and will check in the 'final final'
patch as soon as the tree gets past the smoketest blockers.

thanks much Chris!

I'll be clearing out some time to test out the binary transparency
stuff. Onward!
-P

Comment 60

18 years ago
Putting on [nsbeta2-] radar.    Though mozilla folks may check it in.
Whiteboard: [nsbeta2]PLEASE reconsider for nsbeta2+. → [nsbeta2-]PLEASE reconsider for nsbeta2+.
I've had to remerge this patch again.  I think that the patch had ^Ms in it.  If
I can get approval from the fearsome twosome I'll check it in.
Assignee: pnunn → blizzard
Status: ASSIGNED → NEW
(Assignee)

Comment 62

18 years ago
Thanks, CB. I'll check in and baby sit tinderbox through its
cycles.
-P
Assignee: blizzard → pnunn
(Assignee)

Comment 63

18 years ago
Patch from Chris Hill checked in.
tah-dah.
-pn
Status: NEW → RESOLVED
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 64

18 years ago
Verified, at long last!

Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.