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.

[QA Assigning to self.]

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.

I think this has more to do with the progressive display passes not rescaling. Not the dimensions of each pass. -pn

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.

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.

adding self to cc list

m11

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

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

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

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

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

[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.]

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

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?

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

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

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

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

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

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

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!

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

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

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

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

Adding self to cc list

Per e-mail from Greg, he has relinquished PNG component ownership; thus, reassigning assigned bugs to ImageLib owner.

*** Bug 25221 has been marked as a duplicate of this bug. ***

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

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

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

Altered subject to add 'transparent' keyword.

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.

Doh, didn't read past the first two pages...

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

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

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.

Putting on [nsbeta2-] radar. Not critical to beta2. Putting on nsbeta3 for a fix for PR2.

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

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

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

*** 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?

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

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

PDT: I've replaced 'nsBeta2-' with 'nsBeta2' in the hopes this change will bring this bug back to your attention. -pnunn

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.

Not to worry, CH. -P

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

Putting on [nsbeta2-] radar. Though mozilla folks may check it in.

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.

Thanks, CB. I'll check in and baby sit tinderbox through its cycles. -P

Patch from Chris Hill checked in. tah-dah. -pn

Verified, at long last!