Skip to content
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

Timelapse Sample. #352

Open
wants to merge 59 commits into
base: main
Choose a base branch
from

Conversation

AdityaKBhadragond14
Copy link
Collaborator

@AdityaKBhadragond14 AdityaKBhadragond14 commented May 15, 2024

IMPORTANT: All PRs must be linked to an issue (except for extremely trivial and straightforward changes).

Fixes #233 & #353

Description
This PR contains the Surveillance summary timelapse based on motion detection sample number 7 from above linked issue.
This sample utilizes the following modules:

  • Mp4ReaderSource

  • MotionVectorExtractor

  • ColorConversionXForm

  • H264EncoderNVCodec

  • Mp4WriterSink

The Mp4ReaderSource reads a mp4 file. Mp4ReaderSource next is MotionVectorExtractor which gets the frames which match the given threshold and which are in motion. After MotionVectorExtractor the frames are passed to ColorConversionFormX to convert from BGR tor RGB and then from RGB to YUV420Planar because the H264EncoderNVCodec module only accepts YUV frames. After color conversion next is H264EncoderNVCodec which encodes the frames. After H264EncoderNVCodec next is Mp4WriterSink which writes the frames to disk.
Also in the MotionVectorExtractor I have introduced a flag sendOverlayFrame which when enabled sends the overlay frame else does not send the overlay frames. Since we dont need the overlay frames in this sample setting it to false.
This PR also has the fix for the issue Raw yuv420 output from motion extractor module is having horizontally layered bands on the frame.

Alternative(s) considered

Have you considered any alternatives? And if so, why have you chosen the approach in this PR?

Type

Feature and Bug Fix.

Screenshots (if applicable)

Checklist

  • I have read the Contribution Guidelines
  • I have written Unit Tests
  • I have discussed my proposed solution with code owners in the linked issue(s) and we have agreed upon the general approach

Copy link
Collaborator

@mraduldubey mraduldubey left a comment

Choose a reason for hiding this comment

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

formatting and other comments, restructure the format of tests, new impl for tests is required

@@ -24,6 +24,7 @@ class MotionVectorExtractorProps : public ModuleProps
return ModuleProps::getSerializeSize() + sizeof(sendDecodedFrame) + sizeof(motionVectorThreshold);
}
bool sendDecodedFrame = false;
bool sendOverlayFrame = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

should be added in serialization part below

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -202,6 +208,19 @@ int DetailFfmpeg::decodeAndGetMotionVectors(AVPacket* pkt, frame_container& fram
{
outFrame = makeFrameWithPinId(0, motionVectorPinId);
}
if (sendOverlayFrame == false) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. formatting
  2. put a comment on what is this change about

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -244,8 +264,15 @@ void DetailOpenH264::getMotionVectors(frame_container& frames, frame_sp& outFram
int32_t mMotionVectorSize = mWidth * mHeight * 8;
int16_t* mMotionVectorData = nullptr;
memset(&pDstInfo, 0, sizeof(SBufferInfo));
outFrame = makeFrameWithPinId(mMotionVectorSize, motionVectorPinId);
mMotionVectorData = static_cast<int16_t*>(outFrame->data());
if (sendOverlayFrame)
Copy link
Collaborator

Choose a reason for hiding this comment

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

formatting, in vs code use Ctrl+K, F to auto format

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

@@ -290,8 +318,19 @@ void DetailOpenH264::getMotionVectors(frame_container& frames, frame_sp& outFram
frames.insert(make_pair(motionVectorPinId, outFrame));
}
}

if ((!sDecParam.bParseOnly) && (pDstInfo.pDst[0] != nullptr) && (mMotionVectorSize != mWidth * mHeight * 8))
if(sendOverlayFrame == false){
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this change about ? why is this reqd in the sample PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is required to determine whether the frames are in the motion or not when we set the sendOverlayFrame to false.

@@ -0,0 +1,10 @@
#dependencies

find_package(Threads REQUIRED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Threads is required here, why ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not required forgot to remove this from the CMakeLists was just testing something. Removing it.

${LIBRE_INC_DIR}
${NVCODEC_INCLUDE_DIR}
)
target_link_libraries(
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I remove this then how do I link the aprapipes lib with the samples?

Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the libraries that are not required by the sample boss

find_library(APRA_PIPES_LIB NAMES aprapipes)
add_library(example_aprapipes STATIC ${SOURCE})
add_executable(runner_test ${UT_FILE})
target_include_directories ( example_aprapipes PRIVATE
Copy link
Collaborator

Choose a reason for hiding this comment

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

call it samples

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Renaming it to timelapsesample would be better as there will be multiple samples with this name.

int main() {
LoggerProps loggerProps;
loggerProps.logLevel = boost::log::trivial::severity_level::info;
Logger::setLogLevel(boost::log::trivial::severity_level::info);
Copy link
Collaborator

Choose a reason for hiding this comment

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

both 79 & 80 are not required. one is enough

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

bool stopPipeline();

private:
PipeLine pipeline;
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a method called test, and call from test added in timelapse-sample/ - it should use step - not the pipeline

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


timelapsePipeline -> startPipeline();

timelapsePipeline -> stopPipeline();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is not a test. it has no checks. also, need to use step() instead

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done added an assertion to check the frame size.

void DetailFfmpeg::getMotionVectors(frame_container& frames, frame_sp& outFrame, frame_sp& decodedFrame)
{

bool discardNoisyFrames(int black_pixel_percent, cv::Mat image) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also check if the pixels are all white

{
if ((!sDecParam.bParseOnly) && (pDstInfo.pDst[0] != nullptr) &&
(mMotionVectorSize != mWidth * mHeight * 8) &&
(abs(avgX) > threshold || abs(avgY) > threshold)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add a new bool prop and enable avg based thresholding only then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I discussed this with Mradul he suggested this should not be sample specific. The improvement should be for the module and not sample specific.


cv::cvtColor(yuvImgCV, bgrImg, cv::COLOR_YUV2BGR_I420);
frames.insert(make_pair(rawFramePinId, decodedFrame));
if (!discardNoisyFrames(97, bgrImg)) {
Copy link
Collaborator

@mohammedzakikochargi mohammedzakikochargi Jun 3, 2024

Choose a reason for hiding this comment

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

This should be also checked in the ffmpeg strategy

@AdityaKBhadragond14
Copy link
Collaborator Author

I have addressed the comments please have a look @mohammedzakikochargi .

Copy link
Collaborator

@mraduldubey mraduldubey left a comment

Choose a reason for hiding this comment

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

make some minor changes

${Boost_LIBRARIES}
${FFMPEG_LIBRARIES}
${OpenCV_LIBRARIES}
${JETSON_LIBS}
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you need all libs in the aprapipessampleut ? I dont think so.

${LIBRE_INC_DIR}
${NVCODEC_INCLUDE_DIR}
)
target_link_libraries(
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove the libraries that are not required by the sample boss

mColorchange1 = boost::shared_ptr<ColorConversion>(new ColorConversion(
ColorConversionProps(ColorConversionProps::BGR_TO_RGB)));
//convert frames from RGB to YUV420PLANAR
//the two step color change is done because H264Encoder takes YUV data and we don't have direct BGR to YUV.
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete after YUV data

return true;
}

bool TimelapsePipeline::stopPipeline() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

format this file with Ctrl+k,f

//the two step color change is done because H264Encoder takes YUV data and we don't have direct BGR to YUV.
mColorchange2 = boost::shared_ptr<ColorConversion>(new ColorConversion(
ColorConversionProps(ColorConversionProps::RGB_TO_YUV420PLANAR)));
mSync = boost::shared_ptr<Module>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is sync required

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sync is required by the encoder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sample Pipeline(s) ideas to showcase capabilities
5 participants