HiveBrain v1.2.0
Get Started
← Back to all entries
snippetcppMinor

Median filter for large mask

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
masklargefilterformedian

Problem

I'm trying to implement the fast median filter developed by T.S.Huang (HUANG, T.S. 1981. Two-Dimensional Signal Processing II: Transforms
and Median Filters. Berlin: Springer-Verlag, pp. 209-211.).

http://ieeexplore.ieee.org/stamp/stamp.jsp?arnumber=1163188&tag=1

Please review my code. My most important issue here is speed and I'm will to pay for optimization with more memory. I would like to use later on Cilk plus or Intel TBB in order to optimize the speed more.

Header file:

class TOOLBOX_EXPORT Preprocessor
{

public: 
    void Process(uint16_t*& data, int width, int height);
private:

    typedef enum
    {
        FORWARD,
        BACKWARD
    } TDirection;   

    void AddHist(uint16_t value,std::map &histogram);
    void RemoveHist(uint16_t value,std::map::iterator medianIter, std::map &histogram);
    int GetPixelIndex(int row, int col,int width, int height);

};


.cpp file:

```
void Preprocessor::AddHist(uint16_t value,std::map &histogram)
{
std::map::iterator iter = histogram.find(value);
if(iter != histogram.end())
{
iter->second++;
}
else
{
histogram.insert(std::pair(value,1));
}
}

void Preprocessor::RemoveHist(uint16_t value,std::map::iterator medianIter,std::map &histogram)
{
std::map::iterator iter = histogram.find(value);
if(iter != histogram.end())
{
if(iter->second == 1)
{
if(medianIter != iter)
{
histogram.erase(iter);
}
else
{
iter->second =0;
}
}
else
{
iter->second--;
}
}
}

int Preprocessor::GetPixelIndex(int row, int col,int width, int height)
{
while(!((row >= 0) && (row = 0) && (col = height)
row--;
if(col = width)

Solution

AddHist() is very long-winded for something that's exactly equivalent to ++histogram[value].

RemoveHist() does need to retain the iterator if it's to remove zero entries without repeating the lookup (use auto to avoid spelling out its long type name). But consider whether we really want to spend valuable cycles removing elements. It might be better to retain them, or even to use a plain array of counts instead of a std::map.

It appears that the value from the map is a count, which can't be negative; I think an unsigned type would be a better choice here.

Why do we have a loop in GetPixelIndex() to bring values into range? Even if we don't use std::clamp(), we can simply assign the nearest in-range values:

int Preprocessor::GetPixelIndex(int row, int col, int width, int height)
{
    if (row = height) {
        row = height - 1;
    }

    if (col = width) {
        col = width - 1;
    }

    return row*width + col;
}


General points:

  • Prefer preincrement and predecrement rather than postincrement and postdecrement, particularly for aggregate types such as map iterators.



  • Use values of +1 and -1 as the values for FORWARD and BACKWARD so they can be used directly (e.g. col += direction and direction = -direction) rather than in branches (which are much more expensive).



  • I think the printf() at the end of Process (presumably std::printf?) is some leftover debugging which doesn't belong in a production-quality function.

Code Snippets

int Preprocessor::GetPixelIndex(int row, int col, int width, int height)
{
    if (row < 0) {
        row = 0;
    } else if (row >= height) {
        row = height - 1;
    }

    if (col < 0) {
        col = 0;
    } else if (col >= width) {
        col = width - 1;
    }

    return row*width + col;
}

Context

StackExchange Code Review Q#121950, answer score: 2

Revisions (0)

No revisions yet.