Bug 25578

Summary: Seen crahses in Fixed Point
Product: poppler Reporter: Amit <amitcs06>
Component: generalAssignee: poppler-bugs <poppler-bugs>
Status: RESOLVED FIXED QA Contact:
Severity: normal    
Priority: medium CC: marius.vollmer
Version: unspecified   
Hardware: All   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments: Propsed Patch
Add --enable-single-precision option

Description Amit 2009-12-10 22:39:37 UTC
Seen crash while using fixed point in ARM complier or x86
Comment 1 Amit 2009-12-10 23:05:23 UTC
Created attachment 31969 [details] [review]
Propsed Patch

goo/FixedPoint.cc
This crash is not seen for every ppt. Some ppt there is overflow in datatype for FixedPoint mutiplication and due to which its coming -ve value which is getting used in sqrt function and its returning zero.

Result in divide function is giving crash.
Comment 2 Amit 2009-12-10 23:06:18 UTC
Please find the atached propsed attachment. 

Review it and let me know the status.

Comment 3 Albert Astals Cid 2009-12-11 00:47:21 UTC
Hi Amit, thanks for the patch, some considerations:
 * Could you please post a proper git diff or diff -u instead of what you posted?
 * When you say ppt you mean pdf, right?
 * Do you have a pdf file that reproduces this crash than you can attach?
 * Also, please do not set the bug to fixed since it's not until someone commits a fix into the repository.
Comment 4 Albert Astals Cid 2009-12-11 15:04:09 UTC
The patch is wrong for several reasons:
 * FixedPoint val field is suposed to be a 16.16 value, you are turning it into a 64 bits value which removes the point of using FixedPoint arithmetics and you could as well use the regular double one
 * You made div return 0 when diving by -1

The actual problem is that the multiplication of 215.876 * 215.876 doesn't fit in a 16.16 field

Of course the obvious fix is not using FixedPoint math for that multiplication

diff --git a/splash/SplashFTFont.cc b/splash/SplashFTFont.cc
index d4675f7..65371ba 100644
--- a/splash/SplashFTFont.cc
+++ b/splash/SplashFTFont.cc
@@ -40,6 +40,8 @@
 #include "SplashFTFontFile.h"
 #include "SplashFTFont.h"

+#include <cmath>
+
 //------------------------------------------------------------------------

 static int glyphPathMoveTo(const FT_Vector *pt, void *path);
@@ -67,13 +69,13 @@ SplashFTFont::SplashFTFont(SplashFTFontFile *fontFileA, SplashCoord *matA,
     return;
   }
   face->size = sizeObj;
-  size = splashSqrt(mat[2]*mat[2] + mat[3]*mat[3]);
+  size = sqrt((double)mat[2]*(double)mat[2] + (double)mat[3]*(double)mat[3]);
   if (FT_Set_Pixel_Sizes(face, 0, (int)size)) {
     return;
   }
   // if the textMat values are too small, FreeType's fixed point
   // arithmetic doesn't work so well
-  textScale = splashSqrt(textMat[2]*textMat[2] + textMat[3]*textMat[3]) / size;
+  textScale = sqrt((double)textMat[2]*(double)textMat[2] + (double)textMat[3]*(double)textMat[3]) / size;

   div = face->bbox.xMax > 20000 ? 65536 : 1;
Comment 5 Amit 2009-12-11 20:58:08 UTC
OK thanks for the reply but splshSqrt is meant for fixedPoint also now its
using only sqrt function.

Thanks
Amit



On Sat, Dec 12, 2009 at 4:34 AM, <bugzilla-daemon@freedesktop.org> wrote:

> http://bugs.freedesktop.org/show_bug.cgi?id=25578
>
>
>
>
>
> --- Comment #4 from Albert Astals Cid <tsdgeos@terra.es>  2009-12-11
> 15:04:09 PST ---
> The patch is wrong for several reasons:
>  * FixedPoint val field is suposed to be a 16.16 value, you are turning it
> into
> a 64 bits value which removes the point of using FixedPoint arithmetics and
> you
> could as well use the regular double one
>  * You made div return 0 when diving by -1
>
> The actual problem is that the multiplication of 215.876 * 215.876 doesn't
> fit
> in a 16.16 field
>
> Of course the obvious fix is not using FixedPoint math for that
> multiplication
>
> diff --git a/splash/SplashFTFont.cc b/splash/SplashFTFont.cc
> index d4675f7..65371ba 100644
> --- a/splash/SplashFTFont.cc
> +++ b/splash/SplashFTFont.cc
> @@ -40,6 +40,8 @@
>  #include "SplashFTFontFile.h"
>  #include "SplashFTFont.h"
>
> +#include <cmath>
> +
>  //------------------------------------------------------------------------
>
>  static int glyphPathMoveTo(const FT_Vector *pt, void *path);
> @@ -67,13 +69,13 @@ SplashFTFont::SplashFTFont(SplashFTFontFile *fontFileA,
> SplashCoord *matA,
>     return;
>   }
>   face->size = sizeObj;
> -  size = splashSqrt(mat[2]*mat[2] + mat[3]*mat[3]);
> +  size = sqrt((double)mat[2]*(double)mat[2] +
> (double)mat[3]*(double)mat[3]);
>   if (FT_Set_Pixel_Sizes(face, 0, (int)size)) {
>     return;
>   }
>   // if the textMat values are too small, FreeType's fixed point
>   // arithmetic doesn't work so well
> -  textScale = splashSqrt(textMat[2]*textMat[2] + textMat[3]*textMat[3]) /
> size;
> +  textScale = sqrt((double)textMat[2]*(double)textMat[2] +
> (double)textMat[3]*(double)textMat[3]) / size;
>
>   div = face->bbox.xMax > 20000 ? 65536 : 1;
>
>
> --
> Configure bugmail: http://bugs.freedesktop.org/userprefs.cgi?tab=email
> ------- You are receiving this mail because: -------
> You reported the bug.
>



Comment 6 Albert Astals Cid 2009-12-11 21:34:21 UTC
as i said, you can't fit the result of 215.876 * 215.876 in a FixedPoint (16.16 field) so the only option is not to use FixedPoint for that operation.
Comment 7 Marius Vollmer 2010-01-04 03:27:01 UTC
I have tried to fix this now for Maemo by using single precision floating point instead of fixed point.

Fixed point has the usual fixed point problems, and single precision floating point is actually faster than fixed point on the hardware that we care about in Maemo.
Comment 8 Marius Vollmer 2010-01-04 03:28:10 UTC
Created attachment 32429 [details] [review]
Add --enable-single-precision option
Comment 9 Marius Vollmer 2010-01-04 03:34:06 UTC
Upps, that patch includes the collateral damages to the autocruft.  Here is the relevant part:

Index: poppler/configure.ac
===================================================================
--- poppler.orig/configure.ac	2009-12-30 15:22:10.000000000 +0200
+++ poppler/configure.ac	2009-12-30 15:22:26.000000000 +0200
@@ -67,6 +67,10 @@
               enable_xpdf_headers="no")
 AM_CONDITIONAL(ENABLE_XPDF_HEADERS, test x$enable_xpdf_headers = xyes)
 
+AC_ARG_ENABLE(single-precision,
+[  --enable-single-precision     use single precision arithmetic (instead of double precision)],
+AC_DEFINE(USE_FLOAT,      [1], [Use single precision arithmetic]))
+
 AC_ARG_ENABLE(fixedpoint,
 [  --enable-fixedpoint     use fixed point (instead of floating point) arithmetic],
 AC_DEFINE(USE_FIXEDPOINT, [1], [Use fixed point arithmetic]))
Index: poppler/splash/SplashTypes.h
===================================================================
--- poppler.orig/splash/SplashTypes.h	2009-12-30 15:23:53.000000000 +0200
+++ poppler/splash/SplashTypes.h	2009-12-30 15:24:01.000000000 +0200
@@ -33,8 +33,12 @@
 #include "goo/FixedPoint.h"
 typedef FixedPoint SplashCoord;
 #else
