CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
G-API: IE. Adding support for INT32 type. #19792
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.
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
Conversation
04e831a
to
77dd154
Compare
{ 230, 0, 0 }, | ||
{ 32, 11, 119 }, | ||
{ 0, 74, 111 }, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two things above can be stored into config
namespace not to leave it without namespace..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you want to use namespace for here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, actually the goal is just to use namespace. To have all the supporting stuff in one place and be sure you're referring to the them right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tend do agree, config::colors
or config::keys
would be easier to read when you see it in the code below
CV_Assert(ext == ".xml"); | ||
return model_path.substr(0u, sz - EXT_LEN) + ".bin"; | ||
} | ||
} // anonymous namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's not really needed to store it in an anonymous namespace.
Maybe, to one of around - below or above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, need.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, at your discretion whether to move it into, for example, custom
namespace below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have this feature already? I believe someone in the team has proposed that a while ago -- only passing .xml
so that .bin
is identified automatically
@@ -2005,6 +2005,53 @@ TEST_F(InferWithReshapeNV12, TestInferListYUV) | |||
// Validate | |||
validate(); | |||
} | |||
|
|||
#if INF_ENGINE_RELEASE >= 2020010000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why such #if
needed in there, if it's handled inside giewrapper
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is for findDataFile(). For age gender we can find path for old IE. I don't know old path for semantic segmentation. And old IE isn't tested now, old path for network is deprecated.
old path - look at std::string SUBDIR
*
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I delete it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything's great, thanks. Some cavils are not important
{ 230, 0, 0 }, | ||
{ 32, 11, 119 }, | ||
{ 0, 74, 111 }, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, actually the goal is just to use namespace. To have all the supporting stuff in one place and be sure you're referring to the them right
CV_Assert(ext == ".xml"); | ||
return model_path.substr(0u, sz - EXT_LEN) + ".bin"; | ||
} | ||
} // anonymous namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, at your discretion whether to move it into, for example, custom
namespace below
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please prioritize merge
{ 230, 0, 0 }, | ||
{ 32, 11, 119 }, | ||
{ 0, 74, 111 }, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tend do agree, config::colors
or config::keys
would be easier to read when you see it in the code below
CV_Assert(ext == ".xml"); | ||
return model_path.substr(0u, sz - EXT_LEN) + ".bin"; | ||
} | ||
} // anonymous namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we have this feature already? I believe someone in the team has proposed that a while ago -- only passing .xml
so that .bin
is identified automatically
} | ||
}; | ||
|
||
GAPI_OCV_KERNEL(OCVPostProcessing, PostProcessing) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call it "colorize" or smt like this
cv::resize(maskImg, out, in.size()); | ||
const float blending = 0.3f; | ||
out = in * blending + out * (1 - blending); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd move it out of the kernel, if possible, and make it part of the graph.
Custom kernel could only produce a mask, and the rest done with our standard ops.
There are performance reasons to do that (e.g. involving a fluid backend for *
, +
, 1-
and so on)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, it can be done after we merge this fix itself, so just take all sample-related comments as a to-do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
@alalek I believe it can be merged
G-API: IE. Adding support for INT32 type. * Added support for int32 * Added sample for semantic-segmentation-adas-0001 * Alignment * Alignment 2 * Rstrt build * Removed test for sem seg
Summary:
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.
Magic: