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

Frames decoder gpu without index #4302

Merged
merged 33 commits into from
Oct 13, 2022

Conversation

awolant
Copy link
Contributor

@awolant awolant commented Sep 30, 2022

  • adds an ability to decode the video on the GPU without
    building the internal index

Category:

New feature (non-breaking change which adds functionality)

Description:

  • adds an ability to decode the video on the GPU without
    building the internal index

Additional information:

Affected modules and functionalities:

  • frames_decoder_gpu

Key points relevant for the review:

  • NA

Tests:

  • Existing tests apply
  • New tests added
    • Python tests
    • GTests
      • FramesDecoderGpuTest.VariableFrameRateNoIndex
      • FramesDecoderGpuTest.VariableFrameRateHevcNoIndex
    • Benchmark
    • Other
  • N/A

Checklist

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-3038

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

awolant commented Sep 30, 2022

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6054940]: BUILD STARTED

Comment on lines 39 to 43
if (frames_decoder->HasIndex()) {
return frames_decoder->ProcessPictureDecodeWithIndex(user_data, picture_params);
}

return frames_decoder->ProcessPictureDecodeWithoutIndex(user_data, picture_params);
Copy link
Contributor

Choose a reason for hiding this comment

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

In this case I think that if/else is more verbose about one of the options, as it is not early exit after all.

Suggested change
if (frames_decoder->HasIndex()) {
return frames_decoder->ProcessPictureDecodeWithIndex(user_data, picture_params);
}
return frames_decoder->ProcessPictureDecodeWithoutIndex(user_data, picture_params);
if (frames_decoder->HasIndex()) {
return frames_decoder->ProcessPictureDecodeWithIndex(user_data, picture_params);
} else {
return frames_decoder->ProcessPictureDecodeWithoutIndex(user_data, picture_params);
}

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

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6054940]: BUILD PASSED

@awolant awolant changed the title [WIP] Frames decoder gpu without index Frames decoder gpu without index Sep 30, 2022
@awolant awolant marked this pull request as ready for review September 30, 2022 19:35
Comment on lines 366 to 370
if (HasIndex()) {
return ReadNextFrameWithIndex(data, copy_to_output);
}

return ReadNextFrameWithoutIndex(data, copy_to_output);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick - for symmetry

Suggested change
if (HasIndex()) {
return ReadNextFrameWithIndex(data, copy_to_output);
}
return ReadNextFrameWithoutIndex(data, copy_to_output);
if (HasIndex()) {
return ReadNextFrameWithIndex(data, copy_to_output);
} else {
return ReadNextFrameWithoutIndex(data, copy_to_output);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

}

bool FramesDecoderGpu::EmptyBuffer() const {
for (auto &frame_ : frame_buffer_) {
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
for (auto &frame_ : frame_buffer_) {
for (auto &frame : frame_buffer_) {

Copy link
Contributor

Choose a reason for hiding this comment

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

Done

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@JanuszL
Copy link
Contributor

JanuszL commented Oct 7, 2022

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6116405]: BUILD STARTED

* @note This constructor assumes that the `memory_file` and
* `memory_file_size` arguments cover the entire video file, including the header.
*/
FramesDecoder(const char *memory_file, int memory_file_size, bool build_index = true);

/**
* @brief Number of frames in the video. It returns 0, if this information is unavailable.
*
*
Copy link
Contributor

Choose a reason for hiding this comment

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

When you touch these lines - they should just go away if we don't have anything after "brief".

Copy link
Contributor

Choose a reason for hiding this comment

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

The IDE trimmed the trailing whitespaces. Nevertheless, removed.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6116405]: BUILD PASSED

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@@ -108,11 +114,21 @@ class DLL_PUBLIC FramesDecoderGpu : public FramesDecoder {

BufferedFrame& FindEmptySlot();

bool HasEmptySlot() const;

bool EmptyBuffer() const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Reads like "empty the buffer" == "make it empty".

Suggested change
bool EmptyBuffer() const;
bool BufferEmpty() const;

or

Suggested change
bool EmptyBuffer() const;
bool IsBufferEmpty() const;

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

return false;
}

bool FramesDecoderGpu::EmptyBuffer() const {
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
bool FramesDecoderGpu::EmptyBuffer() const {
bool FramesDecoderGpu::BufferEmpty() const {

or

Suggested change
bool FramesDecoderGpu::EmptyBuffer() const {
bool FramesDecoderGpu::IsBufferEmpty() const {

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@JanuszL
Copy link
Contributor

JanuszL commented Oct 12, 2022

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6161684]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6161733]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6161684]: BUILD PASSED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6161733]: BUILD FAILED

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6167376]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6167376]: BUILD PASSED

@JanuszL JanuszL merged commit 58549fa into NVIDIA:main Oct 13, 2022
@JanuszL JanuszL mentioned this pull request Jan 11, 2023
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.

5 participants