Closed Bug 3195 Opened 26 years ago Closed 24 years ago

Load progressive PNG images -> incomplete rendering

Categories

(Core Graveyard :: GFX, defect, P2)

Product:
Core Graveyard
Component:
GFX
Platform:
Other
Other
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED

People

(Reporter: dbaron, Assigned: pnunn)

References

(
URL
)

Details

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

Attachments

(5 files)

Patch to handle interlaced PNG files
3.18 KB, patch
Details | Diff | Splinter Review
Please disregard the last patch, it had a bug. Here's the real one!
3.19 KB, patch
Details | Diff | Splinter Review
remerged patch
3.09 KB, patch
Details | Diff | Splinter Review
final patch for prog png checkin
2.87 KB, patch
Details | Diff | Splinter Review
the final final patch.
2.59 KB, patch
Details | Diff | Splinter Review 
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.
Assignee: michaelp → pnunn
Status: NEW → ASSIGNED
QA Contact: 4110 → 2792
[QA Assigning to self.]
Target Milestone: M5
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
Target Milestone: M5 → M6
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.
QA Contact: 2792 → 1698
Target Milestone: M6 → M9
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
Assignee: pnunn → newt
Status: ASSIGNED → NEW
Target Milestone: M9 → M11
m11
Status: NEW → ASSIGNED
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.]
Target Milestone: M11 → M12
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. ***
Summary: problems with progressive display of interlaced PNG → Progressive load of PNG images leave incomplete rendering
[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
Target Milestone: M12 → M14
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.
Assignee: newt → pnunn
Status: ASSIGNED → NEW
*** Bug 25221 has been marked as a duplicate of this bug. ***
Status: NEW → ASSIGNED
Target Milestone: M14 → M17
*** 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.
Summary: Progressive load of PNG images leave incomplete rendering → Load progressive or transparent PNG images -> incomplete rendering
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...
Summary: Load progressive or transparent PNG images -> incomplete rendering → Load progressive PNG images -> incomplete rendering
*** 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.
Keywords: patch
Keywords: nsbeta2
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
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
Keywords: mostfreq
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
Whiteboard: [nsbeta2-] → [nsbeta2-]PLEASE reconsider for nsbeta2+.
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+.
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.
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

    
    

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

    
    

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

    
    

    
  
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.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: