tdf#160430: Fix glyph bounds calculation, and use basegfx::B2DRectangle

... instead of tools::Rectangle.

Several problems were there:
1. First, a horizontal bounding rectangle was calculated, with due
rounding; and then the result was rotated, and after that, rounded
again. That made the resulting rotated rectangle coordinates very
imprecise.
2. Also, ceil/floor was applied without normalization; and in case
of rotated font, that meant, that sometimes the range could be not
expanded to cover partially covered pixels, but instead collapsed.
3. The rotation to angles other than 90 degree multiples was done
incorrectly, resulting in cut off parts of characters.
4. For 90 degrees, the imprecise result of sin/cos converted 0.0
into values like 3e-16, which then could be ceil'ed up to 1.

Using B2DRectangle and its transform allows to simplify and fix
the calculations easily, and avoids premature rounding. Render of
rotated text of small size is more stable with this change.

Change-Id: Idffd74b9937feb2418ab76a8d325fdaf4ff841b7
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165553
Tested-by: Jenkins
Reviewed-by: Tomaž Vajngerl <quikee@gmail.com>
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
(cherry picked from commit 8962141a12c966b2d891829925e6203bf8d51852)
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/165619
Reviewed-by: Xisco Fauli <xiscofauli@libreoffice.org>

11 files changed, 85 insertions, 69 deletions

diff --git a/vcl/inc/font/LogicalFontInstance.hxx b/vcl/inc/font/LogicalFontInstance.hxx
index 40d3c57c4e67..73ba2e26a2b1 100644
--- a/vcl/inc/font/LogicalFontInstance.hxx
+++ b/vcl/inc/font/LogicalFontInstance.hxx
@@ -22,6 +22,7 @@
 #include <sal/config.h>
 
 #include <basegfx/polygon/b2dpolypolygon.hxx>
+#include <basegfx/range/b2drectangle.hxx>
 #include <o3tl/hash_combine.hxx>
 #include <rtl/ref.hxx>
 #include <salhelper/simplereferenceobject.hxx>
@@ -100,7 +101,7 @@ public: // TODO: make data members private
     vcl::font::PhysicalFontFace* GetFontFace() { return m_pFontFace.get(); }
     const ImplFontCache* GetFontCache() const { return mpFontCache; }
 
-    bool GetGlyphBoundRect(sal_GlyphId, tools::Rectangle&, bool) const;
+    bool GetGlyphBoundRect(sal_GlyphId, basegfx::B2DRectangle&, bool) const;
     virtual bool GetGlyphOutline(sal_GlyphId, basegfx::B2DPolyPolygon&, bool) const = 0;
     basegfx::B2DPolyPolygon GetGlyphOutlineUntransformed(sal_GlyphId) const;
 
diff --git a/vcl/inc/impfontcache.hxx b/vcl/inc/impfontcache.hxx
index 5ea19b05d9a5..4d197003b279 100644
--- a/vcl/inc/impfontcache.hxx
+++ b/vcl/inc/impfontcache.hxx
@@ -21,10 +21,10 @@
 
 #include <sal/config.h>
 
+#include <basegfx/range/b2drectangle.hxx>
 #include <rtl/ref.hxx>
 #include <o3tl/lru_map.hxx>
 #include <o3tl/hash_combine.hxx>
-#include <tools/gen.hxx>
 
 #include "font/FontSelectPattern.hxx"
 #include "glyphid.hxx"
@@ -59,7 +59,7 @@ struct GlyphBoundRectCacheHash
     }
 };
 
-typedef o3tl::lru_map<GlyphBoundRectCacheKey, tools::Rectangle,
+typedef o3tl::lru_map<GlyphBoundRectCacheKey, basegfx::B2DRectangle,
                       GlyphBoundRectCacheHash> GlyphBoundRectCache;
 
 class ImplFontCache
@@ -86,8 +86,8 @@ public:
                             LogicalFontInstance* pLogicalFont,
                             int nFallbackLevel, OUString& rMissingCodes );
 
-    bool GetCachedGlyphBoundRect(const LogicalFontInstance *, sal_GlyphId, tools::Rectangle &);
-    void CacheGlyphBoundRect(const LogicalFontInstance *, sal_GlyphId, tools::Rectangle &);
+    bool GetCachedGlyphBoundRect(const LogicalFontInstance*, sal_GlyphId, basegfx::B2DRectangle&);
+    void CacheGlyphBoundRect(const LogicalFontInstance*, sal_GlyphId, basegfx::B2DRectangle&);
 
     void                Invalidate();
 };
diff --git a/vcl/inc/impglyphitem.hxx b/vcl/inc/impglyphitem.hxx
index 1fa8454e2ea8..bb08031f3ab6 100644
--- a/vcl/inc/impglyphitem.hxx
+++ b/vcl/inc/impglyphitem.hxx
@@ -20,8 +20,8 @@
 #ifndef INCLUDED_VCL_IMPGLYPHITEM_HXX
 #define INCLUDED_VCL_IMPGLYPHITEM_HXX
 
+#include <basegfx/range/b2drectangle.hxx>
 #include <o3tl/typed_flags_set.hxx>
-#include <tools/gen.hxx>
 #include <vcl/dllapi.h>
 #include <vcl/rendercontext/SalLayoutFlags.hxx>
 #include <rtl/math.hxx>
@@ -89,7 +89,7 @@ public:
         return bool(m_nFlags & GlyphItemFlags::IS_SAFE_TO_INSERT_KASHIDA);
     }
 
-    inline bool GetGlyphBoundRect(const LogicalFontInstance*, tools::Rectangle&) const;
+    inline bool GetGlyphBoundRect(const LogicalFontInstance*, basegfx::B2DRectangle&) const;
     inline bool GetGlyphOutline(const LogicalFontInstance*, basegfx::B2DPolyPolygon&) const;
     inline void dropGlyph();
 
@@ -121,7 +121,7 @@ public:
 };
 
 bool GlyphItem::GetGlyphBoundRect(const LogicalFontInstance* pFontInstance,
-                                  tools::Rectangle& rRect) const
+                                  basegfx::B2DRectangle& rRect) const
 {
     return pFontInstance->GetGlyphBoundRect(m_aGlyphId, rRect, IsVertical());
 }
diff --git a/vcl/qa/cppunit/logicalfontinstance.cxx b/vcl/qa/cppunit/logicalfontinstance.cxx
index 6d5bbc4dafda..2a0e30d50c34 100644
--- a/vcl/qa/cppunit/logicalfontinstance.cxx
+++ b/vcl/qa/cppunit/logicalfontinstance.cxx
@@ -39,25 +39,46 @@ void VclLogicalFontInstanceTest::testglyphboundrect()
 {
     ScopedVclPtr<VirtualDevice> device = VclPtr<VirtualDevice>::Create(DeviceFormat::WITHOUT_ALPHA);
     device->SetOutputSizePixel(Size(1000, 1000));
-    device->SetFont(vcl::Font("Liberation Sans", Size(0, 110)));
+    vcl::Font font("Liberation Sans", Size(0, 110));
+    device->SetFont(font);
 
     const LogicalFontInstance* pFontInstance = device->GetFontInstance();
 
-    tools::Rectangle aBoundRect;
+    basegfx::B2DRectangle aBoundRect;
     const auto LATIN_SMALL_LETTER_B = 0x0062;
     pFontInstance->GetGlyphBoundRect(pFontInstance->GetGlyphIndex(LATIN_SMALL_LETTER_B), aBoundRect,
                                      false);
 
-    const tools::Long nExpectedX = 7;
-    const tools::Long nExpectedY = -80;
-    const tools::Long nExpectedWidth = 51;
-    const tools::Long nExpectedHeight = 83;
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(7.1, aBoundRect.getMinX(), 0.05);
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(-79.7, aBoundRect.getMinY(), 0.05);
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(49.5, aBoundRect.getWidth(), 0.05);
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(80.8, aBoundRect.getHeight(), 0.05);
 
-    CPPUNIT_ASSERT_EQUAL_MESSAGE("x of glyph is wrong", nExpectedX, aBoundRect.getX());
-    CPPUNIT_ASSERT_EQUAL_MESSAGE("y of glyph is wrong", nExpectedY, aBoundRect.getY());
-    CPPUNIT_ASSERT_EQUAL_MESSAGE("height of glyph of wrong", nExpectedWidth, aBoundRect.GetWidth());
-    CPPUNIT_ASSERT_EQUAL_MESSAGE("width of glyph of wrong", nExpectedHeight,
-                                 aBoundRect.GetHeight());
+    font.SetOrientation(900_deg10);
+    device->SetFont(font);
+
+    pFontInstance = device->GetFontInstance();
+
+    pFontInstance->GetGlyphBoundRect(pFontInstance->GetGlyphIndex(LATIN_SMALL_LETTER_B), aBoundRect,
+                                     false);
+
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(-79.7, aBoundRect.getMinX(), 0.05);
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(-56.6, aBoundRect.getMinY(), 0.05);
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(80.8, aBoundRect.getWidth(), 0.05);
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(49.5, aBoundRect.getHeight(), 0.05);
+
+    font.SetOrientation(450_deg10);
+    device->SetFont(font);
+
+    pFontInstance = device->GetFontInstance();
+
+    pFontInstance->GetGlyphBoundRect(pFontInstance->GetGlyphIndex(LATIN_SMALL_LETTER_B), aBoundRect,
+                                     false);
+
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(-51.3, aBoundRect.getMinX(), 0.05);
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(-96.4, aBoundRect.getMinY(), 0.05);
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(92.1, aBoundRect.getWidth(), 0.05);
+    CPPUNIT_ASSERT_DOUBLES_EQUAL(92.1, aBoundRect.getHeight(), 0.05);
 }
 
 CPPUNIT_TEST_SUITE_REGISTRATION(VclLogicalFontInstanceTest);
