195022 - .gif has random horizontal black lines

Status: RESOLVED FIXED

(Core Graveyard :: Image: Painting, defect)

Description

[Jonathan S Abrams]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3b) Gecko/20030223
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.3b) Gecko/20030223

The .gif on this page has random horizontal black lines in Mozilla & Netscape
6/7 for both Mac OS 9 and Mac OS X.  Internet Explorer for Mac OS 9 and Mac OS X
do not display random horizontal black lines in this .gif.  The .gif loads
properly if you control-click on it and choose "View Image".

Reproducible: Always

Steps to Reproduce:
1.  Load web page.
2.
3.

Actual Results:  
Random black horizontal lines are in the .gif.

Expected Results:  
No random black horizontal lines in the .gif.

Might be occuring on Windows too.  Will report back on Wednesday February 26th
to confirm or deny this.

Comment 1

[Jonathan S Abrams]

Attachment: Screenshot of the problem

Comment 2

[Jonathan S Abrams]

Attachment: What it looks like (correct) in IE

Comment 3

[Jonathan S Abrams]

Two days late, but I've discovered that this problem does not exist in
Gecko/20021120 Netscape/7.01 for Windows NT 4.

Comment 4

[Simon Fraser [no longer active]]

I think this is an issue with scaled images with transparency.

Please don't use TIFF screenshots, since the browser can't (easily) display them
directly. PNGs are better.

Comment 5

[Jonathan S Abrams]

Simon,

That web page no longer exists.  I think we're going to have to find a different
page or create a reproducible testcase that isn't website dependent.

Comment 6

[Hiro]

Attachment: The screen shot saved by PNG

This problem is still reproduced.
Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; ja-JP; rv:1.7b) Gecko/20040314
Firefox/0.8.0+

Comment 7

[Hiro]

Attachment: The screen shot of Safari

Comment 8

[Simon Fraser [no longer active]]

Here's what I think is similar:

http://mozilla.org/quality/browser/front-end/testcases/imaging/img-gif-89/img-gif-89.html

see the astro images at the bottom, and the nasty transparency artefacts.

Comment 9

[Hiro]

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

Comment 10

[tor]

What appears to be happening is that CopyDeepMask isn't doing filtered
downsampling for scaling correctly.  It looks to be ignoring the mask
when doing the sample blend, so it blends in the color of the transparent
pixels, which we've set to black for the benefit of win32.

Comment 11

[jhp (no longer active)]

Attachment: partial fix

Well, this patch makes the link at comment #8 look groovy, but the nutshell URL
at the top still shows artifacts.  Don't know why.

Comment 12

[jhp (no longer active)]

Attachment: workaround patch

Actually, the patch I attached previously was wrong, since it made the color
white be transparent also.

This patch uses the RectStretch function in imgScaler.cpp (used by Linux) in
order to scale the source and mask images to the destination's size, then uses
the OS functions to blit everything together to the screen.  The only downside
to this patch is that the RectStretch function doesn't produce images as nice
as the OS functions; just look at the "transparent space image" photos and
compare to what is rendered by Safari.

Comment 13

[Simon Fraser [no longer active]]

RectStretch is probably much slower than letting the OS do it. I'd rather not go
this route.

Comment 14

[TT]

FYI, this bug is still present in 1.7 RC1.  I consider this bug a must-fix for
1.7, because it affects so many images.

Comment 15

[jhp (no longer active)]

Attachment: workaround patch

This patch includes the necessary changes to imgScaler.cpp.

Comment 16

[jhp (no longer active)]

What do we want to do with this bug?  We haven't been able to find an
appropriate fix for this bug, other than the workaround.  And the workaround
patch confines the use of RectStretch to those where the image has a mask and is
scaled.

Comment 17

[Simon Fraser [no longer active]]

I presume this is different from bug 239701?

Comment 18

[jhp (no longer active)]

Yes, unfortunately.

Comment 19

[jhp (no longer active)]

Attachment: workaround patch v1.1

Was using the wrong width and height variables.  Also, reworked the if
statement in ::Draw().

I ran with this patch through quite a few sites, testing many of the pages in
the browser buster, as well as some image test pages, and everything displayed
as expected.

Comment 20

[Simon Fraser [no longer active]]

What do the pageload numbers look like with this patch?

Comment 21

[jhp (no longer active)]

