Dark frame subtraction

[Beep6581]

Originally reported on Google Code with ID 187

What steps will reproduce the problem?
1. Take a shot of several seconds: the image appears with halos and scattered white
(hot) pixels
2. If I had one or more dark shots (shots taken with cap on) I could use them to reduce
fixed noise.
RT lacks the feature to subtract dark frames.

Reported by ffsup2@yahoo.it on 2010-08-16 16:39:10

[Beep6581]

Here is a first implementation of the feature.
That addition required many changes to the process, so there are many other changes
to the GUI and to the engine: all debayering and preprocessing parameters are now on
a proper RAW tab next to other params.
This is a work in progress and has some know issues, but can be tested for feedback:
-color temperature read from camera is not coherent
-progress bar shows only during load (then during develop nothing change in GUI)
-there are no translation

About dark frames there are 2 possibility:
- selecting manually a single shot from everywhere ( with right click on thumb or from
RAW tab)
- autoselect: RT chooses from the direcory set in "preferences" the best match for
the photo, (if there are more than one with equal ISO and shutter they are mediated
to build a template) at startup RT prints in console what has found.

Reported by ffsup2@yahoo.it on 2010-08-18 23:03:28

---

• Attachment: DarkFrame1.patch

[Beep6581]

Pushed to head.

Reported by wyatt.olson on 2010-08-18 23:35:11

• Status changed: PatchApplied

[Beep6581]

Reported by wyatt.olson on 2010-08-18 23:35:48

[Beep6581]

Unfortunately, RawTherapee crashes immediately after clicking on the thumbnail in File
Browser (once, even not doubleclick) after this change.

And while compiling, gcc complains:

dfmanager.cc:193: warning: format ‘%d’ expects type ‘int’, but argument 2 has type
‘size_t’

Ubuntu 10.04 64 bit

Reported by iliaworld@yandex.ru on 2010-08-19 00:24:27

[Beep6581]

Sorry, i forgot to say, the error message is "segmentation fault"

Tomorrow I'll try using gdb, now it's time to sleep

Reported by iliaworld@yandex.ru on 2010-08-19 00:27:09

[Beep6581]

I have been helping to debug this feature, and I think it is the future for the RT workflow.
 Kudos to Fabio for putting it together so speedily; it seems it was a lot of work.

However, I think pushing it to head is premature; it is not sufficiently stable yet.
 I like that the patch is available for testing by all interested parties, but at this
point it might be better to keep head at a more stable point and apply the patch when
it is itself patched.  Meanwhile those who are following these issues threads can download
the patch and apply it themselves, while those who just want a RT that compiles and
runs reasonably stably can pull it from head.  Or is it possible to maintain separate
branches of head that can be easily merged at a later date?

Reported by ejm.60657 on 2010-08-19 01:08:37

[Beep6581]

Sounds good, Emil.

I have backed out the change, and re-added it in the 'darkframe' branch.  To checkout
this branch, make sure your repo is current (hg pull), then switch branches (hg update
darkframe).  To see a list of all branches, use 'hg branches'.  (This is the first
time I have used branches, so I am by no means an expert with them, but if you have
any questions please feel free to ask).

In theory once darkframe development is complete, we can merge with branch 'default'
(hopefully without too many conflicts! ;-).

Cheers

Reported by wyatt.olson on 2010-08-19 03:12:38

[Beep6581]

I've added a new Slicer class, that will be used to handle a single loop for the parallelised
demosaicing process, and also handle sub-region of the image (mainly for the RAW Tab).

Reported by natureh.510 on 2010-08-23 17:07:58

• Labels added: Type-Enhancement
• Labels removed: Type-Defect

[Beep6581]

Please, I need feedback about the features on darkframe branch (with others non win
platform too) before thinking to merge.

Reported by ffsup2@yahoo.it on 2010-10-08 20:40:30

[Beep6581]

Do you want to defer until after the merge the idea of using fast demosaic for initial
load, and final demosaic for preview crops?  That's the major item I would like to
see.

Reported by ejm.60657 on 2010-10-08 20:53:55

[Beep6581]

I like the idea of fast demosaic for initial load, but even if we are working on this
direction there is no real code for this specific feature yet.IMHO it's more important
not to diverge too much from main branch: the risk is too few feedbacks (as this is
the case....)
... and the work to keep in sync a branch is very hard and error prone.

Reported by ffsup2@yahoo.it on 2010-10-08 21:14:15

[Beep6581]

From my tests, it looks to be working well (OSX).  I will take another look at it tonight,
and see if there is anything else I can break.

Cheers

Reported by wyatt.olson on 2010-10-08 21:17:28

[Beep6581]

I have been playing with this some more, and have found a not consistently reproducible
crash.  It seems to be triggered by the following:

1) Open a raw image (happens whether or now I have a pixel mapping file declared)
2) Toggle "Auto Selection" for darkframe.  Sometimes it happens on the first try, other
times it takes a while.

Sometimes it happens right away, and other times it doesn't seem to happen at all :-(

The crash report is as follows:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000106
Crashed Thread:  4

Thread 4 Crashed:
0   rt                              0x0025c8ec rtengine::RawImageSource::cfaCleanFromMap(unsigned
char*) + 770
1   rt                              0x0026207e rtengine::RawImageSource::preprocess(rtengine::procparams::RAWParams
const&) + 2260
2   rt                              0x0029d460 rtengine::ImProcCoordinator::updatePreviewImage(int)
+ 466
3   rt                              0x002a025d rtengine::ImProcCoordinator::process()
+ 175
4   rt                              0x002a206c sigc::bound_mem_functor0<void, rtengine::ImProcCoordinator>::operator()()
const + 78
5   rt                              0x002a1f08 sigc::adaptor_functor<sigc::bound_mem_functor0<void,
rtengine::ImProcCoordinator> >::operator()() const + 20
6   rt                              0x002a1c82 sigc::internal::slot_call0<sigc::bound_mem_functor0<void,
rtengine::ImProcCoordinator>, void>::call_it(sigc::internal::slot_rep*) + 26
7   libglibmm-2.4.1.dylib           0x00b49276 call_thread_entry_slot + 70
8   libglib-2.0.0.dylib             0x0098bf48 g_thread_create_proxy + 152
9   libSystem.B.dylib               0x9616881d _pthread_start + 345
10  libSystem.B.dylib               0x961686a2 thread_start + 34

Since it is not consistently reproducible, I am assuming that it has to do with threading.

The raws I am using are the same ones that I uploaded a while back (the orange wall
shot at ISO 1600, with two darkframes shot at 1/8 and 1/250 at ISO 1600).

Reported by wyatt.olson on 2010-10-09 00:37:17

[Beep6581]

Just to clarify, I am running on OSX, 32 bit, with OpenMP enabled.

Reported by wyatt.olson on 2010-10-09 00:39:37

[Beep6581]

Thank you Wyatt, can you reproduce similar behaviour with other options in RAW tabs?
Or is this peculiar to "Auto Selection"?

Reported by ffsup2@yahoo.it on 2010-10-09 21:52:45

[Beep6581]

Looks like it is not just auto selection :-(  It takes a little while, but I managed
to crash the program when changing demosaic algorithms too:

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x00000000b07d7a6c
Crashed Thread:  4

Thread 4 Crashed:
0   rt                              0x00271022 rtengine::RawImageSource::ahd_demosaic(int,
int, int, int) + 12
1   rt                              0x00262ba6 rtengine::RawImageSource::demosaic(rtengine::procparams::RAWParams
const&) + 222
2   rt                              0x0029d491 rtengine::ImProcCoordinator::updatePreviewImage(int)
+ 515
3   rt                              0x002a025d rtengine::ImProcCoordinator::process()
+ 175
4   rt                              0x002a206c sigc::bound_mem_functor0<void, rtengine::ImProcCoordinator>::operator()()
const + 78
5   rt                              0x002a1f08 sigc::adaptor_functor<sigc::bound_mem_functor0<void,
rtengine::ImProcCoordinator> >::operator()() const + 20
6   rt                              0x002a1c82 sigc::internal::slot_call0<sigc::bound_mem_functor0<void,
rtengine::ImProcCoordinator>, void>::call_it(sigc::internal::slot_rep*) + 26
7   libglibmm-2.4.1.dylib           0x00b49276 call_thread_entry_slot + 70
8   libglib-2.0.0.dylib             0x0098bf48 g_thread_create_proxy + 152
9   libSystem.B.dylib               0x9616881d _pthread_start + 345
10  libSystem.B.dylib               0x961686a2 thread_start + 34

Reported by wyatt.olson on 2010-10-10 00:32:18

[Beep6581]

Also happens with line noise filter (just moving the slider back and forth):

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x0000000011522cbd
Crashed Thread:  4

Thread 4 Crashed:
0   rt                              0x0025c860 rtengine::RawImageSource::cfaCleanFromMap(unsigned
char*) + 630
1   rt                              0x0026207e rtengine::RawImageSource::preprocess(rtengine::procparams::RAWParams
const&) + 2260
2   rt                              0x0029d460 rtengine::ImProcCoordinator::updatePreviewImage(int)
+ 466
3   rt                              0x002a025d rtengine::ImProcCoordinator::process()
+ 175
4   rt                              0x002a206c sigc::bound_mem_functor0<void, rtengine::ImProcCoordinator>::operator()()
const + 78
5   rt                              0x002a1f08 sigc::adaptor_functor<sigc::bound_mem_functor0<void,
rtengine::ImProcCoordinator> >::operator()() const + 20
6   rt                              0x002a1c82 sigc::internal::slot_call0<sigc::bound_mem_functor0<void,
rtengine::ImProcCoordinator>, void>::call_it(sigc::internal::slot_rep*) + 26
7   libglibmm-2.4.1.dylib           0x00b49276 call_thread_entry_slot + 70
8   libglib-2.0.0.dylib             0x0098bf48 g_thread_create_proxy + 152
9   libSystem.B.dylib               0x9616881d _pthread_start + 345
10  libSystem.B.dylib               0x961686a2 thread_start + 34

(This looks to be the same as the first problem I reported, whereas the demosaic crash
looks to be different).

Reported by wyatt.olson on 2010-10-10 00:35:18

[Beep6581]

So seems only parameters in RAW tab cause the crash? Can you confim this? ( this would
be helpfull to diagnose the problem) But be carefull because I discovered another issue
moving sliders of WB in default branch also.(#251)

Reported by ffsup2@yahoo.it on 2010-10-10 18:26:46

[Beep6581]

Well, given that things are intermittent, it is difficult to be sure... that being said,
I tried doing a bunch of stuff (not WB, but sharpening, NR, shadow / highlights, etc)
on the same image and nothing crashed.  Doing the same things with the raw tab had
resulted in crashes on multiple occasions, so I think it is safe to say that that is
the problem.

I wish it was easier to reproduce the problem, but hopefully this helps at least :-(

Cheers

Reported by wyatt.olson on 2010-10-10 20:37:03

[Beep6581]

Wyatt, I can't reproduce your crash, but you can try a little change and report if it
has some effects?
comment 2 lines in  dcrop.cc line:62 and line:203,  "if (!internal)"

Reported by ffsup2@yahoo.it on 2010-10-24 18:05:58

[Beep6581]

Sorry, looks to be no change:  :-(

Thread 4 Crashed:
0   rt                              0x00282bb5 rtengine::RawImageSource::cfaCleanFromMap(unsigned
char*) + 757
1   rt                              0x002b336a rtengine::RawImageSource::preprocess(rtengine::procparams::RAWParams
const&) + 4458
2   rt                              0x002da165 rtengine::ImProcCoordinator::updatePreviewImage(int)
+ 4245
3   rt                              0x002dde7a rtengine::ImProcCoordinator::process()
+ 2298
4   rt                              0x002debbe sigc::internal::slot_call0<sigc::bound_mem_functor0<void,
rtengine::ImProcCoordinator>, void>::call_it(sigc::internal::slot_rep*) + 30
5   libglibmm-2.4.1.dylib           0x00a65276 call_thread_entry_slot + 70
6   libglib-2.0.0.dylib             0x008a7f48 g_thread_create_proxy + 152
7   libSystem.B.dylib               0x923e481d _pthread_start + 345
8   libSystem.B.dylib               0x923e46a2 thread_start + 34

Reported by wyatt.olson on 2010-10-24 20:35:21

[Beep6581]

This is the output of hg diff to show the change I made:

Alpha:rawtherapee wyatt$ hg diff
diff -r 28560ed56e4c rtengine/dcrop.cc
--- a/rtengine/dcrop.cc Sun Oct 17 10:49:16 2010 +0200
+++ b/rtengine/dcrop.cc Sun Oct 24 14:35:30 2010 -0600
@@ -59,7 +59,7 @@

 void Crop::update (int todo, bool internal) {

-    if (!internal)
+    //if (!internal)
         parent->mProcessing.lock ();

     ProcParams& params = parent->params;
@@ -200,7 +200,7 @@

     cropMutex.unlock ();

-    if (!internal)
+    //if (!internal)
         parent->mProcessing.unlock ();
 }

Reported by wyatt.olson on 2010-10-24 20:36:09

[Beep6581]

I found an error in cfaCleanFromMap, please try with latest patch.

Reported by ffsup2@yahoo.it on 2010-10-25 22:00:18

[Beep6581]

Well, now... I think we have a winner!  I tried to crash the thing for a good 5 minutes,
and could not even make it stutter :-)  Looks like you found the problem!

Cheers

Reported by wyatt.olson on 2010-10-26 03:55:03

[Beep6581]

Branch merge patch.

Reported by ffsup2@yahoo.it on 2010-10-30 20:17:39

---

• _Attachment: darkframe_branch_merge.patch_

[Beep6581]

Branch merge additional patch ( code left around )

Reported by ffsup2@yahoo.it on 2010-10-30 21:24:33

---

• _Attachment: darkframe_branch_merge2.patch_

[Beep6581]

tested these patches and all seems fine here.
will you commit these to 'default' branch or do you want to wait for more feedback?

Reported by janrinze on 2010-10-30 22:37:40

[Beep6581]

I have read through the entire patch and there is just one small thing that seems to
be 'off'.
rtengine/CA_correct_RT.cc seems to be working differently than in the 'default' branch.
Is this intentional or did you have a wrong version of that file?

Reported by janrinze on 2010-10-30 23:56:00

[Beep6581]

ejmartin can answer better than me, but I think it is all OK: there were patches on
darkframe branch in the past and lately a patch only on default branch.

I'll wait untill tomorrow evening and if there are no complaints I'll push the changesets

Reported by ffsup2@yahoo.it on 2010-10-31 10:23:44

[Beep6581]

How is it working differently?  

Looking a bit further, I made my last patch on Oct 6, just before Andrey merged his
clone into the default branch.  That latest version somehow didn't get incorporated
into the merge.  Please restore the version from my Oct 6 revision to both branches.

Reported by ejm.60657 on 2010-10-31 13:16:33

[Beep6581]

Actually, it's probably a good idea to check to see whether Andrey's merge clobbered
any other revisions made just prior.

Reported by ejm.60657 on 2010-10-31 13:41:44

[Beep6581]

Although it is good practice to check on changes I don't see any changes to what you
added okt.6th so for now I think all is good.
I have been running with the darkframe patches and the only thing that seems to be
bahaving differently is the hot/dead pixel filter. So after applying this patch there
may be some work to get that part checked out. I doubt it is much work though.

So apart from minor things like 'default' demosaicing method and a change in the way
hot/dead pixel filter works (possibly some setting) this patch works remarkably well!

Since there are multiple new patches in this 'issue' the status "Patch Applied" seems
to be counter intuitive. So i have set this back to "Patch Submitted"
When the darkframe merge is done sometime later today we can change it back to "Patch
Applied".

Reported by janrinze on 2010-10-31 14:39:14

• Status changed: PatchSubmitted

[Beep6581]

Look at the Oct 6 patch.  The lines that are changed in that patch in CA_correct are
not changed in the current code in either the default branch or the darkframe branch.
 That patch was implemented to resolve a bugreport and the CA_correct_RT.cc from that
version should be substituted for the current one in both branches.

Reported by ejm.60657 on 2010-10-31 15:01:33

[Beep6581]

Emil, if there is a patch that fixes a bug then you know you can sumbit that again without
nuch trouble, right?

Reported by janrinze on 2010-10-31 15:14:51

[Beep6581]

Look, I'm just asking that when the darkframe branch is merged, that CA_correct is properly
patched the way it was supposed to be on Oct 6 after my changeset was submitted.  

Reported by ejm.60657 on 2010-10-31 15:45:27

[Beep6581]

Maybe i misunderstand you, but you said it wasn't in the 'default' branch a.t.m. so
I see no justification to have this patch to be responsible for a fix.
However if this patch will fix it then that would be a nice bonus.
Please understand that this patch concentrates on the darkframe additions and hence
i expect it not to touch things that are unrelated to that.

Reported by janrinze on 2010-10-31 16:21:31

[Beep6581]

No, it's my mistake; I must have been looking at some other revision, it's in the current
default branch but not in darkframe.  Fabio will take care of it.

Reported by ejm.60657 on 2010-10-31 17:18:16

[Beep6581]

Ok.. we don't want newer patches to undo fixes already in 'default'.
Thanks for checking it.

Reported by janrinze on 2010-10-31 17:54:30

[Beep6581]

after some playing with the darkframe patches it seems that highlight reconstruction
has stopped working. This could be a bug. I am looking into it.

Reported by janrinze on 2010-10-31 19:00:58

[Beep6581]

Found and corrected.

Reported by ffsup2@yahoo.it on 2010-10-31 22:50:06

---

• _Attachment: darkframe_branch_merge3.patch_

[Beep6581]

Should we close this ticket as completed?  Also is there a need to keep the darkframe
branch in Mercurial, now that things have been merged?

Cheers

Reported by wyatt.olson on 2010-11-04 18:47:21

• Status changed: FixedPendingConfirmation

[Beep6581]

You can close this ticket.
Darkframe branch has been merged, so it can be closed.

Reported by ffsup2@yahoo.it on 2010-11-05 17:46:32

[Beep6581]

Closed.

Should I close the mercurial darkframe branch as well, or do you want to keep that
open for a bit in case you want to compare versions?

Cheers

Reported by wyatt.olson on 2010-11-05 17:48:20

• Status changed: Fixed

[Beep6581]

Sorry, I don't understand the implication of "closing" a branch in mercurial: 
I don't think there will be other commits on that branch. 
Does this mean one cannot update to an old revision into that branch?

Reported by ffsup2@yahoo.it on 2010-11-05 17:58:16

[Beep6581]

No, closing a branch does not limit you to update to it; it is just a flag to indicate
that development has stopped on that branch, and that it will not show in 'hg branches'
unless you add the '--closed' flag to show closed branches.

I will go ahead and close it now.  FYI you can read about closed branches (and how
to do so) at http://mercurial.selenic.com/wiki/PruningDeadBranches (I am using option
1).

Cheers

Reported by wyatt.olson on 2010-11-05 18:01:38