CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
G-API: VPP preprocessing GIEBackend integration #21688
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
@@ -336,6 +343,12 @@ template<typename Net> class Params { | |||
return *this; | |||
} | |||
|
|||
Params<Net>& cfgPreprocessingDeviceContext(void *device_ptr, void *context_ptr) { | |||
desc.device_ptr = cv::util::make_optional(device_ptr); |
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.
If we are leaving device_selector
out of aboard - how we then specify device & context type?
In future on Win/Lin there will possible situation when we should support SYCL/ DX11/VA/OCL and others preprocessing together
I still recommend to reconsider IDeviceSelector
approach to do not brake GAPI API in future
30ee6fd
to
fe02b0a
Compare
@@ -66,6 +67,28 @@ std::string get_weights_path(const std::string &model_path) { | |||
return model_path.substr(0u, sz - EXT_LEN) + ".bin"; | |||
} | |||
|
|||
cv::util::optional<cv::Rect> parse_roi(const std::string &rc) { |
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.
many changes in this files are just reverting back of CustomParseSSD
@@ -84,6 +84,9 @@ struct ParamDesc { | |||
// have 2D (Layout::NC) input and if the first dimension not equal to 1 | |||
// net.setBatchSize(1) will overwrite it. | |||
cv::optional<size_t> batch_size; | |||
|
|||
cv::optional<void *> device_ptr; |
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 propose to accumulate it into single struct
because they are used and initialized together
const cv::Size &in_parent_size, | ||
std::vector<cv::Rect> &out_objects) { | ||
const auto &in_ssd_dims = in_ssd_result.size; | ||
CV_Assert(in_ssd_dims.dims() == 4u); |
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.
GAPI_Assert
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.
ok
ret.size.width = static_cast<int>(inDims[3]); | ||
ret.size.height = static_cast<int>(inDims[2]); | ||
} else { | ||
GAPI_Assert(false && "Unsupported layout for VPP preproc"); |
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 put the layer name here as well.
Probably even path to the model in order to identify which one of infer
nodes failed
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.
good idea, thanks. I'll add LOG here with detailed description
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 about precision? Will it work if user passes FP16
data?
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 checked - it works in both cases
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.
Yep, since it works only with Frame
there is only U8
can be.
} | ||
|
||
auto res = map.emplace(input, ret); | ||
GAPI_Assert(res.second && "Duplicated input info in InputFrameDesc are not allowable"); |
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.
Can't imagine how it's possible
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.
all is possible in production
or after refactoring this component
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 about this assert
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.
About what then?
GAPI_LOG_DEBUG(nullptr, "network input: " << ii->name() << | ||
", tensor dims: " << inDims[0] << ", " << inDims[1] << | ||
", " << inDims[2] << ", " << inDims[3]); | ||
if (layout == InferenceEngine::NHWC) { |
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 about precision? I guess it must be U8
, since NV12
.
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'm not sure how to use it in preproc, because VPP doesn't have this option
if (ctx.uu.preproc_engine_impl) { | ||
GAPI_LOG_DEBUG(nullptr, "Try to use preprocessing for decoded remote frame"); | ||
cv::util::optional<cv::gapi::wip::pp_params> param = | ||
ctx.uu.preproc_engine_impl->is_applicable(frame); |
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.
In fact, is_applicable
was already called from InputFrameDesc
in outMeta
.
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.
this is another is_applicable
which tests if frame is VPL frame or not
const cv::Size &in_parent_size, | ||
std::vector<cv::Rect> &out_objects) { | ||
const auto &in_ssd_dims = in_ssd_result.size; | ||
CV_Assert(in_ssd_dims.dims() == 4u); |
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.
ok
@@ -325,8 +392,12 @@ int main(int argc, char *argv[]) { | |||
} | |||
#endif // HAVE_INF_ENGINE | |||
|
|||
// Turn on VPP preproc | |||
face_net.cfgPreprocessingDeviceContext(accel_device_ptr, accel_ctx_ptr); |
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 don't like idea about this function existence at all - see my comment about DeviceSelector https://github.com/opencv/opencv/pull/21688/files#r820065528
I propose new function setExecutionDevices (IDeviceSelector selector)
as complete solution instead:
And then somewhere in GIEBackend
std::vector<RemoteContext> rctx_array;
for ( auto [d,c] : selector)
{
rctx_array.push_back(CreateRemoteContext(d,c));
}
std::vector<IPreprocEngine> preproc_array;
for ( auto [d,c] : selector)
{
preproc_array.push_back(CreatePreprocEnginet(d,c));
}
But when the team is afraid about IDeviceSelector
introducing and and if they agree about renaming and some other modification to this part - i will apply it, let's waiting for other feedback
GAPI_LOG_DEBUG(nullptr, "network input: " << ii->name() << | ||
", tensor dims: " << inDims[0] << ", " << inDims[1] << | ||
", " << inDims[2] << ", " << inDims[3]); | ||
if (layout == InferenceEngine::NHWC) { |
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.
thanks, but current code has more vast covering when it sticks documented
logic in parsing layout description: my idea is - since layout
is introduced (and documented) then we must adhere its meaning despite on any verbal considerations about behavior
ret.size.width = static_cast<int>(inDims[3]); | ||
ret.size.height = static_cast<int>(inDims[2]); | ||
} else { | ||
GAPI_Assert(false && "Unsupported layout for VPP preproc"); |
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.
good idea, thanks. I'll add LOG here with detailed description
} | ||
|
||
auto res = map.emplace(input, ret); | ||
GAPI_Assert(res.second && "Duplicated input info in InputFrameDesc are not allowable"); |
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.
all is possible in production
or after refactoring this component
GAPI_LOG_DEBUG(nullptr, "network input: " << ii->name() << | ||
", tensor dims: " << inDims[0] << ", " << inDims[1] << | ||
", " << inDims[2] << ", " << inDims[3]); | ||
if (layout == InferenceEngine::NHWC) { |
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'm not sure how to use it in preproc, because VPP doesn't have this option
if (ctx.uu.preproc_engine_impl) { | ||
GAPI_LOG_DEBUG(nullptr, "Try to use preprocessing for decoded remote frame"); | ||
cv::util::optional<cv::gapi::wip::pp_params> param = | ||
ctx.uu.preproc_engine_impl->is_applicable(frame); |
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.
this is another is_applicable
which tests if frame is VPL frame or not
// It was decided to use GFrameDesc as such aggregated network info with limitation | ||
// that VPP/VPL produces cv::MediaFrame only. But it should be not considered as | ||
// final solution | ||
class InputFrameDesc { |
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.
InputFramesDesc
@@ -354,6 +395,69 @@ struct IEUnit { | |||
} | |||
}; | |||
|
|||
bool IEUnit::InputFrameDesc::is_applicable(const cv::GMetaArg &mm) { |
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.
Consider removing
GAPI_LOG_DEBUG(nullptr, "network input: " << ii->name() << | ||
", tensor dims: " << inDims[0] << ", " << inDims[1] << | ||
", " << inDims[2] << ", " << inDims[3]); | ||
if (layout == InferenceEngine::NHWC) { |
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.
Discussed online
// It was decided to use GFrameDesc as such aggregated network info with limitation | ||
// that VPP/VPL produces cv::MediaFrame only. But it should be not considered as | ||
// final solution | ||
class InputFrameDesc { |
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.
BTW, why don't you just leave:
std::unordered_map<std::string, cv::GFrameDesc> vpp_preproc_map;
and
cv::GFrameDesc configureVPPPreprocDesc(const IE::TensorDesc& desc) {
const auto& dims = desc.getDims();
return GFrameDesc{MediaFormat::NV12, cv::Size(dims[3], dims[2])};
}
for (auto &&it : ade::util::zip(ade::util::toRange(uu.params.input_names),
ade::util::toRange(in_metas))) {
...
if (preproc_engin_impl && cv::holds_alternative<cv::GFrameDesc>()) {
vpp_preproc_map.emplace(input_name, configureVPPPreprocDesc(ii->getTensorDesc()));
}
}
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 also accumulate this map
and vpp_preproc_engine
to some single place.
class VPPPreprocUnit {
std::unordered_map<string, MediaFrame> out_keep_alive_frames;
std::shared_ptr<cv::gapi::wip::IPreprocEngine> preproc_engine_impl;
std::unodered_map<string, GFrameDesc> vpp_preproc_map
}
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 intended to be a replacements for
IE::InputInfo:Ptr
andIE::InputInfo::CPtr
without ugly if for load & import - it's not supposed to be a VPL/VPP specific
preproc_map
but input description for possible other preprocess engines
also, because current GIEbackend implementations requires refactoring anyway - i think it is not goof idea to make suggestions about such cosmetic changes before overall refactoring taken place
}; | ||
if (!std::all_of(std::begin(delim), std::end(delim), is_delim)) { | ||
return cv::util::optional<cv::Rect>(); // empty value | ||
|
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.
Extra line
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
|
||
// NB: configure intput param for further preproc | ||
if (uu.net_input_params.is_applicable(mm)) { | ||
const_cast<IEUnit::InputFrameDesc &>(uu.net_input_params).set_param(input_name, ii); |
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 looks like both for()
from if()
and else()
have the same code. We have reshape
option for load and non_const_prepm
for import. But inputs
will look ugly. MB need to refactor this code.
|
||
// NB: configure input param for further preproc | ||
if (uu.net_input_params.is_applicable(mm)) { | ||
const_cast<IEUnit::InputFrameDesc &>(uu.net_input_params).set_param(input_name, ii); |
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.
Does this code deserve own function which will wrap if(load){} else{import} case? Numbers of Mat\Frame inputs in ctx can bring difficulties.
cv::GStreamingCompiled pipeline; | ||
try { | ||
if (opt_roi.has_value()) { |
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.
You can also divide the graph inputs:
cv::GOpaque<cv::Rect> in_roi;
auto inputs = cv::gin(cap);
auto graph_inputs = cv::GIn(in);
if (!opt_roi.has_value()) {
in_roi = custom::LocateROI::on(in);
} else {
graph_inputs += cv::GIn(in_roi); // <------------------------------- here
inputs += cv::gin(opt_roi.value())[0]);
}
auto blob = cv::gapi::infer<custom::FaceDetector>(roi, in);
cv::GArray<cv::Rect> rcs = cv::gapi::parseSSD(blob, sz, 0.5f, true, true);
auto out = cv::gapi::wip::draw::render3ch(in, custom::BBoxes::on(rcs, roi));
pipeline = cv::GComputation(std::move(graph_inputs), cv::GOut(out)) // and move here
.compileStreaming(std::move(face_detection_args));
It's just for information that, I believe, it can be done. But it's not necessary to change
fe593c6
to
77d1930
Compare
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.
Besides comments from others, LGTM, thanks!
@@ -84,6 +85,9 @@ struct ParamDesc { | |||
// have 2D (Layout::NC) input and if the first dimension not equal to 1 | |||
// net.setBatchSize(1) will overwrite it. | |||
cv::optional<size_t> batch_size; | |||
|
|||
cv::optional<cv::gapi::wip::onevpl::Device> vpl_preproc_device; |
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.
IMHO:
Since vpl_preproc_device
& vpl_preproc_ctx
aren't used separately, I'd put them into one struct
P.S Already discussed it, consider as optional
@@ -22,6 +22,7 @@ namespace onevpl { | |||
enum class AccelType: uint8_t { | |||
HOST, | |||
DX11, | |||
CUSTOM_ACCEL_TYPE = 127, |
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.
Actually don't get how it helps.
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.
ok, let's remove it
namespace detail | ||
{ | ||
struct DeviceContextCreator : public IDeviceSelector { | ||
DeviceScoreTable select_devices() const override { return {};} |
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 get the idea of adding select_devices()
& select_context()
that are never used now
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.
Both are part pure virtual methods of IDeviceSelector and DeviceSelector is single one whos responsible for creation Device& Context instances
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.
Let it be, but I don't think that it makes sense to add these methods in advance (they aren't used now).
|
||
template<typename Entity, typename ...Args> | ||
static Entity create_entity(Args &&...args) { | ||
return IDeviceSelector::create<Entity>(std::forward<Args>(args)...); |
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 DeviceContextCreate
is needed if we can create Device
/Context
via IDeviceSelector::create
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.
because IDeviceSelector::create
is protected
method
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.
IMHO overcomplicated, but hope you have a future design :)
@@ -417,6 +495,10 @@ class IECallContext | |||
// Input parameters passed to an inference operation. | |||
cv::GArgs m_args; | |||
cv::GShapes m_in_shapes; | |||
|
|||
// keep alive preprocessed frames | |||
std::mutex keep_alive_frames_mutex; |
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.
IMHO
Why don't you group it to a single struct
as well :)
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 see any benefits: because in comparison with InputFramesDescr
this seems as temporary solution and should be re-designed after refactoring of giebackend has taken place.
But InputFramesDescr
should survive factoring because it has got reusable part as input of ipreproc_engine at least
*out_is_preprocessed = true; | ||
} | ||
} // otherwise it is not suitable frame, then check on other preproc backend or rely on IE plugin | ||
return std::move(in_frame); |
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.
NRVO
should work here, shouldn't 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.
Despite in-param is r-value (cv::MediaFrame &&in_frame) it treated as l-value inside a function . So when you try to return it back - no NRVO has taken place for l-value here and copying would be performed.
Please find more clarified information in "Modern C++ by Meyers" in "Item 25: Use std::move on rvalue references,
std::forward on universal references"
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.
Agree that it will be treated as l-value, but don't get why NRVO
won't work 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.
because conditions for NRVO are not satisfied: in_frame
is not a local variable at least
cv::MediaFrame frame = ctx.inFrame(i); | ||
if (ctx.uu.preproc_engine_impl) { | ||
GAPI_LOG_DEBUG(nullptr, "Try to use preprocessing for decoded remote frame in remote ctx"); | ||
frame = preprocess_frame_impl(std::move(frame), layer_name, ctx, opt_roi, |
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.
Actually the copy of frame
should be almost free
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.
ok
but here we have move-operator by the way
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.
Do frame really moves into somewhere?
I doubt this backend code owns the frame to to move it. It comes as an input, and it may go somewhere else down to the pipeline. It may still work here, though, but may give some false understanding for those reading this code in the future.
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 moves back :)
Look:
cv::MediaFrame frame = ctx.inFrame(i);
frame = preprocess_frame_impl(std::move(frame));
ctx.uu.params.device_id << ", layer: " << layer_name << | ||
"is not supported yet"); | ||
GAPI_Assert(false && "Unsupported ROI blob creation for GPU remote context"); | ||
setBlob(req, layer_name, blob, ctx); |
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 we ignore roi
here, it will cause accuracy problems that will be difficult to debug
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.
because ROI must be applied at preproc-stage before this function invocation. it's fully applicable for current use-cases since "rctx" & "GPU" automatically assumes VPL+Dx11 usages
enum class AccelType: uint8_t { | ||
HOST, | ||
DX11, | ||
|
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.
extra space
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.
to separate functional values and max limited value
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.
ok
using namespace cv::gapi::wip; | ||
GAPI_LOG_INFO(nullptr, "VPP preproc creation requested"); | ||
preproc_engine_impl = | ||
IPreprocEngine::create_preproc_engine<onevpl::VPPPreprocDispatcher>( |
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.
Is preproc enabled with the received context and device parameters?
Can I use context and device for something else? Can it bring dual behavior in the future? (When I set device and context and activate preproc by chance or it is impossible case)
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.
to enable preprocessing we should call face_net.cfgPreprocessingParams(accel_device.value(), accel_ctx.value());
in explicit way
Please see https://github.com/opencv/opencv/pull/21688/files#diff-b4563bcdbe86eb5c8f1f2cce227809b02980777b25f5f4be2bb717826bedbebbR398
cv::GFrameDesc expected_net_input_descr = | ||
ctx.uu.net_input_params.get_param(layer_name); | ||
|
||
// TODO: Find a better place to convifure media format for GPU |
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.
convifure --> configure
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.
thanks
LAST_VALUE = std::numeric_limits<uint8_t>::max() | ||
}; | ||
|
||
GAPI_EXPORTS const char* to_cstring(AccelType type); |
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 we expose and export this function?
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 not?
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 propose to hide it if possible. As far as I can see, this is the implementation detail and external user shouldn't have access to this function
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's not implementation detail: it's public field which described public Device and Context types.
Since Device/Context is public and provides public method AccelType get_type()
then it's good option to provide human-readable type description
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.
Oh, how I see
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.
But why it is const char*
? We're in C++ 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.
"from classic"
also std::string is heavy for simple const-cstring returning operation
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 👍
if (accel_device.has_value() && accel_ctx.has_value()) { | ||
face_net.cfgPreprocessingParams(accel_device.value(), | ||
accel_ctx.value()); | ||
std::cout << "enforce VPP preprocessing on " << device_id << std::endl; |
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.
GAPI_LOG
?
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.
std::cout is widely used in other samples
ret.size.width = static_cast<int>(inDims[3]); | ||
ret.size.height = static_cast<int>(inDims[2]); | ||
} else { | ||
GAPI_Assert(false && "Unsupported layout for VPP preproc"); |
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.
Yep, since it works only with Frame
there is only U8
can be.
|
||
template<typename Entity, typename ...Args> | ||
static Entity create_entity(Args &&...args) { | ||
return IDeviceSelector::create<Entity>(std::forward<Args>(args)...); |
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.
IMHO overcomplicated, but hope you have a future design :)
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.
👍 can merge as-is, but please keep my general comments in mind as part of further re-arch
@@ -126,6 +130,8 @@ template<typename Net> class Params { | |||
, {} | |||
, 1u | |||
, {} | |||
, {} | |||
, {} |
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.
Meh. We should definitely do something with it
Params<Net>& cfgPreprocessingParams(const cv::gapi::wip::onevpl::Device &device, | ||
const cv::gapi::wip::onevpl::Context &ctx) { |
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 this is a strong dependency on the VPL component in the IE component, isn't it?
Ideally the two should be completely isolated at the API level (note I am not talking about the backend internals itself where we have to deal with it)
Are these structures available when VPL is NOT available? (At least to keep the code building)?
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.
Are these structures available when VPL is NOT available? (At least to keep the code building)?
yes, they are
LAST_VALUE = std::numeric_limits<uint8_t>::max() | ||
}; | ||
|
||
GAPI_EXPORTS const char* to_cstring(AccelType type); |
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.
But why it is const char*
? We're in C++ right? :)
struct IDeviceSelector; | ||
struct GAPI_EXPORTS Device { | ||
friend struct IDeviceSelector; | ||
using Ptr = void*; | ||
|
||
~Device(); | ||
const std::string& get_name() const; | ||
Ptr get_ptr() const; | ||
AccelType get_type() const; | ||
private: | ||
Device(Ptr device_ptr, const std::string& device_name, | ||
AccelType device_type); | ||
|
||
std::string name; | ||
Ptr ptr; | ||
AccelType type; | ||
}; | ||
|
||
struct GAPI_EXPORTS Context { | ||
friend struct IDeviceSelector; | ||
using Ptr = void*; | ||
|
||
~Context(); | ||
Ptr get_ptr() const; | ||
AccelType get_type() const; | ||
private: | ||
Context(Ptr ctx_ptr, AccelType ctx_type); | ||
Ptr ptr; | ||
AccelType type; | ||
}; |
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.
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.
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.
gapi::onevpl::Device
& gapi::onevpl::Context
were designed in type-erased way just to carry non-typed device pointer and context to data consumers. They are not related to vpl, just declared in its namespace. From my perspective any 'Priv' and interface like solution are superfluous here. Instead that we should move Device/Context out from 'cv::gapi::wip::onevpl' and put it into cv::gapi::wip
Additionally renaming would required as @TolyaTalamanov mentioned in one of his comment:
face_net.cfgPreprocessingParams(...)
on face_net.cfgVPPPreprocessingParams(...)
because now vpp
intention in preprocessing creation is expressing by namespace onevpl
for which Device/Context belong now
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.
Let's apply these changes in next PR under the same opencv release edition
out_rect = cv::Rect{ center.x - sqside/2 | ||
, center.y - sqside/2 | ||
, sqside | ||
, sqside | ||
}; |
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.
``
out_rect = cv::Rect{ center.x - sqside/2 | |
, center.y - sqside/2 | |
, sqside | |
, sqside | |
}; | |
out_rect = cv::Rect{ center.x - sqside/2 | |
, center.y - sqside/2 | |
, sqside | |
, sqside | |
}; |
cv::MediaFrame* IECallContext::prepareKeepAliveFrameSlot(req_key_t key) { | ||
std::lock_guard<std::mutex> lock(keep_alive_frames_mutex); | ||
return &keep_alive_pp_frames[key]; | ||
} |
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.
*
is way to C-ish, Y it can't be &
? I don't see you returning nullptr
here so not clear what is the case for *
.
*out_is_preprocessed = true; | ||
} | ||
} // otherwise it is not suitable frame, then check on other preproc backend or rely on IE plugin | ||
return std::move(in_frame); |
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 believe there is a compiler warning about that. At least I do remember we've fixed some in the past
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's trick described by Scott Meyers
cv::MediaFrame preprocess_frame_impl(cv::MediaFrame &&in_frame, ...) {
...
return std::move(in_frame);
}
it doesn't break NRVO because it's not applicable here
No warnings at all
cv::MediaFrame frame = ctx.inFrame(i); | ||
if (ctx.uu.preproc_engine_impl) { | ||
GAPI_LOG_DEBUG(nullptr, "Try to use preprocessing for decoded remote frame in remote ctx"); | ||
frame = preprocess_frame_impl(std::move(frame), layer_name, ctx, opt_roi, |
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.
Do frame really moves into somewhere?
I doubt this backend code owns the frame to to move it. It comes as an input, and it may go somewhere else down to the pipeline. It may still work here, though, but may give some false understanding for those reading this code in the future.
@alalek Could you merge it please ? |
There is build warning from builds with OpenVINO 2021.4.x:
|
G-API: VPP preprocessing GIEBackend integration * Add ROI in VPP prepro * Apply comments * Integration to IE * Removed extra invocations * Fix no-vpl compilation * Fix compilations * Apply comments * Use thin wrapper for device & ctx * Implement Device/Context constructor functions * Apply samples comment * Fix compilation * Fix compilation * Fix build * Separate Device&Context from selector, apply other comments * Fix typo * Intercept OV exception with pretty print out
Integration of VPP preprocessing into GIEBackend. This PR had done in term of #21554 PR.
InferROI is supported only
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.
Build Configuration