Skip to content

dnn: cleanup of Halide backend#24123

Closed
fengyuentau wants to merge 5 commits intoopencv:4.xfrom
fengyuentau:halide_cleanup
Closed

dnn: cleanup of Halide backend#24123
fengyuentau wants to merge 5 commits intoopencv:4.xfrom
fengyuentau:halide_cleanup

Conversation

@fengyuentau
Copy link
Copy Markdown
Member

@fengyuentau fengyuentau commented Aug 8, 2023

🚀 Cleanup for OpenCV 5.0.

Merge with opencv/opencv_extra#1081.

Todo:

  1. Clean Halide related from opencv_extra

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 another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the 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

@fengyuentau fengyuentau added category: dnn port to 5.x is needed Label for maintainers. Authors of PR can ignore this cleanup Code cleanup (e.g, drop legacy C-API, legacy unmaintained code) labels Aug 8, 2023
@fengyuentau fengyuentau added this to the 4.9.0 milestone Aug 8, 2023
@fengyuentau fengyuentau requested review from dkurt and vpisarev August 8, 2023 03:58
//! OpenCV is built with Intel OpenVINO or
//! DNN_BACKEND_OPENCV otherwise.
DNN_BACKEND_DEFAULT = 0,
DNN_BACKEND_HALIDE,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That probably breaks enum numeration

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. It also breaks compatibility in case people use enum values directrly instead of enums. Let me discuss with other folks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I propose to remove it in 5.x, but do not touch 4.x to presume API compatibility.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@asmorkalov, Just to clarify, do you mean just a enum value.

@@ -1,988 +0,0 @@
// This file is part of OpenCV project.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Despite the name, this file should not be removed completely because there are single layer tests for other backends. I suggest renaming.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Need further discussion.

Copy link
Copy Markdown
Member

@dkurt dkurt left a comment

Choose a reason for hiding this comment

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

Generally I'm fine with removing Halide backend unless we would like to focus on exotic targets such as Hexagon, PowerPC, Qualcomm, OpenGL, Apple Metal or DirectX (see https://github.com/halide/Halide)

Please take a look at the comments above.

@dkurt
Copy link
Copy Markdown
Member

dkurt commented Aug 8, 2023

🚀 Cleanup for OpenCV 5.0.

If this is a cleanup for OpenCV 5.0 why target branch is 4.x?

@fengyuentau
Copy link
Copy Markdown
Member Author

If this is a cleanup for OpenCV 5.0 why target branch is 4.x?

The motivation is cleaning old backends for the release of OpenCV 5.0. Some cleanings start from 4.x and this is one of them.


testing::internal::ParamGenerator< tuple<Backend, Target> > dnnBackendsAndTargets(
bool withInferenceEngine = true,
bool withHalide = false,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure that this is safe to remove intermediate default argument. Maybe it's better to keep for now and add CV_Assert(!withHalide) inside?

@dkurt dkurt mentioned this pull request Aug 17, 2023
4 tasks
@asmorkalov asmorkalov modified the milestones: 4.9.0, 5.0 Sep 4, 2023
@fengyuentau
Copy link
Copy Markdown
Member Author

Closed in favor of #24231.

@fengyuentau fengyuentau closed this Sep 6, 2023
@fengyuentau fengyuentau deleted the halide_cleanup branch November 21, 2023 03:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category: dnn cleanup Code cleanup (e.g, drop legacy C-API, legacy unmaintained code) port to 5.x is needed Label for maintainers. Authors of PR can ignore this

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants