1187569 - PNGs getting stuck in a pixelated state.

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Caspy7]

Attachment: Bad PNG.png

I'm not sure how relavent it is (but I was told to file this :) I use the Imagus addon to allow me to hover the pointer over images and see a larger/zoomed version of them.
Whenever I was browsing reddit and hovered over http://imgur.com/a/IRE3B?gallery many of its full sized images are PNGs and would be extremely pixelated (see attached screenshot) for a while before resolving, but I was able to replicate more than once it simply becoming stuck at the pixelated state.  If I used the arrow button to leave the problem picture and then go back again, the correct image was shown

If it's of interest, here is the reddit posting for the imgur image gallery in question:
https://www.reddit.com/r/interestingasfuck/comments/3efelk/americas_hidden_treasure_room/

Comment 1

[Sotaro Ikeda [:sotaro]]

seth, can you check the bug?

Comment 2

[Glenn Randers-Pehrson]

The source image corresponding to the attached "BAD png.png" (http://i.imgur.com/hN8Li0k.png) is a 3MB interlaced PNG, so it's not surprising that it would look "pixelated" for a while.  I don't know why it would become "stuck" though.

Comment 3

[Adam Frisby]

Attachment: example.png

Comment 4

[Adam Frisby]

Attachment: example.html

I can no longer reproduce the issue but I was getting similar behavior with "example.png" when the height of the image was 915px.

Comment 5

[Adam Frisby]

The height bit is probably not relevant.

1. Visit: https://en.wikipedia.org/wiki/Security-Enhanced_Linux
2. Hard refresh (Ctrl+F5)
3. Click image on the right

Comment 6

[Caspy7]

If I follow these instructions, but instead of clicking on the image, just hover the image. Then the Imagus addon kicks in and shows the partially-loaded pixelated image.  If I move the mouse away and then back again, the image displays properly.
It's possible this is somehow a problem induced by the addon, but if others are able to demonstrate it by clicking the image, it sounds like an actual issue in Firefox.

Comment 7

[Glenn Randers-Pehrson]

I'm seeing it by simply clicking the image, with nightly.  It looks to me like some interaction between decoding interlaced PNGs and the "first_frame_is_hidden" treatment.  Taking the bug.

Comment 8

[Glenn Randers-Pehrson]

Behavior also observed with FF 40.0 so it's not a new bug.

Comment 9

[Glenn Randers-Pehrson]

I added some debug printout to today's mozilla-central to verify that nsPNGDecoder is processing all seven interlace passes and reaching the end_callback function, whether the image appears clearly or is pixelated.  When it's pixelated, it clears up if I left-click on the image, but there's no indication that nsPNGDecoder re-processes the image at that time.

Comment 10

[Glenn Randers-Pehrson]

Additional clues:  I converted the SELinux image to interlaced GIF and don't see any pixelation.  I also used pngcrush to remove the opaque alpha channel and did not see any change in behavior (still stops sometimes with the pixelated display).  Interestingly, frequently the top of the image looks more pixelated than the bottom.  The SELinux image has 974 rows, which is not divisible by 8.  I added two rows to make it divisible by 8, but observed the same bad behavior.

Comment 11

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

This looks to me like an invalidation issue. It made me notice that nsPNGDecoder is trying to be too smart about sending invalidations, and duplicating decisions that we're making at the Decoder level. I've filed bug 1194900 about fixing that. I doubt that'll fix this bug, though.

I don't have time to investigate beyond that at the moment, but one way things could go wrong is if this invalidation rect is wrong in the interlaced case:

https://dxr.mozilla.org/mozilla-central/source/image/decoders/nsPNGDecoder.cpp?case=true&from=nsPNGDecoder.cpp&offset=0#768

Comment 12

[Glenn Randers-Pehrson]

Yes, we invalidate after every row of every pass of the first image.  I'm testing a scheme that
invalidates after every 8 rows on the first pass, then not at all on the even passes, and only once
at the end of each odd pass (the square ones).  It seems relatively snappy and is not showing
pixelated images when done.

Comment 13

[Glenn Randers-Pehrson]

Attachment: v00-1187569-fewer-invalidations

This suppresses the bug by reducing the number of Invalidations while processing interlaced PNGs.  Instead if invalidating each line of each pass, we only invalidate the entire image at the end of each pass.

Comment 14

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

Comment on attachment 8648454 [details] [diff] [review]
v00-1187569-fewer-invalidations

Review of attachment 8648454 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for investigating this, Glenn! The patch looks good; are you still testing, or is it ready to go?

Some minor nits below:

::: image/decoders/nsPNGDecoder.cpp
@@ +772,5 @@
>          png_longjmp(decoder->mPNG, 1);
>      }
>  
> +    if (!decoder->interlacebuf)
> +    {

Nit: per Mozilla style, the |{| should go on the same line as the |if|.

@@ +775,5 @@
> +    if (!decoder->interlacebuf)
> +    {
> +      // Do line-by-line partial invalidations for non-interlaced images
> +      decoder->PostInvalidation(IntRect(0, row_num, width, 1));
> +    } else if (row_num == (png_uint_32) decoder->mFrameRect.height - 1) {

Nit: please use a C++-style cast.

Comment 15

[Glenn Randers-Pehrson]

(In reply to Seth Fowler [:seth] [:s2h] from comment #14)
> Comment on attachment 8648454 [details] [diff] [review]
> v00-1187569-fewer-invalidations
> 
> Review of attachment 8648454 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for investigating this, Glenn! The patch looks good; are you still
> testing, or is it ready to go?
> 
> Some minor nits below:
> 
> ::: image/decoders/nsPNGDecoder.cpp
> @@ +772,5 @@
> >          png_longjmp(decoder->mPNG, 1);
> >      }
> >  
> > +    if (!decoder->interlacebuf)
> > +    {
> 
> Nit: per Mozilla style, the |{| should go on the same line as the |if|.
> 
> @@ +775,5 @@
> > +    if (!decoder->interlacebuf)
> > +    {
> > +      // Do line-by-line partial invalidations for non-interlaced images
> > +      decoder->PostInvalidation(IntRect(0, row_num, width, 1));
> > +    } else if (row_num == (png_uint_32) decoder->mFrameRect.height - 1) {
> 
> Nit: please use a C++-style cast.

It's good to go except for the review nits, and it'll need to be rebased when 1194900 lands.

Comment 16

[Glenn Randers-Pehrson]

Attachment: v01-1187569-fewer-invalidations

Fixed seth's review nits.  Need tryserver run.

Comment 17

[Glenn Randers-Pehrson]

Comment on attachment 8649512 [details] [diff] [review]
v01-1187569-fewer-invalidations

Uploaded the wrong file...

Comment 18

[Glenn Randers-Pehrson]

Attachment: v02-1187569-fewer-invalidations

Need try server run.

Comment 19

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

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

Comment 20

[Glenn Randers-Pehrson]

Comment on attachment 8649520 [details] [diff] [review]
v02-1187569-fewer-invalidations

The try run looks OK.  Just 3 orange that don't appear to have anything to do with PNG.  R?

Comment 21

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

Comment on attachment 8649520 [details] [diff] [review]
v02-1187569-fewer-invalidations

Review of attachment 8649520 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I think we're ready to land. If inbound's open I'll go ahead and do that now.

Comment 22

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

inbound's not open, alas. =\

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/935545bab632

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/935545bab632

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/935545bab632

Comment 26

[Glenn Randers-Pehrson]

Recently I've been getting these notices occasionally.  I haven't isolated the cause yet, but I suspect it's related to this bug, and the "reordered frames" are passes over an interlaced PNG.

I/SampleTable( 3590): There are reordered frames present.
I/SampleTable( 3590): There are reordered frames present.