Scour corrupts icons with gradients

Ooh, interesting. When viewed in Firefox, scour seems to have made the arrows black. On my system (via librsvg?), the arrows have been made partly-invisible as I mentioned in the description.

For the record, we run scour with --disable-style-to-xml and no other options (aside from -i and -o).

binary:
  $ dpkg -S deja-dup.svg
  app-install-data: /usr/share/app-install/icons/deja-dup.svg

source:
  $ apt-get source app-install-data-ubuntu

Definitely looks like a bug. linearGradient2513 is being removed by scour...

Just tried it on the latest version of Scour and it seems to be ok. Can you please download the trunk version and try it?

The latest version of scour is only one commit ahead of what I (Ubuntu natty) was using. But I tried it, no change.

However, if I don't use --disable-style-to-xml, it works. So it seems like an issue with that flag.

Ah, sorry missed that you were using that flag. Might be able to look
at it at some point in the future, but I think someone else
implemented that flag.

Jeff

On Thu, Jan 13, 2011 at 4:29 PM, Michael Terry
<email address hidden> wrote:
> The latest version of scour is only one commit ahead of what I (Ubuntu
> natty) was using.  But I tried it, no change.
>
> However, if I don't use --disable-style-to-xml, it works.  So it seems
> like an issue with that flag.
>
> --
> You received this bug notification because you are a member of Scouring
> Pads, which is the registrant for Scour.
> https://bugs.launchpad.net/bugs/702423
>
> Title:
>  Scour corrupts deja-dup.svg
>
> Status in Scour - Cleaning SVG Files:
>  New
> Status in “scour” package in Ubuntu:
>  New
>
> Bug description:
>  I noticed that scour messed up deja-dup.svg on my system.  It seems to
>  have made some parts of the svg invisible.
>
>  Correct and scoured svg icons attached.
>
>  ProblemType: Bug
>  DistroRelease: Ubuntu 11.04
>  Package: python-scour 0.25+bzr194-0ubuntu4
>  ProcVersionSignature: Ubuntu 2.6.37-12.26-generic 2.6.37
>  Uname: Linux 2.6.37-12-generic x86_64
>  Architecture: amd64
>  Date: Thu Jan 13 09:46:48 2011
>  PackageArchitecture: all
>  ProcEnviron:
>   LANGUAGE=en_US:en
>   PATH=(custom, user)
>   LANG=en_GB.utf8
>   LC_MESSAGES=en_US.utf8
>   SHELL=/bin/bash
>  SourcePackage: scour
>
>
>

