Bug 296141 - Make digikam compile on ARM
Summary: Make digikam compile on ARM
Status: RESOLVED FIXED
Alias: None
Product: digikam
Classification: Applications
Component: Portability-Compilation (show other bugs)
Version: 2.6.0
Platform: Ubuntu Linux
: NOR normal
Target Milestone: ---
Assignee: Digikam Developers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-16 11:58 UTC by Rohan Garg
Modified: 2017-08-19 20:59 UTC (History)
4 users (show)

See Also:
Latest Commit:
Version Fixed In: 2.6.0
Sentry Crash Report:


Attachments
kubuntu_fix_arm.patch (2.49 KB, patch)
2012-03-16 11:58 UTC, Rohan Garg
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Rohan Garg 2012-03-16 11:58:07 UTC
Created attachment 69659 [details]
kubuntu_fix_arm.patch

Currently digikam does not compile on ARM, I've attached a patch that fixes digikam compiles on ARM
Comment 1 caulier.gilles 2012-03-16 12:04:19 UTC
Just by curiosity, on which ARM device you compile digiKam exactly ?

Gilles Caulier
Comment 2 Rohan Garg 2012-03-16 12:07:55 UTC
Ah, This is a ARM pbuilder that I setup on my laptop, uses qemu-system-arm to emulate a ARM processor
Comment 3 caulier.gilles 2012-03-16 12:50:49 UTC
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
Comment 4 Francesco Riosa 2012-03-16 13:03:57 UTC
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
Comment 5 Francesco Riosa 2012-03-16 13:06:49 UTC
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
Comment 6 Ananta Palani 2012-03-16 13:38:55 UTC
(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
Comment 7 Ananta Palani 2012-03-16 13:40:55 UTC
Or is the precision of 8 necessary for some GPS/lat/long coordinates or something similar?
Comment 8 caulier.gilles 2012-03-19 21:57:22 UTC
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
Comment 9 Francesco Riosa 2012-03-19 22:24:11 UTC
(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
Comment 10 Rohan Garg 2012-03-20 12:41:43 UTC
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
Comment 11 Francesco Riosa 2012-03-20 13:58:01 UTC
(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
Comment 12 Rohan Garg 2012-03-20 14:00:43 UTC
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.
Comment 13 Francesco Riosa 2012-03-20 15:29:15 UTC
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
Comment 14 Francesco Riosa 2012-03-20 15:37:31 UTC
(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.
Comment 15 Rohan Garg 2012-03-21 13:43:05 UTC
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
Comment 16 Rohan Garg 2012-03-26 19:06:03 UTC
Tested a compile and it works!
Thanks Francesco!