Bug 130150 - Broken clipping in PDF export
Summary: Broken clipping in PDF export
Status: RESOLVED FIXED
Alias: None
Product: LibreOffice
Classification: Unclassified
Component: graphics stack (show other bugs)
Version:
(earliest affected)
5.3.5.2 release
Hardware: All All
: medium normal
Assignee: Thorsten Behrens (allotropia)
URL:
Whiteboard: target:7.0.0 target:6.4.3
Keywords: bibisectNotNeeded, filter:pdf, regression
Depends on:
Blocks:
 
Reported: 2020-01-23 19:20 UTC by Jan-Marek Glogowski
Modified: 2020-03-17 10:22 UTC (History)
3 users (show)

See Also:
Crash report or crash signature:


Attachments
Broken clipping in non-form PDF export shows Markierfeld titles (9.16 KB, application/vnd.oasis.opendocument.text)
2020-01-23 19:20 UTC, Jan-Marek Glogowski
Details
Wrongly clipped check boxes in exported PDF. (12.54 KB, application/pdf)
2020-01-23 19:23 UTC, Jan-Marek Glogowski
Details
Some debug patch including a basegfx test. (11.86 KB, patch)
2020-01-23 19:27 UTC, Jan-Marek Glogowski
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Jan-Marek Glogowski 2020-01-23 19:20:35 UTC
Created attachment 157375 [details]
Broken clipping in non-form PDF export shows Markierfeld titles

When converting the attached ODT to PDF, for example via command line like:

soffice --convert-to pdf --outdir . broken_pdf_clipping.odt

or in Writer using PDF export *without* selecting "create PDF form", the resulting PDF document looses the clipping of the existing checkbox titles.

Reverting the patch from bug 99680 fixes this problem.

The history of the problem is something like:

- commit 581806182ac7 ("tdf#99680 modified clipping for PDF export")
- commit 86b47f5138c0 ("tdf#44388: handle the NULL clip correctly for pdf output")
- commit a334752d091f ("vcl109: #i65128# avoid breaking polygonal clip regions into rectangles in PDF export").

The latest patch changed the semantics of the clip region parameters:
- m_bClipRegion = true
- m_aClipRegion.count() == 0

from "// NULL clip, i.e. nothing visible" to "no clipping applied".

This inherently conflicts with the code of PDFWriterImpl::intersectClipRegion, which "intersects clip regions". When intersecting clip regions / polygons, the result might be an empty intersection, but that doesn't mean, no clipping, but nothing visible.

In the attached bug document that's the clip of the check box inside the clip of the table.

Obviously the revert results in bug 99680 again. Inspecting that original bug shows rounding problems with the simple test document attached to that bug (attachment 133268 [details]), resulting in an empty clip region for the first stack object from calling basegfx::utils::solvePolygonOperationAnd in PDFWriterImpl::intersectClipRegion.

And in the end it's also the origin of bug 113449, which is also about rounding problems in intersectClipRegion, but in an other place in the call stack. Both bugs AFAIK fail in stripDispensablePolygons. This one in B2DRange::isInside, the other one in basegfx::utils::isInside, used to detect the case of polygons inside each other. Since these fail, no intersection is detected and the result is empty, or - with tdf#99680 applied - completely unclipped / visible.

Bug 113449 claims "there is no problem with the commit [581806182ac7]", but from my analysis, the patch is wrong, or I misunderstood that analysis.

Still I currently also have no good idea where to fix it and do the rounding to fix the comparison. This is some subtle rounding problem in the basegfx based stack. And it's definitely no easy hack from the analysis point of view, even when a real fix might be as simple as a single round or an adapted comparison.
Comment 1 Jan-Marek Glogowski 2020-01-23 19:23:05 UTC
Created attachment 157376 [details]
Wrongly clipped check boxes in exported PDF.
Comment 2 Jan-Marek Glogowski 2020-01-23 19:27:07 UTC
Created attachment 157377 [details]
Some debug patch including a basegfx test.
Comment 3 Samuel Mehrbrodt (allotropia) 2020-01-31 13:17:01 UTC
> Created attachment 157375 [details]
> Broken clipping in non-form PDF export shows Markierfeld titles

Note: The text that appears in the PDF export is the label of the checkboxes.
To see that in Writer, switch "Form->Design Mode" on, then resize the checkbox to make it larger.

