Bug 89901 - Insertion of DXF image fail to produce a correct image
Summary: Insertion of DXF image fail to produce a correct image
Status: VERIFIED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: Writer (show other bugs)
Version:
(earliest affected)
4.2.0.4 release
Hardware: All All
: medium major
Assignee: Armin Le Grand
URL:
Whiteboard: target:7.0.0 target:6.4.4
Keywords: bibisected, bisected, regression
Depends on:
Blocks: Regressions-GraphicPrimitive2D
  Show dependency treegraph
 
Reported: 2015-03-09 09:29 UTC by Tomas
Modified: 2020-03-27 11:36 UTC (History)
6 users (show)

See Also:
Crash report or crash signature:


Attachments
3.5 DXF insertion results (378.80 KB, application/pdf)
2015-03-09 09:29 UTC, Tomas
Details
4.4.1.2 DXF file insertion results (510.79 KB, application/pdf)
2015-03-09 09:30 UTC, Tomas
Details
Example DXF file used to produce the results (1.04 MB, application/x-zip-compressed)
2015-03-09 09:32 UTC, Tomas
Details
The metafile, extraccted from saved ODG file (1.69 MB, application/octet-stream)
2020-03-19 15:55 UTC, Armin Le Grand
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas 2015-03-09 09:29:10 UTC
Created attachment 113988 [details]
3.5 DXF insertion results

In version 3.5 it is possible to insert a DXF image from a file, see attached example. I earlier got a correction of a problem regarding export to PDF of such an image, see bug #64400. Now I encounter a problem again when trying to produce a pdf document from a dxf file. This time the majority of the entities in the DXF file is missing on the inserted image. Therefore, the problem has nothing to do with PDF export. Instead the problem seems to be related to the import of an image on DXF-format.

I have attached:
1. Dxf file to insert as an image.
2. Results in version 3.5.
3. Results in version 4.4.1.2

The problem is easy to reproduce. Just insert image and select the attached DXF file. Compare the excellent behavior in version 3.5 with the dispappointing appearance in version 4.4.1.2.
Comment 1 Tomas 2015-03-09 09:30:42 UTC
Created attachment 113989 [details]
4.4.1.2 DXF file insertion results
Comment 2 Tomas 2015-03-09 09:32:50 UTC
Created attachment 113990 [details]
Example DXF file used to produce the results

Insertion fails in version 4.4.1.2. Insertion succeeds in version 3.5.
Comment 3 Tomas 2015-03-09 09:34:51 UTC
The same problematic behavior has been observed both on Redhat 6 and on Windows 7.
Comment 4 A (Andy) 2015-03-14 08:03:50 UTC
Reproducible with LO 4.4.1.2, Win 8.1

WRITER and DRAW do not open/insert it correctly.
Comment 5 Matthew Francis 2015-03-20 05:54:46 UTC
Bibisect results from 43all:

8aabf2aee6514311020b855a95a6e44bab3a5b0d is the first bad commit
commit 8aabf2aee6514311020b855a95a6e44bab3a5b0d
Author: Bjoern Michaelsen <bjoern.michaelsen@canonical.com>
Date:   Wed Nov 27 09:32:23 2013 +0000

    source-hash-0aa9ced531b8d85ad067c1d156a9708eea628d78



Not 100% sure, but the most likely in that range is probably:

commit 2e5167528f7566dd9b000e50fc1610b7bf99132a
Author:     Armin Le Grand <alg@apache.org>
AuthorDate: Thu Oct 31 14:43:21 2013 +0000
Commit:     Caolán McNamara <caolanm@redhat.com>
CommitDate: Tue Nov 5 15:24:18 2013 +0000

    Resolves: #i123500# unified Graphic processing to use GraphicPrimitive2D
    
    (cherry picked from commit f5d69b2b8b002ca6905496a9d9065ef76b5641d7)
    
    Conflicts:
        sw/source/core/doc/notxtfrm.cxx
    
    Change-Id: I1758aadcbe97ece271277378e62300b895421768
