433047 - Rendering artifacts in APNG frame

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Joe Drew (not getting mail)]

This is sort of a duplicate of bug 411852, which got morphed. The URL above displays a column or two of what looks like left-over pixels. This is documented in attachment 296750 [details].

Comment 1

[Glenn Randers-Pehrson]

Does applying the libpng-1.2.29-v1 patch from bug #418900 have any effect on this?

Comment 2

[Glenn Randers-Pehrson]

Andrew reports in bug #418900 that applying the libpng-1.2.29v1 patch does fix this particular APNG but that some others no longer work.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Attachment: Linked image

I'm adding the image from the link here for perpetuity.

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Removing the dependency on bug 418900 since libpng-1.2.31 doesn't fix this bug per comments 31-33.

Comment 5

[Glenn Randers-Pehrson]

Attachment: Another APNG file that demonstrates rendering artifacts

Comment 6

[Glenn Randers-Pehrson]

Attachment: Clear "prev_row" before each image as well as before interlace passes

Comment 7

[Glenn Randers-Pehrson]

Comment on attachment 377892 [details] [diff] [review]
Clear "prev_row" before each image as well as before interlace passes

Never mind, that patch won't work because of the return just before the "if (png_ptr->interlaced)" block.

Comment 8

[Glenn Randers-Pehrson]

After applying patches for bug #441971, bug #397593, and bug #463465, the artifacts in the rocket image are still present.  They are evidently caused by some other bug in the APNG decoder.

Comment 10

[Max Stepin]

All frames in this APNG example have blend=over and dispose=background.

Compositing frame isn't fully cleared before the new frame is drawn on it.

modules\libpr0n\src\imgContainer.cpp (line 1575)

If I replace this line
  PRBool needToBlankComposite = PR_FALSE;
by this line:
  PRBool needToBlankComposite = PR_TRUE;

the bug goes away.

Comment 11

[Glenn Randers-Pehrson]

Max's suggestion might unnecessarily defeat some optimizations.  I am testing a patch that hopefully will accomplish the same thing while preserving most of the optimizations.

Comment 12

[Glenn Randers-Pehrson]

> modules\libpr0n\src\imgContainer.cpp (line 1575)
>
> replace this line
>   PRBool needToBlankComposite = PR_FALSE;
> by this line:
>   PRBool needToBlankComposite = PR_TRUE;

I am still seeing the rendering errors with this change.

Comment 13

[Glenn Randers-Pehrson]

Correction, I don't see the rendering errors any more in the original rocket image ("Linked image" attachment).  I'm not sure what the second image ("Another APNG file...") is really supposed to look like, but the background looks wrong.

Comment 14

[Glenn Randers-Pehrson]

Attachment: v01: change needToBlankComposite conditions in imgContainer.cpp

Comment 15

[Glenn Randers-Pehrson]

Tryserver builds for the v01 patch are at
http://build.mozilla.org/tryserver-builds/[e-mail]-v01b-1.9.3-433047-Feb15

With and without this patch, frame 1 is not displayed.

Comment 16

[Max Stepin]

That "frame 1 is not displayed" is a bug 546272.

Let's keep it simple. This one is just about dirty background.

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 426981 [details] [diff] [review]
v01: change needToBlankComposite conditions in imgContainer.cpp

As I understand it, the problem here is that we had a full frame animation that had alpha, and by not clearing we were compositing with the previous frame.

It should be really easy to whip up a testcase for this, so let's do that!

Comment 18

[Max Stepin]

Attachment: problem4.png

Hold on. There is a problem here. Attached animation displays wrong after the patch. Last frame is a blue square on black background. But after the patch, a thin transparent outline appears around that blue square. Switch tabs for more weirdness. 

The same problem with GIF. 

I'll try to investigate this further. On the first glance, something is clearing *too much*, it seems.

Comment 19

[Max Stepin]

OK, figured it out... The fix I proposed in comment 10 is wrong.

The logic behind needToBlankComposite is quite simple - 
it's set to TRUE when the composite is just created, and 
it's set to TRUE when the new loop is started:

modules\libpr0n\src\imgContainer.cpp (line 1575)

  } else if (aNextFrameIndex == 1) {
    // When we are looping the compositing frame needs to be cleared.
    needToBlankComposite = PR_TRUE;
  }

So it's seems correct, except this code I quoted is never reached, because the very first frame is handled earlier, in the first optimization.

So instead of hardcoding frame "1" here: 
if (aNextFrameIndex == 1)
let's do this: 
if (aNextFrameIndex < mAnim->lastCompositedFrameIndex)

That way it will always detect the new loop. 
Seems to work for me, both for rocket, and for the latest attachment.

Comment 20

[Max Stepin]

No, clearing at the new loop is not enough. 

The use of composited frame assumes that frames are handled one after another, 
without breaking the 1->2->3->... sequence. 

But optimizations actually break that sequence. 

That's why the first rocket still has that vertical line artifact. 
A certain middle frame is a full frame, which is handled by optimization.
Right after that a vertical line appears. 

So let's try this:
 
if (aNextFrameIndex != mAnim->lastCompositedFrameIndex+1)

That way if the frame sequence is broken, needToBlankComposite will be TRUE.

Comment 21

[Max Stepin]

Attachment: testcase8.png

OK, this simple testcase detect both possible problems. 

One problem is not clearing background at the start of new loop, 
another is not clearing background after full frame in the middle of the loop.

Every frame in this testcase has only one color. 
If you see more than one color at the same time, that's a bug.

Comment 22

[Glenn Randers-Pehrson]

Attachment: v02: change needToBlankComposite conditions in imgContainer.cpp

This patch incorporates comment #20.  TryServer builds are at
http://build.mozilla.org/tryserver-builds/[e-mail]-v02a-433047-28March

Comment 23

[Max Stepin]

Thanks. TryServer win32 build works fine.

Comment 24

[Max Stepin]

Joe, review?

Comment 25

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

bump, can we have this in Fx 3.6.4... ? ;)

Comment 26

[Max Stepin]

Joe, review?

Comment 27

[Max Stepin]

approval/checkin?

Comment 28

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

So can patch land finally... ? ;)

Comment 29

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/16888d1dd405

Comment 30

[Alfred Kayser]

You may want to change the comment reflecting the new logic.