Add CV_16UC1/GRAY16_LE support to GStreamer backend for VideoWriter by joshdoe · Pull Request #18535 · opencv/opencv

joshdoe · 2020-10-07T18:02:42Z

See #14035 for a companion PR to add CV_16UC1/GRAY16_LE capture support. I'm new to test writing, but added one that seems to make sense. I'm not clear if all PRs are still supposed to be based off 3.4, but I ran into roadblocks trying to build 3.4 at all, master just worked so easily.

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=Linux AVX2,Custom
build_image:Custom=gstreamer:16.04
buildworker:Custom=linux-1,linux-2,linux-4
test_modules:Custom=videoio,python2,python3,java

asmorkalov · 2020-11-24T10:31:55Z

@joshdoe Thanks for the patch! It's very cool that you bring test with the new feature, but there is issue with VideoWriter behavior.

Actual pipeline initialization is delayed till the first frame. It means that open call returns true and VideoWriter reports that it's open, but actual initialization may fail on first frame. It's not what existing code expects to get. I propose to set depth on open call explicitly. Please take a look on cv::VideoWriter Documentation: https://docs.opencv.org/master/dd/d9e/classcv_1_1VideoWriter.html#aff57b7ffbd654ef2f83e7ad76916426b. It takes std::vector<int> to get extra configuration options in key key->value way. You need to introduce new key with output video depth and pass it to GStreamer backed in create_GStreamer_writer (https://github.com/opencv/opencv/blob/master/modules/videoio/src/cap_gstreamer.cpp#L1697). Please re-use existing constants for cv::Mat depth. Default behavior (rgb/gray) should be presumed to not break existing code.

joshdoe · 2020-11-24T17:32:09Z

@asmorkalov thanks for taking a look at this. Do you think I should get rid of all autodetection from first frame, or make it optional based on if VIDEOWRITER_PROP_DEPTH is -1 (but default it to CV_8U)?

joshdoe · 2020-11-24T19:37:54Z

I think I've done what you asked. If you don't like the auto-detection option, that can easily be removed.

joshdoe · 2020-11-24T21:09:14Z

I'm getting failures on the buildbot with the tests I've implemented, however locally with Win64 VS2019 I have no issues. I'm trying to build now on Ubuntu 16.04.

joshdoe · 2020-11-24T21:38:42Z

Tests are passing for me in Ubuntu 16.04 as well. Not sure how to replicate the failures from the buildbot.

joshdoe · 2020-11-25T14:05:10Z

I see the problem now. The buildbot builds the GStreamer backend as a plugin, and I've been building it statically. I've just tried building it as a plugin, and hit a compilation error. From the looks of it, the way plugins are defined, there's no way to pass arbitrary parameters, that is, not all entries from VideoWriterParameters get propagated to cv_writer_open. I see a few options:

Only enable GRAY16_LE support when GStreamer is built statically. This would be suboptimal.
Using Writer_setProperty allow the depth to be set after opening, but before writing the first frame, which would override the input_pix_fmt and caps. This wouldn't require changes outside the GStreamer module sources, however it would be unintuitive I think.
Modify the plugin API to pass VideoWriterParameters to the plugin Writer_open function. This would be a breaking change, unless an Writer_open2 function was introduced. And also the parameters I'm guessing would need to be repacked into a more elementary data structure.

Maybe @mshabunin could give some input on the way forward. Thanks.

asmorkalov · 2020-11-25T14:08:16Z

@joshdoe Let me check the plugin issue with teammates. and I'll come back.

asmorkalov · 2020-11-28T07:56:09Z

@joshdoe I discussed the situation with @alalek , @vpisarev and core team members and we propose
to ntroduce Writer_open_with_params function in addition to existing open call. The call should not include is_color parameter, but get int* array with parameters and size_t parameter for array size. The array should just propagate VideoWriterParameters content. is_color flag and depth option is passed through this array. It's scalable solution that we can re-use in other back-ends too.

joshdoe · 2020-11-30T20:45:14Z

@asmorkalov trying to do this now. There's no state for the OpenCV_VideoIO_Plugin_API_preview struct, so do I also add a Writer_write2 to accept a depth, or do something different? Maybe add state, need to be careful about ABI though...

joshdoe · 2020-12-02T13:22:57Z

@asmorkalov I've taken a stab at this, seems to work on my end, let's see what the buildbot thinks.

joshdoe · 2020-12-02T16:32:37Z

@asmorkalov Builds are passing now after I fixed some warnings in the FFMPEG backend. Do review my API changes, not sure if this is the best way to handle it or not.

asmorkalov

Looks good in general. Please address several local remarks.

asmorkalov · 2020-12-03T11:58:07Z

modules/videoio/test/test_gstreamer.cpp