LibreOffice 3.4.0 already had that bug.
Comment 4 Armin Le Grand 2020-03-02 08:27:37 UTC
Taking a look...
Comment 5 Armin Le Grand 2020-03-02 08:59:37 UTC
Would've been nice to mention reverting which commit of bug 99680 helps here, but seems the one from comment 43 ( 581806182ac7da81115c9675b4e828688178aa04). Also needed to check what 'Q'/'q' op's in PDF - do, see 4.4.3 'Cipping Path Operators' in https://www.adobe.com/content/dam/acom/en/devnet/pdf/pdfs/pdf_reference_archives/PDFReference.pdf
Will now check tdf#99680 - seems indeed that that's the one that was fixed wrong...?
Comment 6 Armin Le Grand 2020-03-02 09:51:52 UTC
During debugging tried to reduce bugdoc further, but crashes when ending design mode, select one of the contriols & using ControlProperties... from context menu. Still reduced, checking source of the clip region...
Comment 7 Xisco Faulí 2020-03-02 10:01:29 UTC
(In reply to Armin Le Grand from comment #6)
> During debugging tried to reduce bugdoc further, but crashes when ending
> design mode, select one of the contriols & using ControlProperties... from
> context menu. Still reduced, checking source of the clip region...

Crash not reproduced in

Version: 7.0.0.0.alpha0+
Build ID: 950e1aec0a984ce40a5038331f491272b51d41fa
CPU threads: 4; OS: Linux 4.19; UI render: default; VCL: x11; 
Locale: en-US (en_US.UTF-8); UI-Language: en-US
Calc: threaded
Comment 8 Armin Le Grand 2020-03-02 10:33:59 UTC
Checking PDF creator
- 1st clip is PageRect (PDFExport::ImplExportPage). creates PDFWriterImpl::updateGraphicsState indirect by action#17 (of 85) -> as expected
- 2nd, 3rd, 4th are MetaISectRegionClipRegionAction, action #22, #46, #50
- 5th, 6th MetaISectRectClipRegionAction, #61, #69
- PDFWriterImpl::updateGraphicsState indirect by action#70 (MetaTextArrayAction)
- PDFWriterImpl::updateGraphicsState indirect by action#76 (MetaRectAction)
Very strange stuff - we have five different ClipRegio-actions in a 88-action metafile. The (MetaTextArrayAction) trigger indeed has an empty clip-PolyPolyon what together with having a clip region *should* mean to clip everyting -> paint nothing. Seems correct and like a proof that tdf#99680 fix was more a shot in the dark, hoping it works...
Comment 9 Armin Le Grand 2020-03-02 11:21:52 UTC
Checked creations:
6x ObjectContactOfPageView::DoProcessDisplay/SdrPageView::DrawLayer
3x CheckBox::ImplDraw
4x OutputDevice::ImplDrawText/Control::DrawControlText
Last one is dubious: Always the same rect, but right < left - old Rect problem (that this is even possible). Not corrected, goes in that form in the clip action. Strange, but never touched, so might be unimportant.
OTOH this *will* influence the orientation of the polyPolygon when used for And/Or clipping stuff - so might be a hint on the problem...
Comment 10 Armin Le Grand 2020-03-02 11:34:11 UTC
Note for ::Region: There is IsEmpty() aka all gets clipped, and there is IsNull() for nothing gets clipped - should usually be represented by setting no Region at all.
Comment 11 Armin Le Grand 2020-03-02 13:25:05 UTC
Jmux (see description)was pretty good aleady. Conclusion:
- Need to revert tdf#99680
- Fix it differently, including e.g. tdf#113449
-> Numerical stuff - sigh...
Deep-diving into tdf#113449 findings right now...
Comment 12 Armin Le Grand 2020-03-02 14:51:10 UTC
Have now reconstructed the case described in tdf#113449 - some numbers were missing - or better: The dumped deltas in the two regions are shortened by dumping, so not too useful.
Alternaitives indeed are:
(1) Make isPointOnPolygon trigger earier/unsharp
(2) Make isInside decisions for bCompXA/bCompXB (and also bCompYA/bCompYB) less unsharp by *not* using fTools::more which internally uses rtl::math::approxEqual. The involved numbers

approxEqual(3075.0236220472434, 3075.0236220472439) -> true

trigger - just in the *last* condition of it which is

    return (d < a * e48 && d < b * e48);

so good at e44 precision, but *not* on e48 precision...
Have to think about it - changes here may have deep impact...
Comment 13 Armin Le Grand 2020-03-02 17:00:37 UTC
Tested some alternatives for basegfx::utils::isPointOnLine:
- cross product direct & with normed vectors
- area calculation -> close to zero -> point on line
- edge length diff -> close to zero -> point on line
- use hypot()/sqrt, use squared distances (also goes to zero, but in squared param spaces)
2nd thought is to use isPointOnLine additionally in 2nd half of isInside 'somehow' - more as reference for trigger than anything else, thus another name but alow check if would trigger...
Comment 14 Armin Le Grand 2020-03-02 19:27:54 UTC
Checked bugdoc from tdf#99680 - much better results, but still *some* missing graphics - need to check deeper...
Comment 15 Armin Le Grand 2020-03-03 08:18:12 UTC
Checking bugdocs/descriptions from tdf#44388...
Comment 16 Armin Le Grand 2020-03-03 08:57:24 UTC
All cases from tdf#44388 work well. Still rare bad cases (much rarer than before, but happens). Trying to find/construct a reproducible case...
Comment 17 Armin Le Grand 2020-03-03 10:30:16 UTC
No reliably reproducable case yet - numerical stuff :-(
Preparing changes for commit, it's a solution working in most cases...
Comment 18 Armin Le Grand 2020-03-03 10:52:31 UTC
Current decent but not complete solution at https://gerrit.libreoffice.org/c/core/+/89874, see there.
Thinking about it, esp. I had started to replace that stuff with two big changes - (1) not relying on orientation and thus speeding up sorting out significantly and (2) improved and completely overhauled cutsAndTouches stuff - the backbone for clipping PolyPolygons.
That part wopuld probably fix this - the basic reason is that And-Op with PolyPolygons is not always reliable for numerical reasons - and improve it. I would guess I would need 2-3 days to complete that and - due to this being relevant and needs care - some days for testing and to ensure stability...
Comment 19 Armin Le Grand 2020-03-03 12:53:30 UTC
CppunitTest_vcl_pdfexport fails - sure, it checks for no empty clipping regions which is now wrong. There are no empty clipping regions allowed (in principle the output might also be blocked when this is the case, but that would be a bigger change).
Have to adapt that as needed, will comment there: PdfExportTest::testTdf99680_2()
Comment 20 Armin Le Grand 2020-03-03 12:55:05 UTC
... and PdfExportTest::testTdf99680(), too...
Comment 21 Armin Le Grand 2020-03-03 16:25:57 UTC
Have now nailed a case where context is lost, it's a writer doc with an embedded OLE where the OLE is a draw document. As many now know writer MapMode is Twip, Draw is 100thMM. Indeed numerical inconsistencies trigger, the MapModes are:

{meUnit=MapPoint (8) maOrigin=X:0, Y:0 maScaleX={mnNumerator=1 mnDenominator=1000 mbValid=true } ...}

and

{mpImplMapMode={m_pimpl=0x0000025d2f08f490 {m_value={meUnit=MapTwip (9) maOrigin=X:1506, Y:848 maScaleX=...} ...} } }

The involved Clip-PolyPolygons (which are rectangles due to being handed in from PDFWriterImpl::intersectClipRegion which converts to PolyPolygon) are slightly different - both say true to basegfx::utils::isRectangle and are slightly different:

last: {maRangeX={mnMinimum=70850.000000000000 mnMaximum=464300.00000000000 } maRangeY={mnMinimum=61050.000000000000 ...} }

{maRangeX={mnMinimum=70894.488188976378 mnMaximum=464371.65354330710 } maRangeY={mnMinimum=61102.068028298498 ...} }

Pretty similar - but numerical stuff - sigh - makes it fail. Additionally to various MapModes is the fact that the used Metafile is on integer, too - (ARGH! I proposed MANY times to change the PDF exporter to primitives which would provide double precision values...)

Or in short: same same but different :-)
Comment 22 Armin Le Grand 2020-03-03 16:28:30 UTC
At least I improved now the numerical precision of transformations (locally) using transformations instead of OutputDevice::LogicToPixel/PixelToLogic, but that's not enough. We will somehow need to detect that these coordinates are 'the same'. BUT: Need to be careful, we only want to trigger on real cases - sigh...
Comment 23 Armin Le Grand 2020-03-03 16:55:26 UTC
Had an idea and it works: If both sources for PDFWriterImpl::intersectClipRegion are ranges (using basegfx::utils::isRectangle) the basegfx::utils::solvePolygonOperationAnd is able to solve the AND-operation always based on these ranges.
Adapted ::solvePolygonOperationAnd to do exactly that and it works, checked with quite some cases...
Also improved precision for transformations, that may make future detections of 'same-but-different' geometry simpler.
Checked quite some test cases from the mentioned tasks, looks good. Doing some more last tests...
Comment 24 Armin Le Grand 2020-03-03 16:59:19 UTC
Looks good - in this state I can give my recommendation to use this
Comment 25 Armin Le Grand 2020-03-03 17:34:26 UTC
Test look good, but not to forget - this is a workaround. Comment 18 stays true, the refactor woluld be needed e.g. when someone rotates those embedded draws-in-writer or similar - that would fail and could only be solved by a real AND-PolyPolygon solver (!) Just to make that clear...
Comment 26 Commit Notification 2020-03-04 00:29:09 UTC
Armin.Le.Grand (CIB) committed a patch related to this issue.
It has been pushed to "master":

https://git.libreoffice.org/core/commit/6dff631f8f4b964b48aadde52a1e1b8b04b9ba53

tdf#130150 Improve clipping in PDF export

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 27 Commit Notification 2020-03-17 10:22:20 UTC
Armin.Le.Grand (CIB) committed a patch related to this issue.
It has been pushed to "libreoffice-6-4":

https://git.libreoffice.org/core/commit/3bd5de707a361fc23d53dd5ad76c147722dfa2e9

tdf#130150 Improve clipping in PDF export

It will be available in 6.4.3.

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.