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.

Sign up for GitHub

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

Jump to bottom

Add support for decoding multiple resolution videos in the same pipeline. #1144

Merged
merged 3 commits into from
Sep 27, 2019
Merged

Add support for decoding multiple resolution videos in the same pipeline. #1144

merged 3 commits into from
Sep 27, 2019

Conversation

a-sansanwal
Copy link
Contributor

Why we need this PR?

What happened in this PR?

  • We store resolution information about each sequence and reconfigure the decoder when we encounter a change.
  • At runtime we try to load the symbol cuvidReconfigureDecoder from libnvcuvid.so.
    If this symbol is found then we assume that the video codec library installed
    supports re-configuring the decoder.
  • If the symbol is missing and we encounter a resolution change during the decode process, we exit
    gracefully and do not process further.
  • If however the symbol cuvidReconfigureDecoder is found then then we call the aforementioned API and perform decode as usual.
  • It's also known that the reconfigure API was supported from driver 396 onwards on x86
    and 415 for PPC. This information is given to the user when we are not able to perform
    decode.
            err << "File " << filename << " is not the same size as previous files. ("
                << width << "x" << height
                << " instead of "
                << max_width_ << "x" << max_height_ << "). "
                << "Install Nvidia driver version >=396 (x86) or >=415 (Power PC) to decode"
                   " multiple resolutions";

What is most important part that reviewers should focus on?

VideoLoader::get_or_open_file in video_loader.cc
CUVideoDecoder::reconfigure and CUVideoDecoder::initialize in cuvideodecoder.cc

Was this PR tested? How?

  • Tested with videoreader example in docs/example/video/ with dataset of different resolution videos
  • For testing that DALI exits gracefully when the API is not present, i removed the symbol cuvidReconfigureDecoder from libnvcuvid.so using a hex editor.
  • Perf was compared using a benchmark application, and there was no performance penalty observed.

@a-sansanwal a-sansanwal force-pushed the video_reader_multiple_resolution branch from 810955e to 67997f5 Compare August 6, 2019 09:26
JanuszL
JanuszL reviewed Aug 6, 2019
View reviewed changes
Copy link
Contributor

@JanuszL JanuszL left a comment

Choose a reason for hiding this comment

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

Please check my comment

@a-sansanwal a-sansanwal changed the title Add support for decoding multiple resolution videos in the same pipeline. [WIP] Add support for decoding multiple resolution videos in the same pipeline. Aug 8, 2019
@a-sansanwal a-sansanwal force-pushed the video_reader_multiple_resolution branch from 67997f5 to 3adac83 Compare August 12, 2019 10:45
@a-sansanwal a-sansanwal changed the title [WIP] Add support for decoding multiple resolution videos in the same pipeline. Add support for decoding multiple resolution videos in the same pipeline. Aug 12, 2019
@a-sansanwal a-sansanwal force-pushed the video_reader_multiple_resolution branch from 3adac83 to b7aaf65 Compare August 13, 2019 08:34
@JanuszL
Copy link
Contributor

JanuszL commented Aug 14, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [857629]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [857629]: BUILD FAILED

JanuszL
JanuszL reviewed Aug 14, 2019
View reviewed changes
dali/pipeline/operators/reader/loader/video_loader.h Outdated
@@ -217,8 +237,9 @@ class VideoLoader : public Loader<GPUBackend, SequenceWrapper> {
int stride_;
int output_height_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Clang is complaining about not used output_height_ and output_width_.

JanuszL
JanuszL reviewed Aug 14, 2019
View reviewed changes
dali/pipeline/operators/reader/nvdecoder/nvdecoder.h Outdated
@@ -168,6 +171,8 @@ class NvDecoder {
bool rgb_;
DALIDataType dtype_;
bool normalized_;
int max_height_;
Copy link
Contributor

Choose a reason for hiding this comment

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

max_height_ and max_width_ are not used (thank you clang build 😄 )

@a-sansanwal a-sansanwal force-pushed the video_reader_multiple_resolution branch from b7aaf65 to 7934281 Compare August 15, 2019 13:13
jantonguirao
jantonguirao reviewed Aug 16, 2019
View reviewed changes
dali/pipeline/operators/reader/loader/video_loader.cc Outdated
if (width_ != codecpar(stream)->width ||
height_ != codecpar(stream)->height ||
codec_id_ != codec_id) {
if (codec_id_ != codec_id) {
Copy link
Contributor

Choose a reason for hiding this comment

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

DALI_ENFORCE(codec_id_ == codec_id, "File " + filename + " is not the same codec as previous files");
would be a little more compact

dali/pipeline/operators/reader/loader/video_loader.cc Outdated
if (max_width_ != width ||
max_height_ != height) {
std::stringstream err;
err << "File " << filename << " is not the same size as previous files. ("
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
err << "File " << filename << " is not the same size as previous files. ("
err << "File " << filename << " does not have the same resolution as previous files. ("

dali/pipeline/operators/reader/loader/video_loader.h Outdated
@@ -138,6 +140,7 @@ class VideoLoader : public Loader<GPUBackend, SequenceWrapper> {

file_label_pair_ = filesystem::get_file_label_pair(file_root_, filenames_,
file_list_);
DALI_ENFORCE(file_label_pair_.size() != 0, "No files were read.");
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
DALI_ENFORCE(file_label_pair_.size() != 0, "No files were read.");
DALI_ENFORCE(!file_label_pair_.empty(), "No files were read.");

dali/pipeline/operators/reader/nvdecoder/cuvideodecoder.cc Show resolved Hide resolved
dali/pipeline/operators/reader/nvdecoder/cuvideodecoder.cc Outdated

if (width < caps_.nMinWidth ||
height < caps_.nMinHeight) {
DALI_FAIL("Video is too small in at least one dimension.");
Copy link
Contributor

Choose a reason for hiding this comment

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

consider rephrasing this in terms of DALI_ENFORCE

dali/pipeline/operators/reader/nvdecoder/cuvideodecoder.cc Outdated

if (width > caps_.nMaxWidth ||
height > caps_.nMaxHeight) {
DALI_FAIL("Video is too large in at least one dimension.");
Copy link
Contributor

Choose a reason for hiding this comment

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

consider rephrasing this in terms of DALI_ENFORCE

dali/pipeline/operators/reader/nvdecoder/cuvideodecoder.cc Outdated
}

if (width * height / 256 > caps_.nMaxMBCount) {
DALI_FAIL("Video is too large (too many macroblocks).");
Copy link
Contributor

Choose a reason for hiding this comment

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

consider rephrasing this in terms of DALI_ENFORCE

dali/pipeline/operators/reader/nvdecoder/cuvideodecoder.h Show resolved Hide resolved
dali/test/python/test_video_pipeline.py Show resolved Hide resolved
@a-sansanwal a-sansanwal force-pushed the video_reader_multiple_resolution branch from 7934281 to e86a2fd Compare August 19, 2019 07:33
@JanuszL
Copy link
Contributor

JanuszL commented Aug 19, 2019

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [863225]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [863225]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [864884]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [864884]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [869869]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [869869]: BUILD FAILED

1 similar comment
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [869869]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [869869]: BUILD PASSED

jantonguirao
jantonguirao approved these changes Sep 20, 2019
View reviewed changes
Copy link
Contributor

@jantonguirao jantonguirao left a comment

Choose a reason for hiding this comment

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

LGTM (just couple of minor comments added)

dali/pipeline/operators/reader/loader/video_loader.h Show resolved Hide resolved
dali/pipeline/operators/reader/nvdecoder/cuvideodecoder.cc Show resolved Hide resolved
dali/pipeline/operators/reader/video_reader_op.h Show resolved Hide resolved
@jantonguirao
Copy link
Contributor

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909480]: BUILD STARTED

@a-sansanwal
Add support for decoding multiple resolutions in the same pipeline.
35938fb 
Signed-off-by: Abhishek Sansanwal <asansanwal@nvidia.com>
@a-sansanwal
Add tests for multiple resolution decode for VideoReader
036ec76 
Description:
Add tests to video_reader_op_test.cc and test_video_pipeline.py
Depends upon NVIDIA/DALI_extra#20

Signed-off-by: Abhishek Sansanwal <asansanwal@nvidia.com>
@a-sansanwal a-sansanwal force-pushed the video_reader_multiple_resolution branch from b251770 to 036ec76 Compare September 20, 2019 09:40
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [909480]: BUILD PASSED

@wantsjean
Copy link

Hi! Can I use this feature in the nightly-build python version right now? if yes, which function?

@a-sansanwal
Copy link
Contributor Author

a-sansanwal commented Sep 24, 2019 •
edited
Loading

@wantsjean

Hi! Can I use this feature in the nightly-build python version right now? if yes, which function?

Right now, you'll have to build it yourself after applying this patch. It's not available in nightly yet because its not merged.

Build steps are here.
https://docs.nvidia.com/deeplearning/sdk/dali-developer-guide/docs/quickstart.html#compiling-dali-from-source-using-docker-builder-recommended

@wantsjean
Copy link

@a-sansanwal I will do, thank you

@jantonguirao
Copy link
Contributor

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [915036]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [915036]: BUILD FAILED

jantonguirao
jantonguirao reviewed Sep 24, 2019
View reviewed changes
dali/pipeline/operators/reader/video_reader_op_test.cc Outdated
}
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

/opt/dali/dali/pipeline/operators/reader/video_reader_op_test.cc:154: Line ends in whitespace. Consider deleting these extra spaces. [whitespace/end_of_line] [4]

jantonguirao
jantonguirao requested changes Sep 24, 2019
View reviewed changes
Copy link
Contributor

@jantonguirao jantonguirao left a comment

Choose a reason for hiding this comment

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

There is a linter error in video_reader_op_test.cc

@a-sansanwal a-sansanwal force-pushed the video_reader_multiple_resolution branch from 59591d1 to b4c194f Compare September 24, 2019 15:17
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [915064]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [915064]: BUILD PASSED

@jantonguirao jantonguirao merged commit 0be012c into NVIDIA:master Sep 27, 2019
@a-sansanwal a-sansanwal deleted the video_reader_multiple_resolution branch October 1, 2019 11:11
00liujj pushed a commit to 00liujj/DALI that referenced this pull request Oct 10, 2019
@a-sansanwal @00liujj
Add support for decoding multiple resolution videos in the same pipel…
6777783 
…ine. (NVIDIA#1144)

* Add support for decoding multiple resolutions in the same pipeline.
* Add tests for multiple resolution decode for VideoReader

Signed-off-by: Abhishek Sansanwal <asansanwal@nvidia.com>
Signed-off-by: Jianjun Liu <00liujj@163.com>
@JanuszL JanuszL mentioned this pull request Nov 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JanuszL JanuszL JanuszL approved these changes

@jantonguirao jantonguirao jantonguirao approved these changes

@klecki klecki Awaiting requested review from klecki

@banasraf banasraf Awaiting requested review from banasraf

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@a-sansanwal @JanuszL @dali-automaton @jantonguirao @wantsjean