Bug 296141

Just by curiosity, on which ARM device you compile digiKam exactly ?

Gilles Caulier

Ah, This is a ARM pbuilder that I setup on my laptop, uses qemu-system-arm to emulate a ARM processor

Git commit e0a2fcfb9f0bc75084e4bf01905524fd5e29edd6 by Gilles Caulier.
Committed on 16/03/2012 at 13:48.
Pushed by cgilles into branch 'master'.

fix compilation under ARM device. Apply patch #69659

M  +1    -1    libs/database/searchxml.cpp
M  +1    -1    libs/database/searchxml.h
M  +2    -2    libs/widgets/graphicsview/regionframeitem.cpp
M  +1    -1    utilities/searchwindow/searchfields.cpp

http://commits.kde.org/digikam/e0a2fcfb9f0bc75084e4bf01905524fd5e29edd6

from #1: <<Typedef for double on all platforms except for those using CPUs with ARM architectures. On ARM-based platforms, qreal is a typedef for float for performance reasons.>>

according to wikipedia [#2] <<This gives from 6 to 9 significant decimal digits precision>> 

should the precision of the comparison (8) be lowered too?

#1 http://doc.qt.nokia.com/4.7-snapshot/qtglobal.html#qreal-typedef
#2 http://en.wikipedia.org/wiki/Single-precision_floating-point_format

And _not_ only for ARM, since it's a value stored in database and thus potentially accessed from other platforms it should be the same for all

(In reply to comment #4)
> from #1: <<Typedef for double on all platforms except for those using CPUs
> with ARM architectures. On ARM-based platforms, qreal is a typedef for float
> for performance reasons.>>
> 
> according to wikipedia [#2] <<This gives from 6 to 9 significant decimal
> digits precision>> 
> 
> should the precision of the comparison (8) be lowered too?
> 
> #1 http://doc.qt.nokia.com/4.7-snapshot/qtglobal.html#qreal-typedef
> #2 http://en.wikipedia.org/wiki/Single-precision_floating-point_format

Probably should be changed to 6 since that is the default for QString::number() anyway:

   http://qt-project.org/doc/qt-4.8/qstring.html#number-2

Or is the precision of 8 necessary for some GPS/lat/long coordinates or something similar?

Francesco,

The float precision is important of course, especialy for geolocation data. Do you know a way to fix qReal usability problem with DB ?

Gilles Caulier

(In reply to comment #8)
> The float precision is important of course, especialy for geolocation data.
> Do you know a way to fix qReal usability problem with DB ?

All the following are guesstimates, take them with a grain of salt:
< cortex-A7 generally don't do floating point (FP)
cortex A7 has VFP extension which should do single and double FP
>= cortex A8 have NEON extensions (A9 optional) which are also suitable to vector processing

So we must assume that ARM _only_support single FP precision. This is baad because it could taint a database for every other architecture.

Assuming we are not using those values for calculations, they could be stored the values as strings instead of number, but this has to be done everywere from EXIV(?) to the insertion in the db.

So really no good ideas here.

Rohan Garg;
   since you have acces to ARM do you have any idea on how a double could be managed?
What was the actual error the compiler gave you?

Links:
http://en.wikipedia.org/wiki/ARM_architecture
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.faqs/ka9347.html

After a discussion on #ubuntu-arm it was concluded that 
a) ARMv7 and above has good double support, hence qreal should typedef to double on ARMv7 architectures and above ( Should go into Qt 5 )

b) For architechtures below v7, only single point precision is available, and double precision is not possible.

Build logs can be found here : https://launchpad.net/ubuntu/+source/digikam/4:2.5.0-1ubuntu1/+build/3215911
https://launchpad.net/ubuntu/+source/digikam/4:2.5.0-1ubuntu1/+build/3215910

(In reply to comment #10)
> After a discussion on #ubuntu-arm it was concluded that 
> a) ARMv7 and above has good double support, hence qreal should typedef to
> double on ARMv7 architectures and above ( Should go into Qt 5 )

sorry, this has already discussed (in a _very_ long thread) and rejected

Decide whether qreal should be double on all platforms.
https://bugreports.qt-project.org/browse/QTBUG-23758
== We'll keep it as it for Qt 5. double on desktop and float on embedded devices.


[Development] Changing qreal to a float
http://www.mail-archive.com/development@qt-project.org/msg01969.html


> b) For architechtures below v7, only single point precision is available,
> and double precision is not possible.
> 
> Build logs can be found here :
> https://launchpad.net/ubuntu/+source/digikam/4:2.5.0-1ubuntu1/+build/3215911
> https://launchpad.net/ubuntu/+source/digikam/4:2.5.0-1ubuntu1/+build/3215910

thanks,
  I see froom the logs that the problem is at link time, i.e. for example qBound() accepting only float on ARM and double on "desktop"

Since we are not using these values to do intensive calculations, and since ARM seem to have a "double" type, just not hardware support for it I would suggest the following:

- in regionframeitem.cpp use whatever, it's not precision dependant (but check it compile ;)
- searchxml.* searchfields.* use double
  convert all usage of qBound, qMax, qMin to something like:
  qreal x = qBound(qreal(min), v, qreal(max));
  double x = std::max<double>(min, std::min(value, max))

Rohan Garg, do you feel brave enough to issue another patch following these guidelines?
It would simplify things a lot.

Thanks in advance

Hi Francesco
Sorry but I'm already spread pretty thin over multiple projects, however I can help test any proposed patches using a ARM builder.

Git commit 42b51fc3af4c724bd3b88ed6cd602ea49baf7f18 by Francesco Riosa.
Committed on 20/03/2012 at 16:15.
Pushed by riosa into branch 'master'.

fix compilation under ARM device.

qReal translate to different types on different architectures.
Consensus is that the current state of affairs will be kept for qt 5.0
too [1].

Luckily the function affected "SearchFieldRangeInt" is only used to
store exposure times [2] where 6 precision digit suffice.

So we add functions to write <float> and cast to <qReal> where needed
and be done with that.

[1] https://bugreports.qt-project.org/browse/QTBUG-23758
[2] ChangeLog 2008-07-07 For use with the exposuretime field ...

P.S. CustomStepsDoubleSpinBox could be converted to
     CustomStepsFloatSpinBox but seem not worth the effort.
     Marcel hope you're ok with this

CCMAIL: marcel.wiesweg@gmx.de

M  +14   -0    libs/database/searchxml.cpp
M  +2    -0    libs/database/searchxml.h
M  +2    -2    libs/widgets/graphicsview/regionframeitem.cpp
M  +1    -1    utilities/searchwindow/searchfields.cpp

http://commits.kde.org/digikam/42b51fc3af4c724bd3b88ed6cd602ea49baf7f18

(In reply to comment #12)
> Hi Francesco
> Sorry but I'm already spread pretty thin over multiple projects, however I
> can help test any proposed patches using a ARM builder.

Ok, I've reverted the previous commit and replaced with a different one, result should be the same.
Testing on arm would be apreciated.

Currently compiling digikam on a pandaboard, will take a couple of hours ( or minutes if the build fails ). Will post the entire log when it's done/fails

Tested a compile and it works!
Thanks Francesco!