EMF linear gradient bug fixes and new features

Bug #1263242 reported by David Mathog
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
Inkscape
Fix Released
Wishlist
David Mathog

Bug Description

The attached patch does the following:

1. Fixes two bugs affecting gradients with 3 or more stops when written to an EMF file as a series of colored slices. These
bugs were a general mangling of the gradient and the wrong color being applied to the first slice above a stop.

2. Brings libUEMF files in Inkscape up to the current release level for that library. Also adds EMF+ pieces into src/libuemf
(even though they are not yet used).

3. Fully implements reading and writing of the EMF horizontal and vertical GradientFill records. (After _finally_ figuring out with the help of Microsoft support why the linear form of this record, implemented exactly to their documentation, was toxic to the Windows GDI library.) This output method is enabled if the new EMF output option "Use native rectangular linear gradients" is checked. If the existing "Convert gradients to colored polygon series" is also checked the former takes precedence. The Gradientfill method can only be applied when the gradient is perfectly aligned with one side of the rectangular area - no tilt at all is allowed. That said, the entire rectangle may be rotated, so long as its gradient rotates with it. Multi-stop linear gradients are written out as individual Gradientfill records. When they are read back in they become a series of rectangles with 2 stop gradients abutted to each other. This looks the same.

4. Partial implementation of the EMF triangle GradientFill record. This type of gradient is used in GDI and is comprised of a triangle with a different color at each corner. SVG has no equivalent gradient. On output to EMF Inkscape never produces this type of record since it does not support this type of gradient. On input it draws the appropriate size triangle, but as a solid, using the first vertex's color.

The attached file may be used to test 1 and 3. To see the bugs from (1) on an unpatched Inkscape read it in, then save it to EMF with "Convert gradients to colored polygon series" checked. Read it back in, and the gradients will be mangled. Do the same thing with the patch applied and the gradients should look the same as in the S (except for some moire effects). It the patched Inkscape is used and "Use native rectangular linear gradients" is checked when the EMF is made, then when that EMF is read back in it will have abutted linear gradients emulating the original 4 stop linear gradients for those cases where the gradient was aligned with one side of the rectangle, and will fall back to slices for the one linear case that isn't aligned and the radial gradient. (That is: original SVG file = 1 rectangle with a 4 stop linear gradient, EMF produced by patched inkscape = 3 abutted rectangles with one 2 stop gradient each, EMF produced by unpatched inkscape = hundreds of slices emulating the gradient.)

To see the triangle gradient open test_libuemf_ref.emf from the libuemf distribution (from sourceforge), which looks like
this in Windows Preview:

   http://libuemf.sourceforge.net/test_libuemf_emf.png

The triangle gradients are on the left edge about halfway up.

Now a huge disclaimer. I do not know of any program other than Inkscape that can make full use of the EMF GradientFill record. EMF files with this record will generally display properly in Windows Picture and Fax Viewer, or any other application like PowerPoint that uses the Windows GDI library to display the EMF file. However, none of them seem to be able to convert from that to their native formats. In PowerPoint, for instance, use "Insert picture" with the EMF file produced above and it will look as it should in the slide. Try to ungroup it though, which would covert it to PowerPoint's own object format, and the GradientFill parts will disappear.

Revision history for this message
David Mathog (mathog) wrote :
Revision history for this message
David Mathog (mathog) wrote :

The patch

su_v (suv-lp)
tags: added: emf exporting gradient importing
Revision history for this message
David Mathog (mathog) wrote :

This patch is a newer version of the preceding one. This one has the latest versions of the libuemf files.

The vast majority of these changes are to the comments, especially the doxygen comments.

Can somebody please test this on a Mac and apply it to trunk if that goes OK?

The functionality doesn't change much (just the somewhat obscure gradient type) but the documentation is much better, and it would be nice to get the Inkscape libUEMF back in sync with the libUEMF distro.

David Mathog (mathog)
tags: added: documentation
Revision history for this message
su_v (suv-lp) wrote :

> Can somebody please test this on a Mac (…)

Trunk r12937 with changes_2014_01_15d.patch applied fails to compile [*] - apparently attempts to include files missing in trunk, and not added by the patch:

  CXX extension/init.o
In file included from ../../src/extension/init.cpp:37:
../../src/extension/internal/emf-inout.h:15:10: fatal error: 'extension/internal/metafile-inout.h' file not found
#include "extension/internal/metafile-inout.h" // picks up PNG
         ^
1 error generated.
make[3]: *** [extension/init.o] Error 1
make[2]: *** [all] Error 2
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

--
[*] tested with llvm-gcc-4.2 and with (outdated) clang from Xcode 4.3.2, on OS X 10.7.5 Note that for tests with latest clang on OS X 10.9 Mavericks (uses libc++ instead of libstcd++), we'll for now have to rely on feedback from the maintainer of the inkscape-devel port in MacPorts (and the build result of the MacPorts built-bots) once the changes are committed to trunk.

su_v (suv-lp)
Changed in inkscape:
assignee: nobody → David Mathog (mathog)
status: New → In Progress
Revision history for this message
David Mathog (mathog) wrote :

I see what is happening. "bzr revert" is not deleting files even though they were "bzr add" at one point. So when I test locally with:

bzr diff >changes.patch
bzr revert
patch -p0 <changes.patch
make

there is no problem on my system during the build because the file is still there. Now I did at one point do "bzr add" on those missing files, but one cycle of the above removes that, so the next cycle creates a diff without the new files in it.
This is because applying the patch does not trigger a "bzr add" on files that are created by the patch. So, when you apply the attached patch, I guess you also need to do a manual

bzr add src/extension/internal/metafile-inout.cpp
bzr add src/extension/internal/metafile-inout.h

Revision history for this message
su_v (suv-lp) wrote :

Latest version yet again misses files. Build failure:

  CC libuemf/uemf_print.o
../../src/libuemf/uemf_print.c:25:10: fatal error: 'upmf_print.h' file not found
#include "upmf_print.h"
         ^
1 error generated.
make[3]: *** [libuemf/uemf_print.o] Error 1
make[2]: *** [all] Error 2
make[1]: *** [all-recursive] Error 1
make: *** [all] Error 2

Note: to have files included in a patch created with 'bzr diff' make sure to add the files beforehand. You can check the status of the files of your local branch anytime with 'bzr status'. Files listed under 'Unknown' will neither be included in a 'bzr diff' nor in a commit.

Revision history for this message
su_v (suv-lp) wrote :

To verify a patch, the command line tool 'diffstat' can be useful (generates an overview of the changes that would be applied with the patch). Sometimes it might also help to compare the file size of the different patch versions: your latest patch for example has half the size of the earlier one, which already missed some files.

Revision history for this message
David Mathog (mathog) wrote :

Aargh! bzr status helps.

The problem is that when I am testing other patches with "bzr revert"/patch cycles it removes all of the "bzr add", and I keep forgetting to add back the ones that were not "in play" for those test cycles. Nothing bad happens on my system since "bzr revert" does not actually remove the files.

Instead of applying patches directly I need to make a point of installing them through a script that automatically converts all of the:

=== added file 'src/libuemf/upmf.c'

in the patch to

bar add 'src/libuemf/upmf.c'

Third patch is the charm, I hope!

Revision history for this message
su_v (suv-lp) wrote :

Latest patch compiles successfully on OS X 10.7.5 with llvm-gcc-4.2 and (outdated) clang from Xcode 4.3.2.

I'm not sure whether the patch achieves what it intends, based on a quick test:

On 2013-12-20 23:31 +0100, David Mathog wrote:
> It the patched Inkscape is used and "Use native rectangular
> linear gradients" is checked when the EMF is made, then when that EMF is
> read back in it will have abutted linear gradients emulating the
> original 4 stop linear gradients for those cases where the gradient was
> aligned with one side of the rectangle, and will fall back to slices for
> the one linear case that isn't aligned and the radial gradient.

The EMF exported with "Use native rectangular linear gradients" checked imports without any fills for objects which have a linear gradient in the original SVG.

Revision history for this message
su_v (suv-lp) wrote :
Revision history for this message
David Mathog (mathog) wrote :

Was "convert gradients to colored polygon series" also checked?

The way it works when that is also checked is that it uses the native
fill if it can, and if it cannot. it falls back to the colored polygons.

If neither is checked then it does nothing with gradients.

If only the native one is selected it cannot fall back to "colored polygons", so nothing is output for
misaligned gradients.

Try it with the attached SVG with all 4 combinations of those buttons, to EMF, then read back in.
(In this example if neither is selected the EMF will be empty, as there is no stroke on the rectangles.)

If it isn't working right please post the SVG that triggers the bug.

Thanks.

Revision history for this message
David Mathog (mathog) wrote :

Example with both output options for gradient selected. The skewed gradient is made up of slices, all the others are native.
(I think.)

Revision history for this message
su_v (suv-lp) wrote :

> Was "convert gradients to colored polygon series" also checked?

No.

Revision history for this message
su_v (suv-lp) wrote :

