378575 - Browser hangs mixing filters and text

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Robert Longson [:longsonr]]

From Tor in bug 378445

The first bug looks like a repeat of the problem that caused a temporary
backout of bug 375141 - using a non-win32 surface causes the cairo win32 font
backend to freak out.  That testcase is filtering text, and the current filter
code uses an image surface.  We'll need to switch to
gfxPlatform::CreateOffscreenSurface, then paint that to an image surface.

Comment 1

[Robert Longson [:longsonr]]

Attachment: patch

Comment 2

[tor]

Comment on attachment 263100 [details] [diff] [review]
patch

>+  memset(tmpImageSurface->Data(), 0, tmpImageSurface->GetSize().height * tmpImageSurface->Stride());
>+  gfxContext tmpImageContext(tmpImageSurface);
>+  tmpImageContext.SetSource(tmpContext.OriginalSurface());
>+  tmpImageContext.Paint();

Possible (untested) alternative to doing a memset:

  tmpImageContext.SetOperator(CAIRO_OPERATOR_SOURCE);

Comment 3

[Robert Longson [:longsonr]]

Attachment: with suggested modification

I think this also fixes sourceAlpha for big endian systems although I have only tested it on small endian.

Comment 4

[tor]

Comment on attachment 263146 [details] [diff] [review]
with suggested modification

>+  nsRefPtr<gfxImageSurface> tmpAlphaSurface;
>   if (requirements & NS_FE_SOURCEALPHA) {
>-    cairo_surface_t *alpha =
>-      cairo_image_surface_create(CAIRO_FORMAT_ARGB32,
>-                                 filterResX, filterResY);
>-
>-    if (!alpha || cairo_surface_status(alpha)) {
>-      if (alpha)
>-        cairo_surface_destroy(alpha);
>+    tmpAlphaSurface =
>+      new gfxImageSurface(surfaceSize, gfxASurface::ImageFormatARGB32);
>+
>+    if (!tmpAlphaSurface || !tmpAlphaSurface->Data()) {
>       FilterFailCleanup(aContext, aTarget);
>       return NS_OK;
>     }
> 
>-    PRUint8 *data = tmpSurface->Data();
>-    PRUint8 *alphaData = cairo_image_surface_get_data(alpha);
>-    PRUint32 stride = tmpSurface->Stride();
>-
>-    for (PRUint32 yy = 0; yy < PRUint32(filterResY); yy++)
>-      for (PRUint32 xx = 0; xx < PRUint32(filterResX); xx++) {
>-        alphaData[stride*yy + 4*xx + GFX_ARGB32_OFFSET_B] = 0;
>-        alphaData[stride*yy + 4*xx + GFX_ARGB32_OFFSET_G] = 0;
>-        alphaData[stride*yy + 4*xx + GFX_ARGB32_OFFSET_R] = 0;
>-        alphaData[stride*yy + 4*xx + GFX_ARGB32_OFFSET_A] =
>-          data[stride*yy + 4*xx + 3];
>+    PRUint8 *data = tmpImageSurface->Data();
>+    PRUint8 *alphaData = tmpAlphaSurface->Data();
>+    PRUint32 stride = tmpImageSurface->Stride();
>+
>+    for (PRInt32 yy = 0; yy < surfaceSize.height; yy++)
>+      for (PRInt32 xx = 0; xx < surfaceSize.width; xx++) {
>+        PRUint8 *pixel = alphaData + stride * yy + 4 * xx;
>+        pixel[GFX_ARGB32_OFFSET_B] = 0;
>+        pixel[GFX_ARGB32_OFFSET_G] = 0;
>+        pixel[GFX_ARGB32_OFFSET_R] = 0;
>+        pixel[GFX_ARGB32_OFFSET_A] =
>+          data[stride * yy + 4 * xx + GFX_ARGB32_OFFSET_A];
>     }
> 
>-    instance.DefineImage(NS_LITERAL_STRING("SourceAlpha"), alpha,
>+    instance.DefineImage(NS_LITERAL_STRING("SourceAlpha"),
>+                         tmpAlphaSurface->CairoSurface(),
>                          nsRect(0, 0, filterResX, filterResY), colorModel);
>   }
> 
>   // this always needs to be defined last because the default image
>   // for the first filter element is supposed to be SourceGraphic
>   instance.DefineImage(NS_LITERAL_STRING("SourceGraphic"),
>-                       tmpSurface->CairoSurface(),
>+                       tmpImageSurface->CairoSurface(),
>                        nsRect(0, 0, filterResX, filterResY), colorModel);

While these are good changes, they are unrelated to this bug and I'd prefer them handled as a seperate bug/checkin.

Comment 5

[Robert Longson [:longsonr]]

Attachment: address review comments

with alpha changes removed

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think gfx should support text drawing onto any surface type. Why don't we fix that, and not this, and back out https://bugzilla.mozilla.org/show_bug.cgi?id=375141#c40? It should be fairly straightforward.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Stuart, see comment 7

Comment 10

[Robert Longson [:longsonr]]

If bug 383512 is OK, I'll put a patch here for the back out of comment 7.

Comment 11

[Robert Longson [:longsonr]]

Attachment: plan B

Once bug 383512 lands we can use image surfaces again.

Comment 12

[tor]

(In reply to comment #11)
> Created an attachment (id=268337) [details]
> plan B
> 
> Once bug 383512 lands we can use image surfaces again.

Why do you want to switch patterns to an image surface?  In this case we don't need to touch the pixels directly, so the main concern is drawing the pattern as fast as possible, which should best served by using a platform native surface.

Comment 13

[Robert Longson [:longsonr]]

(In reply to comment #12)
> Why do you want to switch patterns to an image surface?  In this case we don't
> need to touch the pixels directly, so the main concern is drawing the pattern
> as fast as possible, which should best served by using a platform native
> surface.
> 
Is there any point in even doing nsSVGUtils then?

Comment 14

[tor]

(In reply to comment #13)
> Is there any point in even doing nsSVGUtils then?

Not sure what you're asking here.

Comment 15

[Robert Longson [:longsonr]]

(In reply to comment #14)
> 
> Not sure what you're asking here.
> 

I was referring to comment 7 where roc was suggesting a backout of bug 375141 comment 40

Comment 16

[tor]

(In reply to comment #13)
> (In reply to comment #12)
> > Why do you want to switch patterns to an image surface?  In this case we don't
> > need to touch the pixels directly, so the main concern is drawing the pattern
> > as fast as possible, which should best served by using a platform native
> > surface.
> > 
> Is there any point in even doing nsSVGUtils then?

It seems unlikely that there would be a difference in performance between image and platform surfaces, if we use an image surface that would one less plaform resource (ie, GDI objects) we end up using.

Comment 17

[Robert Longson [:longsonr]]

Attachment: plan B without pattern change

Note this can't be checked in unless and until the patch for bug 383512 is checked in.

Comment 18

[Jonathan Watt [:jwatt]]

Can this be checked in now?

Comment 19

[Robert Longson [:longsonr]]

Comment on attachment 273559 [details] [diff] [review]
plan B without pattern change

Low risk, returning the code to how it used to be.

Comment 20

[Robert Longson [:longsonr]]

checked in.