Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix memory leak in regions(cpu backend) & rgb2ycbcr(api level) #1666

Merged
merged 2 commits into from Dec 19, 2016
Merged

fix memory leak in regions(cpu backend) & rgb2ycbcr(api level) #1666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8590d4
fix memory leak in regions cpu backend
9prady9 Dec 7, 2016
d1e61c2
fix memory leak in rgb ycbcr conversion fns
9prady9 Dec 14, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 11 additions & 12 deletions src/api/c/ycbcr_rgb.cpp
View file
Open in desktop
Expand Up @@ -10,7 +10,6 @@
#include <af/defines.h>
#include <af/dim4.hpp>
#include <af/image.h>
#include <af/index.h>
#include <handle.hpp>
#include <err_common.hpp>
#include <backend.hpp>
Expand Down Expand Up @@ -85,18 +84,18 @@ static af_array convert(const af_array& in, const af_ycc_std standard)

// extract three channels as three slices
// prepare sequence objects
af_seq slice1[4] = { af_span, af_span, {0, 0, 1}, af_span };
af_seq slice2[4] = { af_span, af_span, {1, 1, 1}, af_span };
af_seq slice3[4] = { af_span, af_span, {2, 2, 1}, af_span };
// index the array for channels
af_array ch1Temp=0, ch2Temp=0, ch3Temp=0;
AF_CHECK(af_index(&ch1Temp, in, 4, slice1));
AF_CHECK(af_index(&ch2Temp, in, 4, slice2));
AF_CHECK(af_index(&ch3Temp, in, 4, slice3));
// get Array objects for corresponding channel views
Array<T> X = getArray<T>(ch1Temp);
Array<T> Y = getArray<T>(ch2Temp);
Array<T> Z = getArray<T>(ch3Temp);
const Array<T>& input = getArray<T>(in);
std::vector<af_seq> indices(4, af_span);

indices[2] = {0, 0, 1};
Array<T> X = createSubArray(input, indices, false);

indices[2] = {1, 1, 1};
Array<T> Y = createSubArray(input, indices, false);

indices[2] = {2, 2, 1};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are making another commit in this PR, can you add a comment saying why you are only assigning to indices[2] in all the 3 cases. I think it would help code readability.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shehzan10 Because he is reading the three channels into 3 different arrays ?

@9prady9 Can you check if something like createSubArray(input, {af_span, af_span, {1,1,1}}, false); works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left comments before the beginning of extraction of channels. In any case, i don't have any other commits left in this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know why he is doing it, but it took me a bit to realize it. That's why I said "if you are making another commit", because its not that much of a priority. It just helps anyone in the future who is going to read it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavanky I checked by running the existing tests, why would we need copy's of original Array ? Just indexed Array's are sufficient right ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a possibility that the meta-data may not be properly altered ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@9prady9 Check my comment again. I did not say anything about copy, I was wondering if array initializers work instead of using vectors.

Array<T> Z = createSubArray(input, indices, false);

if (isYCbCr2RGB) {
dim4 dims = X.dims();
Expand Down
86 changes: 43 additions & 43 deletions src/backend/cpu/kernel/regions.hpp
View file
Open in desktop
Expand Up @@ -9,6 +9,7 @@

#pragma once
#include <Array.hpp>
#include <memory.hpp>

namespace cpu
{
Expand Down Expand Up @@ -98,78 +99,77 @@ static void setUnion(LabelNode<T>* x, LabelNode<T>* y)
template<typename T>
void regions(Array<T> out, const Array<char> in, af_connectivity connectivity)
{
const af::dim4 in_dims = in.dims();
const char *in_ptr = in.get();
T *out_ptr = out.get();
const af::dim4 inDims = in.dims();
const char *inPtr = in.get();
T *outPtr = out.get();

// Map labels
typedef typename std::map<T, LabelNode<T>* > label_map_t;
typedef typename label_map_t::iterator label_map_iterator_t;
typedef typename std::unique_ptr< LabelNode<T> > UnqLabelPtr;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use consistent naming conventions within a file.

typedef typename std::map<T, UnqLabelPtr > LabelMap;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used once. It is an unnecessary indirection but not a big deal. Your call

Also. Use the using keyword for type aliasing. They are easier to read and more powerful.

using LabelMap = typename std::map<T, UnqLabelPtr>;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just one character smaller..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easier (for me) to read because it uses the assignment style to define the alias, not because it is shorter.

typedef typename LabelMap::iterator LabelMapIterator;

label_map_t lmap;
LabelMap lmap;

// Initial label
T label = (T)1;

for (int j = 0; j < (int)in_dims[1]; j++) {
for (int i = 0; i < (int)in_dims[0]; i++) {
int idx = j * in_dims[0] + i;
if (in_ptr[idx] != 0) {
for (int j = 0; j < (int)inDims[1]; j++) {
for (int i = 0; i < (int)inDims[0]; i++) {
int idx = j * inDims[0] + i;
if (inPtr[idx] != 0) {
std::vector<T> l;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better if this was a std::array instead of a std::vector. Since std::arrays are allocated on the stack, you don't get the penalty of calling malloc.

I know its not part of this PR. Perhaps in another issue.


// Test neighbors
if (i > 0 && out_ptr[j * (int)in_dims[0] + i-1] > 0)
l.push_back(out_ptr[j * in_dims[0] + i-1]);
if (j > 0 && out_ptr[(j-1) * (int)in_dims[0] + i] > 0)
l.push_back(out_ptr[(j-1) * in_dims[0] + i]);
if (i > 0 && outPtr[j * (int)inDims[0] + i-1] > 0)
l.push_back(outPtr[j * inDims[0] + i-1]);
if (j > 0 && outPtr[(j-1) * (int)inDims[0] + i] > 0)
l.push_back(outPtr[(j-1) * inDims[0] + i]);
if (connectivity == AF_CONNECTIVITY_8 && i > 0 &&
j > 0 && out_ptr[(j-1) * in_dims[0] + i-1] > 0)
l.push_back(out_ptr[(j-1) * in_dims[0] + i-1]);
j > 0 && outPtr[(j-1) * inDims[0] + i-1] > 0)
l.push_back(outPtr[(j-1) * inDims[0] + i-1]);
if (connectivity == AF_CONNECTIVITY_8 &&
i < (int)in_dims[0] - 1 && j > 0 && out_ptr[(j-1) * in_dims[0] + i+1] != 0)
l.push_back(out_ptr[(j-1) * in_dims[0] + i+1]);
i < (int)inDims[0] - 1 && j > 0 && outPtr[(j-1) * inDims[0] + i+1] != 0)
l.push_back(outPtr[(j-1) * inDims[0] + i+1]);

if (!l.empty()) {
T minl = l[0];
for (size_t k = 0; k < l.size(); k++) {
minl = min(l[k], minl);
label_map_iterator_t cur_map = lmap.find(l[k]);
LabelNode<T> *node = cur_map->second;
LabelMapIterator currentMap = lmap.find(l[k]);
LabelNode<T> *node = currentMap->second.get();
// Group labels of the same region under a disjoint set
for (size_t m = k+1; m < l.size(); m++)
setUnion(node, lmap.find(l[m])->second);
setUnion(node, lmap.find(l[m])->second.get());
}
// Set label to smallest neighbor label
out_ptr[idx] = minl;
}
else {
outPtr[idx] = minl;
} else {
// Insert new label in map
LabelNode<T> *node = new LabelNode<T>(label);
lmap.insert(std::pair<T, LabelNode<T>* >(label, node));
out_ptr[idx] = label++;
lmap.insert(std::make_pair(label, UnqLabelPtr(new LabelNode<T>(label))));
outPtr[idx] = label++;
}
}
}
}

std::set<T> removed;

for (int j = 0; j < (int)in_dims[1]; j++) {
for (int i = 0; i < (int)in_dims[0]; i++) {
int idx = j * (int)in_dims[0] + i;
if (in_ptr[idx] != 0) {
T l = out_ptr[idx];
label_map_iterator_t cur_map = lmap.find(l);
for (int j = 0; j < (int)inDims[1]; j++) {
for (int i = 0; i < (int)inDims[0]; i++) {
int idx = j * (int)inDims[0] + i;
if (inPtr[idx] != 0) {
T l = outPtr[idx];
LabelMapIterator currentMap = lmap.find(l);

if (cur_map != lmap.end()) {
LabelNode<T>* node = cur_map->second;
if (currentMap != lmap.end()) {
LabelNode<T>* node = currentMap->second.get();

LabelNode<T>* node_root = find(node);
out_ptr[idx] = node_root->getMinLabel();
LabelNode<T>* nodeRoot = find(node);
outPtr[idx] = nodeRoot->getMinLabel();

// Mark removed labels (those that are part of a region
// that contains a smaller label)
if (node->getMinLabel() < l || node_root->getMinLabel() < l)
if (node->getMinLabel() < l || nodeRoot->getMinLabel() < l)
removed.insert(l);
if (node->getLabel() > node->getMinLabel())
removed.insert(node->getLabel());
Expand All @@ -179,11 +179,11 @@ void regions(Array<T> out, const Array<char> in, af_connectivity connectivity)
}

// Calculate final neighbors (ensure final labels are sequential)
for (int j = 0; j < (int)in_dims[1]; j++) {
for (int i = 0; i < (int)in_dims[0]; i++) {
int idx = j * (int)in_dims[0] + i;
if (out_ptr[idx] > 0) {
out_ptr[idx] -= distance(removed.begin(), removed.lower_bound(out_ptr[idx]));
for (int j = 0; j < (int)inDims[1]; j++) {
for (int i = 0; i < (int)inDims[0]; i++) {
int idx = j * (int)inDims[0] + i;
if (outPtr[idx] > 0) {
outPtr[idx] -= distance(removed.begin(), removed.lower_bound(outPtr[idx]));
}
}
}
Expand Down