Nevermind - it looks as if the error is specific to that one local branch used for the first test (that branch does have a few other unrelated changes besides 'changes_2014_01_16c.patch'). Works as described with a different build which uses _only_ the patch from this report ('changes_2014_01_16c.patch'). I'll commit the patch as is later tonight. Sorry for the noise.

Revision history for this message
su_v (suv-lp) wrote :

<off-topic>
JFYI: as far as I can tell this change (a test related to the remaining issue in bug #1231990) caused the failure described in comments 9-10:

=== modified file 'src/2geom/coord.h'
--- src/2geom/coord.h 2011-08-27 12:36:15 +0000
+++ src/2geom/coord.h 2014-01-17 01:05:32 +0000
@@ -52,7 +52,7 @@
 typedef double Coord;
 typedef int IntCoord;

-const Coord EPSILON = 1e-5; //1e-18;
+const Coord EPSILON = 1e-9; //1e-18;

 inline Coord infinity() { return std::numeric_limits<Coord>::infinity(); }

</off-topic>

Revision history for this message
su_v (suv-lp) wrote :

Committed to trunk in revision 12953.

Changed in inkscape:
milestone: none → 0.91
status: In Progress → Fix Committed
Revision history for this message
David Mathog (mathog) wrote :

Hmm, something is broken in that release.

bzr revert
bzr upgrade
make
src/inkscape
  (load test_libuemf_ref.emf interactively)

all of the images are missing, each says "linked image not found" with a little "X" in it. I didn't implement that change, so I cannot tell you off hand where it came from.

Revision history for this message
su_v (suv-lp) wrote :

> all of the images are missing, each says "linked image not found" with a little "X"
> in it. I didn't implement that change, so I cannot tell you off hand where it
> came from.

Possibly due to recent changes in r12939 or r12945 - the embedded images in the EMF test file are also missing with r12952 (i.e. without the latest EMF patch).

Revision history for this message
David Mathog (mathog) wrote :

Here is a quick test case to show the problem. When it loads things are totally screwed up. There are two copies of that warning message, and the actual image is not in the SVG file at all.

Revision history for this message
su_v (suv-lp) wrote :

Looks like embedded images are generally broken right now (AFAICT rev >= 12945), this ought to be tracked separately.

Revision history for this message
David Mathog (mathog) wrote :

Tracked? The patch at that revision needs to be rolled back. With it in inkscape cannot load any images, even directly from a png file!

Revision history for this message
David Mathog (mathog) wrote :

Submitted as bug #1270334.

Revision history for this message
David Mathog (mathog) wrote :

That attache small patch corrects a bug in rectangular gradient handling. In brief, if the coordinates were rectangular but not transformed within inkscape they were handled properly, whereas if they were transformed (rotated, in particular) from an orientation square with the axes, they were not handled properly. Will post a test svg and a result emf to show what should happen. Before the patch the lower set of rectangles end up sliced, after the patch they are correctly written as rectangular gradients (not sliced).

Revision history for this message
David Mathog (mathog) wrote :

Test file, large assortment of rectangular gradients, with and without subsequent transformations.

Revision history for this message
David Mathog (mathog) wrote :

Correct output from preceding example, found when both of the EMF gradient output options are checked. All of the rectangular gradients from the SVG have been written in that form into the EMF. There is no slicing and dicing to emulate gradients.

Revision history for this message
David Mathog (mathog) wrote :

For post #12

s/That attache /The attached /

Revision history for this message
su_v (suv-lp) wrote :

> Before the patch the lower set of rectangles end up sliced, after
> the patch they are correctly written as rectangular gradients (not sliced).

Can't reproduce with r12966 (orig trunk without any changes) - the gradients in the exported EMF reopened in Inkscape are not sliced, but scaled & positioned incorrectly. The patched build outputs an EMF file which AFAICT renders correctly when reopened in Inkscape though (no slices either).

Not sure what to make of this mismatch of the bug to be fixed by the patch - the outcome however seems to be correct either way. Commit anyway?

Revision history for this message
su_v (suv-lp) wrote :
Revision history for this message
David Mathog (mathog) wrote :

It is a mystery to me why the test case worked on your machine at r12966, but at least I understand why it works with the patch applied. So yes, please commit. Thanks.

Revision history for this message
su_v (suv-lp) wrote :

> It is a mystery to me why the test case worked on your machine at r12966, (…)

I get the same result using inkscape-trunk from the official PPA (r12954, r12966) on Ubuntu 13.04 (64 bit, VM, host: OS X 10.7.5): no slices - just rectangular gradients with incorrect scale and position when reopening the exported EMF in Inkscape. At least the "mystery" does not appear to be specific to my own local trunk builds on OS X. The underlying hardware ("machine") is still the same though - how come Inkscape's EMF export would differ depending on what kind of hardware a binary is running on?

Patch committed in r12968 anyway.

Revision history for this message
su_v (suv-lp) wrote :

(bug importance set to 'Wishlist' because the main patch mostly adds support for new/enhanced features and updates internal libUEMF copy to latest upstream version - please comment if you disagree)

Changed in inkscape:
importance: Undecided → Wishlist
Revision history for this message
David Mathog (mathog) wrote :

re: 30. I am not sure why I see slices and you don't. It might indicate an uninitialized variable, as those can have different phenotypes on different machines (or change over time on the same machine, or with compiler versions.) I don't recall valgrind showing anything, but will test that again. With the patch the scaling and rotation problems you saw went away, right? That at least makes sense because the little patch addresses certain cases where an inkscape transformation was not applied.

re:31. Wishlist is fine.

Revision history for this message
su_v (suv-lp) wrote :

> With the patch the scaling and rotation problems you saw went away, right?

Yes.

Revision history for this message
David Mathog (mathog) wrote :

OK.

Checked for use of uninitialized variables and didn't see any in the sections of code which were modified.

However...

It is getting harder and harder to resolve problems in Inkscape with valgrind, because there are so many warnings. In today's run the log file was 16.6M in size and contained:

1750 Conditional jump or move depends on uninitialised value(s)
22 Syscall param writev(vector[...]) points to uninitialised byte(s)
11970 are possibly lost in loss record
744 are definitely lost in loss record
228 Invalid read of size

with this disheartening (because the test only exercised a tiny fraction of the code) summary:
==12164== definitely lost: 698,304 bytes in 17,949 blocks
==12164== indirectly lost: 2,234,831 bytes in 85,144 blocks
==12164== possibly lost: 32,151,449 bytes in 385,470 blocks
==12164== still reachable: 16,104,016 bytes in 294,364 blocks
==12164== suppressed: 0 bytes in 0 blocks

with 44166 loss records!

This isn't all inkscape's fault. There were a ton of warnings about __wcslen_sse2, as well
as who knows how many issues related to what looks like normal GTK usage. The first is definitely, and the second
is probably, a Valgrind issue.

Revision history for this message
David Mathog (mathog) wrote :

The attached patch addresses a couple of issues with respect to EMF linear gradients, plus some other EMF/WMF issues which might as well go here as anywhere else. It has been tested on Linux and Windows.

EMF issues:

1. Snap linear gradient angle/rectangle edge to nearest 1/100th of a degree. Use explicit rounding for width height and bounding boxes. This greatly improves the numerical stability for this sort of structure in repeated cycles of save/read in EMF format. It still isn't perfect, but it is much better than it was.

2. Relaxed the "is 90 degrees" corner test so that anything better than about 89.9 degrees counts as a right angle. The previous limit was much too strict, so after a couple of read/write cycles arbitrarily oriented rectangles tended to fail the test and their gradients were written using the "sliced" method.

3. A scaling issue was found when linear gradient rectangles were written to EMF using trunk code. The corner position was correct but the size was too large. Possibly a patch issue from one of the preceding patches. This patch sets things straight when applied to current trunk.

4. Include newer version of libTERE's text_reassemble.c. This one looks at wide x kerns and inserts one or two spaces, if they will fit. Previously if "one two" was written to EMF as "one", "two", with no spaces, the text reassembler would place them together properly, and it would look like the missing space was there, but a text "copy" would contain "onetwo" because the space was not actually present. In the new version it puts them together, sees the large kern, and adds one space (in this case) on the leading edge of "two". Subsequently a text copy has "one two", as expected. Space reintroduction is currently limited to two spaces.

5. Newer text_reassemble.c detects text written backwards and does not try to assemble it. That is, if the text says that it is a L->R language and the records are in order "A","B","C", but the positions are at 200, 150, 100, previously the text assembler would put those together, and the SVG renderer would draw the structure going the wrong way. In the new version this sort of thing is assumed to not be text per se, or an error made by whatever generated the EMF, and the pieces are brought in separately in their specified positions.

6. Fixed a couple of comment issues.

WMF issues

7. Added support for "textout" records. (Or rather, fixed the broken support.)

8. Code for handling Placeable headers when WINDOWORG records are also present. The WMF specification is a bit
vague on what should happen here, but in the one test case in hand, it appears that if the placeable offset matches the
WINDOWORG offset then the former is supposed to be ignored.

9. Added support for CREATEPATTERNBRUSH records. So far I have never seen one of these in the wild, only DIBCREATEPATTERNBRUSH.

10. The work around for the offset when WMF files were opened wasn't in trunk. (It was for EMF). Reintroduced it.

Will post some examples next.

Revision history for this message
David Mathog (mathog) wrote :

This example shows the missing spaces change. Open before the patch, select all the text, paste, and you will find that there are no spaces. After the patch, the spaces are present in the text.

Revision history for this message
David Mathog (mathog) wrote :

This WMF file, which came from some other bug in Launchpad (but now I cannot find it again) used to crash trunk. With the new textout record support, and the fix for the placeable header with a nonzero offset, it opens correctly. Note this WMF assumes that clipping is working, and that is not supported. Consequently parts of the error bars can be seen extending leftward out of some of the graph regions and under the rotated Y axis text labels.

Revision history for this message
David Mathog (mathog) wrote :

Found it, the preceding example came from bug #1270990.

Revision history for this message
David Mathog (mathog) wrote :

Forgot one item. This patch also includes two valgrind suppression files: vg_fc.supp wcslen_sse2.supp

When valgrind is run like this:
 --suppressions=./wcslen_sse2.supp --suppressions=./vg_fc.supp

the first suppresses the wcslen_sse2 messages, and the second suppresses certain sets of messages about fontconfig. (The latter may indicate real memory issues, but these are outside of inkscape, so they don't belong in the valgrind output for an inkscape run.)

Revision history for this message
su_v (suv-lp) wrote :

On 2014-02-08 02:05 +0100, David Mathog wrote:
> 10. The work around for the offset when WMF files were opened wasn't
> in trunk. (It was for EMF). Reintroduced it.

Please clarify - does this refer to bug #1250250, and does this patch include parts of the last patch attached there? If it only addresses bug #1250250 for WMF, but not EMF - which other patch of yours already committed then included the fix for EMF?

Revision history for this message
su_v (suv-lp) wrote :

On 2014-02-08 02:27 +0100, David Mathog wrote:
> Forgot one item. This patch also includes two valgrind suppression
> files: vg_fc.supp wcslen_sse2.supp

I'm not going to include the two files when committing 'changes_2014_01_07b_emfonly.patch' for two reasons:
1) I'd rather avoid mixing different types of changes in one commit.
2) AFAIU one of the suppression files uses a hard-coded platform-, machine-architecture-, and SO-version-specific path which in this form doesn't appear useful if maintained under revision-control in the repository. Can these explicit paths be overridden with command line arguments when invoking valgrind, or are there more 'portable' solutions applicable to suppression files?

+ obj:/usr/lib/i386-linux-gnu/libfontconfig.so.1.4.4

I would recommend to discuss the details about adding valgrind suppression files to the repository with core developers on the mailing list first.

Revision history for this message
su_v (suv-lp) wrote :

'changes_2014_01_07b_emfonly.patch' (without valgrind suppression files) committed in r13008.

Revision history for this message
David Mathog (mathog) wrote :

re 40:

For bug #1250250 the patch changes_2013_11_12b.patch had analogous modifications for wmf-inout.cpp and emf-inout.cpp to work around the offset issue. Somewhere between then and now trunk ended up in a state where wmf-inout.cpp had lost that modification, but emf-inout.cpp retained it. I have not researched how that happened, the current patch just makes sure that the change is in for both of them.

Re 42:
Good point about the suppression files, will take that to the developers list.

Revision history for this message
David Mathog (mathog) wrote :

Hmm. Something has screwed up EMF/WMF image output in trunk. As of right now they are changing the number of bits per pixel. Will hunt that down ASAP. (Test, use test_libuemf_ref.emf file, read it in, write it out, read it back in again. The test images are all screwed up. The error is on EMF/WMF write, input is still working properly.)

Revision history for this message
su_v (suv-lp) wrote :

AFAICT the regressions with bitmap images when round-trip editing the EMF reference file is related to the changes in rev 13002:
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/13002>

(tests with archived builds up to r13001 produce expected results, r13002 and later revisions differ).

Revision history for this message
David Mathog (mathog) wrote :

New thread for the image issue in bug #1278645

13002 didn't change the lines I had to change to "fix" the problem for EMF/WMF, but that is no surprise since I thought the problem was probably elsewhere, and was just manifesting there.

Bryce Harrington (bryce)
Changed in inkscape:
status: Fix Committed → Fix Released
To post a comment you must log in.