-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix highlights reconstruction issues #20183
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
Fix highlights reconstruction issues #20183
Conversation
|
Should this change the artifacts because it still looks the same as before for me? |
Some pre-demosaic code still requires snapping to sensor pattern.
44c7c39 to
6c7f027
Compare
|
|
@TurboGit i would suggest to merge this now for master and 5.4.1 About the OpenCL issues, it used to work on my nvidia system but does not on AMD. After more reports i would know more - possibly we might want to disable on AMD for now. |
1. In some OpenCL kernels we want to make sure we read valid data making sure we don't take negatives/NaN from image. This has always been an issue with AMD hardware when using some optimizer flags. 2. Observed artefacts resulted from opacity outside the 0-1 range when rewriting the cfa data. This was happening on CPU and OpenCL codepath. 3. False color representation didn't work correctly in OpenCL code path.
|
Friends, got it. For laplacian hightlights there were two issues:
@Jiyone you might also be interested :-) Release note: |
- also do mem checks
|
@TurboGit added 3rd commit, that's just simple maintenance so not necessarily for 5.4.1 |
Huu ? 🤔 |
|
Confirmed fixed for me |
TurboGit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was all good on my side, merged in master & 5.4.x branch. Thanks!
|
sorry wasn't able to check earlier on mac - I'm not sure if i exactly got the issue: |
|
@jenshannoschwalm : The commit Reintroduce demosaic snapping for stability has introduced 9 regressions:
I need your feedback to understand this. Why only those? I mean the commit changes roi x, y for all RAW, so why only those tests are affected? They are all sharing the same 2 RAWs (mire1.cr2 & xtransIV.raf) with many other tests. Probably because all of them are using flip/orientation or lens. Is that the proper explanation? TIA |
Absolutely :-) BTW i will do a rework on this making sure all pre-demosaic modules don't require this "snapping" |
Some highlights code didn't work correctly, so an urgent fix. See #20167
Removing the snapper from demosaicing
modify_roi_inlead to various problems in highlights reconstruction module, so let's keep it. It got unnoticed likely because almost-always peoply just stay with opposed ... Arrgh. Anyway, reintroducing the snapper doesn't hurt at all as we over-calculate the canvas so smooth moving-around works still perfectly.To test here is
Busker in Shadow.zip
There is still one annoying issue here on AMD/rustiCL, if i zoom in or just drag the main canvas around there are very strong artefacts, Very bad colors, sometimes the whole screen is blu or red ...
So i would like you all to test on your systems and report. There are some parts of the OpenCL code that are definitely out-of-1.2 specs, for sure that worked correctly some time ago on my nvidia system.
@TurboGit @piratenpanda (yes the video was like what i have but much-much more annoying here) @MStraeten as on mac please.