550537 - [DW] "Get help" image missing from https://bugzilla.mozilla.org/

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[u88484]

The "Get help" image is missing from https://bugzilla.mozilla.org/ but it shows up in the task bar previews on windows 7.  Disable direct write and the button shows up after restart.

Comment 1

[u88484]

The image is:
https://bugzilla.mozilla.org/skins/custom/images/help.gif

which shows up just fine when loaded by itself.

Comment 2

[u88484]

Hmm, load the image and switch tabs caused the favicon of the image to disappear until you hover over another tab.  One of the times I did that caused the tab close button to turn into a white box.

Comment 3

[Bas Schouten (:bas.schouten)]

Can't reproduce any of this, could be one of those refresh issues some people are having? If Direct2D is also enabled that is.

Comment 4

[Leman Bennett [Omega]]

I see this issue too. I think its related to GIF images that also have anchors. Various website logos in GIF format that often have the front page linked, the logo often won't display or jumps out of place. It seems to be random.

Comment 5

[[not reading bugmail]]

its not just the help button, its all 4 images when you hover over the mouse sometimes that makes them show and disappear.. just wave your mouse over them a few times using:
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100305 Minefield/3.7a3pre (.NET CLR 3.5.30729) ID:20100305151213

I thought it was just me!  weird..

Comment 6

[Bas Schouten (:bas.schouten)]

I suspect this is a regression from the CLEAR/SOURCE patch.. Did any of you see this in 20100304 or earlier?

Comment 7

[[not reading bugmail]]

I think your probably correct the regression range is:

works:
http://hg.mozilla.org/mozilla-central/rev/0f19401e22d1

Broken:
http://hg.mozilla.org/mozilla-central/rev/204de1667911

BTW: I used these two links to narrow it down:
http://hourly-archive.localgho.st/hourly-archive2/mozilla-central-win32/
and 
http://hg.mozilla.org/mozilla-central/pushloghtml

Comment 8

[[not reading bugmail]]

(In reply to comment #6)
> I suspect this is a regression from the CLEAR/SOURCE patch.. 

Bug 549652 landed on Thu Mar 04 11:31:20 2010 -0800 for cset:

http://hg.mozilla.org/mozilla-central/rev/204de1667911

> Did any of you see this in 20100304 or earlier?

It wasn't in 20100303 as the regression range occurred after as noted above.

Comment 9

[Bas Schouten (:bas.schouten)]

Attachment: Don't use source as a faster way when using D2D

So, it seems we made a little mistake when we thought nothing would be using source anymore. Our optimization in cairo is not perfect, occasionally imgFrame gets images with an Alpha channel, it then creates an ARGB32 surface.

Now if later it gets told that that surface actually has no alpha, it 'optimizes' the operator to use SOURCE. However when this comes into cairo we see a surface -with- alpha and don't know we can optimize it back to OVER. imgFrame should optimize to OVER when using D2D.

Comment 10

[[not reading bugmail]]

Attachment: Weird affects coloring on plugins

Would that be related to this issue I started seeing also?

Comment 12

[Jeff Muizelaar [:jrmuizel]]

A couple of comments. First, this patch doesn't seem to fix the actual bug in the D2D backend. Second, in a lot of these cases it probably makes more sense to just remove the OVER->SOURCE optimization because cairo already detects many of these cases.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 430787 [details] [diff] [review]
Don't use source as a faster way when using D2D

>diff --git a/modules/libpr0n/src/imgFrame.cpp b/modules/libpr0n/src/imgFrame.cpp
>--- a/modules/libpr0n/src/imgFrame.cpp
>+++ b/modules/libpr0n/src/imgFrame.cpp
>@@ -395,17 +395,17 @@ void imgFrame::Draw(gfxContext *aContext
> 
>   if (mSinglePixel && !doPadding && ImageComplete()) {
>     // Single-color fast path
>     // if a == 0, it's a noop
>     if (mSinglePixelColor.a == 0.0)
>       return;
> 
>     if (op == gfxContext::OPERATOR_OVER && mSinglePixelColor.a == 1.0)
>-      aContext->SetOperator(gfxContext::OPERATOR_SOURCE);
>+      aContext->SetOperator(OptimalFillOperator());
> 

This case is easily detectable by the backend so we should probably just remove the optimization.

>     aContext->SetDeviceColor(mSinglePixelColor);
>     aContext->NewPath();
>     aContext->Rectangle(aFill);
>     aContext->Fill();
>     aContext->SetOperator(op);
>     aContext->SetDeviceColor(gfxRGBA(0,0,0,0));
>     return;
>@@ -449,17 +449,17 @@ void imgFrame::Draw(gfxContext *aContext
>       // transparent pixels in the padding or undecoded area
>       format = gfxASurface::ImageFormatARGB32;
>       surface = gfxPlatform::GetPlatform()->CreateOffscreenSurface(size, format);
>       if (!surface || surface->CairoStatus() != 0)
>         return;
> 
>       // Fill 'available' with whatever we've got
>       gfxContext tmpCtx(surface);
>-      tmpCtx.SetOperator(gfxContext::OPERATOR_SOURCE);
>+      tmpCtx.SetOperator(OptimalFillOperator());

This is just a memcpy.

>       if (mSinglePixel) {
>         tmpCtx.SetDeviceColor(mSinglePixelColor);
>       } else {
>         tmpCtx.SetSource(ThebesSurface(), gfxPoint(aPadding.left, aPadding.top));
>       }
>       tmpCtx.Rectangle(available);
>       tmpCtx.Fill();
>     }
>@@ -558,17 +558,17 @@ void imgFrame::Draw(gfxContext *aContext
>       // matter what we do here, but we should avoid trying to
>       // create a zero-size surface.
>       if (!needed.IsEmpty()) {
>         gfxIntSize size(PRInt32(needed.Width()), PRInt32(needed.Height()));
>         nsRefPtr<gfxASurface> temp =
>           gfxPlatform::GetPlatform()->CreateOffscreenSurface(size, format);
>         if (temp && temp->CairoStatus() == 0) {
>           gfxContext tmpCtx(temp);
>-          tmpCtx.SetOperator(gfxContext::OPERATOR_SOURCE);


This one is a pixel aligned tiled memcpy. We can probably deal with that in the d2d backend.

>+          tmpCtx.SetOperator(OptimalFillOperator());
>           nsRefPtr<gfxPattern> tmpPattern = new gfxPattern(surface);
>           if (tmpPattern) {
>             tmpPattern->SetExtend(gfxPattern::EXTEND_REPEAT);
>             tmpPattern->SetMatrix(gfxMatrix().Translate(needed.pos));
>             tmpCtx.SetPattern(tmpPattern);
>             tmpCtx.Paint();
>             tmpPattern = new gfxPattern(temp);
>             if (tmpPattern) {
>@@ -626,17 +626,17 @@ void imgFrame::Draw(gfxContext *aContext
>         pattern->SetExtend(gfxPattern::EXTEND_PAD);
>         pattern->SetFilter(aFilter);
>         break;
>     }
>   }
> 
>   if ((op == gfxContext::OPERATOR_OVER || pushedGroup) &&
>       format == gfxASurface::ImageFormatRGB24) {
>-    aContext->SetOperator(gfxContext::OPERATOR_SOURCE);
>+    aContext->SetOperator(OptimalFillOperator());
>   }

This one should be able to go away too because of the surface format.

Comment 14

[Bas Schouten (:bas.schouten)]

(In reply to comment #13)
> (From update of attachment 430787 [details] [diff] [review])
> >@@ -558,17 +558,17 @@ void imgFrame::Draw(gfxContext *aContext
> >       // matter what we do here, but we should avoid trying to
> >       // create a zero-size surface.
> >       if (!needed.IsEmpty()) {
> >         gfxIntSize size(PRInt32(needed.Width()), PRInt32(needed.Height()));
> >         nsRefPtr<gfxASurface> temp =
> >           gfxPlatform::GetPlatform()->CreateOffscreenSurface(size, format);
> >         if (temp && temp->CairoStatus() == 0) {
> >           gfxContext tmpCtx(temp);
> >-          tmpCtx.SetOperator(gfxContext::OPERATOR_SOURCE);
> 
> 
> This one is a pixel aligned tiled memcpy. We can probably deal with that in the
> d2d backend.

Well, we could in theory, but I think it would require quite a bit of logic to detect it.. (is the path a box? If it is a box are the edges pixel aligned? If it is, is the source a surface pattern? If it is, is it untransformed? If it is go through a completely different code path.)

> >@@ -626,17 +626,17 @@ void imgFrame::Draw(gfxContext *aContext
> >         pattern->SetExtend(gfxPattern::EXTEND_PAD);
> >         pattern->SetFilter(aFilter);
> >         break;
> >     }
> >   }
> > 
> >   if ((op == gfxContext::OPERATOR_OVER || pushedGroup) &&
> >       format == gfxASurface::ImageFormatRGB24) {
> >-    aContext->SetOperator(gfxContext::OPERATOR_SOURCE);
> >+    aContext->SetOperator(OptimalFillOperator());
> >   }
> 
> This one should be able to go away too because of the surface format.

No - mind you, the source -surface- is ARGB32. It's just that the imgFrame knows its -actual- alpha channel is completely opaque (see SetHasNoAlpha()). To a backend it would not immediately be obvious this can go away. i.e. D2D doesn't know it can do OVER instead of SOURCE. GDI can't figure out it can do SOURCE instead of OVER, without them analyzing the entire surface's alpha channel.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #14)
> (In reply to comment #13)
> > (From update of attachment 430787 [details] [diff] [review] [details])
> > >@@ -558,17 +558,17 @@ void imgFrame::Draw(gfxContext *aContext
> > >       // matter what we do here, but we should avoid trying to
> > >       // create a zero-size surface.
> > >       if (!needed.IsEmpty()) {
> > >         gfxIntSize size(PRInt32(needed.Width()), PRInt32(needed.Height()));
> > >         nsRefPtr<gfxASurface> temp =
> > >           gfxPlatform::GetPlatform()->CreateOffscreenSurface(size, format);
> > >         if (temp && temp->CairoStatus() == 0) {
> > >           gfxContext tmpCtx(temp);
> > >-          tmpCtx.SetOperator(gfxContext::OPERATOR_SOURCE);
> > 
> > 
> > This one is a pixel aligned tiled memcpy. We can probably deal with that in the
> > d2d backend.
> 
> Well, we could in theory, but I think it would require quite a bit of logic to
> detect it.. (is the path a box? If it is a box are the edges pixel aligned? If
> it is, is the source a surface pattern? If it is, is it untransformed? If it is
> go through a completely different code path.)
> 

cairo already has code to do this detection. I think we should be able to just implement a composite method for those cases.

> > >@@ -626,17 +626,17 @@ void imgFrame::Draw(gfxContext *aContext
> > >         pattern->SetExtend(gfxPattern::EXTEND_PAD);
> > >         pattern->SetFilter(aFilter);
> > >         break;
> > >     }
> > >   }
> > > 
> > >   if ((op == gfxContext::OPERATOR_OVER || pushedGroup) &&
> > >       format == gfxASurface::ImageFormatRGB24) {
> > >-    aContext->SetOperator(gfxContext::OPERATOR_SOURCE);
> > >+    aContext->SetOperator(OptimalFillOperator());
> > >   }
> > 
> > This one should be able to go away too because of the surface format.
> 
> No - mind you, the source -surface- is ARGB32. It's just that the imgFrame
> knows its -actual- alpha channel is completely opaque (see SetHasNoAlpha()). To
> a backend it would not immediately be obvious this can go away. i.e. D2D
> doesn't know it can do OVER instead of SOURCE. GDI can't figure out it can do
> SOURCE instead of OVER, without them analyzing the entire surface's alpha
> channel.

Indeed. I assumed that format was the cairo format and didn't change. This case is probably the only place we'd need OptimalFillOperator()

Comment 16

[Bas Schouten (:bas.schouten)]

Attachment: Avoid using source as a faster op when using D2D

Updated as per comments.

Comment 17

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 431692 [details] [diff] [review]
Avoid using source as a faster op when using D2D

>diff --git a/modules/libpr0n/src/imgFrame.cpp b/modules/libpr0n/src/imgFrame.cpp
>--- a/modules/libpr0n/src/imgFrame.cpp
>+++ b/modules/libpr0n/src/imgFrame.cpp
>@@ -394,19 +394,16 @@ void imgFrame::Draw(gfxContext *aContext
>   gfxContext::GraphicsOperator op = aContext->CurrentOperator();
> 
>   if (mSinglePixel && !doPadding && ImageComplete()) {
>     // Single-color fast path
>     // if a == 0, it's a noop
>     if (mSinglePixelColor.a == 0.0)
>       return;
> 
>-    if (op == gfxContext::OPERATOR_OVER && mSinglePixelColor.a == 1.0)
>-      aContext->SetOperator(gfxContext::OPERATOR_SOURCE);
>-

Let's do this part in a separate patch.

>     aContext->SetDeviceColor(mSinglePixelColor);
>     aContext->NewPath();
>     aContext->Rectangle(aFill);
>     aContext->Fill();
>     aContext->SetOperator(op);
>     aContext->SetDeviceColor(gfxRGBA(0,0,0,0));
>     return;
>   }
>@@ -626,17 +623,17 @@ void imgFrame::Draw(gfxContext *aContext
>         pattern->SetExtend(gfxPattern::EXTEND_PAD);
>         pattern->SetFilter(aFilter);
>         break;
>     }
>   }
> 
>   if ((op == gfxContext::OPERATOR_OVER || pushedGroup) &&
>       format == gfxASurface::ImageFormatRGB24) {
>-    aContext->SetOperator(gfxContext::OPERATOR_SOURCE);
>+    aContext->SetOperator(OptimalFillOperator());
>   }
> 
>   // Phew! Now we can actually draw this image
>   aContext->NewPath();
> #ifdef MOZ_GFX_OPTIMIZE_MOBILE
>   pattern->SetFilter(gfxPattern::FILTER_FAST); 
> #endif
>   aContext->SetPattern(pattern);
>@@ -951,8 +948,23 @@ PRBool imgFrame::GetCompositingFailed() 
> {
>   return mCompositingFailed;
> }
> 
> void imgFrame::SetCompositingFailed(PRBool val)
> {
>   mCompositingFailed = val;
> }
>+
>+gfxContext::GraphicsOperator imgFrame::OptimalFillOperator()
>+{
>+#ifdef XP_WIN
>+  if (gfxWindowsPlatform::GetPlatform()->GetRenderMode() ==
>+      gfxWindowsPlatform::RENDER_DIRECT2D) {
>+        // D2D -really- hates operator source.
>+        return gfxContext::OPERATOR_OVER;
>+  } else {
>+#endif
>+    return gfxContext::OPERATOR_SOURCE;
>+#ifdef XP_WIN
>+  }
>+#endif
>+}

Perhaps this function should be inline? Though it probably doesn't matter.

>diff --git a/modules/libpr0n/src/imgFrame.h b/modules/libpr0n/src/imgFrame.h
>--- a/modules/libpr0n/src/imgFrame.h
>+++ b/modules/libpr0n/src/imgFrame.h
>@@ -135,16 +135,23 @@ public:
>   }
> 
> 
> private: // methods
>   PRUint32 PaletteDataLength() const {
>     return ((1 << mPaletteDepth) * sizeof(PRUint32));
>   }
> 
>+  /**
>+   * This returns the fastest operator to use for solid surfaces which have no
>+   * alpha channel or their alpha channel is uniformly opaque.
>+   * This differs per render mode.
>+   */
>+  gfxContext::GraphicsOperator OptimalFillOperator();
>+
> private: // data
>   nsRefPtr<gfxImageSurface> mImageSurface;
>   nsRefPtr<gfxASurface> mOptSurface;
> #if defined(XP_WIN) && !defined(WINCE)
>   nsRefPtr<gfxWindowsSurface> mWinSurface;
> #elif defined(XP_MACOSX)
>   nsRefPtr<gfxQuartzImageSurface> mQuartzSurface;
> #endif

Comment 18

[Bas Schouten (:bas.schouten)]

Pushed http://hg.mozilla.org/mozilla-central/rev/e3d4574c2bb9.

Comment 19

[u88484]

Verified fixed using hourly build Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a3pre) Gecko/20100312 Minefield/3.7a3pre ID:20100312053645

Comment 20

[[not reading bugmail]]

Comment on attachment 430791 [details]
Weird affects coloring on plugins

looks like this was mainly bug 549056.