-
-
Notifications
You must be signed in to change notification settings - Fork 55.7k
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
G-API: Introduce cv::MediaFrame, a host type for cv::GFrame #18415
Conversation
modules/gapi/src/api/media.cpp
Outdated
} | ||
|
||
cv::MediaFrame::View::~View() { | ||
GAPI_Assert(m_cb != nullptr); |
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 this may be dangerous, as GAPI_Assert
throws exceptions and ~View
could be called in context of an unwinding stack.
@mshabunin can the below issue be related to #18397 ?
|
@dmatveev , yes the location of OMZ have been changed. @alalek , is it possible to change environment variables |
Fixed (missing test data on build nodes after #18393 ) |
|
||
class GAPI_EXPORTS MediaFrame::IAdapter { | ||
public: | ||
virtual ~IAdapter() = 0; |
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 = 0 looks misleading since ~IAdapter is implemented. So it simply can be = default
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 never misleading as it was quite common in pre-Cxx11 era.
My main concern about = default
is that when a media.hpp
is included in the user application, its destructor will be generated by the user's compiler (different from ours)? I don't trust much to this :)
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 my point here is that = 0
usually says that user must implement this method in a derived class, but actually it turns out that there is an implementation in a base class and user can utilize this base class implementation, that's what I meant by misleading. You can ask your compiler to generate a destructor by putting = default
in a destructor definition at .cpp file
std::unique_ptr<IAdapter> adapter; | ||
}; | ||
|
||
cv::MediaFrame::MediaFrame() { |
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 simply write = default
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.
Same concern as 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.
But that's a .cpp, you can ask your compiler to generate this constructor here by
cv::MediaFrame::MediaFrame() = default;
e0bda12
to
0b22a4c
Compare
@rgarnov @AsyaPronina please have a look again. Need to merge this earlier to get the integration working. |
Windows build failed due to PCH generation problem (out of disk?) |
@alalek can you please proceed with merge? |
// | ||
// Copyright (C) 2020 Intel Corporation | ||
|
||
#include "precomp.hpp" |
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.
relative paths should be used (opencv_world build with PCH is broken - precomp.hpp
from another module may be used)
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.
Will fix in a follow-up MR, thanks
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.
Wait.. We've been never using relative paths for precomp.hpp
:
grep -Rn "precomp.h" .
./api/ft_render.cpp:7:#include "precomp.hpp"
./api/garray.cpp:8:#include "precomp.hpp"
./api/gcall.cpp:8:#include "precomp.hpp"
./api/gcomputation.cpp:8:#include "precomp.hpp"
./api/gframe.cpp:8:#include "precomp.hpp"
./api/gframe.cpp~:8:#include "precomp.hpp"
./api/ginfer.cpp:8:#include "precomp.hpp"
./api/gkernel.cpp:8:#include "precomp.hpp"
./api/gmat.cpp:8:#include "precomp.hpp"
./api/gnode.cpp:8:#include "precomp.hpp"
./api/gopaque.cpp:8:#include "precomp.hpp"
./api/gorigin.cpp:8:#include "precomp.hpp"
./api/gproto.cpp:8:#include "precomp.hpp"
./api/gscalar.cpp:8:#include "precomp.hpp"
./api/kernels_core.cpp:8:#include "precomp.hpp"
./api/kernels_imgproc.cpp:8:#include "precomp.hpp"
./api/kernels_nnparsers.cpp:8:#include "precomp.hpp"
./api/kernels_video.cpp:8:#include "precomp.hpp"
./api/operators.cpp:8:#include "precomp.hpp"
./api/render.cpp:1:#include "precomp.hpp"
./backends/common/gcompoundbackend.cpp:8:#include "precomp.hpp"
./backends/common/gcompoundkernel.cpp:8:#include "precomp.hpp"
./backends/cpu/gcpubackend.cpp:8:#include "precomp.hpp"
./backends/cpu/gcpucore.cpp:8:#include "precomp.hpp"
./backends/cpu/gcpuimgproc.cpp:8:#include "precomp.hpp"
./backends/cpu/gcpukernel.cpp:8:#include "precomp.hpp"
./backends/cpu/gcpuvideo.cpp:8:#include "precomp.hpp"
./backends/fluid/gfluidbackend.cpp:8:#include "precomp.hpp"
./backends/fluid/gfluidbuffer.cpp:8:#include "precomp.hpp"
./backends/fluid/gfluidcore.cpp:9:#include "precomp.hpp"
./backends/fluid/gfluidimgproc.cpp:9:#include "precomp.hpp"
./backends/ie/giebackend.cpp:7:#include "precomp.hpp"
./backends/ocl/goclbackend.cpp:8:#include "precomp.hpp"
./backends/ocl/goclcore.cpp:8:#include "precomp.hpp"
./backends/ocl/goclimgproc.cpp:8:#include "precomp.hpp"
./backends/openvx/govxbackend.cpp~:9:#include "precomp.hpp"
./backends/plaidml/gplaidmlbackend.cpp:10:#include "precomp.hpp"
./backends/plaidml/gplaidmlcore.cpp:8:#include "precomp.hpp"
./backends/render/grenderocvbackend.cpp:7:#include "precomp.hpp"
./compiler/gcompiled.cpp:8:#include "precomp.hpp"
./compiler/gcompiler.cpp:8:#include "precomp.hpp"
./compiler/gislandmodel.cpp:8:#include "precomp.hpp"
./compiler/gmodel.cpp:8:#include "precomp.hpp"
./compiler/gmodelbuilder.cpp:14:#include "precomp.hpp"
./compiler/gstreaming.cpp:8:#include "precomp.hpp"
./compiler/passes/dump_dot.cpp:8:#include "precomp.hpp"
./compiler/passes/exec.cpp:8:#include "precomp.hpp"
./compiler/passes/helpers.cpp:8:#include "precomp.hpp"
./compiler/passes/intrin.cpp~:8:#include "precomp.hpp"
./compiler/passes/islands.cpp:8:#include "precomp.hpp"
./compiler/passes/kernels.cpp:8:#include "precomp.hpp"
./compiler/passes/meta.cpp:8:#include "precomp.hpp"
./compiler/passes/streaming.cpp:7:#include "precomp.hpp"
./compiler/passes/transformations.cpp:7:#include "precomp.hpp"
./executor/gexecutor.cpp:8:#include "precomp.hpp"
./executor/gstreamingexecutor.cpp:7:#include "precomp.hpp"
Is this something new we should follow now?
@alalek thanks! |
* G-API: Introduce cv::MediaFrame, a host type for cv::GFrame * G-API: RMat -- address review comments
This PR brings a new data structure
cv::MediaFrame
, which is a host-type for a graph-basedGFrame
. The idea ofcv::MediaFrame
is to handle various media formats (also tied to an external memory) under the same uniform hood.There's only a
cv::MediaFrame
class itself + tests in this PR, no integration with the runtime part of G-API done yet (to be pushed and merged separately).Documentation is by now internal, but will be published as Doxygen with the upcoming PRs.
Patch to opencv_extra has the same branch name.