The pageload numbers were statistically identical, since not many pages use
scaled images with masks.  However, I did create a small test page using 10
scaled images with masks, and that one was about 20% slower with this patch. 
However, that is an edge case, and most pages will not show a performance hit.

Comment 22

[Simon Fraser [no longer active]]

Comment on attachment 149451 [details] [diff] [review]
workaround patch v1.1

OK, I'll go along with that. Ideally we'd be able to plugin in an
Altivec-enhanced version of our own scaling routines, but later.

Comment 23

[jhp (no longer active)]

A while back, I'd tried to convert this to use Quartz, but ran into many issues.  That may be another way 
to go.  Do you have any experience with Quartz?

Also, do you think we should try to get this in for 1.7, or is it too late?

Comment 24

[TT]

FYI, this problem goes away for me if I switch to millions of colors.

Comment 25

[jhp (no longer active)]

Timur, that's very strange.  I've recreated this bug on two separate systems,
both with millions of colors.

Comment 26

[jhp (no longer active)]

Attachment: workaround patch v1.2

Managed to completely screw printing with my last patch.  Turns out, we don't
need to worry about this bug for printing, since it worked fine to begin with.

Comment 27

[Simon Fraser [no longer active]]

Comment on attachment 149762 [details] [diff] [review]
workaround patch v1.2

> static void
>+Stretch32(unsigned x1, unsigned x2, unsigned y1, unsigned y2,
>+	  unsigned yr, unsigned yw,
>+	  unsigned aStartRow, unsigned aStartColumn, unsigned aEndColumn,
>+	  unsigned char *aSrcImage, unsigned aSrcStride,
>+	  unsigned char *aDstImage, unsigned aDstStride);
>+

Why don't these functions use PR types like the rest of mozilla?
At least they should specify "unsigned int".

> static void
>+Stretch32(unsigned x1, unsigned x2, unsigned y1, unsigned y2,
>+	  unsigned yr, unsigned yw,
>+	  unsigned aStartRow, unsigned aStartColumn, unsigned aEndColumn,
>+	  unsigned char *aSrcImage, unsigned aSrcStride,
>+	  unsigned char *aDstImage, unsigned aDstStride)
>+{
>+    int e;
>+    unsigned dx, dy, d;
>+    unsigned char *src, *dst;
>+
>+    dx = x2 - x1;
>+    dy = y2 - y1;
>+    e = dy - dx;
>+    dy += 1;
>+    src = aSrcImage + yr * aSrcStride + 4 * y1;
>+    dst = aDstImage + (yw - aStartRow) * aDstStride;
>+    if (!dx)
>+	dx = 1;
>+    for (d = 0; d <= aEndColumn; d++) {
>+	if (d >= aStartColumn) {
>+	    dst++;
>+	    *dst++ = src[1];
>+	    *dst++ = src[2];
>+	    *dst++ = src[3];
>+	}
>+	while (e >= 0) {
>+	    src += 4;
>+	    e -= dx;
>+	}
>+	e += dy;
>+    }
>+}

Does this code make any assuptions about the platform pixmap format
(ARGB vs. RGBA for example) that might be different for different platforms?
Since the data you're messing with here is Mac GWorld data, it would seem
that this is indeed the case.

>Index: gfx/src/mac/nsDeviceContextMac.h
>===================================================================
>RCS file: /cvsroot/mozilla/gfx/src/mac/nsDeviceContextMac.h,v
>retrieving revision 1.54
>diff -u -6 -p -r1.54 nsDeviceContextMac.h
>--- gfx/src/mac/nsDeviceContextMac.h	17 Apr 2004 21:52:30 -0000	1.54
>+++ gfx/src/mac/nsDeviceContextMac.h	1 Jun 2004 16:51:49 -0000
>@@ -55,12 +55,13 @@ class nsDeviceContextMac : public Device
> {
> public:
>   nsDeviceContextMac();
> 
>   NS_IMETHOD  Init(nsNativeWidget aNativeWidget);  
> 
>+  using DeviceContextImpl::CreateRenderingContext;
>   NS_IMETHOD  CreateRenderingContext(nsIRenderingContext *&aContext);
>   NS_IMETHOD  SupportsNativeWidgets(PRBool &aSupportsWidgets);
> 
>   NS_IMETHOD  GetScrollBarDimensions(float &aWidth, float &aHeight) const;
>   NS_IMETHOD  GetSystemFont(nsSystemFontID anID, nsFont *aFont) const;

er, shouldn't that be a comment?

>Index: gfx/src/mac/nsImageMac.cpp
>===================================================================

>+
>+  if (!mMaskGWorld) {
>+    ::CopyBits(::GetPortBitMapForCopyBits(mImageGWorld),
>+               ::GetPortBitMapForCopyBits(destPort),
>+               &srcRect, &dstRect, srcCopy, nsnull);
>+  } else {

Please follow the brace style from the surrounding code here ({ on new line).


>+      if ((aSWidth != aDWidth) || (aSHeight != aDHeight))
>+      {
>+        // If scaling an image that has a mask...
>+
>+        // Bug 195022 - Seems there is a bug in the Copy{Deep}Mask functions
>+        // where scaling an image that has a mask can cause some ugly artifacts
>+        // to appear on screen.  To work around this issue, we use the functions
>+        // in imgScaler.cpp to do the actual scaling of the source image and mask.
>+
>+        GWorldPtr tempSrcGWorld = nsnull, tempMaskGWorld = nsnull;
>+
>+        // scale the source
>+        char* scaledSrcBits;
>+        PRInt32 tmpRowBytes;
>+        PRInt16 pixelDepthSrc = ::GetPixDepth(::GetGWorldPixMap(mImageGWorld));
>+        OSErr err = CreateGWorld(aDWidth, aDHeight, pixelDepthSrc,
>+                                 &tempSrcGWorld, &scaledSrcBits, &tmpRowBytes);
>+        if (err != noErr)  return NS_ERROR_FAILURE;
>+
>+        RectStretch(aSWidth, aSHeight, aDWidth, aDHeight,
>+                    0, 0, aDWidth - 1, aDHeight - 1,
>+                    mImageBits, mRowBytes, scaledSrcBits, tmpRowBytes,
>+                    pixelDepthSrc);
>+        
>+        // scale the mask
>+        char* scaledMaskBits;
>+        err = CreateGWorld(aDWidth, aDHeight, mAlphaDepth, &tempMaskGWorld,
>+                           &scaledMaskBits, &tmpRowBytes);
>+        if (err != noErr)  return NS_ERROR_FAILURE;
>+
>+        RectStretch(aSWidth, aSHeight, aDWidth, aDHeight,
>+                    0, 0, aDWidth - 1, aDHeight - 1,
>+                    mMaskBits, mAlphaRowBytes, scaledMaskBits, tmpRowBytes,
>+                    mAlphaDepth);
>+
>+        Rect tmpRect;
>+        ::SetRect(&tmpRect, 0, 0, aDWidth, aDHeight);
>+
>+        CopyBitsWithMask(::GetPortBitMapForCopyBits(tempSrcGWorld),
>+                         ::GetPortBitMapForCopyBits(tempMaskGWorld),
>+                         mAlphaDepth, ::GetPortBitMapForCopyBits(destPort),
>+                         tmpRect, tmpRect, dstRect, PR_TRUE);

You are leaking both GWorlds here. This is a big leak.

>+      } else {
>+        // not scaling...
>+        CopyBitsWithMask(::GetPortBitMapForCopyBits(mImageGWorld),
>+                         ::GetPortBitMapForCopyBits(mMaskGWorld),
>+                         mAlphaDepth, ::GetPortBitMapForCopyBits(destPort),
>+                         srcRect, maskRect, dstRect, PR_TRUE);
>+      }
>     }
>   }
>-  else  // not printing
>-  {
>-    CopyBitsWithMask(::GetPortBitMapForCopyBits(mImageGWorld),
>-        mMaskGWorld ? ::GetPortBitMapForCopyBits(mMaskGWorld) : nsnull, mAlphaDepth,
>-        ::GetPortBitMapForCopyBits(destPort),
>-        srcRect, maskRect, dstRect, PR_TRUE);
>-  }
>   
>-  return NS_OK;
>+  return rv;

Didn't we bail earlier if rv was not NS_OK?

Comment 28

[jhp (no longer active)]

(In reply to comment #27)
> Why don't these functions use PR types like the rest of mozilla?
> At least they should specify "unsigned int".

That's the 'style' for that file; the other StretchXX functions have the same
definition.

> > static void
> >+Stretch32(unsigned x1, unsigned x2, unsigned y1, unsigned y2,
> 
> Does this code make any assuptions about the platform pixmap format
> (ARGB vs. RGBA for example) that might be different for different platforms?
> Since the data you're messing with here is Mac GWorld data, it would seem
> that this is indeed the case.

Stretch32 is only used on Mac OS X.  I'll add a comment as such.

> >+  using DeviceContextImpl::CreateRenderingContext;
> 
> er, shouldn't that be a comment?

No, this gets rid of a very annoying warning.  The gist is that the Mac header
only overrides one of the CreateRenderingContext functions, so the compiler
spits out a warning that the other such functions defined in the class
DeviceContextImpl are hidden by our overridden function.  This lets the compiler
know that we are using the parent's functions unless explicitly overridden.

I'll attach a new patch to fix the other issues.

Comment 29

[Simon Fraser [no longer active]]

> No, this gets rid of a very annoying warning.

I've never seen that usage before; sure it will be OK with all the compilers
that might hit this file (which is I guess various gcc versions only at this point)?

Comment 30

[jhp (no longer active)]

Attachment: workaround patch v1.3

How about this?  I also added use of StPixelLocker before calling RectStretch
and CopyBitsWithMask.  Not quite sure if it's necessary, but it is there in the
print case.

Comment 31

[Simon Fraser [no longer active]]

Comment on attachment 149765 [details] [diff] [review]
workaround patch v1.3

Some minor points:
Please add a comment in Stretch32 after the row:
  if (d >= aStartColumn) {
saying that this will only work for ARGB pixmaps. The "only used by Mac OS X"
is too easily missed.

Please comment the
  +  using DeviceContextImpl::CreateRenderingContext;
line.

The pixel lockers are a good idea (required on OS 9, probably not on OS X but
they don't hurt).

r=sfraser with those changes.

Comment 32

[tor]

Comment on attachment 149765 [details] [diff] [review]
workaround patch v1.3

I'd say just change the "dst++;" line to "*dst++ = src[0];" to generalize
the function and avoid possibly tripping someone up in the future.

In your experiments I thought the problem with CopyDeepMask was only
when doing 1-bit alpha.  This version of the patch is going down the
scaler code for both 1-bit and 8-bit alpha.

The code reformating should have really been in a seperate bug/patch.

sr=tor with the first two items addressed.

Comment 33

[jhp (no longer active)]

Attachment: patch that was checked in

This was the patch that I checked in to the trunk.  Tim, you were right: this
issue only affects images with 1-bit masks.  So it further reduces the case
where we need to go down the workaround path.

Comment 34

[jhp (no longer active)]

Comment on attachment 149840 [details] [diff] [review]
patch that was checked in

Would be nice to have this for 1.7, since it fixes some image corruption.  I
believe the fix is low risk, since the workaround fix is only for scaled images
with a 1-bit mask.

Comment 35

[Christian :Biesinger (don't email me, ping me on IRC)]

hm... it looks like this patch broke the camino tinderboxes

Comment 36

[jhp (no longer active)]

OK, thanks.  I just checked in an update to gfx/src/Makefile.in.  Hopefully
that's all that was missing.

Comment 37

[Asa Dotzler [:asa]]

Comment on attachment 149840 [details] [diff] [review]
patch that was checked in

a=asa (on behalf of drivers) for checkin to Mozilla 1.7. We're wrapping things
up and so this will need to land today if it's going to make the release.

Comment 38

[tor]

Checked in on MOZILLA_1_7_BRANCH for pedemonte.

Comment 39

[Stuart Morgan]

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

Comment 40

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This looks like it caused a ~1% pageload time regression on btek.

Comment 41

[tor]

dbaron: doubtful given that this was a mac-only change and btek is linux.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The imgScaler.cpp change is not mac-only.

Comment 43

[tor]

Unless the compiler is inlining through a function pointer, the single
additional case in a setup switch() isn't going to statistically change
runtime.

Comment 44

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The addition of extra code could move commonly used sections of code farther
apart (e.g., into different pages), slowing things down, or something like that.

Comment 45

[Mike Pinkerton (not reading bugmail)]

this code created a topcrasher for camino. what's up?

Comment 46

[jhp (no longer active)]

Pinkerton, could you provide more details?

Comment 47

[Mike Pinkerton (not reading bugmail)]

see bug 247542 and bug 247548

Comment 48

[TT]

I can verify that this bug has been fixed for me with Mozilla 1.7.  Thanks!