From: Nils P. <ni...@ti...> - 2013-02-18 18:56:02
|
Hi, I'm a software engineer working for Red Hat, maintaining the ufraw package in Fedora among (many) other things. I also maintain the gimp package and dabble a bit in its development upstream. When a post on the gimp developer ML[1] called my attention to that the ufraw plug-in doesn't yet send high bit depth data to GIMP, I spent some time on it. The patch I ended up with so far[2] works, but I'm not quite content with it: - It has way too many #ifdefs to my taste. Not sure what to do about it though, as it has to use gegl API to work with high bit depths. - I think it should also let users of the upcoming gimp versions specify a different precision than just 8bpp and 16bpp integer (e.g. 32bpp int, 16/32bpp float) that suits their workflow. But that may better be done in a separate change. It'd be great if you'd review/comment on these changes. If you want to commit these changes, please do so in a separate commit of its own -- I've submitted a bugfix in the past and it got lumped together with some completely unrelated changes, which made putting together my changes with the changes from upstream difficult (I use "git cvsimport" to be able to work with git rather than CVS for ufraw because I totally depend on some of the tools that come with it). If you feel that you can trust me with this at some point, I'd appreciate commit access on SF eventually -- just point me to any rules I need to know regarding review of changes before commit, coding style, etc. While I worked on this, I found that ufraw wouldn't work with the default optimization options if built with gcc-4.7.2 (as on Fedora 18) on a x86_64 machine -- some calculations of crop with and height failed and got set to zero in this case. I'm following this up with my colleague who knows way more than compilers, optimization and all that, but here's the gist, just so you're aware of it: - Use the optimization options "-O3 -fno-math-errno -funsafe-math-optimizations -ffinite-math-only" which stem from "-O3 -ffast-math" in configure.ac ("-ffast-math" expands to six seemingly unsafe math optimization flags of which the three listed are crucial for the error to show). - I'm sure the number of registers is crucial as well, so likely to show up only on x86_64. - The error first shows up with the commit "Fix various lensfun scale and crop problems. Add --auto-crop and --aspect-ratio command line options. ..." from 2011-11-19. It happens in ufraw_ufraw.c around this change: + if ((double)uf->autoCropWidth / uf->autoCropHeight > aspectRatio) + uf->autoCropWidth = uf->autoCropHeight * aspectRatio + 0.5; + else + uf->autoCropHeight = uf->autoCropWidth / aspectRatio + 0.5; Adding a printf call to the else block makes the problem vanish, this is a very likely a compiler optimization oddity. Anyway, afterwards the computed height isn't correct, but set to zero. Nils [1]: http://thread.gmane.org/gmane.comp.video.gimp.devel/23783/focus=23785 [2]: http://paste.fedoraproject.org/3296 -- Nils Philippsen / Wilhelmstraße 22 / D-71229 Leonberg ni...@ti... / ni...@re... PGP fingerprint: C4A8 9474 5C4C ADE3 2B8F 656D 47D8 9B65 6951 3011 Ever noticed that common sense isn't really all that common? |
From: Nils P. <ni...@ti...> - 2013-02-19 13:13:21
|
On Mon, 2013-02-18 at 19:37 +0100, Nils Philippsen wrote: > The patch I ended up with so far[2] works, but I'm not quite > content with it: [...] > [2]: http://paste.fedoraproject.org/3296 Hmm, I don't think it's a good idea to only post this somewhere with a lifetime of only 24 hours... I've attached the patch to this mail. Nils -- Nils Philippsen / Wilhelmstraße 22 / D-71229 Leonberg ni...@ti... / ni...@re... PGP fingerprint: C4A8 9474 5C4C ADE3 2B8F 656D 47D8 9B65 6951 3011 Ever noticed that common sense isn't really all that common? |
From: Udi F. <udi...@us...> - 2013-02-26 19:28:25
|
Thanks for the Gimp 2.9 patch. > + if ((double)uf->autoCropWidth / uf->autoCropHeight > aspectRatio) > + uf->autoCropWidth = uf->autoCropHeight * aspectRatio + 0.5; > + else > + uf->autoCropHeight = uf->autoCropWidth / aspectRatio + 0.5; Can you test the latest CVS? I added a call to floor(). We had issues before with the compiler handling rounding of float to int. I'm not sure if it is a compiler bug or a compiler feature. Udi |
From: Nils P. <ni...@ti...> - 2013-03-02 08:28:56
|
On Tue, 2013-02-26 at 13:27 -0600, Udi Fuchs wrote: > Thanks for the Gimp 2.9 patch. > > > + if ((double)uf->autoCropWidth / uf->autoCropHeight > aspectRatio) > > + uf->autoCropWidth = uf->autoCropHeight * aspectRatio + 0.5; > > + else > > + uf->autoCropHeight = uf->autoCropWidth / aspectRatio + 0.5; > > > Can you test the latest CVS? I added a call to floor(). We had issues > before with the compiler handling rounding of float to int. I'm not > sure if it is a compiler bug or a compiler feature. It doesn't help here, sorry. I'd like to isolate a short reproducer for a meaningful bug report to gcc, but so far whenever I've more than looked at code in the vicinity of where the bug must happen, the bug didn't show up. I hate it when that happens ;-). Nils -- Nils Philippsen / Wilhelmstraße 22 / D-71229 Leonberg ni...@ti... / ni...@re... PGP fingerprint: C4A8 9474 5C4C ADE3 2B8F 656D 47D8 9B65 6951 3011 Ever noticed that common sense isn't really all that common? |