cudaarithm: do not reinterpret misaligned rows in bitwise ops - #4215
Open
SichenLiang wants to merge 1 commit into
Open
cudaarithm: do not reinterpret misaligned rows in bitwise ops#4215SichenLiang wants to merge 1 commit into
SichenLiang wants to merge 1 commit into
Conversation
bitwise_and/or/xor/not without a mask process the rows as 32-bit or 16-bit words whenever the row width in bytes allows it, without looking at the data pointers or at the steps. Two ordinary matrices break that. A ROI whose x offset is not a multiple of the word size misaligns every row; that is the reported case. And a matrix from cv::cuda::createContinuous() has step = cols * elemSize(), which need not be a multiple of the word size either, so rows after the first can be misaligned even at x offset 0. Both end in a "misaligned address" CUDA error that is sticky: it also breaks every later CUDA call in the process. Take a word path only when the row width and the data pointer and step of every operand are aligned to that word size, and fall back to the 16-bit path, or to the byte path when even that does not hold. This is the check cudev::TransformDispatcher makes on data and step before it vectorises, in modules/cudev/include/opencv2/cudev/grid/detail/transform.hpp; its own fallback does not help here, because that fallback still dereferences the type the caller reinterpreted the row as. Inputs whose pointers and steps are already aligned keep the path they had. Add Bitwise_Array_Misaligned: the four operations on CV_8UC1, CV_8UC3 and CV_16UC1, at a row width in bytes that is a multiple of 4 and, for the 8-bit types, at one that is a multiple of 2 only, with the misalignment coming either from a one element ROI x offset or from a createContinuous() step, and applied to src1, to src2, to the destination or to all three, so that a check that misses one operand is not enough. 192 instances, all 192 of which fail without the change above. Fixes opencv#4211
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
cv::cuda::bitwise_and,bitwise_or,bitwise_xorandbitwise_notwithout amask choose a 32-bit or a 16-bit word path from the row width in bytes alone,
without looking at the data pointers or at the steps. Two kinds of ordinary
matrix break that.
A ROI whose x offset is not a multiple of the word size misaligns every row;
that is the reported case. And a matrix from
cv::cuda::createContinuous()hasstep = cols * elemSize(), which need not be a multiple of the word sizeeither, so rows after the first can be misaligned even at x offset 0:
createContinuous(8, 17, CV_8UC1)gives step 17 and crashes today with no ROIinvolved. Both end in a
misaligned addressCUDA error that is sticky: it alsobreaks every later CUDA call in the process, so one fault can turn a whole test
run red.
Take a word path only when the row width and the data pointer and step of every
operand are aligned to that word size, and fall back to the 16-bit path, or to
the byte path when even that does not hold. This is the check
cudev::TransformDispatchermakes on data and step before it vectorises(
modules/cudev/include/opencv2/cudev/grid/detail/transform.hpp). Its ownfallback does not help here, because that fallback still dereferences the type
the caller reinterpreted the row as, which is why the fault surfaces inside
TransformDispatcher<false>rather than at the call site. Inputs whose pointersand steps are already aligned keep the path they had.
Bitwise_Array_Misalignedcovers the four operations on CV_8UC1, CV_8UC3 andCV_16UC1, with the misalignment coming either from a one-element ROI x offset or
from a
createContinuous()step, applied tosrc1, tosrc2, to thedestination, or to all three, so that a check which misses one operand is not
enough. 192 instances, all 192 of which fail without the change.
One thing worth knowing before reading a CI result.
opencv_test_cudaarithmshares a single
cv::theRNG()stream across the whole process (CUDA_TEST_Pdoes not call
cvtest::testSetUp(), so--test_seeddoes not move it), and afew
CUDA_Arithmtests compare a GPU result against a CPU one on random datathat can land on a tie. Adding any test that draws from that stream shifts where
those tests sample it. On an unmodified library, with no change to
bitwise_mat.cuand no test added to it, a throwaway test that consumes 47bytes of random data is enough to turn the suite red on
CUDA_Arithm/ThresholdOtsu.Accuracy/8; 4 of the first 100 draw offsets I trieddo it, and
CUDA_Arithm/PolarToCart.Accuracyis sensitive in the same way. Thesuite is green here with this patch and its tests, but if you see either of
those fail on this PR, it is that pre-existing sensitivity and not this change.
I am happy to hand over the reproducer.
Fixes #4211
Pull Request Readiness Checklist
See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request
Patch to opencv_extra has the same branch name.