+#if USE_FLOAT
+typedef float SplashCoord;
+#else
 typedef double SplashCoord;
 #endif
+#endif
 
 //------------------------------------------------------------------------
 // antialiasing
Index: poppler/splash/SplashMath.h
===================================================================
--- poppler.orig/splash/SplashMath.h	2009-12-30 15:25:05.000000000 +0200
+++ poppler/splash/SplashMath.h	2009-12-30 15:27:53.000000000 +0200
@@ -18,48 +18,72 @@
 #if USE_FIXEDPOINT
   return FixedPoint::abs(x);
 #else
+#if USE_FLOAT
+  return fabsf(x);
+#else
   return fabs(x);
 #endif
+#endif
 }
 
 static inline int splashFloor(SplashCoord x) {
   #if USE_FIXEDPOINT
     return FixedPoint::floor(x);
   #else
+  #if USE_FLOAT
+    return (int)floorf(x);
+  #else
     return (int)floor(x);
   #endif
+  #endif
 }
 
 static inline int splashCeil(SplashCoord x) {
 #if USE_FIXEDPOINT
   return FixedPoint::ceil(x);
 #else
+#if USE_FLOAT
+  return (int)ceilf(x);
+#else
   return (int)ceil(x);
 #endif
+#endif
 }
 
 static inline int splashRound(SplashCoord x) {
 #if USE_FIXEDPOINT
   return FixedPoint::round(x);
 #else
+#if USE_FLOAT
+  return (int)floorf(x + 0.5);
+#else
   return (int)floor(x + 0.5);
 #endif
+#endif
 }
 
 static inline SplashCoord splashSqrt(SplashCoord x) {
 #if USE_FIXEDPOINT
   return FixedPoint::sqrt(x);
 #else
+#if USE_FLOAT
+  return sqrtf(x);
+#else
   return sqrt(x);
 #endif
+#endif
 }
 
 static inline SplashCoord splashPow(SplashCoord x, SplashCoord y) {
 #if USE_FIXEDPOINT
   return FixedPoint::pow(x, y);
 #else
+#if USE_FLOAT
+  return powf(x, y);
+#else
   return pow(x, y);
 #endif
+#endif
 }
 
 static inline SplashCoord splashDist(SplashCoord x0, SplashCoord y0,
@@ -81,8 +105,12 @@
     return dya * FixedPoint::sqrt(dxa / dya + 1);
   }
 #else
+#if USE_FLOAT
+  return sqrtf(dx * dx + dy * dy);
+#else
   return sqrt(dx * dx + dy * dy);
 #endif
+#endif
 }
 
 #endif
Comment 10 Albert Astals Cid 2010-01-09 16:09:18 UTC
That patch seems ok to me, only remaining question is, does nokia allow to license that code under the GPL2 or later?

Also we have lines in code with copyright like
// Copyright (C) 2010 Foo Bar <foo@bar.com>

What should i use in this case?
Comment 11 Marius Vollmer 2010-01-12 02:37:03 UTC
(In reply to comment #10)
> That patch seems ok to me, only remaining question is, does nokia allow to
> license that code under the GPL2 or later?

Yes.  I don't think my patch is copyrightable, actually, being so small and repetitive.

If you need a more formal statement, I can provide that as well.  I could add a header to the patch, with the usual GPL2 or later statement.

> Also we have lines in code with copyright like
> // Copyright (C) 2010 Foo Bar <foo@bar.com>
> 
> What should i use in this case?

Hmm, are you asking in general, or is this related to the patch?

Poppler is already used in Maemo since version 0.10 at least, so it is considered legally OK for that by Nokia.

But personally, I recommend to spell out the license in each source file, following the instructions that come with the GPL2.
Comment 12 Albert Astals Cid 2010-01-12 14:56:36 UTC
Patch commited
Comment 13 Marius Vollmer 2010-01-14 00:54:27 UTC
(In reply to comment #12)
> Patch commited

Thank you!

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.