Skip to content

GAPI: Addition new Color conversion kernels to CPU backend. #18516

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 14, 2020
Merged

GAPI: Addition new Color conversion kernels to CPU backend. #18516

merged 3 commits into from
Oct 14, 2020

Conversation

anna-khakimova
Copy link
Member

@anna-khakimova anna-khakimova commented Oct 5, 2020 •
edited
Loading

Color conversion kernels were added:

  1. BGR2RGB
  2. BGR2I420
  3. RGB2I420
  4. I4202BGR
  5. I4202RGB

Published for review on 7th of October.

@dmatveev please take a look.

force_builders=Custom,Custom Win,Custom Mac,Linux32,Win32
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

build_image:Custom=centos:7
buildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

Xbuild_image:Custom=ubuntu-openvino-2020.3.0:16.04
Xbuild_image:Custom Win=openvino-2020.3.0
Xbuild_image:Custom Mac=openvino-2020.3.0

test_modules:Custom=gapi
test_modules:Custom Win=gapi
test_modules:Custom Mac=gapi

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

@anna-khakimova anna-khakimova force-pushed the ak/bgr2rgb branch 2 times, most recently from 1c99c39 to e93ac18 Compare October 6, 2020 10:57
@anna-khakimova anna-khakimova force-pushed the ak/bgr2rgb branch 7 times, most recently from 24105f9 to c228da1 Compare October 6, 2020 22:57
@anna-khakimova anna-khakimova changed the title GAPI: Add BGR2RGB kernel to CPU backend. GAPI: Add new kernels to CPU backend. Oct 7, 2020
@dmatveev dmatveev self-requested a review October 7, 2020 10:30
@dmatveev dmatveev self-assigned this Oct 7, 2020
@anna-khakimova anna-khakimova changed the title GAPI: Add new kernels to CPU backend. GAPI: Addition new kernels to CPU backend. Oct 7, 2020
@anna-khakimova
Copy link
Member Author

@dmatveev please take a look

dmatveev
dmatveev reviewed Oct 8, 2020
View reviewed changes
Copy link
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.

Looks good but please fix comments

modules/gapi/include/opencv2/gapi/imgproc.hpp Outdated
The function converts an input image from I420 color space to BGR.
The conventional ranges for B, G, and R channel values are 0 to 255.

Output image must be 8-bit unsigned 3-channel image input_height*2/3 @ref CV_8UC3.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rephrase text here and above on the image height. Currently it doesn't read good.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@anna-khakimova anna-khakimova changed the title GAPI: Addition new kernels to CPU backend. GAPI: Addition new Color conversion kernels to CPU backend. Oct 12, 2020
@anna-khakimova
Copy link
Member Author

@dmatveev please take a look. Comment was applied.

@anna-khakimova
Copy link
Member Author

@alalek please merge

alalek
alalek reviewed Oct 13, 2020
View reviewed changes
modules/gapi/include/opencv2/gapi/imgproc.hpp Outdated
The function converts an input image from BGR color space to RGB.
The conventional ranges for B, G, and R channel values are 0 to 255.

Output image must be 8-bit unsigned 3-channel image @ref CV_8UC3.
Copy link
Member

Choose a reason for hiding this comment

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

must be

probably this description is not accurate, because output format is defined by "implementation" instead of caller.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, probably simple is is enough

Copy link
Member Author

@anna-khakimova anna-khakimova Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@alalek Half of the comments in this file phrased as "Output image must be 8-bit unsigned 3-channel image ". For example for YUV2RGB kernel:

/** @brief Converts an image from YUV color space to RGB.
The function converts an input image from YUV color space to RGB.
The conventional ranges for Y, U, and V channel values are 0 to 255.

Output image must be 8-bit unsigned 3-channel image @ref CV_8UC3.

@note Function textual ID is "org.opencv.imgproc.colorconvert.yuv2rgb"

@param src input image: 8-bit unsigned 3-channel image @ref CV_8UC3.

@sa RGB2Lab, RGB2YUV
*/
GAPI_EXPORTS GMat YUV2RGB(const GMat& src);

However, for some reason, attention was paid only to my comment. The remaining 14 color conversion kernels will remain with comments formulated as "Output image must be 8-bit unsigned ...". I thought we were fighting for uniformity in the comments. But apparently I was wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

modules/gapi/include/opencv2/gapi/imgproc.hpp

Output image must be 8-bit unsigned 1-channel image. @ref CV_8UC1.
Width of I420 output image must be the same as width of input image.
Height of I420 output image must be equal 3/2 from height of input image.
Copy link
Member

Choose a reason for hiding this comment

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

3/2 from height

Why is not tuple of Y and UV?

/cc @TolyaTalamanov (implemented drawing over NV12 frames)

Copy link
Contributor

Choose a reason for hiding this comment

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

@alalek tuple of Y and UV is what we'll do next, but the current request is to provide a cvtColor-based counterpart.

Copy link
Member

Choose a reason for hiding this comment

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

Please note that there is no C++ overloads for return type only. So it will be hard to improve that without breaking of existed code.
Just keep in mind.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok

@anna-khakimova anna-khakimova requested a review from alalek October 14, 2020 14:10
@alalek alalek merged commit 4c048a4 into opencv:master Oct 14, 2020
@alalek alalek mentioned this pull request Nov 27, 2020
Merged
a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023
Merge pull request opencv#18516 from anna-khakimova:ak/bgr2rgb
0287fa2 
GAPI: Addition new Color conversion kernels to CPU backend.

* Add BGR2RGB kernel to CPU backend

* Add BGR2I420 and RGB2I420 kernels to CPU backend

* Add I4202BGR and I4202RGB kernels to CPU backend
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmatveev dmatveev dmatveev approved these changes

@alalek alalek Awaiting requested review from alalek

Assignees

@dmatveev dmatveev

Labels
category: g-api / gapi
Projects
None yet
Milestone
4.5.1
Development

Successfully merging this pull request may close these issues.
3 participants
@anna-khakimova @dmatveev @alalek