Image Decoder to have consistent behavior across backends #2843

JanuszL · 2021-04-06T15:21:04Z

Why we need this PR?

Pick one, remove the rest

Rework Image decoder behavior to be consistent across backends

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

What solution was applied:
Consistent handling of ANY_DATA image type. TIFFs support any number of channels, the rest of the formats are limited to 1-channel and 3-channel
Added tests to check that mixed and CPU backends produce the same results.
Affected modules and functionalities:
nvjpeg_decoder_decoupled_api.h
image decoder tests
Key points relevant for the review:
NA
Validation and testing:
new tests are added
Documentation (including examples):
NA

Relates to #2842

JIRA TASK: [DALI-1948]

dali-automaton · 2021-04-06T16:07:28Z

CI MESSAGE: [2240196]: BUILD STARTED

dali-automaton · 2021-04-06T17:14:23Z

CI MESSAGE: [2240196]: BUILD FAILED

dali-automaton · 2021-04-06T23:15:11Z

CI MESSAGE: [2242201]: BUILD STARTED

dali-automaton · 2021-04-07T03:33:40Z

CI MESSAGE: [2242201]: BUILD FAILED

mzient · 2021-04-07T08:03:35Z

dali/image/generic_image.cc

+  DALI_ENFORCE(image_type != DALI_ANY_DATA, "Host decoder doesn't support ANY_DATA image type for "
+                                            "this image");


You can pass IMREAD_ANYCOLOR and/or IMREAD_UNCHANGED to imdecode to keep the original number of channels.

Checked: neither of these would force OpenCV to keep original color format in JPEGs.

This PR is to make sure we are at least consistent and fail instead of providing random results or out-of-bound memory access.
Supporting multichannel image formats is a full-fledged feature, not a quick fix.

I don't know why it would result in out-of-bounds access. Just throwing away support for alpha in PNGs (which OpenCV does support) seems quite wasteful.

JanuszL · 2021-04-07T12:20:33Z

dali/test/python/test_operator_decoder.py

+                      batch_size=batch_size_alias_test, N_iterations=10,
+                      eps = 1e-03)
+
+def test_image_decoder_multi_fail():


This fails for CUDA 10 as we don't have nvJPEG2000 there, and we just use host fallback that doesn't apply enforce, but just converts the image to RGB.

- adds a check inside the decoder if the requested image format matches the underlying image - adds check only or JPEG2000 as in case of JPEG only RGB images are supported, for the host fallback DALI always convert to the expected format if requested Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

…g. RGBA) to RGB. Signed-off-by: Joaquin Anton <janton@nvidia.com>

Signed-off-by: Joaquin Anton <janton@nvidia.com>

JanuszL · 2021-04-08T16:36:37Z

dali/image/generic_image.cc

+  const auto shape = PeekShapeImpl(encoded_buffer, length);
+  if (image_type == DALI_ANY_DATA)
+    image_type = shape[2] == 1 ? DALI_GRAY : DALI_RGB;
+  const auto C = IsColor(image_type) ? 3 : 1;


Suggested change

const auto shape = PeekShapeImpl(encoded_buffer, length);

if (image_type == DALI_ANY_DATA)

image_type = shape[2] == 1 ? DALI_GRAY : DALI_RGB;

const auto C = IsColor(image_type) ? 3 : 1;

if (image_type == DALI_ANY_DATA) {

const auto shape = PeekShapeImpl(encoded_buffer, length);

image_type = shape[2] == 1 ? DALI_GRAY : DALI_RGB;

}

const auto C = IsColor(image_type) ? 3 : 1;

Peek shape is not free.

JanuszL · 2021-04-08T16:38:07Z

dali/image/generic_image.cc

+  assert(shape[0] == H);
+  assert(shape[1] == W);


Maybe we do not need that.

JanuszL · 2021-04-08T16:49:09Z

dali/operators/decoder/nvjpeg/permute_layout.cu

+  auto r = input[tid];
+  auto g = input[tid + comp_size];
+  auto b = input[tid + 2 * comp_size];
+  output[tid] = 0.299f * r + 0.587f * g + 0.114f * b;


Can't we just use rgb_to_y ?

JanuszL · 2021-04-08T16:52:10Z

dali/test/python/test_operator_decoders_image.py

+    files = glob.glob(os.path.join(test_data_root, "db/single/multichannel/tiff_multichannel") + "/*.tif*")
+    _testimpl_image_decoder_consistency_multichannel(files)
+
+def test_image_decoder_consistenty_multichannel_png_with_alpha():


It is png and jp2 with alpha.

JanuszL · 2021-04-08T17:06:24Z

dali/test/python/test_operator_decoders_image.py

+def test_image_decoder_consistenty_jpeg():
+    files = glob.glob(os.path.join(test_data_root, "db/single/jpeg/113") + "/*.jpg*")
+    _testimpl_image_decoder_consistency(files)
+
+def test_image_decoder_consistenty_jpeg2k():
+    files = glob.glob(os.path.join(test_data_root, "db/single/jpeg2k/0") + "/*.jp2*")
+    _testimpl_image_decoder_consistency(files)
+
+def test_image_decoder_consistenty_bmp():
+    files = glob.glob(os.path.join(test_data_root, "db/single/bmp/0") + "/*.bmp*")
+    _testimpl_image_decoder_consistency(files)
+
+def test_image_decoder_consistenty_png():
+    files = glob.glob(os.path.join(test_data_root, "db/single/png/0") + "/*.png*")
+    _testimpl_image_decoder_consistency(files)


Suggested change

def test_image_decoder_consistenty_jpeg():

files = glob.glob(os.path.join(test_data_root, "db/single/jpeg/113") + "/*.jpg*")

_testimpl_image_decoder_consistency(files)

def test_image_decoder_consistenty_jpeg2k():

files = glob.glob(os.path.join(test_data_root, "db/single/jpeg2k/0") + "/*.jp2*")

_testimpl_image_decoder_consistency(files)

def test_image_decoder_consistenty_bmp():

files = glob.glob(os.path.join(test_data_root, "db/single/bmp/0") + "/*.bmp*")

_testimpl_image_decoder_consistency(files)

def test_image_decoder_consistenty_png():

files = glob.glob(os.path.join(test_data_root, "db/single/png/0") + "/*.png*")

_testimpl_image_decoder_consistency(files)

def test_image_decoder_consistency():

for img_type in test_good_path:

data_path = os.path.join(test_data_root, good_path, img_type)

files = glob.glob(data_path + "/*/*.[!txt]*")

yield _testimpl_image_decoder_consistency, files[0:10]

Signed-off-by: Joaquin Anton <janton@nvidia.com>

JanuszL · 2021-04-12T16:26:29Z

dali/test/python/test_operator_decoders_image.py

+    _testimpl_image_decoder_consistency_multichannel(files)
+
+def _testimpl_image_decoder_consistency(img_format, img_out_type):
+    files = get_img_files(os.path.join(test_data_root, good_path, img_format))


Does it make sense to limit the files to 10 or so?
Or it is better to run the test for all of them?

I don't see the need of limiting the files we send to the reader, we'll test more or less files depending on the batch size and number of iterations

JanuszL

LGTM but I cannot approve as this PR is based on my own.

jantonguirao · 2021-04-15T16:24:19Z

Closed in favor of #2867

JanuszL mentioned this pull request Apr 6, 2021

Illegal memory access error during JP2 images decoding #2842

Closed

JanuszL force-pushed the proper_channel_check branch from f32d81d to 124cb79 Compare April 6, 2021 16:04

JanuszL force-pushed the proper_channel_check branch from 124cb79 to 2db85ec Compare April 6, 2021 23:13

mzient reviewed Apr 7, 2021

View reviewed changes

JanuszL changed the title ~~Add a proper check for the requested number of channels~~ [WIP] Add a proper check for the requested number of channels Apr 7, 2021

JanuszL commented Apr 7, 2021

View reviewed changes

JanuszL and others added 5 commits April 8, 2021 18:29

Mixed Image Decoder to allow converting from more than 3 channges (e.…

ee14d70

…g. RGBA) to RGB. Signed-off-by: Joaquin Anton <janton@nvidia.com>

Fix linter

77154c3

Signed-off-by: Joaquin Anton <janton@nvidia.com>

Typo

dd2ba60

Signed-off-by: Joaquin Anton <janton@nvidia.com>

Consistent behavior of ImageDecoder across backends

c5995be

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao force-pushed the proper_channel_check branch from fd693eb to c5995be Compare April 8, 2021 16:29

jantonguirao changed the title ~~[WIP] Add a proper check for the requested number of channels~~ Image Decoder to have consistent behavior across backends Apr 8, 2021

JanuszL commented Apr 8, 2021

View reviewed changes

jantonguirao assigned mzient and JanuszL Apr 9, 2021

Code review fixes

4a774cc

Signed-off-by: Joaquin Anton <janton@nvidia.com>

JanuszL commented Apr 12, 2021

View reviewed changes

jantonguirao closed this Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Image Decoder to have consistent behavior across backends #2843

Image Decoder to have consistent behavior across backends #2843

JanuszL commented Apr 6, 2021 •

edited by jantonguirao

dali-automaton commented Apr 6, 2021

dali-automaton commented Apr 6, 2021

dali-automaton commented Apr 6, 2021

dali-automaton commented Apr 7, 2021

mzient Apr 7, 2021

mzient Apr 7, 2021

mzient Apr 7, 2021 •

edited

JanuszL Apr 7, 2021

mzient Apr 7, 2021

JanuszL Apr 7, 2021

jantonguirao Apr 7, 2021

JanuszL Apr 8, 2021

JanuszL Apr 8, 2021

JanuszL Apr 8, 2021

JanuszL Apr 8, 2021 •

edited

JanuszL Apr 8, 2021 •

edited

JanuszL Apr 12, 2021

jantonguirao Apr 13, 2021

JanuszL left a comment

jantonguirao commented Apr 15, 2021

		DALI_ENFORCE(image_type != DALI_ANY_DATA, "Host decoder doesn't support ANY_DATA image type for "
		"this image");

Image Decoder to have consistent behavior across backends #2843

Image Decoder to have consistent behavior across backends #2843

Conversation

JanuszL commented Apr 6, 2021 • edited by jantonguirao

Why we need this PR?

What happened in this PR?

dali-automaton commented Apr 6, 2021

dali-automaton commented Apr 6, 2021

dali-automaton commented Apr 6, 2021

dali-automaton commented Apr 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Apr 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanuszL Apr 8, 2021 • edited

Choose a reason for hiding this comment

JanuszL Apr 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanuszL left a comment

Choose a reason for hiding this comment

jantonguirao commented Apr 15, 2021

JanuszL commented Apr 6, 2021 •

edited by jantonguirao

mzient Apr 7, 2021 •

edited

JanuszL Apr 8, 2021 •

edited

JanuszL Apr 8, 2021 •

edited