Skip to content
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

[CodeCamp2023-305] Improve Visualizer Rendering Speed with OpenCV Backend #1285

Open
wants to merge 49 commits into
base: main
Choose a base branch
from

Conversation

KerwinKai
Copy link
Contributor

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

As #1101 described.

Modification

I have shown the design ideas in functions such as __init__, get_image, draw_lines, etc. The basic idea is to add a parameter called backend to the Visualizer to control whether the rendering backend used for drawing is cv2 or matplotlib. The design idea of the drawing function is basically the same as that of draw_lines, that is, the original parameters of the function can be input into the drawing function of cv2 as much as possible. If it is not possible, a prompt will be thrown. This is my first draft, which shows the overall implementation idea of this task. If my idea have any problems, please correct me in time. I will complete the remaining code, documents and test cases as soon as possible.

BC-breaking (Optional)

Does the modification introduce changes that break the backward-compatibility of the downstream repos?
If so, please describe how it breaks the compatibility and how the downstream projects should modify their code to keep compatibility with this PR.

Use cases (Optional)

If this PR introduces a new feature, it is better to list some use cases here, and update the documentation.

Checklist

  1. Pre-commit or other linting tools are used to fix the potential lint issues.
  2. The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  3. If the modification has potential influence on downstream projects, this PR should be tested with downstream projects, like MMDet or MMCls.
  4. The documentation has been modified accordingly, like docstring or example tutorials.

@KerwinKai KerwinKai marked this pull request as ready for review August 3, 2023 07:30
Copy link
Collaborator

@HAOCHENYE HAOCHENYE left a comment

Choose a reason for hiding this comment

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

It could be better to provide the visualization result of cv2 backend in the PR messages 😄

mmengine/visualization/utils.py Outdated Show resolved Hide resolved
mmengine/visualization/visualizer.py Show resolved Hide resolved
mmengine/visualization/visualizer.py Show resolved Hide resolved
mmengine/visualization/visualizer.py Outdated Show resolved Hide resolved
mmengine/visualization/visualizer.py Outdated Show resolved Hide resolved
mmengine/visualization/visualizer.py Show resolved Hide resolved
mmengine/visualization/visualizer.py Show resolved Hide resolved
mmengine/visualization/visualizer.py Outdated Show resolved Hide resolved
mmengine/visualization/visualizer.py Outdated Show resolved Hide resolved
mmengine/visualization/visualizer.py Outdated Show resolved Hide resolved
@KerwinKai
Copy link
Contributor Author

Oki, I will update my code and docstring mentioned above as soon as posible.

@HAOCHENYE
Copy link
Collaborator

Please present the visualization results using both the cv2 and matplotlib backends to verify their correctness.

KerwinKai and others added 6 commits August 6, 2023 21:04
Co-authored-by: Mashiro <57566630+HAOCHENYE@users.noreply.github.com>
Co-authored-by: Mashiro <57566630+HAOCHENYE@users.noreply.github.com>
Co-authored-by: Mashiro <57566630+HAOCHENYE@users.noreply.github.com>
Co-authored-by: Mashiro <57566630+HAOCHENYE@users.noreply.github.com>
Co-authored-by: Mashiro <57566630+HAOCHENYE@users.noreply.github.com>
@KerwinKai
Copy link
Contributor Author

Please present the visualization results using both the cv2 and matplotlib backends to verify their correctness.

Hi, here is my vis result:https://github.com/KerwinKai/cv2_backend_for_mmengine/tree/main/cv2_backend_result

@KerwinKai
Copy link
Contributor Author

I want to ask a question about mypy check. Seeing the error report, it seems that there is still str in the output list, but the color_str2rgb function returns only one tuple, and I feel that str should not be obtained.

6161691332247_ pic

@KerwinKai
Copy link
Contributor Author

I want to ask a question about mypy check. Seeing the error report, it seems that there is still str in the output list, but the color_str2rgb function returns only one tuple, and I feel that str should not be obtained.

6161691332247_ pic

fix it by adding # type:ignore

@KerwinKai
Copy link
Contributor Author

Here is my new result, the self._image order rgb in matplotlib backend and bgr in cv2 backend:https://github.com/KerwinKai/cv2_backend_for_mmengine/tree/main/cv2_backend_result

Here is an example to draw and save point image in two backend:

image = np.zeros((500, 500, 3))
    backends = ['matplotlib', 'cv2']
    # test draw_points
    function = 'draw_points'
    for backend in backends:
        vis = Visualizer(image=image,
                         backend=backend)
        vis.draw_points(
            np.array([[100, 300], [200, 400]]),
            colors=['g', 'r']
        )
        result_img = vis.get_image()
        save_name = f'./images/{function}_{backend}.png'
        if backend == 'matplotlib':
            plt.imshow(result_img)
            plt.axis('off')
            plt.savefig(save_name, bbox_inches='tight', pad_inches = 0)
        else:
            cv2.imwrite(save_name, result_img)

See more details in: https://github.com/KerwinKai/cv2_backend_for_mmengine/blob/main/cv2_backend_result/generate.py

@zhouzaida
Copy link
Member

Hi, how about standardizing the input format to be RGB for all interfaces? This would facilitate interface consistency. If necessary in the future, we can provide an additional parameter to specify the color space. Therefore, set_image does not need to perform color space conversion.

@KerwinKai
Copy link
Contributor Author

KerwinKai commented Aug 12, 2023

Hi, how about standardizing the input format to be RGB for all interfaces? This would facilitate interface consistency. If necessary in the future, we can provide an additional parameter to specify the color space. Therefore, set_image does not need to perform color space conversion.

Does that mean that all functions such as draw_text input are sorted by rgb.
For example:

vis = Visualizer(image=image,
                         backend='cv2')
vis.draw_points(
            np.array([[100, 300], [200, 400]]),
            colors=[(255, 0, 0)])

We will get a red line because colors inputs are all in RGB order.

@KerwinKai
Copy link
Contributor Author

Got it, standard the input format to be RGB for all interfaces now and update relate docstring.

line_styles=['-', '-.'],
line_widths=[1, 2])
line_styles=line_styles,
line_widths=[1, 2.1])
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to change 2 to 2.1 here?

Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>
KerwinKai and others added 2 commits August 14, 2023 11:25
Co-authored-by: Zaida Zhou <58739961+zhouzaida@users.noreply.github.com>
@zhouzaida
Copy link
Member

Could you also update the images in https://github.com/KerwinKai/cv2_backend_for_mmengine/tree/main/cv2_backend_result/images using the latest commit?

@KerwinKai
Copy link
Contributor Author

Could you also update the images in https://github.com/KerwinKai/cv2_backend_for_mmengine/tree/main/cv2_backend_result/images using the latest commit?

Oki, already update it.

@KerwinKai
Copy link
Contributor Author

It seems like that https://github.com/KerwinKai/cv2_backend_for_mmengine/blob/main/cv2_backend_result/images/draw_binary_masks_cv2.png is different from https://github.com/KerwinKai/cv2_backend_for_mmengine/blob/main/cv2_backend_result/images/draw_binary_masks_matplotlib.png.

I fix my code to orgin version in main mmengine and update https://github.com/KerwinKai/cv2_backend_for_mmengine/tree/main/cv2_backend_result/images again. The different now I thick may cause by line 1194.

        if self.backend == 'matplotlib':
            self.ax_save.imshow(
                img,
                extent=(0, self.width, self.height, 0),
line1194  interpolation='nearest')
        else:
            self._image = img

@KerwinKai
Copy link
Contributor Author

It seems like that https://github.com/KerwinKai/cv2_backend_for_mmengine/blob/main/cv2_backend_result/images/draw_binary_masks_cv2.png is different from https://github.com/KerwinKai/cv2_backend_for_mmengine/blob/main/cv2_backend_result/images/draw_binary_masks_matplotlib.png.

I fix my code to orgin version in main mmengine and update https://github.com/KerwinKai/cv2_backend_for_mmengine/tree/main/cv2_backend_result/images again. The different now I thick may cause by line 1194.

        if self.backend == 'matplotlib':
            self.ax_save.imshow(
                img,
                extent=(0, self.width, self.height, 0),
line1194  interpolation='nearest')
        else:
            self._image = img

Here is a use example in larger size image: https://github.com/KerwinKai/cv2_backend_for_mmengine/tree/main/cv2_backend_result/test_binary_masks_in_larger_image

@qianyizhang
Copy link

this seems like a solid enhancement, why not getting merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants