Skip to content

Zerocopy gstreamer videocapture#18377

Draft
michaelgruner wants to merge 2 commits intoopencv:4.xfrom
RidgeRun:zerocopy-gstreamer-videocapture
Draft

Zerocopy gstreamer videocapture#18377
michaelgruner wants to merge 2 commits intoopencv:4.xfrom
RidgeRun:zerocopy-gstreamer-videocapture

Conversation

@michaelgruner
Copy link
Copy Markdown
Contributor

@michaelgruner michaelgruner commented Sep 21, 2020

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: N/A
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable N/A?
  • Patch to opencv_extra has the same branch name. N/A
  • The feature is well documented and sample code can be built with the project CMake: N/A

Description

Removes an exisiting memory copy when creating a Mat from a Gstreamer buffer, when using the VideoCapture class. To do so, the pointer to the Gstreamer buffer data is shared in the Mat. To handle the pointer's lifespan, a Gstreamer custom allocator is proposed so that when the Mat's data is to be destroyed, the data is properly returned and freed by Gstreamer.

Gstreamer is the preferred multimedia framework for embedded devices. Considering this, performance is typically a priority. Currently, capturing with OpenCV is slower than capturing with plain Gstreamer. I'm keeping track of the improvements here.

Pending Issues

  • Gstreamer buffer is being mapped as READONLY, while the Mat is READ WRITE. This is currently WIP.

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Sep 22, 2020

@michaelgruner Thanks for the contribution!

@mshabunin
Copy link
Copy Markdown
Contributor

mshabunin commented Sep 22, 2020

@michaelgruner , does it work when GStreamer backend is built as a plug-in? -DVIDEOIO_PLUGIN_LIST=gstreamer cmake option.

@michaelgruner
Copy link
Copy Markdown
Contributor Author

michaelgruner commented Sep 22, 2020

I'm not sure. I'll give it a try.

@michaelgruner
Copy link
Copy Markdown
Contributor Author

michaelgruner commented Oct 2, 2020

Just tested. It works as with built-in.

[ INFO:0] global /home/mgruner/RidgeRun/OpenCV/OpenCV/opencv/modules/videoio/src/videoio_registry.cpp (191) VideoBackendRegistry VIDEOIO: Enabled backends(8, sorted by priority): FFMPEG(1000); GSTREAMER(990); INTEL_MFX(980); MSMF(970); V4L2(960); CV_IMAGES(950); CV_MJPEG(940); FIREWIRE(930)
[ INFO:0] global /home/mgruner/RidgeRun/OpenCV/OpenCV/opencv/modules/videoio/src/backend_plugin.cpp (359) getPluginCandidates VideoIO pluigin (GSTREAMER): glob is 'libopencv_videoio_gstreamer*.so', 1 location(s)
[ INFO:0] global /home/mgruner/RidgeRun/OpenCV/OpenCV/opencv/modules/videoio/src/backend_plugin.cpp (366) getPluginCandidates     - /home/mgruner/RidgeRun/OpenCV/OpenCV/opencv/uninstalled/lib: 1
[ INFO:0] global /home/mgruner/RidgeRun/OpenCV/OpenCV/opencv/modules/videoio/src/backend_plugin.cpp (370) getPluginCandidates Found 1 plugin(s) for GSTREAMER
[ INFO:0] global /home/mgruner/RidgeRun/OpenCV/OpenCV/opencv/modules/videoio/src/backend_plugin.cpp (175) libraryLoad load /home/mgruner/RidgeRun/OpenCV/OpenCV/opencv/uninstalled/lib/libopencv_videoio_gstreamer.so => OK
[ INFO:0] global /home/mgruner/RidgeRun/OpenCV/OpenCV/opencv/modules/videoio/src/backend_plugin.cpp (236) PluginBackend Video I/O: loaded plugin 'GStreamer OpenCV Video I/O plugin'

@asmorkalov asmorkalov requested a review from VadimLevin October 21, 2020 09:41
@vpisarev
Copy link
Copy Markdown
Contributor

vpisarev commented Dec 1, 2020

@asmorkalov, shouldn't we just merge it?

@alalek
Copy link
Copy Markdown
Member

alalek commented Dec 1, 2020

@vpisarev Consider reviewing this first. Current MatAllocator behavior is up to you - nobody understand this logic completely.

As you can see several methods are just not implemented - do we really need them in this basic interface?

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Dec 3, 2020

@vpisarev . I retriggered Ci build to get status and looking the code right now. Will come back with review results today.

@alalek
Copy link
Copy Markdown
Member

alalek commented Dec 3, 2020

I retriggered Ci build to get status

Please note, that NONE of triggered builds from default "pre-commit" scope performs building/testing of GStreamer code.

@asmorkalov asmorkalov requested review from asmorkalov and removed request for VadimLevin December 9, 2020 14:35
@asmorkalov asmorkalov self-assigned this Dec 9, 2020
@michaelgruner
Copy link
Copy Markdown
Contributor Author

michaelgruner commented Dec 21, 2020

@alalek is this something I need to do on my side as external contributor?

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Dec 22, 2020

@michaelgruner Thanks for your contribution. Alexander works on release right now. We have a bunch of changes related to multimedia and plugins support and plan to integrate your work after release with plugins API rework.

alalek
alalek reviewed Jan 15, 2021
View reviewed changes
modules/videoio/src/cap_gstreamer.cpp Outdated
Comment on lines +582 to +1061
try {
dst.getMatRef() = GStreamerMat(sample.get());
} catch (cv::Exception &e) {
CV_WARN(e.err);
return false;
}
Copy link
Copy Markdown
Member

@alalek alalek Dec 3, 2020

Choose a reason for hiding this comment

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

This part introduces regression.

OutputArray is a wrapper over multiple container types, including UMat.

Copy link
Copy Markdown
Contributor Author

@michaelgruner michaelgruner Feb 10, 2021

Choose a reason for hiding this comment

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

May I get some clarification on this comment?

Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov Feb 16, 2021

Choose a reason for hiding this comment

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

retrieveFrame method has OuputArray as parameter. OutputArray is proxy class that wraps cv::Mat, cv::UMat, std::vector and some other containers to provide flexibility to OpenCV users. In general case dst can be UMat and the proposed solution fails. OutputArray has kind() method that reports original container type. You need to check it before assignment. Other cases can be handled with copy as it was done before.

modules/videoio/src/cap_gstreamer.cpp
{
flags += CV_MAT_TYPE(_type);
rows = _rows;
cols = _cols;
Copy link
Copy Markdown
Member

@alalek alalek Dec 3, 2020

Choose a reason for hiding this comment

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

fillMat

naming is not accurate and confusing.

There are setSize() / finalizeHdr() / updateContinuityFlag() calls in the core module.
The best option is trying to reuse these calls (to keep library consistent by avoiding duplication of code).
Or at least, rename to fillMatHeader().

modules/videoio/src/cap_gstreamer.cpp
if (!sample)
return false;
if (false == gst_buffer_map (gstdata->buffer, &(gstdata->info), GST_MAP_READ)) {
CV_Error(Error::Code::StsUnsupportedFormat, ("Unable to map GstBuffer"));
Copy link
Copy Markdown
Member

@alalek alalek Dec 3, 2020

Choose a reason for hiding this comment

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

memory leak is caused here (new gstdata)

modules/videoio/src/cap_gstreamer.cpp
gstdata->sample.release();

delete gstdata;
delete u;
Copy link
Copy Markdown
Member

@alalek alalek Dec 3, 2020

Choose a reason for hiding this comment

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

Perhaps it is better to swap order of delete operators to avoid having of dangling pointers.

modules/videoio/src/cap_gstreamer.cpp Outdated
Comment on lines +573 to +596
* \brief CvCapture_GStreamer::retrieveFrame
* \return IplImage pointer. [Transfer Full]
* Retrieve the previously grabbed buffer, and wrap it in an IPLImage structure
Copy link
Copy Markdown
Member

@alalek alalek Dec 3, 2020

Choose a reason for hiding this comment

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

I know that it is copy-paste, but I believe we should not copy outdated comments anyway.

modules/videoio/src/cap_gstreamer.cpp
UMatData* u = new UMatData(this);
u->data = u->origdata = gstdata->info.data;
u->size = gstdata->info.size;
u->userdata = gstdata;
Copy link
Copy Markdown
Member

@alalek alalek Dec 3, 2020

Choose a reason for hiding this comment

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

@vpisarev Is it valid usage of userdata field?

modules/videoio/src/cap_gstreamer.cpp
UMatUsageFlags /*usageFlags*/) const
{
CV_Error(Error::Code::StsBadFunc, ("GStreamer allocator may not alloc raw data"));
return nullptr;
Copy link
Copy Markdown
Member

@alalek alalek Dec 3, 2020

Choose a reason for hiding this comment

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

CV_Error() throws exception (marked with "noreturn" attribute).

return nullptr; is unreachable code.

asmorkalov
asmorkalov reviewed Jan 20, 2021
View reviewed changes
modules/videoio/src/cap_gstreamer.cpp
pipeline.release();
}
CV_Error(Error::Code::StsBadFunc, ("GStreamer allocator may not alloc raw data"));
return false;
Copy link
Copy Markdown
Contributor

@asmorkalov asmorkalov Jan 19, 2021

Choose a reason for hiding this comment

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

return is unreachable too.

@asmorkalov asmorkalov mentioned this pull request Jan 21, 2021
Merged
6 tasks
@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Jan 27, 2021

@michaelgruner Could you rebase you PR to current master and fix remarks?

@michaelgruner
Copy link
Copy Markdown
Contributor Author

michaelgruner commented Jan 27, 2021 via email

@asmorkalov
Copy link
Copy Markdown
Contributor

asmorkalov commented Feb 10, 2021

@michaelgruner Friendly reminder.

@michaelgruner michaelgruner force-pushed the zerocopy-gstreamer-videocapture branch from ff38ea7 to 2efaffd Compare February 10, 2021 15:29
@michaelgruner michaelgruner marked this pull request as draft May 19, 2022 18:04
@michaelgruner
Copy link
Copy Markdown
Contributor Author

michaelgruner commented May 19, 2022

Hey all, I'm sorry I got lost for a while there. I'm back with some time to get this through. I'm rebasing the changes to the latest master and figuring out conflicts.

@michaelgruner
Copy link
Copy Markdown
Contributor Author

michaelgruner commented May 26, 2022

Pulling #22018 first as a dependency.

@michaelgruner
Implement GStreamer allocator to keep the GstBuffer alive through the…
c8800a3 
… mat lifespan

Instead of copying the data from the GstSample into a Mat, the
allocator keeps a mapped reference of the GstBuffer pulled from
the pipeline. By implementing a custom GStreamerAllocator we instruct
OpenCV on how to properly unmap and unref the GstBuffer, even if the
data was copied to a regular Mat.
@michaelgruner michaelgruner force-pushed the zerocopy-gstreamer-videocapture branch from 2efaffd to 25df008 Compare June 29, 2022 06:50
@michaelgruner michaelgruner force-pushed the zerocopy-gstreamer-videocapture branch from 25df008 to 7a3fbbb Compare June 29, 2022 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@asmorkalov asmorkalov asmorkalov left review comments

@alalek alalek alalek left review comments

Assignees

@asmorkalov asmorkalov

Labels

category: videoio RFC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@michaelgruner @asmorkalov @mshabunin @vpisarev @alalek