-
Notifications
You must be signed in to change notification settings - Fork 127
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
Fixes and improvements for StereoDepth, ColorCamera #82
Conversation
Works with LRcheck or LRcheck+Subpixel for now. The updated FW also fixes some crashes with LRcheck mode enabled
/** | ||
* @param align Set the disparity/depth alignment to the perspective of one camera | ||
*/ | ||
void setDepthAlign(Properties::DepthAlign align); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use CameraBoardSocket
to which to align to, instead of new DepthAlign
enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently DepthAlign
contains: { AUTO = -1, RIGHT, LEFT, CENTER, RGB }
RIGHT
(default) andLEFT
would map to the respective 'rectified' streams.CENTER
would just displace the disparity map (each pixel by its disparity_value / 2), whileRGB
(not yet implemented, defaulting with a warning to CENTER) would need an additional 'warp' step (extrinsics alignment / resolution scaling). For boards like BW1092 where the RGB camera is not centered, the disparity displacement would need to be done by a different amount.
Mapping to the real LEFT and RIGHT (from CameraBoardSocket
), we would need to do and inverse homography transform, which should be easily doable with the mechanism for RGB in place. But it incurs the additional warp overhead, and in many cases it's not needed (as could be used directly with the rectified streams).
Should then we extend DepthAlign
as: { AUTO = -1, RECTIFIED_RIGHT, RECTIFIED_LEFT, CENTER, RGB, LEFT, RIGHT }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about if we separate this two to:
- CameraBoardSocket <- which camera it references (can be up to number of MIPI lanes, not added yet to enum)
- DepthAlign <- "mode" to referenced, like: RECTIFIED, ORIGINAL, CENTER, .. (TBD)
I think we'd cover all cases with this, without too much repetition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely adding a separate config with CameraBoardSocket
is good, I will add it.
But one case where combining these two might be confusing is with e.g. CameraBoardSocket::RGB and mode: RECTIFIED.
Or with images from host, or running Stereo on OAK-1: one input from the RGB camera, the other from host (actually forwarded by another OAK-1), as one customer wants to use. Actually I think here we should have a way to pass a minimal StereoCalibration structure to StereoDepth, that won't refer to CameraBoardSocket entries, but simply to the 'left' and 'right' inputs to the node. We can discuss further on luxonis/depthai-shared#19
So I was thinking to reference based on the I/Os of the StereoDepth node. Something like
DepthAlign: RECTIFIED_RIGHT, RECTIFIED_LEFT, RECTIFIED_CENTER, INPUT_LEFT, INPUT_RIGHT, OTHER_CAMERA
.
When OTHER_CAMERA
is set, will specify it by CameraBoardSocket
. And for example running standard stereo on OAK-D, INPUT_RIGHT
would be equivalent to OTHER_CAMERA
+ CameraBoardSocket::RIGHT
.
Thoughts on that? Also how to have the API, two separate functions, one with two parameters, or an overloaded function (pass either DepthAlign or CameraBoardSocket, and then remove OTHER_CAMERA
)?
CC @saching13
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What martin suggested works best if we ignore the concept that left, and right are our two global shutter cameras.
In stereo node we set left camera and right camera using CameraBoardSocket
and we have
DepthAlign <- "mode" to referenced, like: RECTIFIED, ORIGINAL, CENTER, .. (TBD)
In DepthAlign mode let's have only RECTIFIED_LEFT, RECTIFIED_RIGHT and CENTER.
And setAlignment(cameraBoardSocket)
will make sure to align to any camera's that we have.
And another overload function or something like setExtendedAlignment
takes DepthAlign
mode. But in this case RECTIFIED_LEFT
will be the rectified_left
frame of the camera/image passed to the stereo left. and RECTIFIED_RIGHT
will be the rectified_right
frame of the camera/image passed as the right image to the stereo node.
One such case is where rgb camera can be stereo left and right camera can be stereo right.
And CENTER would not be the center of the device but the center of the stereo baseline.
Thoughts ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any resolution here? I think I've understood, and a lot of these are sounding good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed as an overloaded function, as the options (DepthAlign
and CameraBoardSocket
) cannot be combined, and would be confusing to set both.
…egrees', will be refactored when the new calibration structure is integrated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also don't forget to add outputs 'isp' & 'raw' to the list of outputs
std::vector<Node::Output> ColorCamera::getOutputs() {
return {video, preview, still};
}
src/pipeline/node/ColorCamera.cpp
Outdated
void ColorCamera::setIspScale(int numerator, int denominator) { | ||
properties.ispScale.horizNumerator = numerator; | ||
properties.ispScale.horizDenominator = denominator; | ||
properties.ispScale.vertNumerator = numerator; | ||
properties.ispScale.vertDenominator = denominator; | ||
} | ||
|
||
void ColorCamera::setIspScaleFull(int horizNum, int horizDenom, int vertNum, int vertDenom) { | ||
properties.ispScale.horizNumerator = horizNum; | ||
properties.ispScale.horizDenominator = horizDenom; | ||
properties.ispScale.vertNumerator = vertNum; | ||
properties.ispScale.vertDenominator = vertDenom; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets overload these two as well.
As well as an tuple overload (plays nicely with initializer lists and tuples/lists in python)
setIspScale(int numerator, int denominator)
setIspScale(std::tuple<int, int> scale)
setIspScale(int horizNum, int horizDenom, int vertNum, int vertDenom)
setIspScale(std::tuple<int, int> horizScale, std::tuple<int, int> vertScale)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also same for setPreviewSize
, setVideoSize
and setStillSize
Output isp{*this, "isp", Output::Type::MSender, {{DatatypeEnum::ImgFrame, false}}}; | ||
Output raw{*this, "raw", Output::Type::MSender, {{DatatypeEnum::ImgFrame, false}}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing docstring
TODO depth and subpixel (U16)
- add `isp` and `raw` to ColorCamera list of outputs, add docstrings - overloaded `setIspScale`, with tuple inputs as options - also overloaded `setPreviewSize`, `setVideoSize` and `setStillSize` with tuple inputs
remove non-implemented setNoiseReductionStrength. Updated FW: all initial controls can be applied for Mono/ColorCamera (no longer limited to focus settings for Color)
@@ -118,7 +118,7 @@ cv::Mat ImgFrame::getCvFrame() { | |||
} break; | |||
|
|||
case Type::YUV420p: | |||
cv::cvtColor(frame, output, cv::ColorConversionCodes::COLOR_YUV420p2BGR); | |||
cv::cvtColor(frame, output, cv::ColorConversionCodes::COLOR_YUV2BGR_IYUV); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About the change here, it may look counter-intuitive, but the YUV420p frames we receive from device (currently from ColorCamera::isp
output that is added on this PR) have the plane order: Y, U, V.
In OpenCV the format YUV420p
is defined as an equivalent to YV12
:
cv::COLOR_YUV420p2BGR = COLOR_YUV2BGR_YV12,
https://docs.opencv.org/4.5.1/d8/d01/group__imgproc__color__conversions.html
While YV12
has the plane order: Y, V, U:
https://www.fourcc.org/pixel-format/yuv-yv12/
to prevent an out-of-memory situation due to too many threads created
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd only apply the --parallel 8 option to CI: main.workflow.yml
and
And leave --parallel elsewhere.
So I added |
(still not optimized)
The problem with
Or add this directly into the code block. On other hand, for script like in
Also apparently |
don't link depth in pipeline if not used, and other cleanup. Update FW: ispScale factors simplification done on device, other bugfixes
The change in discussion (--parallel 8 in Readme) was already cherry-picked to develop (so no longer appears on this PR)
Update shared: stereo_fixes merged
.setEmptyCalibration()
and already rectified frames streamed from hostsetOutputRectified
/setOutputDepth
, the outputs will be automatically enabled when connected in the pipelineisp
andraw
outputs, scale config forisp
. Currently when the scaling is enabled, will need to disablepreview
scaling (if the output is used). TODO fix.Related PR: luxonis/depthai-shared#23