| CARVIEW |
Navigation Menu
-
-
Notifications
You must be signed in to change notification settings - Fork 56.2k
[G-API]: Add serialization mechanism for cv::MediaFrame #20329
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]: Add serialization mechanism for cv::MediaFrame #20329
Conversation
|
@TolyaTalamanov @AsyaPronina @mpashchenkov please, review |
| @@ -300,14 +306,48 @@ static cv::util::optional<GCompileArg> exec(const std::string& tag, cv::gapi::s1 | |||
|
|
|||
| template<typename T> struct deserialize_runarg; | |||
|
|
|||
| template<typename RMatAdapterType> | |||
| struct deserialize_runarg { | |||
| template<typename T, typename... Types> inline | |||
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 you really need inline 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.
You are right, removed
| }; | ||
|
|
||
| template<typename T, typename... Types> | ||
| struct deserialize_runarg<std::tuple<T, Types...>> { |
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.
Add a little doc/example 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.
Done
| * | ||
| * @note The actual logic is implemented by frame's adapter class. | ||
| * Does nothing by default. | ||
| * | ||
| * @param os Bytestream to store serialized MediaFrame data in. | ||
| */ | ||
| void serialize(cv::gapi::s11n::IOStream& os) const; |
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 does this method needs to be here? Is RMat implemented the same 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.
Yes, we did the same thing for RMat
|
|
||
| template<typename T, typename... Types> inline | ||
| typename std::enable_if<std::is_base_of<MediaFrame::IAdapter, T>::value, GRunArg>:: | ||
| type deserializeRMatHelper(cv::gapi::s11n::IIStream& is, uint32_t idx) { |
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 it still RMatHelper? I see this case applies to T being a MediaFrame::IAdapter.
Also just noticed RMat::Adapter isn't I? I will file a task for 5.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.
I added a note about how it works. Basically, yes - it's RMatHelper since it's being called for RMat RunArg, but we need enable_ifs for all adapter bases (it won't compile the other way)
| return os; | ||
| } | ||
| IIStream& operator>> (IIStream& is, cv::MediaFrame &) { | ||
| // Stub | ||
| GAPI_Assert(false && "cv::MediaFrame serialization is not supported!"); | ||
| util::throw_error(std::logic_error("operator>> for MediaFrame should never be called")); |
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.
Here and in RMat text I'd suggest an alternative. If this operator shouldn't be called, then what should be ?
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, that's not possible at our level
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.
Extended the message though
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, just referring to cv::gapi::deserialize -- isn'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.
Changed the message
| std::string m_str; | ||
| public: | ||
| MyMediaFrameAdapter() = default; | ||
| MyMediaFrameAdapter(cv::Mat m, int value, const std::string& str) |
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.
const cv::Mat& m, const int 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.
I just copypasted it from another test
| virtual void deserialize(cv::gapi::s11n::IIStream& is) override { | ||
| is >> m_value >> m_str; | ||
| } | ||
| int getVal() { return m_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.
const 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.
I just copypasted it from another test
| is >> m_value >> m_str; | ||
| } | ||
| int getVal() { return m_value; } | ||
| std::string getStr() { return m_str; } |
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.
const 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.
I just copypasted it from another test
| EXPECT_EQ(sc, out_sc); | ||
|
|
||
| cv::Mat out_mat = cv::util::get<cv::Mat>(out[2]); | ||
| EXPECT_EQ(0, cv::norm(mat, out_mat)); |
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.
For such check I'd add some initialization for mat, don't you think so?
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 just copypasted it from another test. There are like 20 places here without mat initialization, let's do it separately
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, it's actually initialized - it's an eye matrix (cv::Mat::eye())
| cv::Mat mat2 = cv::Mat::eye(cv::Size(128, 64), CV_8UC3); | ||
|
|
||
| auto frame = cv::MediaFrame::Create<MyMediaFrameAdapter>(mat, 42, "It actually works"); | ||
| cv::RMat rmat = cv::make_rmat<MyRMatAdapter>(mat2, 24, "Hello there"); |
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.
auto?
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.
Done
| cv::Mat mat2 = cv::Mat::eye(cv::Size(128, 64), CV_8UC3); | ||
|
|
||
| auto frame = cv::MediaFrame::Create<MyMediaFrameAdapter>(mat, 42, "It actually works"); | ||
| cv::RMat rmat = cv::make_rmat<MyRMatAdapter>(mat2, 24, "Hello there"); |
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.
auto?
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.
Done
|
@TolyaTalamanov @AsyaPronina @dmatveev please, take a look once again |
|
@dmatveev please, take a look once again |
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.
Great! Go ahead and merge! 👍
|
Need to fix compilation issues. |
smirnov-alexey
force-pushed
the
as/mediaframe_serialization
branch
from
9f718ad to
8cbcd52
Compare
|
@alalek can we merge this, please? |
…ialization [G-API]: Add serialization mechanism for cv::MediaFrame * Stub initial interface * Fix templates for deserialization * Fix tests * Disable a warning on windows * Address review comments * Change enable_ifs to other template helpers * Resolve ambiguous template * Fix warnings in docs
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.