LibRaw requires too much CPU time on import with CAMERA developer
Submitted by Lucas Beeler
Link to original bug (#719294)
Description
---- Reported by lucas@yorba.org 2013-04-12 15:32:00 -0700 ----
Original Redmine bug id: 6812
Original URL: http://redmine.yorba.org/issues/6812
Searchable id: yorba-bug-6812
Original author: Lucas Beeler
Original description:
From Joseph Bylund on the mailing list:
Hi all,
I did a little experimentation with profiling raw imports (you can find an example sysprof profile here: http://ubuntuone.com/6PrDbY32fdP2QXT2g8LSbf). And found that libraw is actually the limiting factor during imports while using the camera developer (probably 75%+ of time). Is it reasonable to file a bug against slow importing?
thanks,
-Joe
The amount of CPU time libraw is using seems ridiculously high when importing with the RAW developer set to Camera mode. In Camera mode, the libraw pipeline shouldn't be run at all. Instead, we should merely pluck the embedded JPEG out of the RAW file. This is just reading bytes and shouldn't be CPU-intensive at all. However, I suspect that even in Camera mode, libraw still runs the full RAW development pipeline on each image. This is a serious performance problem. We should investigate and fix it.
Related issues:
- related to shotwell - 6745: With the developer set to 'Camera', when importing an RAW... (Open)
- duplicated by shotwell - Feature #7416: Importing 1100 RAW files takes forever with CPU hot, low IO (Duplicate)
---- Additional Comments From shotwell-maint@gnome.bugs 2013-08-29 11:55:00 -0700 ----
History
Comment 1
Updated by Joe Bylund 6 months ago
Looks like the raw is being developed for the thumbnails at ThumbnailCache.generate_for_photo from Photo.prepare_for_import.
Comment 2
Updated by Jim Nelson 6 months ago
-
Target version deleted (
<strike>
_0.15.0_</strike>
)
Comment 3
Updated by Joe Bylund 3 months ago
- File generate_for_photo_alt.vala added
What do you think of in ThumbnailCache.generate_for_photo only getting one thumbnail, at the largest size, and then downscaling that thumbnail to make the others? This eliminates one of raw load/unpack/read's. I'm attaching a copy of the function so you know what I'm talking about. While this makes this function a lot faster for me (2x) I'm still not seeing any improvement in the overall import speed.
Comment 4
Updated by Jim Nelson 3 months ago
- Target version set to 0.15.0
I think this is certainly a direction worth considering. If you looked back over the course of time at this method, you will see it bouncing back and forth between two approaches: loading the image scaled twice, or loading the image scaled at one size then scaling it down for the second. With normal JPEG this was a bit of a wash, but the second scaling meant the thumbnails were slightly less crisp, so we went with two scaled load-and-decodes. But with RAW, the slight improvement may not be worth it.
I'm unsurprised it didn't help the overall import time -- Shotwell import has been highly parallelized and speeding up one task doesn't necessarily improve the overall time. The problem with this approach is that it taxes the system more than some users would like.
In any event, this seems to be an improvement. Could you turn this into a patch and attach here?
Comment 5
Updated by Joe Bylund 3 months ago
- File 6812_remove_double_read.patch added
What about doing the scaled read at twice the size of the largest thumbnail, this should mean that all the thumbnails are pretty crisp, though maybe not as good as starting from full size every time. It still eliminates one load/unpack/scaled_read for raw files, and doesn't resort to an unscaled read (which is slower for raw's due to a different, more expensive, demoasicing/interpolation).
I'd think leave it open, but bump it to 0.16.
Also, is it kosher to treat a size as an int like this?
-Joe
Comment 6
Updated by Jim Nelson 3 months ago
- Status changed from Open to Review
I would do something like this:
int size = (int) Size.BIG * 2;
Those are enums for a reason, even if their values are set to their pixel sizes.
An easy change for me to make when I review this. No need for another patch, unless you'd like to clean it up yourself.
Comment 7
Updated by Jim Nelson 3 months ago
I see now why you can't make that an int so easily; the Size enum's scaling function is called.
I'd rather not muck around too much, so I added a comment and left it as-is.
Thanks!
Comment 8
Updated by Anonymous 3 months ago
- Status changed from Review to Fixed
Applied in changeset 3a424909.
Comment 9
Updated by Jim Nelson 3 months ago
- Status changed from Fixed to Open
- Target version changed from 0.15.0 to 0.16.0
Joe has asked that I re-open, as he feels the performance can be improved even further.
--- Bug imported by chaz@yorba.org 2013-11-25 22:11 UTC ---
This bug was previously known as bug 6812 at http://redmine.yorba.org/show_bug.cgi?id=6812 Imported an attachment (id=262790) Imported an attachment (id=262791)
Unknown milestone "unknown in product shotwell. Setting to default milestone for this product, "---". Setting qa contact to the default for this product. This bug either had no qa contact or an invalid one. Resolution set on an open status. Dropping resolution CC member joseph.bylund+shotwell@gmail.com does not have an account here The original submitter of attachment 262790 was joseph.bylund+shotwell@gmail.com, but he doesn't have an account here. Reassigning to the person who moved it here: chaz@yorba.org. The original submitter of attachment 262791 was joseph.bylund+shotwell@gmail.com, but he doesn't have an account here. Reassigning to the person who moved it here: chaz@yorba.org.
Version: 0.15.1
Resolution: RESOLVED FIXED