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

Video tests utils and refactor #3620

Merged
merged 14 commits into from
Jan 15, 2022
Merged

Conversation

awolant
Copy link
Contributor

@awolant awolant commented Jan 14, 2022

Description

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring (Redesign of existing code that doesn't affect functionality)
  • Other (e.g. Documentation, Tests, Configuration)

What happened in this PR

  • It adds new feature needed because of we want to be able to compare frames on avarage difference
  • Refactoring to improve naming in video loader

Checklist

Tests

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
    • Benchmark
    • Other
  • N/A

Documentation

  • Existing documentation applies
  • Documentation updated
    • Docstring
    • Doxygen
    • RST
    • Jupyter
    • Other
  • N/A

DALI team only

Requirements

  • Implements new requirements
  • Affects existing requirements
  • N/A

REQ IDs: N/A

JIRA TASK: DALI-2252

Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
Signed-off-by: Albert Wolant <awolant@nvidia.com>
@awolant
Copy link
Contributor Author

awolant commented Jan 14, 2022

!build

@jantonguirao jantonguirao self-assigned this Jan 14, 2022
}

void VideoTestBase::CompareFramesAvg(
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: the name sounds as if you were comparing the average of each frame. Maybe AvgError would do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@JanuszL JanuszL self-assigned this Jan 14, 2022
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3756531]: BUILD STARTED

@awolant
Copy link
Contributor Author

awolant commented Jan 14, 2022

@JanuszL @jantonguirao I have a question about this PR for you. I have some more utilities in the video test that I use for myself for test/debug purposes. One is SaveFrame method in the tests that allows me to save the frame as image and look at it. Second is this improvement to Compare... methods where I compare pixels in parallel. This gives me a significant boost in performance, when I run those video tests.
Do you think I could include them in this PR for future use, even if for example SaveFrame will not be called anywhere in the tests? I think this might be useful to others in the future, and since I already have that I think it would be ok, but wanted to consult it with you.

@JanuszL
Copy link
Contributor

JanuszL commented Jan 14, 2022

@JanuszL @jantonguirao I have a question about this PR for you. I have some more utilities in the video test that I use for myself for test/debug purposes. One is SaveFrame method in the tests that allows me to save the frame as image and look at it. Second is this improvement to Compare... methods where I compare pixels in parallel. This gives me a significant boost in performance, when I run those video tests. Do you think I could include them in this PR for future use, even if for example SaveFrame will not be called anywhere in the tests? I think this might be useful to others in the future, and since I already have that I think it would be ok, but wanted to consult it with you.

Yes, I think it would be useful to add it, even add it into the test but ifdef out so during debugging it can be easily turned on.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3756531]: BUILD PASSED

@jantonguirao
Copy link
Contributor

o you think I could include them in this PR for future use, even if for example SaveFrame will not be called anywhere in the tests?

Yes, I think those would be useful.

Signed-off-by: Albert Wolant <awolant@nvidia.com>
@awolant
Copy link
Contributor Author

awolant commented Jan 14, 2022

!build

@awolant awolant changed the title Refactor video code Video tests utils and refactor Jan 14, 2022
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3757039]: BUILD STARTED

uint8_t *GetCfrFrame(int video_id, int frame_id) { return cfr_frames_[video_id][frame_id].data; }

uint8_t *GetVfrFrame(int video_id, int frame_id) { return vfr_frames_[video_id][frame_id].data; }

void SaveFrame(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a docstring to describe how to use it?
What is the meaning of frame_id, sample_id, and batch_id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Albert Wolant <awolant@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3757039]: BUILD PASSED

Comment on lines 32 to 33
int batch_size = nb_elements / nb_threads;
int batch_remainder = nb_elements % nb_threads;
Copy link
Contributor

Choose a reason for hiding this comment

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

A suggestion: You could get a more even distribution of work per thread doing something like this:

for (int i = 0; i < nb_threads; ++i) {
  int start = nb_elements * i / nb_threads;
  int end = nb_elements * (i+1) / nb_threads;
  threads[i] = std::thread(func, start, end, i);
}

This way you also don't need to treat the last batch differently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

int frame_id,
int sample_id,
int batch_id,
std::string folder_path,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
std::string folder_path,
const std::string &folder_path,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Albert Wolant <awolant@nvidia.com>
@awolant
Copy link
Contributor Author

awolant commented Jan 14, 2022

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3757576]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [3757576]: BUILD PASSED

@awolant awolant merged commit ffa6028 into NVIDIA:main Jan 15, 2022
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jan 23, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Feb 21, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request May 13, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
cyyever pushed a commit to cyyever/DALI that referenced this pull request Jun 7, 2022
* Add video tests utilities

Signed-off-by: Albert Wolant <awolant@nvidia.com>
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.

4 participants