+    // generate a noise frame
+    Size frame_size(320, 240);
+    Mat frame = Mat::zeros(frame_size, CV_16U);
+    cv::randu(frame, 0, 65535);


Please use theRNG(); as random generator to get reproducible results.

cv::randu() uses cv::theRNG() internally, so code is fine here.

I'm not sure why the numbers have to be reproducible, as I'm simply comparing what I've generated to what I read back in. If this change request still stands, would I do it like this?

cv::theRNG().state = 123456; cv::randu(frame, 0, 65535);

Not needed.
theRNG() initial state is initialized by OpenCV test framework (using the same value) before calling of each test case body.

BTW, initial theRNG() state is configurable through command-line (however this mode is not used widely). Test data should not rely on its value.

asmorkalov · 2020-12-03T11:59:45Z

modules/videoio/test/test_gstreamer.cpp

+
+    // generate a noise frame
+    Size frame_size(320, 240);
+    Mat frame = Mat::zeros(frame_size, CV_16U);


Mat::zeros() call is ambiguous as you fill the mat with random values.

Mat::zeros() call is ambiguous as you fill the mat with random values.

Not correct.

zeros

fill with zeros values.

Ambiguous isn't the right word to use, but since all the values get overwritten immediately, it is unnecessary to initialize to zero, even if it is a very fast operation. I've changed this to simply Mat(frame_size, CV_16U)

Sorry for my English, ambiguous is not the right word. I mean redundant.

asmorkalov · 2020-12-03T12:00:41Z

modules/videoio/test/test_gstreamer.cpp

+    Mat written_frame(frame_size, CV_16U);
+    std::ifstream fs(temp_file, std::ios::in | std::ios::binary);
+    fs.read((char*)written_frame.ptr(0), frame_size.width * frame_size.height * 2);
+    ASSERT_TRUE(fs);


I think it should be before read call.

I believe this assertion ensures that .read() call is successful (it is really necessary).

I did some tests, I could put an assertion both before and after .read(), however the second assertion will catch the same issue that the first assertion would, that is if a file is missing or can't be opened. I left this as is for now.

asmorkalov · 2020-12-03T12:04:39Z

modules/videoio/src/backend_plugin.cpp

+            std::vector<int> vint_params;
+            for (const auto& param : params.getAll())
+            {
+                vint_params.push_back(param.key);
+                vint_params.push_back(param.value);
+            }
+            int* c_params = &vint_params[0];
+            size_t n_params = vint_params.size() / 2;


I propose to extract this code as method of VideoWriterParameters class. E.g. return std::vector and pass it's content to C code as plain C array with size. We can potentially optimized the class behavior without plugins code modification.

Ok I added getIntVector().

alalek · 2020-12-03T12:31:59Z

modules/videoio/test/test_gstreamer.cpp

+    // compare to make sure it's identical
+    Mat diff;
+    absdiff(frame, written_frame, diff);
+    Scalar total = sum(diff);
+    ASSERT_EQ(total[0], 0);


Please use this for "identity" checks:

EXPECT_EQ(0, cv::norm(frame, written_frame, NORM_INF));

asmorkalov

Looks good to me. Please squash the commits and I think we can merge the patch.

joshdoe · 2020-12-03T12:56:22Z

Looks good to me. Please squash the commits and I think we can merge the patch.

@asmorkalov Squash all three? I thought it might be useful to keep the plugin API change as a separate one, maybe squash the two GStreamer-specific ones?

asmorkalov · 2020-12-03T13:15:07Z

Yes, dedicated commit for API change is good idea. Thanks.

alalek

Loading/using of existed FFmpeg plugin on Windows is broken:

[ INFO:0] global C:\build\precommit_windows64\opencv\modules\videoio\src\backend_plugin.cpp (211) cv::impl::PluginBackend::PluginBackend Video I/O: plugin is incompatible: C:\build\precommit_windows64\build\bin\Release\opencv_videoio_ffmpeg451_64.dll

...

[ RUN      ] videoio/videoio_ffmpeg.write_big/0, where GetParam() = ("", "avi", 4096x4096)
[     SKIP ] FFmpeg backend was not found
[       OK ] videoio/videoio_ffmpeg.write_big/0 (0 ms)

I will take a look how to fix this on this week.

This will allow arbitrary parameters to be passed to plugin backends

This introduces a new property VIDEOWRITER_PROP_DEPTH, which defaults to CV_8U, but for GStreamer can be set to CV_16U. Also, fix another test to not fail if plugin isn't found, copying logic from the read_write test.

joshdoe · 2020-12-03T19:34:10Z

@alalek looks like any existing plugin will fail with the API_VERSION changing to 1. A backwards compatible change would require PluginBackend to probe for multiple values of API_VERSION, this is something naive I tried and works with a quick check:

for (int i = API_VERSION; i >= 0; --i)
{
    CV_LOG_INFO(NULL, "Video I/O: checking if plugin supports API_VERSION " << i);
    plugin_api_ = fn_init(ABI_VERSION, i, NULL);
    if (plugin_api_)
        break;
}

if (!plugin_api_)
{
    CV_LOG_INFO(NULL, "Video I/O: plugin is incompatible: " << lib->getName());
    return;
}

I also squashed the two GStreamer-specific commits.

alalek

LGTM 👍

Thank you for contribution!

Add CV_16UC1/GRAY16_LE support to GStreamer backend for VideoWriter * videoio(backend): add Writer_open_with_params to plugin API This will allow arbitrary parameters to be passed to plugin backends * videoio(gstreamer): add GRAY16_LE/CV_16UC1 writing support to GStreamer This introduces a new property VIDEOWRITER_PROP_DEPTH, which defaults to CV_8U, but for GStreamer can be set to CV_16U. Also, fix another test to not fail if plugin isn't found, copying logic from the read_write test. * videoio(plugin): fix handling plugins with previous API level * videoio: coding style * fix warning

asmorkalov added category: videoio feature labels Oct 9, 2020

asmorkalov requested a review from VadimLevin October 21, 2020 09:40

asmorkalov requested review from asmorkalov and removed request for VadimLevin November 24, 2020 10:32

asmorkalov self-assigned this Nov 24, 2020

mshabunin mentioned this pull request Nov 24, 2020

add Y16/GRAY16_LE support to gstreamer #14035

Closed

joshdoe closed this Nov 24, 2020

joshdoe force-pushed the gray16_gstreamer_writing branch from 69abe4b to e1a8fc0 Compare November 24, 2020 19:29

joshdoe reopened this Nov 24, 2020

joshdoe force-pushed the gray16_gstreamer_writing branch from 41bc9eb to fff866d Compare November 24, 2020 20:00

asmorkalov added pr: Discussion Required and removed pr: Discussion Required labels Nov 25, 2020

joshdoe force-pushed the gray16_gstreamer_writing branch from fff866d to 7378aa0 Compare December 2, 2020 13:21

joshdoe force-pushed the gray16_gstreamer_writing branch from 7378aa0 to 7468782 Compare December 2, 2020 15:40

asmorkalov requested changes Dec 3, 2020

View reviewed changes

alalek reviewed Dec 3, 2020

View reviewed changes

joshdoe force-pushed the gray16_gstreamer_writing branch from 7468782 to 77d12cb Compare December 3, 2020 12:45

asmorkalov approved these changes Dec 3, 2020

View reviewed changes

alalek requested changes Dec 3, 2020

View reviewed changes

joshdoe added 2 commits December 3, 2020 14:32

videoio(backend): add Writer_open_with_params to plugin API

42e0925

This will allow arbitrary parameters to be passed to plugin backends

joshdoe force-pushed the gray16_gstreamer_writing branch from 77d12cb to ba4a822 Compare December 3, 2020 19:34

alalek added 3 commits December 4, 2020 02:12

videoio(plugin): fix handling plugins with previous API level

7f9778c

videoio: coding style

25c2309

fix warning

9299143

alalek approved these changes Dec 5, 2020

View reviewed changes

alalek merged commit 541a09b into opencv:master Dec 5, 2020

alalek mentioned this pull request Dec 31, 2020

VideoWriter support for 16-bit depth images and color-alpha color imags #10623

Closed

alalek mentioned this pull request Apr 9, 2021

(5.x) Merge 4.x #19885

Merged

Uh oh!

Conversation

joshdoe commented Oct 7, 2020 • edited by alalek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Readiness Checklist

Uh oh!

asmorkalov commented Nov 24, 2020

Uh oh!

joshdoe commented Nov 24, 2020

Uh oh!

joshdoe commented Nov 24, 2020

Uh oh!

joshdoe commented Nov 24, 2020

Uh oh!

joshdoe commented Nov 24, 2020

Uh oh!

joshdoe commented Nov 25, 2020

Uh oh!

asmorkalov commented Nov 25, 2020

Uh oh!

asmorkalov commented Nov 28, 2020

Uh oh!

joshdoe commented Nov 30, 2020

Uh oh!

joshdoe commented Dec 2, 2020

Uh oh!

joshdoe commented Dec 2, 2020

Uh oh!

asmorkalov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asmorkalov left a comment

Choose a reason for hiding this comment

Uh oh!

joshdoe commented Dec 3, 2020

Uh oh!

asmorkalov commented Dec 3, 2020

Uh oh!

alalek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshdoe commented Dec 3, 2020

Uh oh!

alalek left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

joshdoe commented Oct 7, 2020 •

edited by alalek

Loading

alalek left a comment •

edited

Loading