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

Make sure that lossless support is disabled when it fails to initialize #4934

Merged
merged 3 commits into from
Jul 17, 2023
Merged

Make sure that lossless support is disabled when it fails to initialize #4934

merged 3 commits into from
Jul 17, 2023

Conversation

JanuszL
Copy link
Contributor

@JanuszL JanuszL commented Jun 30, 2023

  • older versions on nvJPEG don't support lossless JPEG,
    so when such one is used make sure it is disabled

Category:

Other (e.g. Documentation, Tests, Configuration)

Description:

  • older versions on nvJPEG don't support lossless JPEG,
    so when such one is used make sure it is disabled

Additional information:

Affected modules and functionalities:

  • lossless jpegs

Key points relevant for the review:

  • NA

Tests:

  • Existing tests apply
    • NvJpegLosslessDecoderTest*
    • test_dali_variable_batch_size.test_image_decoders*
  • New tests added
    • Python tests
    • GTests
    • 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: N/A

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8818540]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8818771]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8818771]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8824496]: BUILD FAILED

@JanuszL JanuszL marked this pull request as draft July 1, 2023 07:31
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8824757]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8824757]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8912748]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8912748]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8913650]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8915274]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8915274]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8921621]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8921621]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8927414]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8927414]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8929466]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8929466]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8933095]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8933097]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8933095]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8933097]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8942963]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8942963]: BUILD FAILED

@JanuszL JanuszL force-pushed the lossless_hardening branch 2 times, most recently from 828b068 to 9f9c87d Compare July 12, 2023 10:40
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8944124]: BUILD STARTED

@JanuszL
Make sure that lossless support is disabled when it fails to initialize
0a9e825 
- older versions on nvJPEG don't support lossless JPEG,
  so when such one is used make sure it is disabled

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

CI MESSAGE: [8945257]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8944124]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8944124]: BUILD PASSED

@JanuszL JanuszL marked this pull request as ready for review July 12, 2023 15:29
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8945257]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8945257]: BUILD PASSED

@JanuszL
Review fix
90558d3 
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
jantonguirao
jantonguirao reviewed Jul 17, 2023
View reviewed changes
dali/imgcodec/decoders/nvjpeg_lossless/nvjpeg_lossless.cc Outdated
// some nvJPEG version doesn't support NVJPEG_BACKEND_LOSSLESS_JPEG so disable it if
// it failed to initialize
DALI_WARN_ONCE("The available nvJPEG library version doesn't support Lossless format, please "
" update to the latest one.");
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
" update to the latest one.");
"update to the latest one.");

You already have a space in the previous line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

jantonguirao
jantonguirao reviewed Jul 17, 2023
View reviewed changes
dali/imgcodec/decoders/nvjpeg/nvjpeg_helper.h Outdated
@@ -25,6 +25,11 @@
#include "dali/core/cuda_error.h"
#include "dali/core/format.h"

#if WITH_DYNAMIC_NVJPEG_ENABLED
bool nvjpegIsSymbolAvailable(const char *name);
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't seem to be using this in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@JanuszL
Review fix
8a68a23 
Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8996798]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8996798]: BUILD FAILED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [8996798]: BUILD PASSED

@JanuszL JanuszL merged commit f4d8e6c into NVIDIA:main Jul 17, 2023
4 checks passed
@JanuszL JanuszL deleted the lossless_hardening branch July 17, 2023 12:50
JanuszL added a commit to JanuszL/DALI that referenced this pull request Oct 13, 2023
@JanuszL
Make sure that lossless support is disabled when it fails to initiali…
24f413d 
…ze (NVIDIA#4934)

- older versions on nvJPEG don't support lossless JPEG,
  so when such one is used make sure it is disabled

Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jantonguirao jantonguirao jantonguirao approved these changes

@stiepan stiepan stiepan approved these changes

Assignees

@jantonguirao jantonguirao

@stiepan stiepan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@JanuszL @dali-automaton @jantonguirao @stiepan