Affine transform #51
Affine transform #51
Conversation
|
Why not add a couple of test cases pls ? @yashrajsapra @joiskash |
There was a problem hiding this comment.
Thanks @yashrajsapra for this PR! I have done an initial round of review. Please address the comments. Also as Akhil has requested , please add some unit tests. Make sure you go through the checklist before adding them.
base/CMakeLists.txt
Outdated
| hunter_add_package(ncursesw) | ||
| find_package(ncursesw CONFIG REQUIRED) | ||
| SET(CURSES_INCLUDE_DIR ${NCURSESW_ROOT}/include/ncursesw) | ||
| SET(CURSES_LIBRARIES PkgConfig::ncursesw) | ||
| ENDIF(NOT ENABLE_ARM64) | ||
|
|
||
| IF(ENABLE_CUDA) | ||
| enable_language(CUDA) | ||
| enable_language(CUDA) |
There was a problem hiding this comment.
indentation seems wrong here
Format the code using ctrl+shift+I . Do this for all files
base/CMakeLists.txt
Outdated
| hunter_add_package(ncursesw) | ||
| find_package(ncursesw CONFIG REQUIRED) | ||
| SET(CURSES_INCLUDE_DIR ${NCURSESW_ROOT}/include/ncursesw) | ||
| SET(CURSES_LIBRARIES PkgConfig::ncursesw) |
There was a problem hiding this comment.
Why have you changed the indentation?
Format the code using ctrl+shift+I
| int x=0; | ||
| int y = 0; | ||
| float scale = 1.0f; |
There was a problem hiding this comment.
No hard coding values. Pleas remove from function definition also
| } | ||
|
|
||
| ImageMetadata::ImageType imageType = ImageMetadata::MONO; | ||
| if (mFrameType == FrameMetadata::RAW_IMAGE) |
There was a problem hiding this comment.
Why is this if statement required? I think the switch case above is already taking care of this
| if (mFrameType == FrameMetadata::RAW_IMAGE) | ||
| { | ||
| auto rawMetadata = FrameMetadataFactory::downcast<RawImageMetadata>(metadata); | ||
| int x, y, w, h; |
There was a problem hiding this comment.
What is x and y? use width and height instead of w an h. Use more explanatory variable names instead of x & y
| int x, y, w, h; | ||
| w = rawMetadata->getWidth(); | ||
| h = rawMetadata->getHeight(); | ||
| RawImageMetadata outputMetadata(w, h, rawMetadata->getImageType(), rawMetadata->getType(), 512, rawMetadata->getDepth(), FrameMetadata::DMABUF, true); |
There was a problem hiding this comment.
What is the hard coded value of 512?
| FrameMetadata::FrameType mFrameType; | ||
| int depth; | ||
| int channels; | ||
| NppiSize srcSize[4]; | ||
| NppiRect srcRect[4]; | ||
| int srcPitch[4]; | ||
| size_t srcNextPtrOffset[4]; | ||
| NppiSize dstSize[4]; | ||
| NppiRect dstRect[4]; | ||
| int dstPitch[4]; | ||
| size_t dstNextPtrOffset[4]; | ||
|
|
||
| double shiftX; | ||
| double shiftY; | ||
| void *ctx; | ||
| NppStreamContext nppStreamCtx; |
There was a problem hiding this comment.
Move this above the function for better readability
| FrameMetadata::FrameType mFrameType; | ||
| int depth; | ||
| int channels; | ||
| NppiSize srcSize[4]; |
There was a problem hiding this comment.
Add comments for the variables that are tough to understand explaining in one line what they are for.
| si = props.scale * sin(props.angle * PI / 180); | ||
| co = props.scale * cos(props.angle * PI / 180); | ||
| double shx, shy; | ||
| shx = (1 - co) * (srcSize[0].width / 2) + si * srcSize[0].height / 2; |
There was a problem hiding this comment.
Try not to use acronyms that are not standard. Not sure what shx means
| { | ||
| auto frame = frames.cbegin()->second; | ||
| auto outFrame = makeFrame(mDetail->mFrameLength); | ||
| cudaFree(0); |
There was a problem hiding this comment.
Do we always have to call cudaFree?
There was a problem hiding this comment.
You can put this in processSOS
| auto frame = frames.cbegin()->second; | ||
| auto outFrame = makeFrame(mDetail->mFrameLength); | ||
| cudaFree(0); | ||
| cudaMemset(static_cast<DMAFDWrapper *>(outFrame->data())->getCudaPtr(), 0, outFrame->size()); |
There was a problem hiding this comment.
Change this to cudaMemsetAsync and pass the Cuda stream. Avoid using the default cuda stream. In Compute you are using stream so here also you can use the same
|
@yashrajsapra please update this branch and lets try to close it by this week. We only need to support DMA and GPU . Later enhancement can be to check pin input type and then based on that use DMA/GPU or CPU memory |
|
@yashrajsapra any updates? |
|
This has a been addressed in #227 now |
IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).
#36
Description
Added Affine Transform Module.
Currently supported only on jetson boards.
Alternative(s) considered
Type
Feature
Screenshots (if applicable)
Checklist