diff --git a/vcl/source/font/LogicalFontInstance.cxx b/vcl/source/font/LogicalFontInstance.cxx
index 0c21cba47548..fbb115825828 100644
--- a/vcl/source/font/LogicalFontInstance.cxx
+++ b/vcl/source/font/LogicalFontInstance.cxx
@@ -26,6 +26,8 @@
 #include <font/LogicalFontInstance.hxx>
 #include <impfontcache.hxx>
 
+#include <basegfx/matrix/b2dhommatrixtools.hxx>
+
 LogicalFontInstance::LogicalFontInstance(const vcl::font::PhysicalFontFace& rFontFace,
                                          const vcl::font::FontSelectPattern& rFontSelData)
     : mxFontMetric(new FontMetricData(rFontSelData))
@@ -166,47 +168,39 @@ void LogicalFontInstance::IgnoreFallbackForUnicode(sal_UCS4 cChar, FontWeight eW
         maUnicodeFallbackList.erase(it);
 }
 
-bool LogicalFontInstance::GetGlyphBoundRect(sal_GlyphId nID, tools::Rectangle& rRect,
+bool LogicalFontInstance::GetGlyphBoundRect(sal_GlyphId nID, basegfx::B2DRectangle& rRect,
                                             bool bVertical) const
 {
+    // TODO/FIXME: bVertical handling here is highly suspicious. When it's true, it may
+    // return different rectangle, depending on if this glyph was cached already or not.
+
     if (mpFontCache && mpFontCache->GetCachedGlyphBoundRect(this, nID, rRect))
         return true;
 
     auto* pHbFont = const_cast<LogicalFontInstance*>(this)->GetHbFont();
     hb_glyph_extents_t aExtents;
-    bool res = hb_font_get_glyph_extents(pHbFont, nID, &aExtents);
+    if (!hb_font_get_glyph_extents(pHbFont, nID, &aExtents))
+        return false;
 
-    if (res)
-    {
-        double nXScale = 0, nYScale = 0;
-        GetScale(&nXScale, &nYScale);
+    double nXScale = 0, nYScale = 0;
+    GetScale(&nXScale, &nYScale);
 
-        double fMinX = aExtents.x_bearing;
-        double fMinY = aExtents.y_bearing;
-        double fMaxX = aExtents.x_bearing + aExtents.width;
-        double fMaxY = aExtents.y_bearing + aExtents.height;
+    double fMinX = aExtents.x_bearing * nXScale;
+    double fMinY = -aExtents.y_bearing * nYScale;
+    double fMaxX = (aExtents.x_bearing + aExtents.width) * nXScale;
+    double fMaxY = -(aExtents.y_bearing + aExtents.height) * nYScale;
+    rRect = basegfx::B2DRectangle(fMinX, fMinY, fMaxX, fMaxY);
 
-        tools::Rectangle aRect(std::floor(fMinX * nXScale), -std::ceil(fMinY * nYScale),
-                               std::ceil(fMaxX * nXScale), -std::floor(fMaxY * nYScale));
-        if (mnOrientation && !bVertical)
-        {
-            // Apply font rotation.
-            const double fRad = toRadians(mnOrientation);
-            const double fCos = cos(fRad);
-            const double fSin = sin(fRad);
-
-            rRect.SetLeft(fCos * aRect.Left() + fSin * aRect.Top());
-            rRect.SetTop(-fSin * aRect.Left() - fCos * aRect.Top());
-            rRect.SetRight(fCos * aRect.Right() + fSin * aRect.Bottom());
-            rRect.SetBottom(-fSin * aRect.Right() - fCos * aRect.Bottom());
-        }
-        else
-            rRect = aRect;
+    if (mnOrientation && !bVertical)
+    {
+        // Apply font rotation.
+        rRect.transform(basegfx::utils::createRotateB2DHomMatrix(-toRadians(mnOrientation)));
     }
 
-    if (mpFontCache && res)
+    if (mpFontCache)
         mpFontCache->CacheGlyphBoundRect(this, nID, rRect);
-    return res;
+
+    return true;
 }
 
 sal_GlyphId LogicalFontInstance::GetGlyphIndex(uint32_t nUnicode, uint32_t nVariationSelector) const
diff --git a/vcl/source/font/fontcache.cxx b/vcl/source/font/fontcache.cxx
index c0dba153502e..ce4ba6adf64c 100644
--- a/vcl/source/font/fontcache.cxx
+++ b/vcl/source/font/fontcache.cxx
@@ -252,7 +252,7 @@ void ImplFontCache::Invalidate()
     m_aBoundRectCache.clear();
 }
 
-bool ImplFontCache::GetCachedGlyphBoundRect(const LogicalFontInstance *pFont, sal_GlyphId nID, tools::Rectangle &rRect)
+bool ImplFontCache::GetCachedGlyphBoundRect(const LogicalFontInstance *pFont, sal_GlyphId nID, basegfx::B2DRectangle &rRect)
 {
     if (!pFont->GetFontCache())
         return false;
@@ -269,7 +269,7 @@ bool ImplFontCache::GetCachedGlyphBoundRect(const LogicalFontInstance *pFont, sa
     return false;
 }
 
-void ImplFontCache::CacheGlyphBoundRect(const LogicalFontInstance *pFont, sal_GlyphId nID, tools::Rectangle &rRect)
+void ImplFontCache::CacheGlyphBoundRect(const LogicalFontInstance *pFont, sal_GlyphId nID, basegfx::B2DRectangle &rRect)
 {
     if (!pFont->GetFontCache())
         return;
diff --git a/vcl/source/gdi/CommonSalLayout.cxx b/vcl/source/gdi/CommonSalLayout.cxx
index bcf6f54639e8..455428e7f396 100644
--- a/vcl/source/gdi/CommonSalLayout.cxx
+++ b/vcl/source/gdi/CommonSalLayout.cxx
@@ -534,12 +534,12 @@ bool GenericSalLayout::LayoutText(vcl::text::ImplLayoutArgs& rArgs, const SalLay
                     {
                         // We need glyph's advance, top bearing, and height to
                         // correct y offset.
-                        tools::Rectangle aRect;
+                        basegfx::B2DRectangle aRect;
                         // Get cached bound rect value for the font,
                         GetFont().GetGlyphBoundRect(nGlyphIndex, aRect, true);
 
-                        nXOffset = -(aRect.Top() / nXScale  + ( pHbPositions[i].y_advance
-                                    + ( aRect.GetHeight() / nXScale ) ) / 2.0 );
+                        nXOffset = -(aRect.getMinX() / nXScale  + ( pHbPositions[i].y_advance
+                                    + ( aRect.getHeight() / nXScale ) ) / 2.0 );
                     }
 
                 }
diff --git a/vcl/source/gdi/pdfwriter_impl.cxx b/vcl/source/gdi/pdfwriter_impl.cxx
index a5365e681b3b..fba7a85db430 100644
--- a/vcl/source/gdi/pdfwriter_impl.cxx
+++ b/vcl/source/gdi/pdfwriter_impl.cxx
@@ -7091,7 +7091,7 @@ void PDFWriterImpl::drawLayout( SalLayout& rLayout, const OUString& rText, bool
     else if ( eAlign == ALIGN_TOP )
         aOffset.AdjustY(GetFontInstance()->mxFontMetric->GetAscent() );
 
-    tools::Rectangle aRectangle;
+    basegfx::B2DRectangle aRectangle;
     nIndex = 0;
     while (rLayout.GetNextGlyph(&pGlyph, aPos, nIndex, &pGlyphFont))
     {
@@ -7109,7 +7109,7 @@ void PDFWriterImpl::drawLayout( SalLayout& rLayout, const OUString& rText, bool
             else
             {
                 aAdjOffset = basegfx::B2DPoint(aOffset.X(), aOffset.Y());
-                aAdjOffset.adjustX(aRectangle.Left() + (aRectangle.GetWidth() - aEmphasisMark.GetWidth()) / 2 );
+                aAdjOffset.adjustX(aRectangle.getMinX() + (aRectangle.getWidth() - aEmphasisMark.GetWidth()) / 2 );
             }
 
             aAdjOffset = aRotScale.transform( aAdjOffset );
diff --git a/vcl/source/gdi/sallayout.cxx b/vcl/source/gdi/sallayout.cxx
index d052ab53221b..756a427f1dda 100644
--- a/vcl/source/gdi/sallayout.cxx
+++ b/vcl/source/gdi/sallayout.cxx
@@ -214,12 +214,15 @@ bool SalLayout::GetOutline(basegfx::B2DPolyPolygonVector& rVector) const
     return (bAllOk && bOneOk);
 }
 
+// No need to expand to the next pixel, when the character only covers its tiny fraction
+static double trimInsignificant(double n) { return std::round(n * 1e5) / 1e5; }
+
 bool SalLayout::GetBoundRect(tools::Rectangle& rRect) const
 {
     bool bRet = false;
-    rRect.SetEmpty();
 
-    tools::Rectangle aRectangle;
+    basegfx::B2DRectangle aUnion;
+    basegfx::B2DRectangle aRectangle;
 
     basegfx::B2DPoint aPos;
     const GlyphItem* pGlyph;
@@ -230,22 +233,19 @@ bool SalLayout::GetBoundRect(tools::Rectangle& rRect) const
         // get bounding rectangle of individual glyph
         if (pGlyph->GetGlyphBoundRect(pGlyphFont, aRectangle))
         {
-            if (!aRectangle.IsEmpty())
+            if (!aRectangle.isEmpty())
             {
-                aRectangle.AdjustLeft(std::floor(aPos.getX()));
-                aRectangle.AdjustRight(std::ceil(aPos.getX()));
-                aRectangle.AdjustTop(std::floor(aPos.getY()));
-                aRectangle.AdjustBottom(std::ceil(aPos.getY()));
-
+                aRectangle.transform(basegfx::utils::createTranslateB2DHomMatrix(aPos));
                 // merge rectangle
-                if (rRect.IsEmpty())
-                    rRect = aRectangle;
-                else
-                    rRect.Union(aRectangle);
+                aUnion.expand(aRectangle);
             }
             bRet = true;
         }
     }
+    rRect = tools::Rectangle(rtl::math::approxFloor(trimInsignificant(aUnion.getMinX())),
+                             rtl::math::approxFloor(trimInsignificant(aUnion.getMinY())),
+                             rtl::math::approxCeil(trimInsignificant(aUnion.getMaxX())),
+                             rtl::math::approxCeil(trimInsignificant(aUnion.getMaxY())));
 
     return bRet;
 }
diff --git a/vcl/source/outdev/font.cxx b/vcl/source/outdev/font.cxx
index 2086db7f6341..9c043731591a 100644
--- a/vcl/source/outdev/font.cxx
+++ b/vcl/source/outdev/font.cxx
@@ -951,7 +951,7 @@ void OutputDevice::ImplDrawEmphasisMarks( SalLayout& rSalLayout )
     aOffset += Point( nEmphasisWidth2, nEmphasisHeight2 );
 
     basegfx::B2DPoint aOutPoint;
-    tools::Rectangle aRectangle;
+    basegfx::B2DRectangle aRectangle;
     const GlyphItem* pGlyph;
     const LogicalFontInstance* pGlyphFont;
     int nStart = 0;
@@ -971,7 +971,7 @@ void OutputDevice::ImplDrawEmphasisMarks( SalLayout& rSalLayout )
             else
             {
                 aAdjPoint = aOffset;
-                aAdjPoint.AdjustX(aRectangle.Left() + (aRectangle.GetWidth() - aEmphasisMark.GetWidth()) / 2 );
+                aAdjPoint.AdjustX(aRectangle.getMinX() + (aRectangle.getWidth() - aEmphasisMark.GetWidth()) / 2 );
             }
 
             if ( mpFontInstance->mnOrientation )
diff --git a/vcl/workben/listglyphs.cxx b/vcl/workben/listglyphs.cxx
index def2ff818122..341006d433dd 100644
--- a/vcl/workben/listglyphs.cxx
+++ b/vcl/workben/listglyphs.cxx
@@ -120,7 +120,7 @@ int ListGlyphs::Main()
              nChar = pCharMap->GetNextChar(nChar))
         {
             auto nGlyphIndex = pFontInstance->GetGlyphIndex(nChar);
-            tools::Rectangle aGlyphBounds;
+            basegfx::B2DRectangle aGlyphBounds;
             pFontInstance->GetGlyphBoundRect(nGlyphIndex, aGlyphBounds, false);
             std::cout << "Codepoint: " << pFontFace->GetGlyphName(nGlyphIndex)
                       << "; glyph bounds: " << aGlyphBounds << "\n";