-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Pipe scaling redesign #18958
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Pipe scaling redesign #18958
Conversation
👍 |
I won't get around to trying this out until the weekend. This sounds like a good idea, but what's the performance impact on thumbnail generation (especially without OpenCL) if everything up to demosaic always uses full-resolution data? That and a slower first rendering for darkroom view could negatively affect responsiveness. |
Would this be the case after every wipe of the cache? So every time you select a different history item or press undo? On older/less capable machines, would the user see more tiling for those first few modules that are now going to process the full image? Would this suggest that the location of the pipescale module should be a per-user setting rather than a per-image setting? If the ioporder of the module gets saved in the xmp/database, then could hypothetically loading an image prepared on a more capable machine kill a lower powered one? How would I go about changing its location before loading the image (or for all images)? (Maybe I could apply a non-default order, but that would also erase any other modules' custom locations?) If the location is customizable (i.e. the pipescale module is visible and draggable) I believe there is a mechanism to limit its location to before certain other modules? (can't remember now how that is done). So ashift/crop/lens could be kept safe? Is this meant to (eventually) replace (or be integrated with) "high quality processing" or is there a subtle difference in intended use? |
A cumulative answer - i did a lot of code run analysis to confirm: All stuff happening in the pipe before the pipescale module is done just once, after that we have a 100% cache hit rate for input of pipescale except one of the modules before have a parameter changed, then we have to recalculate from what the cache might already have , in general the cache hit rate is also better now as we always have a roi of roi->x = 0, roi_y = 0 roi->width = full and roi_height = full. Everything that might reduce performance for the mentioned first run would be from modules after demosaic, (demosaic currently uses full data mostly if watching the full image in darkroom) those currently run on downscaled data thus possibly would have to tile. But keep in mind, from now on all that stuff would never be recalculated when zooming in/out or dragging around the selected part of the image main darkroom window. Yes - there is a performance penalty for low quality thumbnails non starting from raw data as we don't have the low-quality demosaicing any longer. But - that is not happening if you are using the the inpaint opposed for highlights anyway. HQ redering does the whole pipe, this PR does HQ rendering up to the new module but the big point is the idea that most user edits while developing the image happen in modules after that. This means you can easily switch on heavy stuff for highlights, the new capture sharpen, denoising, raw ca correction or lens code even on slower machines and the responsiveness will be as if those were not switched on (after the initial run in darkroom. Yes - we could probably setup a user defined iop_order of that module, not sure how to handle that though and i would not think it's important. About the upper limit, i am not sure yet how to handle all cases with the mentioned modules crop/ashift. Above lens is certainly safe. |
I like the idea a lot. Mostly because I use Diffuse and Sharpen quite often for clarity / local contrast, which requires high quality processing for an accurate preview I can work with. What I'm not entirely sure about is the UI: As modules are meant for some image processing steps, it may be confusing for a non-technical user to have such a "dummy"-module. |
e76cc38
to
17a69bf
Compare
force-pushed a simpler version (still learning how to do things this way) providing most things working and by far easier to read (just the third commit) Still with capture sharpen included so beware your edits and database. |
Basically yes.
Right, i am also not sure how to do this "best". I am just sure about this: |
17a69bf
to
c753ca1
Compare
This would be my proposal: That way it's easily accessible if needed, but out of the way otherwise. There are potentially other modules that could benefit from this as well, such as #18926, but also hypothetical histogram and color picker readouts (I seem to remember a discussion on that). |
c753ca1
to
148f4ec
Compare
Understood. But could HQ processing now be implemented as just a special case of this new functionality where the new module is placed all the way at the end of the pipe? We don't want to have duplication of mostly the same functionality in different places. (Even if that allows making them behave just slightly differently, especially when that might just confuse users about the intent and expected behavior for no good reason) Could you explain your reasons to implement this as a module, rather than fully within the pixelpipe processing code? I understand you get reordering functionality for free, but maybe there were other considerations? My main doubt is whether this should be a per-image setting (which gets saved with the rest of the pipe history) or a per-user setting (which stays the same when switching between images). Also, once this module is introduced, it will need to be supported "forever", even if, say, the aforementioned integration with HQ processing for example might lead to a different approach that doesn't use the module anymore or if other optimisations render it irrelevant/suboptimal.
You are absolutely right; the wiping only happens for the preview pipe, so not relevant here.
The module order lib doesn't normally control the visibility of modules; imho this would fit more logically in the modulegroups hamburger menu. Or even as a special case (ctrl+click) of the "show all active modules" modulegroup. I would not particularly want to advocate for that last option, but it does have the advantage that the result is immediately visible, whereas toggling an option when in a modulegroup that doesn't contain any of the impacted modules leads to no visible results. "technical" might not be the best description. There's already an icon in modulegroups for "technical" modules. Maybe "internal" or even "pixelpipe"? If a special category of "internal" modules is created, could/should we exclude these from saving in history/database/xmp? Their location could then be stored in a pref (after any relocation by dragging) and restored when loading/switching images. Again, not advocating to implement this and then find reasons to use it, but if having these types of operations visible in the module list is deemed to be the best approach, then maybe they would need to be treated as special cases. |
Capture sharpening has been implemented to work inside the demosaic module so it's raw only. Credits to: Ingo Weyrich (heckflosse67@gmx.de), he implemented the original algorithm for rawtherapee, this implementation is based on his work, especially the convolution kernels. CPU and OpenCL code paths are both available. Demosaic module gets more parameters so there is a version bump, one still unused float parameter has been reserved. A "mini manual" Capture sharpening (CS) tries to recover details lost due to in-camera blurring, which can be caused by diffraction, the anti-aliasing filter or other sources of gaussian-type blur. Prerequisites are - good white balance parameters (same requirement as for highlights reconstruction or demosaic) - no chromatic aberration, you might want to add the "raw chromatic aberration" module - sensor noise will be amplified by CS controls: 1. capture sharpen switches CS on if above zero and defines the strength of overall effect. CS works in an iterative process, this defines the number of iterations, mostly a setting of 10 will be enough. 2. radius defines the basic convolution gaussian sigma. This should not be set by "creative means" but to the blurring radius of the optical system and sensor, too large values will lead to artifacts like halos. Calculating a correct radius is provided internally. This will be done either if you a) click on the button besides the slider b) activate capture sharpen the first time after resetting to demosaic defaults or developing old edits. 3. contrast threshold As sensor noise will be amplified by CS we take some care about this by a per pixel variance analysis and restrict CS to locations with higher variance. The default is good for low to medium ISO images. 4. corner boost Increase the radius in image corners. We assume a circle of 1/2 of image size to be "sharp" (only use main radius), locations outside this center circle get an increased convolution radius.
1. Remove code that hinted we could snap to other locations than upper/left corner of sensor pattern. Not a good idea at all due to what we provide in rawprepare. 2. Remove special snapper cases for passthru modes to avoid exposing jumps when changing demosaicers and it's simply not worth trying to do so. 3. Amaze tiling should happen with a slightly larger overlap 4. As we demosaic at a snap position in all cases the color smoothing and green averaging for bayer sensors don't require roi x/yshifts. 5. If we use the VNG demosaicer for some good reason like maze patterns we will very likely want good quality output as from all other demosaicers for details mask or capture sharpening so let's always use the "quality phase". This is **not** required / wanted for a) mask passthru mode b) low-frequency content of dual demosaicers 6. Avoid one copy cl buffer step for vng linear only
Analysis: With current pixelpipe and cache design we must reprocess the full pipe whenever we zoom in/out or drag around, as we select another area and thus the hash won't match. Solution: 1. Decouple crop&scale from demosaic module and introduce a "pipescale" module that presents the currently chosen roi. 2. Make sure modules before pipescale always process full image data. 3. By doing so we will have a 100% cache hit rate for the pipescale modulule if there are no changed parameters in any of the modules before. As those modules before pipescale could include very performance costly algorithms the UI will now be far more responsive. 4. The default pipescale position is now right after demosaicing, the current implementation allows dragging it's position further up the pipe so it could also include costly stuff like denoising or even notorious modules like dehaze as that loves to be processed with full image data. 5. As we now always have full image data available before and including demosaicing all raw-only modules can use simplified code reducing code and complexity. It will also be possible to provide details mask while demosaic tiling. 6. The scale module is also available for non-raw files allowing crop&scale to be done **after** colorin. (One problem here so far: the preview pipe scaling is not fixed so far) 7. The low quality demosaicers are gone as we always have full data to be processed.
148f4ec
to
a009566
Compare
Pricipally yes.
Very simple answer. The basic idea - where it started - was, how can we improve the pipe performance assuming that while editing most early modules in the pipe are possibly switched on/off or modified but won't be touched later and their output could be thought of as stable. I had no idea about how that would feel like UI wise without introducing performance drops. Thus a module as that could be investigated easily. (Also i wouldn't know how to handle that directly in pixelpipe code without introducing at lot of hairy things ...) While testing this i also observed that it could be used for "special" non-raw images that require downscaling after colorin.
Well, this is not a strong point or even irrelevant. We have to maintain downscaling - now done in demosaic - anyway. Doing this in a separate module reduces code complexity at many places. |
Two things that have been daunting me for quite some time
pinging @dterrahe @ralfbrown @TurboGit here for feedback.
Please note that this PR includes the capture sharpening commit (that includes a demosaic version bump ...)
Redesign pixelpipe input scaling
Analysis: With current pixelpipe and cache design we must reprocess the full pipe whenever we zoom in/out or
drag around, as we select another area and thus the hash won't match.
Solution:
As those modules before pipescale could include very performance costly algorithms the UI will be far more responsive.
(One problem here so far: the preview pipe scaling is not fixed so far)
EDIT: Would appreciate feedback from all: