Bug 38683 – SVG FilterEffects need more detailed DRT information

• Component: SVG
• Status: RESOLVED FIXED

[Dirk Schulze]

SVG FilterEffects need more detailed DRT information. Some effects don't get dumped at all.

[Dirk Schulze]

Created attachment 55294 [details]
Patch

[Zoltan Herczeg]

Only the mac expected results are affected?

[Dirk Schulze]

(In reply to comment #2)
> Only the mac expected results are affected?

No, will fix other platform results, after the upload.

[Eric Seidel (no email)]

Comment on attachment 55294 [details]
Patch

WebCore/platform/graphics/filters/FEBlend.cpp:163
 +      case FEBLEND_MODE_DARKEN:
I prefer to write this sort of code in terms of helper functions which convert from the ENUM to a char*.  That would simplify this code slightly and make it easier to re-use.

WebCore/platform/graphics/filters/FEColorMatrix.cpp:202
 +          ts << "UNKNOWN";
Again here. :)

WebCore/platform/graphics/filters/FEColorMatrix.cpp:226
 +      if (!m_values.isEmpty()) {
I would have also made this a separate function... but I'm all crazy about using lots of little inlines. :)

WebCore/platform/graphics/filters/FEColorMatrix.h:54
 +          TextStream& externalRepresentation(TextStream& ts, int indent) const;
no "ts" needed.

WebCore/platform/graphics/filters/FEComponentTransfer.cpp:220
 +      ts << "{red:   type=\"" << m_redFunc.type << "\" slope=\""
Should use a helper function. :)


only looked through the first little bit, but there is more cleanup needed here.  The copy/paste with the m_redFunc stuff is r-. :(

[Dirk Schulze]

(In reply to comment #4)
> (From update of attachment 55294 [details])
> WebCore/platform/graphics/filters/FEBlend.cpp:163
>  +      case FEBLEND_MODE_DARKEN:
> I prefer to write this sort of code in terms of helper functions which convert
> from the ENUM to a char*.  That would simplify this code slightly and make it
> easier to re-use.
> 
> WebCore/platform/graphics/filters/FEColorMatrix.cpp:202
>  +          ts << "UNKNOWN";
> Again here. :)
> 
> WebCore/platform/graphics/filters/FEColorMatrix.cpp:226
>  +      if (!m_values.isEmpty()) {
> I would have also made this a separate function... but I'm all crazy about
> using lots of little inlines. :)
> 
> WebCore/platform/graphics/filters/FEColorMatrix.h:54
>  +          TextStream& externalRepresentation(TextStream& ts, int indent)
> const;
> no "ts" needed.
> 
> WebCore/platform/graphics/filters/FEComponentTransfer.cpp:220
>  +      ts << "{red:   type=\"" << m_redFunc.type << "\" slope=\""
> Should use a helper function. :)
> 
> 
> only looked through the first little bit, but there is more cleanup needed
> here.  The copy/paste with the m_redFunc stuff is r-. :(

We're talking about regression tests. While I still find the comments usefull, I don't think we need inlines or other tricks to make DRT faster. It's more important to make the rendering faster. This patch is just made to make DRT results more usefull.

[Dirk Schulze]

(In reply to comment #4)
> (From update of attachment 55294 [details])
> WebCore/platform/graphics/filters/FEBlend.cpp:163
>  +      case FEBLEND_MODE_DARKEN:
> I prefer to write this sort of code in terms of helper functions which convert
> from the ENUM to a char*.  That would simplify this code slightly and make it
> easier to re-use.
> 
> WebCore/platform/graphics/filters/FEColorMatrix.cpp:202
>  +          ts << "UNKNOWN";
> Again here. :)
> 
> WebCore/platform/graphics/filters/FEColorMatrix.cpp:226
>  +      if (!m_values.isEmpty()) {
> I would have also made this a separate function... but I'm all crazy about
> using lots of little inlines. :)
> 
> WebCore/platform/graphics/filters/FEColorMatrix.h:54
>  +          TextStream& externalRepresentation(TextStream& ts, int indent)
> const;
> no "ts" needed.
> 
> WebCore/platform/graphics/filters/FEComponentTransfer.cpp:220
>  +      ts << "{red:   type=\"" << m_redFunc.type << "\" slope=\""
> Should use a helper function. :)
> 
> 
> only looked through the first little bit, but there is more cleanup needed
> here.  The copy/paste with the m_redFunc stuff is r-. :(

Also I don't find it usefull to make the DRT output longer on using FEBLEND_MODE_DARKEN instead of just DARKEN, the type is clear and also the effect name. Using the complete name doesn't give more information, but makes the code harder to read because of the longer lines and the fewer differences between the values. All would begin with FEBLEND_MODE_.
But the comment about FEComponentTransfer.cpp is correct. Will invastigate in this.

[Dirk Schulze]

Created attachment 55462 [details]
Updated Patch

This patch considers eric's comments. With the excaption of more inliners.

[Nikolas Zimmermann]

Comment on attachment 55462 [details]
Updated Patch

Next round of review...

WebCore/platform/graphics/filters/FEBlend.cpp:148
 +  static TextStream& operator<<(TextStream& ts, BlendModeType t)
Rename 't' to 'type', no abbrevations. Use a const-reference to BlendModeType for consistency.

WebCore/platform/graphics/filters/FEColorMatrix.cpp:198
 +  static TextStream& operator<<(TextStream& ts, ColorMatrixType t)
Ditto.

WebCore/platform/graphics/filters/FEColorMatrix.cpp:226
 +      if (!m_values.isEmpty()) {
I'd say it's fine to have _no_ inline helper function here, it's pretty small and not used anywhere else.

WebCore/platform/graphics/filters/FEColorMatrix.h:54
 +          TextStream& externalRepresentation(TextStream& ts, int indent) const;
Omit the 'ts'.

WebCore/platform/graphics/filters/FEComponentTransfer.cpp:188
 +  static TextStream& operator<<(TextStream& ts, const ComponentTransferType& t)
Rename 't' to 'type'.

WebCore/platform/graphics/filters/FEComposite.cpp:176
 +  static TextStream& operator<<(TextStream& ts, CompositeOperationType t)
Rename 't' to 'type' and use a const-reference.

WebCore/platform/graphics/filters/SourceAlpha.cpp:80
 +      ts << "[SourceAlpha ";
As FilterEffect::externalRepresentation dumps nothing, this will result in "[SourceAlpha ]" - this is a bit unfortunate :(
If FilterEffect should dump subregions in future, you should not need to call it at all here? just dump ts << "[SourceAlpha]";

WebCore/rendering/RenderTreeAsText.h:50
 +  void writeIndent(TextStream& ts, int indent);
Omit the ' ts'.

WebCore/svg/graphics/filters/SVGFEImage.cpp:74
 +      ts << "]\n";
Can you at least dump the image size here? I don't like "[feImage ]".

[Nikolas Zimmermann]

Comment on attachment 55462 [details]
Updated Patch

Oops, I hit submit, instead of add, the review continues:

WebCore/svg/graphics/filters/SVGFEMerge.cpp:99
 +              (*it).get()->externalRepresentation(ts, indent + 1);
No need for the .get(), remove it.


WebCore/svg/graphics/filters/SVGFETile.cpp:94
 +      FilterEffect::externalRepresentation(ts);
Same here "[feTile ]" is not nice. :( Maybe just leave out the trailing space behind "feTile" for now here...

LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-blend-01-b-expected.txt:10
 +              [SourceGraphic ]
That's what I meant before, this is not nice - should be fixed if you apply all comments.

LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-composite-02-b-expected.txt:13
 +              [feImage ]
Ditto.

LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-comptran-01-b-expected.txt:15
 +                  {red:   type="IDENTITY" slope="1.00" intercept="0.00" amplitude="1.00" exponent="1.00" offset="0.00"}
No need to line up red / green / alpha, etc. The other fields don't have fixed width, so I'd just leave out the padding.

LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-offset-01-b-expected.txt:8
 +            [feMerge ]
Did I forget to mention feMerge before? Also looks not nice :-) Can you fix the trailing space.

Ok enough for now, will review the results in detail after you've provided new baselines :-)
A general note on externalRepresentation in the headers: can you add virtual prefixes everywhere? After all it's defined in the FilterEffect baseclass?

[Dirk Schulze]

Created attachment 55482 [details]
Patch

[Dirk Schulze]

Created attachment 55484 [details]
Patch

[Nikolas Zimmermann]

Comment on attachment 55484 [details]
Patch

Looks almost perfect, though some comments again. As FilterEffect::externalRep doesn't dump anything for now, but will do in future, make it future-proof.
Move away from:
ts << "[feFoo ";
FilterEffect::externalRepresentation(ts);
ts << "mode";

to
ts << "[feFoo";
FilterEffect::externalRepresentation(ts);
ts << " mode";

Whenever you change the externalRepresentation() you don't need to touch the other callsites.

In detail:
WebCore/platform/graphics/filters/FEBlend.cpp:176
 +      ts << "[feBlend ";
Here in FEBlend.

WebCore/platform/graphics/filters/FEColorMatrix.cpp:223
 +      ts << "[feColorMatrix ";
Ditto.

WebCore/platform/graphics/filters/FEComponentTransfer.cpp:227
 +      ts << "[feComponentTransfer ";
Ditto.

WebCore/platform/graphics/filters/FEComposite.cpp:207
 +      ts << "[feComposite ";
Ditto.

WebCore/platform/graphics/filters/FEGaussianBlur.cpp:145
 +      ts << "[feGaussianBlur ";
Ditto.

WebCore/svg/graphics/filters/SVGFEConvolveMatrix.cpp:164
 +      ts << "[feConvolveMatrix ";
Ditto.

WebCore/svg/graphics/filters/SVGFEDiffuseLighting.cpp:127
 +      ts << "[feDiffuseLighting ";
Ditto.

WebCore/svg/graphics/filters/SVGFEDisplacementMap.cpp:161
 +      ts << "[feDisplacementMap ";
Ditto

WebCore/svg/graphics/filters/SVGFEFlood.cpp:83
 +      ts << "[feFlood ";
Ditto.

WebCore/svg/graphics/filters/SVGFEImage.cpp:74
 +      ts << "[feImage ";
Ditto.

WebCore/svg/graphics/filters/SVGFEMerge.cpp:93
 +      ts << "[feMerge ";
Ditto.

WebCore/svg/graphics/filters/SVGFEMorphology.cpp:179
 +      ts << "[feMorphology ";
Ditto.

WebCore/svg/graphics/filters/SVGFEOffset.cpp:104
 +      ts << "[feOffset "; 
Ditto.

WebCore/svg/graphics/filters/SVGFESpecularLighting.cpp:139
 +      ts << "[feSpecularLighting ";
Ditto.

WebCore/svg/graphics/filters/SVGFETurbulence.cpp:136
 +      ts << "[feTurbulence ";
Ditto.

General style comments:
WebCore/platform/graphics/filters/FEColorMatrix.cpp:198
 +  static TextStream& operator<<(TextStream& ts, const ColorMatrixType& t)
Rename 't' to 'type'.

WebCore/platform/graphics/filters/FEComponentTransfer.cpp:213
 +  static TextStream& operator<<(TextStream& ts, const ComponentTransferFunction& f)
Rename 'f' to 'function'.

WebCore/platform/graphics/filters/SourceAlpha.cpp:81
 +  
Omit the newline.

WebCore/platform/graphics/filters/SourceGraphic.cpp:74
 +  
Ditto.

Comments regarding the DRT results:

LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-color-01-b-expected.txt:14
 +              [feColorMatrix type="MATRIX" values="0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 "]
Here's a trailing space in feColorMatrix value dumping that should be removed.

LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-composite-02-b-expected.txt:13
 +              [feImage image-size="150x150"]
Why not rename it to size? feImage image-size looks odd.

LayoutTests/platform/mac/svg/batik/filters/filterRegions-expected.txt:26
 +          [feFlood flood-color="#FF0000" flood-opacity="1.00"]
feFlood flood-color/opacity, same issue. maybe just dump color & oapcity here.

Just small issues remaining, the patch looks great as well as the results!

[Dirk Schulze]

(In reply to comment #12)
> LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-color-01-b-expected.txt:14
>  +              [feColorMatrix type="MATRIX" values="0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 "]
> Here's a trailing space in feColorMatrix value dumping that should be removed.
We don't know the count of values that's why I took number << " ", spaces wouldn't go away for " " << number.

> 
> LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-composite-02-b-expected.txt:13
>  +              [feImage image-size="150x150"]
> Why not rename it to size? feImage image-size looks odd.
If we later dump the subRegion, it should be more clear about what size we are  talking. There are much more sizes we could dump.

> 
> LayoutTests/platform/mac/svg/batik/filters/filterRegions-expected.txt:26
>  +          [feFlood flood-color="#FF0000" flood-opacity="1.00"]
> feFlood flood-color/opacity, same issue. maybe just dump color & oapcity here.
The attributes are called that way. I fear, that someone mixes it up with the color or opacity property, they are _not_ the same, and we should name them different.

> 
> Just small issues remaining, the patch looks great as well as the results!

[Dirk Schulze]

(In reply to comment #13)
> (In reply to comment #12)
> > LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-color-01-b-expected.txt:14
> >  +              [feColorMatrix type="MATRIX" values="0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00
I'll also fix this issue, but still believe it's the best to leave feFlood and feImage as they are.
feFlood because there is a difference between opacity and flood-opacity. Evn if opacity has no effect on feFlood. The same for color, the attributes have another sense.
feImage because of the different possibilities for size. Size could be subRegion, the scaled subRegion, the drawing Area or as it is here: the image size.

[Dirk Schulze]

Created attachment 55501 [details]
Patch

[Darin Adler]

Comment on attachment 55501 [details]
Patch

Seems OK.

I worry a little about having so much serialization code that is only for DumpRenderTree. I wish there was a way to repurpose something that's actually useful directly in production environments.

I also think that "external representation" is a strange name for a function that writes to a stream. I'd name it with a verb instead of a noun.

[Nikolas Zimmermann]

(In reply to comment #13)
> (In reply to comment #12)
> > LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-color-01-b-expected.txt:14
> >  +              [feColorMatrix type="MATRIX" values="0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 0.33 0.33 0.33 0.00 0.00 "]
> > Here's a trailing space in feColorMatrix value dumping that should be removed.
> We don't know the count of values that's why I took number << " ", spaces wouldn't go away for " " << number.
Huh? You can check wheter the iterator is at end() - 1, and decide not to add a space then.

> 
> > 
> > LayoutTests/platform/mac/svg/W3C-SVG-1.1/filters-composite-02-b-expected.txt:13
> >  +              [feImage image-size="150x150"]
> > Why not rename it to size? feImage image-size looks odd.
> If we later dump the subRegion, it should be more clear about what size we are  talking. There are much more sizes we could dump.
Fine with me.

> 
> > 
> > LayoutTests/platform/mac/svg/batik/filters/filterRegions-expected.txt:26
> >  +          [feFlood flood-color="#FF0000" flood-opacity="1.00"]
> > feFlood flood-color/opacity, same issue. maybe just dump color & oapcity here.
> The attributes are called that way. I fear, that someone mixes it up with the color or opacity property, they are _not_ the same, and we should name them different.
Okay, fine with me as well.

[Dirk Schulze]

(In reply to comment #16)
> (From update of attachment 55501 [details])
> I also think that "external representation" is a strange name for a function that writes to a stream. I'd name it with a verb instead of a noun.

This name is not only used by filters. For example RenderLayer and FrameView are also using this name. I'll land it with out changes of this name because of consistence reasons.

[Dirk Schulze]

Comment on attachment 55501 [details]
Patch

Clearing flags on attachment: 55501

Committed r59069: <http://trac.webkit.org/changeset/59069>

[Dirk Schulze]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/59069 might have broken GTK Linux 32-bit Release and Qt Linux Release

[Dirk Schulze]

(In reply to comment #21)
> http://trac.webkit.org/changeset/59069 might have broken GTK Linux 32-bit Release and Qt Linux Release

New baseline for qt and gtk in r59070 and r59071 windows will also need two test updates. I'm still waiting for the bot.

[Dirk Schulze]

New baseline for win landed in r59073. Two updates were needed.