Bug 84199 - [EMF] FILEOPEN Wrong Bezier curve display due to missing SetPolyfillMode implementation
Summary: [EMF] FILEOPEN Wrong Bezier curve display due to missing SetPolyfillMode impl...
Status: ASSIGNED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: graphics stack (show other bugs)
Version:
(earliest affected)
3.6.7.2 release
Hardware: Other All
: medium normal
Assignee: Hossein
URL:
Whiteboard:
Keywords: filter:docx, filter:emf
Depends on: 142021
Blocks: EMF-WMF 138147
  Show dependency treegraph
 
Reported: 2014-09-22 20:57 UTC by Yousuf Philips (jay) (retired)
Modified: 2022-04-27 14:48 UTC (History)
9 users (show)

See Also:
Crash report or crash signature:


Attachments
sample doc (94.01 KB, application/vnd.openxmlformats-officedocument.wordprocessingml.document)
2014-09-22 20:57 UTC, Yousuf Philips (jay) (retired)
Details
pdf export of the sample file in Word 2007 (66.97 KB, application/pdf)
2014-09-22 20:59 UTC, Yousuf Philips (jay) (retired)
Details
3.6.6.2 vs 3.6.7.2 screenshot (59.04 KB, image/png)
2014-10-20 20:43 UTC, tommy27
Details
Extracted EMF image with wrong bezier curve (3.23 KB, image/emf)
2021-05-13 10:44 UTC, Bartosz
Details
ink-sample.docx converted to PDF by LO 3.6.6.2 (48.81 KB, application/pdf)
2021-08-06 16:49 UTC, Hossein
Details
ink-sample.docx converted to PDF by LO 3.6.7.2 (48.81 KB, application/pdf)
2021-08-06 16:50 UTC, Hossein
Details
Minimal EMF image on which the issue is visible (636 bytes, image/emf)
2021-08-12 12:16 UTC, Bartosz
Details
Minimal EMF image exported to PNG via Paint (463 bytes, image/png)
2021-08-12 12:17 UTC, Bartosz
Details
Minimal EMF image without SetPolyfillMode record (624 bytes, image/emf)
2021-08-12 12:18 UTC, Bartosz
Details
Minimal EMF image without SetPolyfillMode record exported to PNG by Paint (483 bytes, image/png)
2021-08-12 12:19 UTC, Bartosz
Details
Minimal EMF file containing two circles overlapped, one is the result of saving another in LO Draw (1.30 KB, image/emf)
2021-08-13 13:56 UTC, Hossein
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Yousuf Philips (jay) (retired) 2014-09-22 20:57:38 UTC
Created attachment 106692 [details]
sample doc

Steps:
1) Open attached file
2) Notice that it is blank
3) Test same file in MS Word

Tested in 4.4 master on Linux.
Comment 1 Yousuf Philips (jay) (retired) 2014-09-22 20:59:06 UTC
Created attachment 106693 [details]
pdf export of the sample file in Word 2007
Comment 2 Yousuf Philips (jay) (retired) 2014-09-23 10:37:40 UTC
The author of the document stated that they created these hand writing objects using a Windows PC tablet with active digitizer pen, but that this could also be achieved with any touch screen using Word's pen tools.
Comment 3 tommy27 2014-10-20 20:42:06 UTC
tested under Win7x64
DocX reader 1.0 can read the file which is shown as blank in LibO 4.4.0.0+ master, 4.3.2.2 

I tested older releases and I can tell that image looks almost fine in 3.6.6.2, then image starts disappearing in 3.6.7.2 and is definitively gone in 4.0.4.2 (I don't have early 4.0.x releases to test)

here's the list of bugfixes in 3.6.7.2:
https://wiki.documentfoundation.org/Releases/3.6.7/RC1
https://wiki.documentfoundation.org/Releases/3.6.7/RC2

maybe we can see something suspect that caused the regression
Comment 4 tommy27 2014-10-20 20:43:17 UTC Comment hidden (obsolete)
Comment 5 Yousuf Philips (jay) (retired) 2014-10-20 22:36:04 UTC Comment hidden (obsolete)
Comment 6 Xisco Faulí 2014-10-21 10:37:28 UTC Comment hidden (obsolete)
Comment 7 Matthew Francis 2014-12-29 11:07:56 UTC
The behaviour appears to have changed across the below two commits

Adding Cc: to kendy@collabora.com. Could you possibly take a look at this? Thanks


commit 9d3c0aa1e64ae97ddc305df3873f977051f0b317
Author: Jan Holesovsky <kendy@collabora.com>
Date:   Wed Nov 20 21:17:50 2013 +0100

    Related fdo#61272: Revert "wmf-mm-text.diff: Fix WMF rendering, n#417818"
    
    This approach to WMF breaks EMF reading, need to revert it, and fix a
    different way.
    
    This reverts commit db1b08d217ebbdd1b0296e1da260bf314a77acf5.
    
    Conflicts:
        vcl/source/filter/wmf/winmtf.cxx
        vcl/source/filter/wmf/winmtf.hxx
    
    Change-Id: I8f779791153f2e1faa086c91b82b3e8b93304f3b

commit 198b17dc5e182dfb2e5c930458764c7b3e6c914f
Author: Jan Holesovsky <kendy@collabora.com>
Date:   Wed Nov 20 21:11:24 2013 +0100

    Related fdo#61272: Revert "wmf-mm-text-1.diff: Fix WMF rendering, n#417818"
    
    This approach to WMF breaks EMF reading, need to revert it, and fix a
    different way.
    
    This reverts commit 16eaa5e7c1208034bb3244fea9e6d9491ccb5501.
    
    Conflicts:
        vcl/source/filter/wmf/winmtf.cxx
    
    Change-Id: I59076d0a65d91ba3a1f3ebb48d8f7a542859d351
Comment 8 Chris Sherlock 2014-12-29 14:19:45 UTC
The Beziers in some of the embedded EMFs have 12 points! That's just not possible, because the number of points must be:

(number of curves x 3) + 1

As per the spec in [MS-EMF], see section 2.3.5.17.

Whether that is the issue, I don't know. But it does tend to indicate some issues with the EMF file, however we should actually just detect this and ignore the extraneous points in the Bezier.
Comment 9 Chris Sherlock 2014-12-29 14:27:40 UTC
Oh, I see. After that record there is a close figure record. Good to know about this, because I'll quote what Microsoft say in their own documentation about MS-EMF:

  2.3.5.17 EMR_POLYBEZIER16 Record

  Count (4 bytes): A 32-bit unsigned integer that specifies the total number of
  points. This value MUST be one more than three times the number of curves to be
  drawn, because each Bezier curve requires two control points and an endpoint,
  and the initial curve requires an additional starting point.

They neglected to mention that if you have this close figure record directly after then it acts as the extra point they mentioned - you need to use the first point as the end-point.
Comment 10 Robinson Tryon (qubit) 2015-12-10 07:40:10 UTC Comment hidden (obsolete)
Comment 11 Xisco Faulí 2016-09-10 15:57:35 UTC Comment hidden (obsolete)
Comment 12 Xisco Faulí 2016-09-26 17:09:06 UTC Comment hidden (obsolete)
Comment 13 QA Administrators 2017-11-12 11:01:32 UTC Comment hidden (obsolete)
Comment 14 Timur 2020-09-15 07:14:02 UTC Comment hidden (obsolete)
Comment 15 Bartosz 2021-05-13 10:44:56 UTC
Created attachment 171957 [details]
Extracted EMF image with wrong bezier curve
Comment 16 Hossein 2021-08-06 16:45:35 UTC Comment hidden (obsolete)
Comment 17 Hossein 2021-08-06 16:49:32 UTC
Created attachment 174119 [details]
ink-sample.docx converted to PDF by LO 3.6.6.2
Comment 18 Hossein 2021-08-06 16:50:08 UTC
Created attachment 174120 [details]
ink-sample.docx converted to PDF by LO 3.6.7.2
Comment 19 Hossein 2021-08-06 16:56:52 UTC
(In reply to tommy27 from comment #4)
> Created attachment 108136 [details]
> 3.6.6.2 vs 3.6.7.2 screenshot

The "3.6.6.2 vs 3.6.7.2 screenshot" is misleading, because the display of shapes is nearly identical in both versions, when zooming in enough.

Please look at attachment 174119 [details] and attachment 174120 [details], which are created using RPM versions of official LO binaries:

https://downloadarchive.documentfoundation.org/libreoffice/old/3.6.6.2/rpm/x86_64/

https://downloadarchive.documentfoundation.org/libreoffice/old/3.6.7.2/rpm/x86_64/
Comment 20 Hossein 2021-08-06 16:59:38 UTC Comment hidden (obsolete)
Comment 21 Hossein 2021-08-07 14:29:00 UTC
(In reply to Hossein from comment #16)

> there are actually 2 problems:
> 
> 1. In some versions of LO, the shape does not display at all (blank display)
>    + This problem seems to be fixed in the recent versions
> 
> 2. In most versions of LO, the shape displays with some holes inside it
> (wrong display)
>    + This problem still exists.
I've created minimal examples out of attachment 106692 [details], and it seems that the second problem is actually the lack of SetPolyfilMode implementation. There are 2 modes for PolygonFillMode:

Alternate = 1
Winding = 2

First one is implemented, and the second one is ignored.

So, the remaining problem is actually tdf#142021
Comment 22 Xisco Faulí 2021-08-11 09:23:53 UTC Comment hidden (obsolete)
Comment 23 Xisco Faulí 2021-08-11 09:25:37 UTC
(In reply to Xisco Faulí from comment #22)
> (In reply to Hossein from comment #20)
> > (In reply to Xisco Faulí from comment #6)
> > > bibisected:
> > 
> > This bibisect is not useful, because there is no commit-by-commit bisect
> > repository for LibreOffice 3. Also, it is not obvious what symptom is
> > actually the subject of this bibisect.
> 
> the issue was introduced in this range:
> https://cgit.freedesktop.org/libreoffice/core/log/
> ?qt=range&q=6ab3148d965be12592e8927b4c7868634e714932..
> 1581b1fc3ac82a7bd62df968226e98604a4ca52d

Commits from Jan Holesovsky in the range look related
Comment 24 Hossein 2021-08-12 10:55:43 UTC
> (In reply to Hossein from comment #16)
> 
> > there are actually 2 problems:
> > 
> > 1. In some versions of LO, the shape does not display at all (blank display)
> >    + This problem seems to be fixed in the recent versions
> > 
> > 2. In most versions of LO, the shape displays with some holes inside it
> > (wrong display)
> >    + This problem still exists.
> I've created minimal examples out of attachment 106692 [details], and it
> seems that the second problem is actually the lack of SetPolyfilMode
> implementation. There are 2 modes for PolygonFillMode:
> 
> Alternate = 1
> Winding = 2
> 
> First one is implemented, and the second one is ignored.
> 
> So, the remaining problem is actually tdf#142021
The first problem is fixed now, and the source of the 2nd problem is known: lacking in EMF feature SetPolyfilMode in import/export/display, and it is not a regression. I'm working on it, and I hope I can fix it soon.

The problem is more clearly discussed in tdf#142021.

Xisco, Do you want to call this a duplicate, or mark this one as resolved (no longer blank display), and continue work on tdf#142021?
Comment 25 Bartosz 2021-08-12 12:16:49 UTC
Created attachment 174225 [details]
Minimal EMF image on which the issue is visible
Comment 26 Bartosz 2021-08-12 12:17:31 UTC
Created attachment 174226 [details]
Minimal EMF image exported to PNG via Paint
Comment 27 Bartosz 2021-08-12 12:18:36 UTC
Created attachment 174227 [details]
Minimal EMF image without SetPolyfillMode record
Comment 28 Bartosz 2021-08-12 12:19:30 UTC
Created attachment 174228 [details]
Minimal EMF image without SetPolyfillMode record exported to PNG by Paint
Comment 29 Bartosz 2021-08-13 13:40:05 UTC
The record responsible for drawing is EMR_FILLPATH:
https://github.com/LibreOffice/core/blob/master/emfio/source/reader/emfreader.cxx#L1430
and
https://github.com/LibreOffice/core/blob/master/emfio/source/reader/mtftools.cxx#L1310

which is using DrawPolyPolygon method to draw the polygons
https://github.com/LibreOffice/core/blob/master/vcl/source/outdev/polygon.cxx#L36

@Hossein Did you find which Primitive should be used for control of Filling mode?
Comment 30 Hossein 2021-08-13 13:56:42 UTC
Created attachment 174258 [details]
Minimal EMF file containing two circles overlapped, one is the result of saving another in LO Draw

@Bartosz
Thank you for uploading the minimal examples. My minimal examples were mostly similar to what you've created, but at last I removed everything and kept only one circle.
Other than that, I saved an EMF with LibreOffice, then opened the original and the changed file in Inkscape, and then put both of them at the exact same position. In this way, I could see the differences, that was 1-2 pixel in positions, and then I found the difference in SetPolyfillMode. It is currently ignored by LO.

You can check for yourself by invoking 'printemf minimal5-mix.emf', and see the differences.

https://github.com/LibreOffice/core/blob/master/emfio/source/reader/emfreader.cxx#L1033

case EMR_SETPOLYFILLMODE :
break;
Comment 31 Hossein 2021-08-13 14:08:40 UTC
(In reply to Bartosz from comment #29)
> The record responsible for drawing is EMR_FILLPATH:
> https://github.com/LibreOffice/core/blob/master/emfio/source/reader/
> emfreader.cxx#L1430
> and
> https://github.com/LibreOffice/core/blob/master/emfio/source/reader/mtftools.
> cxx#L1310
> 
> which is using DrawPolyPolygon method to draw the polygons
> https://github.com/LibreOffice/core/blob/master/vcl/source/outdev/polygon.
> cxx#L36
> 
> @Hossein Did you find which Primitive should be used for control of Filling
> mode?

To make sure that the problem is the SetPolyfillMode, I temporarily changed the fill mode in the Cairo back-end, and the problem went away: There was no hole in the handwriting anymore. Check it for yourself:

Look at SvpSalGraphics::getCairoContext(), and change this:

    cairo_set_fill_rule(cr, CAIRO_FILL_RULE_EVEN_ODD);

to this one:

    cairo_set_fill_rule(cr, CAIRO_FILL_RULE_WINDING);

Yes, the DrawPolyPolygon method is doing most of the rendering of the handwriting shapes here. I am working on a patch to fix the problem. My approach to fix this bug is to understand what svgio does for the FillRule (comparable to FillMode here) and re-create it in emfio. I am uploading a WIP patch to Gerrit soon.
Comment 32 Bartosz 2021-08-16 10:03:29 UTC
Nice catch Hossein!

It seems that FillRule could be set by using SvtGraphicFill:
https://git.libreoffice.org/core/+blame/master/emfio/source/reader/mtftools.cxx#1589

It could be done through constructor:

https://git.libreoffice.org/core/+blame/master/include/vcl/graphictools.hxx#203

    SvtGraphicFill( const tools::PolyPolygon&  rPath,
                    Color               aFillColor,
                    double              fTransparency,
                    FillRule            aFillRule,
                    FillType            aFillType,              // TODO: Multitexturing
                    const Transform&    aFillTransform,
                    bool                bTiling,
                    HatchType           aHatchType,             // TODO: vector of directions and start points
                    Color               aHatchColor,
                    GradientType        aGradientType,          // TODO: Transparent gradients (orthogonal to normal ones)
                    Color               aGradient1stColor,      // TODO: vector of colors and offsets
                    Color               aGradient2ndColor,
                    sal_Int32           aGradientStepCount,     // numbers of steps to render the gradient. gradientStepsInfinite means infinitely many.
                    const Graphic&      aFillGraphic );
Comment 33 Bartosz 2022-02-07 00:54:40 UTC
The ongoing Changes during review: 
https://gerrit.libreoffice.org/c/core/+/120477