Skip to main content
Code Review

Return to Answer

replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

You wrote your own quick_sort() method. Experience tells me that it's likely to have bugs in it. (I haven't spent time analyzing it to see for sure, but I know that every time I try to implement it myself, I end up screwing up some obscure case that only comes up after shipping it!) Use the standard library function qsort() qsort() instead.

You can calculate a histogram for an array of floats just like you would for integers. You can then use the histogram to find the median use the histogram to find the median.

You wrote your own quick_sort() method. Experience tells me that it's likely to have bugs in it. (I haven't spent time analyzing it to see for sure, but I know that every time I try to implement it myself, I end up screwing up some obscure case that only comes up after shipping it!) Use the standard library function qsort() instead.

You can calculate a histogram for an array of floats just like you would for integers. You can then use the histogram to find the median.

You wrote your own quick_sort() method. Experience tells me that it's likely to have bugs in it. (I haven't spent time analyzing it to see for sure, but I know that every time I try to implement it myself, I end up screwing up some obscure case that only comes up after shipping it!) Use the standard library function qsort() instead.

You can calculate a histogram for an array of floats just like you would for integers. You can then use the histogram to find the median.

Source Link
user1118321
  • 11.9k
  • 1
  • 20
  • 46

It's always cool to see image processing stuff here! Looks like some interesting stuff you're doing. There is a lot of code here, so I'll just give you some general ideas of what I think could be improved.

Give Things Useful Names

A lot of your functions and variables have meaningless or hard to understand names. For example lp2() doesn't tell me much. I can see from reading it for a few minutes that it's an integer log base 2 function. So maybe call it intLogBase2()?

Also, you have medfilt2() function that I presume does a median filter. Why "2"? Is it because it's 2D? Is there a medfilt() function that it replaced? If so, I don't see it, so why not just call it medfilt()?

Use const For Things That Don't Change

Most of your functions arguments are read-only. The function doesn't change them at all. When that's the case, you should declare them as const. It lets the compiler and anyone reading the code know that you don't intend for that variable to change in the function.

Don't Reinvent The Wheel

You wrote your own quick_sort() method. Experience tells me that it's likely to have bugs in it. (I haven't spent time analyzing it to see for sure, but I know that every time I try to implement it myself, I end up screwing up some obscure case that only comes up after shipping it!) Use the standard library function qsort() instead.

But it turns out you don't need to sort anything anyway...

There's A Faster Way To Calculate The Median

It turns out that floats were designed so that if you treat their bit patterns as integers and compare them, you'll get the correct answer. You can use this to your advantage. (I'd document in the comments of the function that you're doing it since it's non-obvious.)

You can calculate a histogram for an array of floats just like you would for integers. You can then use the histogram to find the median.

One trick is to do multiple passes. For example, cast each float to a 32-bit int. Then find the high byte of the median. Next, run the histogram again. But this time, if the high byte is less than the high byte of the median, simply count it as "less than" the median. If the high byte is greater than the median, just ignore it. If the high byte exactly equals the median, then put it into one of your 256 bins. At the end, you'll know the highest and second-highest bytes of the median by adding the number of "less than" values plus the sum of all bins up to the median. Do the same for the second-lowest byte and lowest byte.

Or, if you're OK with an approximation, bin the values into some set number of bins (like 1000, or whatever), and then just sort or run a histogram of the ones in the bin containing the 50th percentile.

I assume your convn() function is a convolution function? Is your convolution kernel separable? If so, you could do 3 1D passes over it and save quite a bit of time. If not, could you use an FFT to convert to frequency space and do the convolution there? (A convolution in frequency space is just a multiply, so it's faster.)

Do You Need To Allocate So Much Memory?

Your functions allocate and free a lot of memory. Can any of the tasks be done in place? Alternately, if the buffers are frequently the same size (either the size of the entire image or the size of a tile of the image) perhaps you could use a pool of pre-allocated buffers of the right size?

lang-c

AltStyle によって変換されたページ (->オリジナル) /