Comment 6 Matthew Francis 2015-04-15 02:15:18 UTC
Built to check - commit 2e5167528f7566dd9b000e50fc1610b7bf99132a is indeed the culprit
Comment 7 Robinson Tryon (qubit) 2015-12-13 11:12:03 UTC Comment hidden (obsolete)
Comment 8 Björn Michaelsen 2016-08-03 19:18:40 UTC
@Armin: Care to have a look maybe?
Comment 9 Xisco Faulí 2016-09-26 15:03:07 UTC
Adding Cc: to Armin Le Grand
Comment 10 Armin Le Grand 2016-09-29 08:30:44 UTC
I have no idea how dxf worked before, but if it worked in LO and not in AOO and it was added in LO only for Writer and not in the general primitive case for GraphicObject, it might have been removed. Would need to search in older versions how dxf was imported/interpreted and add that more globally to GraphicPrimitive decomposition/rendering, that would make dxf work in all apps immediately.
Comment 11 Armin Le Grand 2016-09-29 15:56:13 UTC
Looks like dxf is converted to metafile and this is not painted well. When breaking it up (set BP at MIN_ACTIONS_FOR_DIALOG in drviews2.cxx due to the Break dialog not working, force to DoImportMarkedMtf) all geometry appears. This means that the import to metafile is pretty good, just the rendering is not.
For a long and good quality fix, imort to primitives would be good. For a fast fix, rendering has to be checked.
Comment 12 Armin Le Grand 2016-09-29 15:56:59 UTC
...and not to forget: The Break() dialog needs to be fixed. Do we have a task for that?
Comment 13 Armin Le Grand 2016-09-30 10:36:13 UTC
Created Bug 102852 for the BreakDlg problem
Comment 14 QA Administrators 2017-10-23 13:59:58 UTC Comment hidden (obsolete)
Comment 15 QA Administrators 2020-03-01 03:01:04 UTC Comment hidden (obsolete)
Comment 16 Armin Le Grand 2020-03-19 15:45:58 UTC
The DXF gets imported by DXF2GDIMetaFile::Convert to a metafile. That has a size of 2,79 x 3,94 or internally { LT=[0 , 0] RB=[7098 , 10000] [7099 x 10001] } with 25958 entries. Ths is interpreted as 100thmm, so the foloating point numbers from the DXF are gone. Problem is that most items have just 2-5 units (100thmm) difference and thus are *tiny*. Example (from dump):

 <line startx="3148" starty="5658" endx="3145" endy="5658" style="solid" width="0" dashlen="0" dashcount="0" dotlen="0" dotcount="0" distance="0" join="round" cap="butt"/>
 <line startx="3145" starty="5659" endx="3148" endy="5659" style="solid" width="0" dashlen="0" dashcount="0" dotlen="0" dotcount="0" distance="0" join="round" cap="butt"/>
 <polyline style="solid" width="0" dashlen="0" dashcount="0" dotlen="0" dotcount="0" distance="0" join="round" cap="butt">
  <point x="3179" y="5664"/>
  <point x="3194" y="5664"/>
  <point x="3194" y="5665"/>
  <point x="3189" y="5665"/>
  <point x="3188" y="5666"/>
 </polyline>

I will try to extract the  metafile...
Comment 17 Armin Le Grand 2020-03-19 15:55:39 UTC
Created attachment 158820 [details]
The metafile, extraccted from saved ODG file
Comment 18 Armin Le Grand 2020-03-19 16:25:17 UTC
Inserting/opening that metafile creates (of course) the same result.

First problem is DXF2GDIMetaFile. It should/may be changed to convert to primitives if no save file is needed. If needed, maybe SVG is a good alternative. For a BugFix like this is too much work, though.

Still - when breaking up the graphic, geometry is there. Thus, analyzing situation...
Comment 19 Armin Le Grand 2020-03-19 16:36:47 UTC
There are two break-up scenarios:

(a) wmfemfhelper::interpretMetafile used from MetafilePrimitive2D::create2DDecomposition at paint time

(b) ImpSdrGDIMetaFileImport used by SdrEditView::DoImportMarkedMtf to convert to SdrObjects

While (b) creates 23653 SdrObjects which do show the geometry (astounding with the low integer data with very small sizes) closer looking reveals the bad quality. Exporting to PDF also. Still, there is data to be seen.