<path inkscape:r_cy="true" inkscape:r_cx="true" style="opacity: 1; fill:
 url(&quot;#linearGradient2513&quot;) rgb(0, 0, 0); rest irrelevant" d="omitted for brevity"/>

findReferencedElements has this in its code, at line 451, which depends on the "referencing properties" being XML attributes:

    # now get all style properties and the fill, stroke, filter attributes
    styles = node.getAttribute('style').split(';')
    for attr in referencingProps:
        styles.append(':'.join([attr, node.getAttribute(attr)]))

meaning that a value of "" overwrites the value for the 'fill' property coming from style="". A fix would be to add a check for getAttribute(attr) != ''.

I'm tired and need to sleep, but had to debug this. If I have time tomorrow I can also try to fix it, first seeing if the fix is needed in any more locations.

Apologies for the hasty diagnostic; a reference is actually found correctly by calls to findReferencingProperty... That will teach me to debug while tired. But I can see that the bug occurs on the latest Scour with the --disable-style-to-xml option, so marking Confirmed.

Louis: thank you for working through it so late, even if the final cause has not yet shown revealed itself! It is appreciated.

I isolated it to removeDuplicateGradients or something it calls.

In Scour revision 195, elements referencing a duplicate gradient are updated for renaming only in xlink:href and XML fill=, stroke= etc. attributes, so anything in style= doesn't get updated.

I confirmed that it's removeDuplicateGradients messing up the document, because with its code to rename and remove gradients commented out, deja-dup.svg renders correctly again.

However, adding code to update style= (basically a call to renameID from --shorten-ids's code, but without updating the dup gradient's ID before removing it) makes the problem worse. It removes the blueness of the arrow completely in Firefox.

Something's very wrong here...

Same with Totem’s shipped SVG logo that is used as a big placeholder upon starting totem.

This is how it should look:
http://git.gnome.org/browse/totem/plain/data/icons/scalable/totem.svg

Attached is how it looks now

the same with the Evolution icon.

This scouring process seems to be quite lossy.

towolf: Off-topic, but what is the font being used in the browser in that evolution screenshot?

Back on-topic; we could probably be a bit more pragmatic is choosing which SVG to ship. Rendering both before.svg and after.svg with 'rsvg' (or other rasteriser of choice) and then comparing the resulting bitmaps should be enough; if they're the same, result, and if not, revert back and ship the unmodified one.

The problem with removeDuplicateGradients is the comparison of two gradient stops, which doesn’t look at the style attribute. So for scour, two stops might look the same, although they specify completely different colors (or opacities) within their style attribute. Since Inkscape stores stop colors in the style attribute, without converting styles to XML attributes, scour will consider each and every two-stop gradient of Inkscape as one and the same, and will remove all but the first (unless you told it to unpack styles first). A quick fix would be to include the style attribute in the list of attributes to look at when deciding whether two gradient stops are identical. That is, replace the line

 for attr in ['offset', 'stop-color', 'stop-opacity']:

with the line

 for attr in ['offset', 'stop-color', 'stop-opacity', 'style']:

A more ambitious fix would be to scan the style attribute for the relevant information. It seems a similar problem could occur in removeDuplicateGradientStops, which also doesn’t check the style attribute to determine whether two stops are identical.

Your proposed fix doesn't do anything for the problem described in this bug report. But since it does fix an error in the code, I'll apply it anyway. Thanks :)

This bug report will remain open because its root cause has not been addressed yet.

Disk Utility a.k.a. palimpsest is also affected.

As this affects more cases (like humanity-icon-theme), I'll add the extra rsvg before-after comparison to dh_scour. It'll take longer to build, but is the safe option.

This breaks a lot of icons in humanity-icon-theme > Bug #732427

Once this bug is fixed, we need to rebuild Humanity.

<pitti> I'll keep the master bug on my list then, please mark it as a dupe (and perhaps add a humanity task to do a rebuild after whatever fix we apply)

I tested using rsvg/convert/diff to compare the 'raw' (well, PPM) result before and after, but unfortunately there is always just a little noise in the data, like

-22102 22102 22102 62451 62451 62451 58082 58082 58082 57825 57825 57825
+22102 22102 22102 62451 62451 62451 58339 58339 58339 57825 57825 57825

I'm afraid we can't get 100% exact matches with scour, we'd need a kind of fuzzy comparison.

I wrote a little fuzzy SVG comparison script:

# fails when scouring deja-dup.svg with --disable-style-to-xml (which gives the buggy result):
$ ~/cmpsvg deja-dup-orig.svg deja-dup-scour-style.svg
difference: 5.51%
Images differ by more than 0.50%
1

(1 is the exit code)

# succeeds with scouring with no options:
$ ~/cmpsvg deja-dup-orig.svg deja-dup-scour.svg
difference: 0.14%
0

I set the default rejection threshold to 0.5%.

I integrated this into dh_scour now. If I run dh_scour in a built deja-dup tree, I now get

$ dh_scour -v
[...]
 scour -i deja-dup-symbolic.svg -o deja-dup-symbolic.svg.opt --disable-style-to-xml
        cmpsvg deja-dup-symbolic.svg deja-dup-symbolic.svg.opt
[...]
        difference: 0.00%

[...]
 scour -i deja-dup.svg -o deja-dup.svg.opt --disable-style-to-xml
 /home/martin/debian/scour/debian/cmpsvg deja-dup.svg deja-dup.svg.opt
[...]
 Original file size: 42305 bytes; new file size: 17825 bytes (42.13%)
difference: 5.51%
Images differ by more than 0.50%
dh_scour: scour was not able to process deja-dup.svg

and the file is left untouched.

This bug was fixed in the package scour - 0.25+bzr194-3

---------------
scour (0.25+bzr194-3) unstable; urgency=low

  * Add debian/cmpsvg: Simple python script for fuzzy-comparing two SVGs. Add
    python-rsvg and python-cairo dependencies for this.
  * debian/python-scour.install: Install cmpsvg.
  * debian/dh_scour: Call cmpsvg after scour, and only accept the optimized
    file if cmpsvg succeeds. This avoids shipping broken SVGs due to scour
    bugs. (LP: #702423)
 -- Martin Pitt <email address hidden> Fri, 11 Mar 2011 17:51:09 +0100

This bug was fixed in the package humanity-icon-theme - 0.5.3.6

---------------
humanity-icon-theme (0.5.3.6) natty; urgency=low

  [ K Vishnoo Charan Reddy ]
  * Remove extra symlinks; (LP: #593388)

  [ Martin Pitt ]
  * debian/control: Bump python-scourt build dependency to the version which
    avoids corrupted gradient SVGs. (LP: #702423)
 -- Martin Pitt <email address hidden> Fri, 11 Mar 2011 17:53:06 +0100

Importance upgraded to High since this is a document-corrupting bug.

@Louis, if you check the build from : https://launchpad.net/ubuntu/+source/humanity-icon-theme/0.5.3.5/+buildjob/2310457 ,
Not only were the color gradients drastically modified, In some cases the icons alpha was increased/modified too.
In some icons, especially the icons in "actions" folder, strokes were completely removed. ex: /actions/24/go-bottom.svg from the deb when compared to the actual icon > http://bazaar.launchpad.net/~ubuntu-art-pkg/humanity/release/view/head:/Humanity/actions/24/go-bottom.svg

@towolf: Off-topic, but like Paul, i'd also like to know what is the font being used in the browser in that evolution screenshot? ;-)

>> Louis Simard, #14

> In Scour revision 195, elements referencing a duplicate gradient
> are updated for renaming only in xlink:href and XML fill=, stroke=
> etc. attributes, so anything in style= doesn't get updated.

Looks like my hunch was right. Gradients were being removed for being duplicates of others, but the style= attribute was never updated to match. So gradients that were removed from the file were still referred to by graphics elements.

Jan Thor's _getStyle and _setStyle patch was useful to shorten the code needed for ths fix. It's a diff of under 10 lines for scour.py, with a new unit test for the duplicate-gradient name-updating behavior with --disable-style-to-xml.

The unit test passes. I need to perform more tests with files gathered from this bug report, then I'll commit a fix.

A fix for this bug is in Scour trunk revision 206. Martin Pitt, please evaluate and possibly integrate this fix in addition to your difference-checking tool into Ubuntu's scour and dh_scour.

Out of courtesy, I'm not going to modify the status in Ubuntu, since for you the bug is worked around.

Off-topic end: I believe this is the Ubuntu Font Family <http://font.ubuntu.com/>, which is included in Ubuntu 10.10 (Maverick) and 11.04 (Natty)

pitti : Just an FYI, build 0.5.3.6 is not quite perfect , I've not checked all icons but i found that these icons are messed up :
/usr/share/icons/Humanity/apps/48/wmtweaks.svg
/usr/share/icons/Humanity/apps/32/wmtweaks.svg
/usr/share/icons/Humanity/apps/48/evolution.svg
/usr/share/icons/Humanity/categories/48/applications-sports.svg

The first 3 might be trivial, but I'm not comfortable that the icons do not come out as they were designed.
The change to the sports icon which appears in Software center is very odd.

I realize that ISO size trumps 4 icons.. ;)
But, I'm *very* uncomfortable using scour for building Humanity .. :(
I'll let you know if i find more icons, I'd have to check 1800+ icons which have been modified.
We really can not be checking every icon, each time we build..

@Vish (#33)

Please test with Scour trunk revision 206 or 207 on the erroneous icons you've already discovered; the 0.5% difference check might not trigger false positives anymore with this fix in Scour.

@Louis, I tested the latest rev, and WOW! there is drastic improvement from the version we used to build build 0.5.3.5!
It works perfectly in 99% of the images I tested, and I tested it on a lot of svgs ;-)
But i found it stumbling on this one > http://bazaar.launchpad.net/~ubuntu-art-pkg/humanity/release/view/head:/Humanity/devices/48/gnome-dev-pcmcia.svg , it added stroke to the text "pcmcia" when the text was only a fill. (in humanity, we save the svgs as plain svg which often causes inkscape to not store the text as text but rather as paths, if that makes any difference..)

Btw, I just realized that Scour is what is used for the optimized svg option in inkscape! I was wondering how to batch convert all the svgs into optimized svgs ? > https://answers.launchpad.net/ubuntu/+source/scour/+question/148801

@pitti:
I'v found some more modified icons (either colors or missing lines):
/usr/share/icons/Humanity/actions/48/window-new.svg
/usr/share/icons/Humanity/apps/32/gnome-panel-force-quit.svg
/usr/share/icons/Humanity/categories/48/applications-geography.svg
/usr/share/icons/Humanity/categories/48/applications-utilities.svg
/usr/share/icons/Humanity/devices/48/media-cdr.svg
/usr/share/icons/Humanity/devices/48/media-cdrom-audio.svg
/usr/share/icons/Humanity/devices/48/media-cdrw.svg
/usr/share/icons/Humanity/devices/48/media-dvd.svg

Since latest scour has vastly improved and none of these svgs have a problem, IMO, we could rebuild humanity and *lower* difference check to 0.05% (or even 0.01% would be best ;) ) .
Just to save space on the ISO ;p

Louis Simard [2011-03-12 8:30 -0000]:
> A fix for this bug is in Scour trunk revision 206. Martin Pitt, please
> evaluate and possibly integrate this fix in addition to your difference-
> checking tool into Ubuntu's scour and dh_scour.

Thanks Louis! Yes, I'll update the package to the current bzr version
next Monday.

@Martin Pitt (#36)

Once the last bzr version is in the repository, all of the bugs I've added "Affects: Ubuntu's scour" to, can be marked Fix Committed or Fix Released at once. I listed them because the package needed to be updated to have fixes from Scour trunk.

@Vish (#35)

That's a different issue, for which I've opened bug 734019, "Incorrect property inheritance".

I'll do a new upstream snapshot/upload now. I'll walk through bzr log and check/close the referenced packaging bugs. Thanks!

Reopening this, as the cmpsvg addition is only a workaround for our dh_scour toolchain, but if you use scour manually you'd still get the corruption. This will get closed for good with the next upload then.

This bug was fixed in the package scour - 0.25+bzr207-1

---------------
scour (0.25+bzr207-1) unstable; urgency=low

  * New upstream bzr snapshot:
    - Add the option --no-renderer-workaround.
    - Fix gradient corruption. (LP: #702423)
    - Add option to only remove autogenerated id's. (LP: #492277)
    - Fix Windows line ending handling. (LP: #717826)
    - Fix occasional production of empty defs. (LP: #717254)
    - Remove more attributes with default values. (LP: #714731)
    - Fix wrong handling of file:// references. (LP: #708515)
    - Delete text attributes from groups with only non-text elements.
      (LP: #714727)
    - Remove unnecessary text-align properties from non-text elements.
      (LP: #714720)
    - Fix wrong optimization of 0-length Bézier curves. (LP: #714717)
    - Fix decimal number processing in rule_elliptical_arc(). (LP: #638764)
    - Fix transform matrix order. (LP: #722544)
  * Drop 01-bzr195.patch, upstream now.
  * debian/cmpsvg: Fix computation of difference to add up the absolute
    difference of each pixel.
  * debian/cmpsvg: Show percentages with three digits after comma.
  * debian/cmpsvg: Lower default treshold to 0.05%.
 -- Martin Pitt <email address hidden> Tue, 15 Mar 2011 09:28:45 +0100

This bug is fixed in release 0.26 of Scour.