360316 - Avoid group opacity when possible

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[tor]

As it seems to be a somewhat common authoring mistake to use "opacity" when "fill-opacity" or "stroke-opacity" was intended, we should try to avoid doing extra work when the same results are generated.

Comment 1

[tor]

Attachment: avoid group opacity when possible

Comment 2

[tor]

Attachment: update to tip

Comment 3

[Jonathan Watt [:jwatt]]

Comment on attachment 247099 [details] [diff] [review]
update to tip

>+PRBool
>+nsSVGUtils::CanOptimizeOpacity(nsIFrame *aFrame)
>+{
>+  if (!(aFrame->GetStateBits() & NS_STATE_SVG_FILTERED)) {
>+    if (aFrame->GetType() == nsGkAtoms::svgImageFrame) {
>+      return PR_TRUE;

Since fill and stroke are meaningless on an image, can't the image case just be hardcoded and omitted from this function?

>+    } else if (aFrame->GetType() == nsGkAtoms::svgPathGeometryFrame) {

The 'else' would be redundant given the return.

>+      nsSVGGeometryFrame *geom = NS_STATIC_CAST(nsSVGGeometryFrame*, aFrame);
>+      if (!(geom->HasFill() && geom->HasStroke()))

That returns true if there's only a stroke, but not if there's only a fill. Think you want something like:

      PRBool hasFill = geom->HasFill();
      PRBool hasStroke = geom->HasStroke();
      if (hasFill && !hasStroke || !hasFill && hasStroke)

>+        return PR_TRUE;
>+    }
>+  }
>+  return PR_FALSE;
>+}

Comment 4

[Jonathan Watt [:jwatt]]

> That returns true if...

Uh, never mind that. Not reading it right.

Comment 5

[tor]

(In reply to comment #3)
> (From update of attachment 247099 [details] [diff] [review] [edit])
> >+PRBool
> >+nsSVGUtils::CanOptimizeOpacity(nsIFrame *aFrame)
> >+{
> >+  if (!(aFrame->GetStateBits() & NS_STATE_SVG_FILTERED)) {
> >+    if (aFrame->GetType() == nsGkAtoms::svgImageFrame) {
> >+      return PR_TRUE;
> 
> Since fill and stroke are meaningless on an image, can't the image case just be
> hardcoded and omitted from this function?

The filter check would still be needed in nsSVGImageFrame, so I thought it might be best to just keep all the logic in one method.

Comment 6

[tor]

Attachment: Remove else

Comment 7

[Jonathan Watt [:jwatt]]

Comment on attachment 247442 [details] [diff] [review]
Remove else

>+++ layout/svg/base/src/nsSVGGeometryFrame.h	4 Dec 2006 17:50:08 -0000
>@@ -110,11 +110,12 @@ protected:
>   nsSVGPaintServerFrame *GetPaintServer(const nsStyleSVGPaint *aPaint);
> 
>   NS_IMETHOD InitSVG();
> 
> private:
>   nsresult GetStrokeDashArray(double **arr, PRUint32 *count);
>   float GetStrokeDashoffset();
>   void RemovePaintServerProperties();
>+  float OptimizeOpacity(float aOpacity);

Can you call this "MaybeOptimizeOpacity" and add something like the following comment above it?

  // If we can avoid the expense of group opacity, we multiply the fill-opacity
  // or stroke-opacity (whichever one is present) by the value of the 'opacity'
  // property, and elsewhere pretend the 'opacity' property has a value of 1.


>@@ -292,18 +292,21 @@ nsSVGImageFrame::PaintSVG(nsSVGRenderSta
> 
>     nsCOMPtr<nsIDOMSVGMatrix> fini = GetImageTransform();
> 
>     if (GetStyleDisplay()->IsScrollableOverflow()) {
>       gfx->Save();
>       nsSVGUtils::SetClipRect(gfx, ctm, x, y, width, height);
>     }
> 
>-    nsSVGUtils::CompositeSurfaceMatrix(gfx, mSurface, fini,
>-                                       mStyleContext->GetStyleDisplay()->mOpacity);
>+    float opacity = 1.0f;
>+    if (nsSVGUtils::CanOptimizeOpacity(this))
>+      opacity = GetStyleDisplay()->mOpacity;

I have a bit of a post-Christmas party hangover right now, but I believe this logic should be inverted.


>+  /* Using opacity instead of fill or stroke opacity on a geometry
>+   * object seems to be a common authoring mistake.  If we're not
>+   * applying filters and not both stroking and filling, we can
>+   * generate the same result without going through a push/pop
>+   * group. */
>+  static PRBool
>+  CanOptimizeOpacity(nsIFrame *aFrame);

Can you make that "Using group opacity..." and "without going through the overhead of...".

With those changes r=jwatt.

Comment 8

[tor]

(In reply to comment #7)
> >+  float OptimizeOpacity(float aOpacity);
> 
> Can you call this "MaybeOptimizeOpacity" and add something like the following
> comment above it?
> 
>   // If we can avoid the expense of group opacity, we multiply the fill-opacity
>   // or stroke-opacity (whichever one is present) by the value of the 'opacity'
>   // property, and elsewhere pretend the 'opacity' property has a value of 1.

Renamed and added a modification of the suggested comment.

> >@@ -292,18 +292,21 @@ nsSVGImageFrame::PaintSVG(nsSVGRenderSta
> >-    nsSVGUtils::CompositeSurfaceMatrix(gfx, mSurface, fini,
> >-                                       mStyleContext->GetStyleDisplay()->mOpacity);
> >+    float opacity = 1.0f;
> >+    if (nsSVGUtils::CanOptimizeOpacity(this))
> >+      opacity = GetStyleDisplay()->mOpacity;
> 
> I have a bit of a post-Christmas party hangover right now, but I believe this
> logic should be inverted.

I think the logic is correct, and have added a comment to try clarifying the issue.

Comment 9

[tor]

Attachment: updated patch

Comment 10

[Jonathan Watt [:jwatt]]

Comment on attachment 250218 [details] [diff] [review]
updated patch

Ah, I see. r=jwatt

Comment 11

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

Comment on attachment 250218 [details] [diff] [review]
updated patch

+  if (nsSVGUtils::CanOptimizeOpacity(this))
+    aOpacity *= GetStyleDisplay()->mOpacity;

Braces please

+    if (nsSVGUtils::CanOptimizeOpacity(this))
+      opacity = GetStyleDisplay()->mOpacity;

Ditto.

+    if (aFrame->GetType() == nsGkAtoms::svgImageFrame)
+      return PR_TRUE;
+    if (aFrame->GetType() == nsGkAtoms::svgPathGeometryFrame) {

Pull up the call to aFrame->GetType() to a variable so we only call it once.

Comment 12

[tor]

Attachment: Checkin version (adjusted per roc's comments)

Comment 13

[tor]

Checked in.