Indeed (a) also imports and creates all the data as primitives, but for *some* reason these do not get painted. It may be
- tryDrawPolygonStrokePrimitive2DDirect uses DrawPolyLineDirect and that OutputDevice::DrawPolyLine and that OutputDevice::DrawPolyLineDirect and that sets bPixelSnapHairline which makes e.g. WinSalGraphicsImpl::drawPolyLine execute impPixelSnap. That again does transform to discrete (pixel) space, snaps, and converts back. For small objects, all point coordinates get the same for mentioned small distances
- There may be other reasons, need to investigate (hard with a metafile with 25000 actions :-)
Comment 20 Armin Le Grand 2020-03-19 17:49:15 UTC
Indeed bPixelSnapHairline leads to matching coordinates, but when supressing it still no geometry drawn - sigh...
Comment 21 Armin Le Grand 2020-03-20 17:53:03 UTC
After trying massive and many changes, debugging deep and wide, I started to doubt that std::deque now used in primitive2d::Primitive2DContainer works - and tried

        for(size_t nAction(0); nAction < nCount && rTargetHolders.Current().size() < 5000; nAction++)

in implInterpretMetafile to limit the number of created primitives - and see there - it works! WTF...
Also tried in-between in BaseProcessor2D::process other loops:

- temp copy in vetor: does not work...
                const std::vector<primitive2d::Primitive2DReference> aTemp(rSource.begin(), rSource.end());

- loop using 'for(const primitive2d::Primitive2DReference& rrRef : rSource)': does not work...

Thus the motor block parts shown *are* the last parts of the 23653 primitives being created - sigh.

This was changted to std::deque in commit cd47cf36ebb70a3d6897b0abbb0b3cd8b290e1af. It was a std::deque before as I could see, but since this was moved to new files it is hard to check who and when and why changed to std::deque. Since I ninitially developed that stuff I am pretty sure it was not a std::deque initially...

Anyways, of course std::deque should work. It is still possible that it's an error in one of the in-between stages, e.g. wmfemfhelper::TargetHolder uses a

        std::vector< std::unique_ptr<drawinglayer::primitive2d::BasePrimitive2D> > aTargets;

as temporary creation target. Of course it is not acceptable that any std::vector like stl construct holds less than 23653 entries - but now how to find the reason that stuff is lost in-between somewhere...?
Comment 22 Armin Le Grand 2020-03-20 18:32:26 UTC
Tried with binary, works with 25401, breaks with 25402 when using

        const size_t nMax(16384 + 5000 + 2500 + 1250 + /*625 + 310*/ + 155 + 77 + /*38*/ + 19 + 10 + 5 + /*2*/ + 1);
        for(size_t nAction(0); nAction < nCount && rTargetHolders.Current().size() < nMax; nAction++)

in implInterpretMetafile as limit for created primitives.
Tried next to check for

                    if(!xReference.is())

in BaseProcessor2D::process to see if there are empty slots in these 26599 primitives - no, all are interpreted as set. This means that for the visible motor parts, these get returned multiple times.
There are hints in https://en.cppreference.com/w/cpp/container/deque:

"As opposed to std::vector, the elements of a deque are not stored contiguously: typical implementations use a sequence of individually allocated fixed-size arrays, with additional bookkeeping, which means indexed access to deque must perform two pointer dereferences, compared to vector's indexed access which performs only one."

Thus an error in std::deque acessing the wrong partial arays seems a possibility...
Comment 23 Xisco Faulí 2020-03-23 09:17:38 UTC
@Noel, @Stephan, any comment regarding Armin's observations in comment 21 and comment 22 ?
Comment 24 Noel Grandin 2020-03-23 09:52:12 UTC
It is extraordinarily unlikely that std::deque has a bug. The STL is incredibly broadly tested.

For the record, I did the conversion to std::deque, because it showed perf increases when drawing very complex diagrams, since 
  (a) we use that particular list as an append-only data structure
  (b) we almost never index into it, rather using iterators over it
which is exactly the use-case for std::deque.

Possibly we could switch to importing these as FieldUnit::PIXEL instead of FieldUnit::MM_100TH, and then set a custom DPI on the image to get the right physical size. That would allow us to use the full range of our integer values.
Comment 25 Armin Le Grand 2020-03-24 15:34:27 UTC
Just tried (not too complicated) to go back to std::vector - and it's the same. That's good - I would've been very nervous seeing such a bug in stl - sigh...
Still - what happens on that 25401 to 25402 mark...? This may get interesting...

