Skip to content

Fix TSAN error in RGB2HSV.#21948

Closed
vrabaud wants to merge 2 commits intoopencv:4.xfrom
vrabaud:3.4_rgb2hsv
Closed

Fix TSAN error in RGB2HSV.#21948
vrabaud wants to merge 2 commits intoopencv:4.xfrom
vrabaud:3.4_rgb2hsv

Conversation

@vrabaud
Copy link
Copy Markdown
Contributor

@vrabaud vrabaud commented May 4, 2022

initialized can be read while the if condition is not done yet and initialized updated.

Pull Request Readiness Checklist

  • 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

int hr = hrange;
const int* hdiv_table = hr == 180 ? hdiv_table180 : hdiv_table256;

cv::AutoLock lock(*mutex);
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.

This change is not valid as it serializes parallel code.

There is more generic problem related to TSAN. It expects std::atomic usage - OpenCV codebase is still not ported to them (atomics is C++11 feature).
Solution should be complete - otherwise problem is not resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I fixed the serialization. For TSAN and atomic, I can also only submit this patch to 4.x.
I indeed believe there are other TSAN issues with 3.x that are unfixable anyway.

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.

In general there is no bug if multiple threads writes sdiv_table/hdiv_table180/hdiv_table256 tables values in parallel (result is be the same anyway).
As stated in PRs description, the problem is related to initialized load/store.

Also there is no reason to initialize tables in workers if they use the same data. Moved initialization to constructor.

Proposed patch:
alalek@pr21948_r

/cc @vpisarev

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thx, of course, that is the proper fix. Thx ! Closing this

CvtColorLoop(src_data, src_step, dst_data, dst_step, width, height, RGB2HSV_b(scn, blueIdx, hrange));
else
if(depth == CV_8U) {
Mutex cvtColorLockInstance;
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.

Using different mutexes from multiple threads doesn't help to avoid initialization data race (because they are different)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants