crash with defocus adjustement

[atsju]

1. Create wavefront
2. use defocus adjustment (I use the box at the top with corresponding arrows) to add or remove some defocus
3. DFTFringe crashes

Maybe I did something I'm not supposed to do. I do not know this feature, juste wanted to play with it a bit

Desktop (please complete the following information):

• OS: Win10
• DFTFringe version: 7.2.1 official release

Here is the igram I use with corresponding outline. I reproduced with other ones too 90deg.zip

[gr5]

This is probably the same crash in #151. Almost certainly a duplicate of #151.

I've experienced this also. If I don't use arrows and type in the values to reduce computation, I can try maybe 5 or 10 values before a crash. If you just mess around it's pretty easy for me to make it crash within 10 seconds.

At the same time, it's pretty easy to get it to not crash. Be mindful of the values you enter. Wait for the waveform to change before entering the next value. Highlight only the digit you want to change and type the new value rather than deleting and then entering (which requires 2 recalculations).

If there was a recalculate button this could probably avoid the crash.

[atsju]

Correct. Probably a duplicate. However only little defocus is needed to make it crash

[gr5]

I'm going to look at this next.

[gr5]

I found the bug. Wasn't too hard although it took an hour or two.

The problem is in line 1279 of zernikeprocess.cpp below (circled in red). When you move the defocus slider it instantly emits at least 2 events typically. The m_row vector is cleared just above at 1273 and then when it processes events it calls this function again, adds in a lot of values and then returns and we add more values to m_row such that later m_row (and m_col) have double the size as desired.

Then it crashes in zernikeProcess.cpp line 779 because it uses m_row in the for(i...) loop and goes well past the end of m_rowTheta.

I'm not sure what the fix is. Maybe disable this line of code if we are doing defocus. I'm actually thinking this can cause other bugs as well - like if you click on "recompute" twice in a row quickly? I tried that but it didn't crash. Maybe it knows it doesn't need to call rhotheta(). But it seems like there could be other ways this could cause a crash.

[gr5]

I'm thinking when the defocus event happens I trigger a timer and let more defocus events happen and restart the timer each time and when the timer times out (say 1 second) then do all the calculations and display the modified wavefront. if more events occur while processing we will restart the timer. The timer will also keep track of if we are still processing the last defocus and "extend the timer" until the previous processing is done.

Or skip the timer all together - all defocus events go into our own queue. When done processing the current defocus event, we skip to the end of the queue and process that one. We don't even need a queue - just keep track of the final defocus value that needs to be "processed and displayed".

Also if the final defocus value that we now need to process and display matches the defocus value that we are already displaying then we can just ignore it.

[githubdoe]

I wish I could get my mind back into this. At present I don't have any insights into it. There was a time and maybe still is when rhotheta calculation tried to be smart and not update itself when there was no need. If that help you any?

[gr5]

It's just that the circled line of code can allow this function to call itself. Which is bad. I'm working on a solution.

[atsju]

Sorry, I won't even open the code because I have no time. However the "timer" workaround doesn't sound like a proper fix. I don't know what the proper way to do is but with the right signal architecture and QT usage I don"t see the need of a queue either. Again without opening the code I won't enforce anything or block your idea. Just sharing my though

[gr5]

So I added code to make it only run one at a time but it still crashed due to the 2 second timer where it does all the wavefronts. I could add the code to there HOWEVER...

I just commented out that offending line of code (circled above) and the problem went away so I will make a PR with just that change.

[gr5]

Dale, what is that line of code for? I assumed it was for updating progress bars but when I commented it out and tested versus the latest release there was no difference in progress bar activity.

I tried a 1000X1000 DFT (to make it slower) and tried things like hitting "recalculate" button and defocus stuff and just processing igrams. Also changed to annulus type igram and back (annulus igrams definitely show a progress bar).

There was no progress bar other than for annulus and that line of code doesn't seem to be used for annulus. so:

Dale: What is that (circled in red) line of code for anyway? If not for progress bars?

[githubdoe]

It lets the gui have some cycles for any work it needs to do not just progress bars. Used where you have some heavy CPU intense operation that may take time to return. It helps the GUI stay responsive.

[githubdoe]

Using the defocus operation is problematic. The user would love to see immediate response of the graphs. However anytime the defocus value changes that triggers a cpu intense calculation that takes time. So I tried a compromise which is do not change anything until the control is in a stable position. It uses the timer to do that. If the control is moved within the timer time then the timer is reset and the graphs are not updated. When the timer finally times out it is responsible for getting the graphs updated. However it seems that one can then move the control again while the computations are being done which causes problems.

[githubdoe]

I think progress bar is one item that does not need the process events to be called because it is called in the loop to update it.

Perhaps the issue is then that once the timer has let the zernike process start then if another mouse defocus event happens then we have problems. Maybe we don't let the timer call the zernike process if there is a zernike process already running. Or maybe you can remove the process events without hurting GUI response which is I think what you are trying.

Assuming I'm understanding all that you have been trying to say.

[gr5]

Yes I think you understand my latest change. I summarize my final change here: pull request #156

It works nice - with 1000X1000 pixels the gui is slow but not too bad. With 200X200 pixels the responsiveness is great - you can drag around the defocus slider and it quickly updates the graph.

[gr5]

I'm hoping Dale, you or @atsju can approve these pull requests so I can merge them in.

[gr5]

fixed in pr #156

[gr5]

Fix will be in next release > 7.2.1