@Noel: Better change DL to double/transformations, I did some already in AW080 and this may be good stuff for some Tenders (will be too much for one...)
Comment 26 Armin Le Grand 2020-03-24 18:16:21 UTC
Debugged and debugged, no error found yet. But when commenting out line 584 in drawinglayer\source\tools\wmfemfhelper.cxx

//            createFillPrimitive(basegfx::B2DPolyPolygon(rPolygon), rTarget, rProperties);

which creates element 25402 all lines are shown. Of course filled polys are missing (only a view), but why does creating drawinglayer::primitive2d::PolyPolygonColorPrimitive2D change the behaviour? Poly in question is

 <polygon>
  <point x="47" y="9953"/>
  <point x="7051" y="9953"/>
  <point x="7051" y="47"/>
  <point x="47" y="47"/>
  <point x="47" y="9953"/>
 </polygon>

which has nothing special...
Comment 27 Armin Le Grand 2020-03-24 18:37:57 UTC
Tried to create PolyPolygonGradientPrimitive2D instead of PolyPolygonColorPrimitive2D, but (after switching off skia under Win which needs ages to draw gradients) no diff - still missing stuff - no idea yet
Comment 28 Armin Le Grand 2020-03-25 16:38:27 UTC
Oh boy - sometimes your mind is just so blocked...
After having debugged all those variants of RefCounting/uniquePtr's and related and finding no error it stroke me like a flash (in a moment far away from my machine, btw...): This has nothing to do with objects/primitives being lost/wrong accessed. It's much simpler - the object 25402 *is* as follows:

 <polygon>
  <point x="0" y="10000"/>
  <point x="7098" y="10000"/>
  <point x="7098" y="0"/>
  <point x="0" y="0"/>
  <point x="0" y="10000"/>
 </polygon>

with the last pen defining action having been (very early)

 <fillcolor color="#ffffff"/>

which is interpreted as white. Thus a rect in dxf-import-page-size gets painted filled white -> all stuff painted before is overpainted. It's a geometrical problem, not an obj-lifetime/mem-or-whatever one.

So why is this different in Breaking object? Because the color in the MetaFillColorAction is not 'white' as dumped above, but

0xffffffff

which equals the (old) COL_TRANSPARENT definition. Thus, OutputDevice switches *off* filling polygons when this gets set (see OutputDevice::SetFillColor). It works due to DXF-to-metafile converter using a 'real' but unused-for-paint VDev instance just for holding states which does that while the primitive creator uses local states independent from OutputDevice stuff - to be more future-proof - and misses this. Sigh. Those small, grown details...

I was tricked by (a) the dump of metafile saying #ffffff and (b) the sheer size of the file in processing, so was hard to see/find.

Now it's easy, doing the fix...
Comment 29 Noel Grandin 2020-03-25 16:43:01 UTC
Very good work Armin!
Comment 30 Commit Notification 2020-03-25 17:47:31 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/5b14a18f4748c7b09b432cfae5577c966213a80b

tdf#89901 corectly interpret COL_TRANSPARENT

It will be available in 7.0.0.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.
Comment 31 Xisco Faulí 2020-03-27 10:16:38 UTC
Verified in

Version: 7.0.0.0.alpha0+
Build ID: 5a94ac9eec4a63708262b2389aa2a434aa47112e
CPU threads: 4; OS: Linux 4.19; UI render: default; VCL: gtk3; 
Locale: en-US (en_US.UTF-8); UI-Language: en-US
Calc: threaded

@Armin, thanks for fixing this issue!!
Comment 32 Commit Notification 2020-03-27 11:36:54 UTC
Armin Le Grand committed a patch related to this issue.
It has been pushed to "libreoffice-6-4":

https://git.libreoffice.org/core/commit/4f8325dbcb63627997289889a377a4893e03fcf1

tdf#89901 corectly interpret COL_TRANSPARENT

It will be available in 6.4.4.

The patch should be included in the daily builds available at
https://dev-builds.libreoffice.org/daily/ in the next 24-48 hours. More
information about daily builds can be found at:
https://wiki.documentfoundation.org/Testing_Daily_Builds

Affected users are encouraged to test the fix and report feedback.