Skip to content

[G-API]: Add serialization mechanism for cv::MediaFrame#20329

Merged
alalek merged 10 commits intoopencv:masterfrom
smirnov-alexey:as/mediaframe_serialization
Jul 13, 2021
Merged

[G-API]: Add serialization mechanism for cv::MediaFrame#20329
alalek merged 10 commits intoopencv:masterfrom
smirnov-alexey:as/mediaframe_serialization

Conversation

@smirnov-alexey
Copy link
Copy Markdown
Contributor

@smirnov-alexey smirnov-alexey commented Jun 29, 2021 •
edited
Loading

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or other license that is incompatible with OpenCV
  • The PR is proposed to proper branch
  • There is reference to original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake
force_builders=Custom,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f

Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom=ubuntu-openvino-2021.3.0:20.04
build_image:Custom Win=openvino-2021.3.0
build_image:Custom Mac=openvino-2021.3.0

test_modules:Custom=gapi,python2,python3,java
test_modules:Custom Win=gapi,python2,python3,java
test_modules:Custom Mac=gapi,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

@smirnov-alexey
Copy link
Copy Markdown
Contributor Author

smirnov-alexey commented Jul 1, 2021

@TolyaTalamanov @AsyaPronina @mpashchenkov please, review

TolyaTalamanov
TolyaTalamanov reviewed Jul 1, 2021
View reviewed changes
modules/gapi/include/opencv2/gapi/s11n.hpp Outdated

template<typename RMatAdapterType>
struct deserialize_runarg {
template<typename T, typename... Types> inline
Copy link
Copy Markdown
Contributor

@TolyaTalamanov TolyaTalamanov Jul 1, 2021

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 ?

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

Choose a reason for hiding this comment

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

You are right, removed

smirnov-alexey
smirnov-alexey commented Jul 2, 2021
View reviewed changes
modules/gapi/include/opencv2/gapi/s11n.hpp Outdated
};

template<typename T, typename... Types>
struct deserialize_runarg<std::tuple<T, Types...>> {
Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 2, 2021

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

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

Choose a reason for hiding this comment

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

Done

@dmatveev dmatveev self-assigned this Jul 5, 2021
@dmatveev dmatveev added this to the 4.5.4 milestone Jul 5, 2021
dmatveev
dmatveev reviewed Jul 5, 2021
View reviewed changes
modules/gapi/include/opencv2/gapi/media.hpp
Comment on lines +140 to +146
*
* @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;
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Jul 5, 2021

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?

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

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

modules/gapi/include/opencv2/gapi/s11n.hpp Outdated

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) {
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Jul 5, 2021

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 :)

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

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)

modules/gapi/src/backends/common/serialization.cpp Outdated
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"));
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Jul 5, 2021

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 ?

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

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

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

Choose a reason for hiding this comment

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

Extended the message though

Copy link
Copy Markdown
Contributor

@dmatveev dmatveev Jul 7, 2021

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?

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 8, 2021

Choose a reason for hiding this comment

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

Changed the message

AsyaPronina
AsyaPronina reviewed Jul 6, 2021
View reviewed changes
modules/gapi/test/s11n/gapi_s11n_tests.cpp
std::string m_str;
public:
MyMediaFrameAdapter() = default;
MyMediaFrameAdapter(cv::Mat m, int value, const std::string& str)
Copy link
Copy Markdown
Contributor

@AsyaPronina AsyaPronina Jul 6, 2021

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...

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

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

modules/gapi/test/s11n/gapi_s11n_tests.cpp
virtual void deserialize(cv::gapi::s11n::IIStream& is) override {
is >> m_value >> m_str;
}
int getVal() { return m_value; }
Copy link
Copy Markdown
Contributor

@AsyaPronina AsyaPronina Jul 6, 2021

Choose a reason for hiding this comment

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

const method?

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

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

modules/gapi/test/s11n/gapi_s11n_tests.cpp
is >> m_value >> m_str;
}
int getVal() { return m_value; }
std::string getStr() { return m_str; }
Copy link
Copy Markdown
Contributor

@AsyaPronina AsyaPronina Jul 6, 2021

Choose a reason for hiding this comment

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

const method?

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

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

modules/gapi/test/s11n/gapi_s11n_tests.cpp
EXPECT_EQ(sc, out_sc);

cv::Mat out_mat = cv::util::get<cv::Mat>(out[2]);
EXPECT_EQ(0, cv::norm(mat, out_mat));
Copy link
Copy Markdown
Contributor

@AsyaPronina AsyaPronina Jul 6, 2021

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?

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

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

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 8, 2021

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())

modules/gapi/test/s11n/gapi_s11n_tests.cpp Outdated
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");
Copy link
Copy Markdown
Contributor

@AsyaPronina AsyaPronina Jul 6, 2021

Choose a reason for hiding this comment

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

auto?

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

Choose a reason for hiding this comment

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

Done

modules/gapi/test/s11n/gapi_s11n_tests.cpp Outdated
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");
Copy link
Copy Markdown
Contributor

@AsyaPronina AsyaPronina Jul 6, 2021

Choose a reason for hiding this comment

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

auto?

Copy link
Copy Markdown
Contributor Author

@smirnov-alexey smirnov-alexey Jul 6, 2021

Choose a reason for hiding this comment

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

Done

@smirnov-alexey
Copy link
Copy Markdown
Contributor Author

smirnov-alexey commented Jul 6, 2021

@TolyaTalamanov @AsyaPronina @dmatveev please, take a look once again

@smirnov-alexey
Copy link
Copy Markdown
Contributor Author

smirnov-alexey commented Jul 8, 2021

@dmatveev please, take a look once again

dmatveev
dmatveev approved these changes Jul 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@dmatveev dmatveev left a 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! 👍

@alalek
Copy link
Copy Markdown
Member

alalek commented Jul 8, 2021

Need to fix compilation issues.

@smirnov-alexey smirnov-alexey force-pushed the as/mediaframe_serialization branch from 9f718ad to 8cbcd52 Compare July 13, 2021 13:36
@smirnov-alexey
Copy link
Copy Markdown
Contributor Author

smirnov-alexey commented Jul 13, 2021

@alalek can we merge this, please?

@alalek alalek merged commit 5179e37 into opencv:master Jul 13, 2021
@alalek alalek mentioned this pull request Oct 15, 2021
Merged
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
@smirnov-alexey
Merge pull request opencv#20329 from smirnov-alexey:as/mediaframe_ser…
fd3ddf9 
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AsyaPronina AsyaPronina AsyaPronina left review comments

@TolyaTalamanov TolyaTalamanov TolyaTalamanov left review comments

@dmatveev dmatveev dmatveev approved these changes

Assignees

@dmatveev dmatveev

Labels

category: g-api / gapi feature

Projects

None yet

Milestone

4.5.4

Development

Successfully merging this pull request may close these issues.

5 participants

@smirnov-alexey @alalek @dmatveev @AsyaPronina @TolyaTalamanov