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

Imgcodec's decode operator #4223

Merged
merged 14 commits into from
Nov 15, 2022
Merged

Imgcodec's decode operator #4223

merged 14 commits into from
Nov 15, 2022

Conversation

szkarpinski
Copy link
Collaborator

@szkarpinski szkarpinski commented Sep 2, 2022

Category:

New feature (non-breaking change which adds functionality)

Description:

In this PR I add a set of experimental decoding operators which are analogous to the existing ones, but use the new imgcodec decoder architecture, on which we worked during our internship. The added operators are experimental.decoders.image, experimental.decoders.image_slice, experimental.decoders.image_crop, experimental.decoders.image_random_crop and peek_shape.

Changes in operator functionality

The new operators attempt to mimic the old ones, but also adds a new feature: dtype argument, which allows the user to specify the desired output type.

Missing features in the decoders

The individual decoders don't support all required features yet, so some tests in test/python/decoder/test_imgcodec.py had to be disabled. Each disabled test is commented with an appropriate TODO, explaining what's missing.

The decoder-specific parameters are passed to the ImageDecoder, but a significant part of them is not yet used by the decoders. This should be fixed in a follow-up once the decoders are complete.

Code walkthrough

decoder.h

DecoderBase class

It's the abstract base of all decoders, being responsible for:

  • Handling decoder-specific arguments (see GetDecoderSpecificArguments)
  • Inferring the output shapes (see SetupShapes)
    It also has two virtual functions for implementing ROI handling in the subclasses (more details below):
  • SetupRoiGenerator, allowing some initial setup of the ROI-handling code based on operator arguments.
  • GetRoi, which is used to ask for ROI for a particular sample

WithCropAttr/WithSliceAttr/WithRandomCropAttr mixins

All three of those are used to turn an image operator class (either cpu or mixed) into an image_crop, image_slice or image_random_crop operator, respectively. They provide appropriate overloads for SetupRoiGenerator and GetRoi.

They use existing SliceAttr, CropAttr and RandomCropAttr classes, and convert the CropWindowGenerators output by them into ROI using RoiFromCropWindowGenerator utility function.

ImgcodecHostDecoder and ImgcodecMixedDecoder

They simply do the actual decoding on CPU or GPU. There's not much in them, because most of operator-related work is done by DecoderBase, and most of decoder-related work is done by lower-level ImageDecoder class (#4224). Anyway, the implementation of those is in decoder.cc.

decoder.cc

Most of this of this file are op schemas, with the common part extracted to ImgcodecDecoderAttr schema. Those are copy-pasted from the old decoder, with some minor changes: the dtype argument is added and an information is added that EXIF metadata is no longer disregarded. The docs would probably need some polishing once we're done adding the features.

peek_image_shape.cc/h

The peek_shape operator is implemented here, with most of the code being stolen from the old operator. The actual shape-peeking is moved to operator_utils.h, because it's used both by peek_shape and the decoder.

Affected modules and functionalities:

Apart from the actual decoder, this PR contains minor changes to other files, which were required to make the operator working properly. These changes are:

  • A fix to nvJPEG synchronization issue was temporarily borrowed from Fix nvJPEG decoder synchronization #4292
  • A fix for nvJPEG2K ANY_DATA handling was borrowed from Fix support for ANY_DATA in nvJPEG2K #4299
  • When encountering an EXIF header it couldn't parse, the JPEG parser DALI_FAIL'ed. As we need the EXIF header for applying orientation only, such behaviour was way too aggressive. I made the JPEG parser ignore the unreadable EXIF instead of failing

Key points relevant for the review:

Tests:

The tests are copy-pasted from decoders/test_image.py, adjusted to use experimental.decoders.image instead. The tests for missing features are commented-out.

  • Existing tests apply
  • 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

@szkarpinski szkarpinski force-pushed the imgcodec-operator branch 5 times, most recently from 36b3923 to 9cd6c47 Compare September 26, 2022 07:04
@szkarpinski szkarpinski force-pushed the imgcodec-operator branch 4 times, most recently from 8b6d59e to 8cc5230 Compare September 28, 2022 14:43
@szkarpinski szkarpinski changed the title [WiP] Base of Imgcodec's decode operator Imgcodec's decode operator Sep 28, 2022
@szkarpinski szkarpinski marked this pull request as ready for review September 28, 2022 15:58
@jantonguirao jantonguirao assigned mzient and unassigned szalpal Sep 29, 2022
szkarpinski and others added 10 commits November 14, 2022 12:46
author Szymon Karpiński <skarpinski@nvidia.com> 1662129348 +0200
committer Michal Zientkiewicz <michalz@nvidia.com> 1665501980 +0200

Add imgcodec decoder

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Tests cleanup

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Undo C++17 fix

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Change old EXIF note

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Add missing decoder-specific arguments

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Keep persistent ImageDecoder

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Use BatchVector

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Use CalculateOutputShape

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Fix support for ANY_DATA in nvJPEG2K

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Review fixes

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Remove unused tests

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Revert "Fix support for ANY_DATA in nvJPEG2K"

This reverts commit 592bceb.

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Add tests for peek_shape

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Use fn.experimental.peek_image_shape

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Use persistent thread pool

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Provide source info to ImageSource

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Add device_id_ member

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Deprecate memory_stats argument

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Remove mentions of memory_stats from the docs

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Add adjust_orientation to decoder

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Extend args of peek_image_shape

Signed-off-by: Szymon Karpiński <skarpinski@nvidia.com>

Bulk copy + threading.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Issue immediate copy even without fallback.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Start NVJPEG multiple-buffering.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

NVPEG multiple buffering.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
* Python linter.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@@ -21,6 +21,7 @@
#include <unordered_map>
#include "dali/imgcodec/decoders/memory_pool.h"
#include "dali/core/cuda_error.h"
#include "dali/core/cuda_stream_pool.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be removed I guess.

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6485048]: BUILD FAILED

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6499506]: BUILD STARTED

ForEachThread(*tp_, [&](int tid) noexcept {
CUDA_CALL(cudaSetDevice(device_id));
per_thread_resources_[tid] = {nvjpeg2k_handle_, device_memory_padding, device_id_};
AccessOrder s = per_thread_resources_[tid].cuda_stream;
Copy link
Contributor

Choose a reason for hiding this comment

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

what's s and h for?

@@ -86,7 +86,7 @@ class DLL_PUBLIC NvJpeg2000DecoderInstance : public BatchParallelDecoderImpl {
}
};

static constexpr int kNumParallelTiles = 1; // TODO(michalz): Use a different memory resource
static constexpr int kNumParallelTiles = 2; // TODO(michalz): Use a different memory resource
Copy link
Contributor

Choose a reason for hiding this comment

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

2 is enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think more is too much - each tile has its own state and associated memory consumption.

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6499574]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6499574]: BUILD FAILED

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>
@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6501260]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [6501260]: BUILD PASSED

@mzient mzient merged commit ca4167b into NVIDIA:main Nov 15, 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.

9 participants