Fix RandomBBoxCrop errors while using crop_shape argument #2605

wonrzrzeczny · 2021-01-12T13:01:35Z

Why we need this PR?

Pick one, remove the rest

It fixes anchor/shape data type returned by calling RandomBBoxCrop with supplied crop_shape/input_shape argument and prevents segfaults in case crop_shape > input_shape.

What happened in this PR?

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

What solution was applied:
Anchor and shape outputs of RandomBBoxCrop call with provided crop_shape/input_shape are cast to int before being returned, which prevents pixel dimensions from being interpreted as relative by other operators (eg. Slice). Additionally, anchor selection is handled differently when crop_shape > input_shape, which prevents segfaults previously caused by using std::uniform_int_distribution with incorrect constructor params.
Affected modules and functionalities:
Changes to dali/operators/image/crop/bbox_crop.cc
Key points relevant for the review:
NA
Validation and testing:
NA
Documentation (including examples):
NA

JIRA TASK: [Use DALI-XXXX or NA]

Signed-off-by: Kacper Kluk <kk406184@students.mimuw.edu.pl>

dali/operators/image/crop/bbox_crop.cc

Signed-off-by: Kacper Kluk <kk406184@students.mimuw.edu.pl>

JanuszL · 2021-01-13T09:06:08Z

!build

dali-automaton · 2021-01-13T09:15:04Z

CI MESSAGE: [1976940]: BUILD STARTED

dali-automaton · 2021-01-13T11:16:39Z

CI MESSAGE: [1976940]: BUILD PASSED

JanuszL · 2021-01-13T11:16:51Z

dali/operators/image/crop/bbox_crop.cc

-    auto &anchor_out = ws.template OutputRef<CPUBackend>(0);
-    anchor_out.Resize(uniform_list_shape(num_samples, {ndim}));
-    auto anchor_out_view = view<float>(anchor_out);
+    DALIDataType output_dtype = has_crop_shape_ ? DALI_INT32 : DALI_FLOAT;


I wonder if this is a good idea to change the output type based on presence of one of the arguments.
Maybe I always should be DALI_FLOAT? Or just DALI_INT32 and use cast operator after that operator?

I think that if user provided crop_shape argument (which represents the exact size in pixels of desired crop region) then output anchors and shapes should also be returned as integers. Returning them as relative float values could potentially result in output shape being off by 1 pixel due to fp arithmetic errors and this behaviour could break the pipeline in some use cases.

How about returning always FLOAT in that case (but with switch that is by default on that changes it to int - so we won't break the existing code while introducing new functionality).

This requirement for different output type depending on whether crop_shape argument was provided or not is imposed because of the way Slice operator interprets its input arguments (int -> pixel coords, float -> normalized coords). I believe that current fix won't break any existing code - any previous usage of RandomBBoxCrop operator with crop_shape provided required additional cast to int to be declared explicitly in pipeline - current fix will provide this cast unnecessary, although the results will be the same.

The only other solution I see is to always return int values (in pixel coordinates), even when using scaling instead of crop_shape (possibly with switch to return the output in normalized coords instead). However, this will probably require much more existing code to be rewritten.

There's another problem. We cannot return pixel coordinates with scaling provided as we lack image shape data.

I think that returning different types should be OK, given that Slice operator interprets those differently.

We've discussed internally with the team and came to the conclusion that changing the output type of the operator silently, based on the presence of another argument, is not a good user experience, since it could break some existing code. So, for the sake of backward compatibility we should keep DALI_FLOAT by default.

I can suggest some solutions:

Use normalized_anchor=False, normalized_shape=False in Slice, so that you don't need to cast to int32

Cast to int32 (it adds extra overhead, not recommended)

Add a output_dtype argument to this operator, and only use int32 when it was explicitly requested.

Personally, I've found pixel coordinates returned as a DALI_FLOAT confusing, because of the way Slice works by default, although I understand your point. Currently I've reverted my changes (leaving only the sigsegv fix). Maybe additional clarification should be added to RandomBBoxCrop docs?

jantonguirao · 2021-01-14T08:29:51Z

dali/test/python/test_operator_random_bbox_crop.py

    for d in range(ndim):
        assert(anchor[d] >= 0.0 and anchor[d] <= input_shape[d])
        assert shape[d] == expected_crop_shape[d], "{} != {}".format(shape, expected_crop_shape)
-        assert(anchor[d] + shape[d] > 0.0 and anchor[d] + shape[d] <= input_shape[d])
+        assert(anchor[d] + shape[d] > 0.0)


why removing the second check?

Bottom right corner can be outside original image boundaries if crop_shape > input_shape.

jantonguirao · 2021-01-14T08:31:20Z

dali/operators/image/crop/bbox_crop.cc

-    anchor_out.Resize(uniform_list_shape(num_samples, {ndim}));
-    auto anchor_out_view = view<float>(anchor_out);
+    DALIDataType output_dtype = has_crop_shape_ ? DALI_INT32 : DALI_FLOAT;
+    TYPE_SWITCH(output_dtype, type2id, type, (int, float), (


Suggested change

TYPE_SWITCH(output_dtype, type2id, type, (int, float), (

TYPE_SWITCH(output_dtype, type2id, T, (int, float), (

would be more in line with other parts of the codebase.

jantonguirao · 2021-01-14T08:32:53Z

dali/operators/image/crop/bbox_crop.cc

-    auto &anchor_out = ws.template OutputRef<CPUBackend>(0);
-    anchor_out.Resize(uniform_list_shape(num_samples, {ndim}));
-    auto anchor_out_view = view<float>(anchor_out);
+    DALIDataType output_dtype = has_crop_shape_ ? DALI_INT32 : DALI_FLOAT;


I think that returning different types should be OK, given that Slice operator interprets those differently.

Signed-off-by: Kacper Kluk <kk406184@students.mimuw.edu.pl>

JanuszL · 2021-01-19T09:28:05Z

!build

dali-automaton · 2021-01-19T09:33:09Z

CI MESSAGE: [1993040]: BUILD STARTED

dali-automaton · 2021-01-19T11:29:57Z

CI MESSAGE: [1993040]: BUILD PASSED

Signed-off-by: Kacper Kluk <kk406184@students.mimuw.edu.pl>

JanuszL · 2021-01-20T12:12:59Z

!build

dali-automaton · 2021-01-20T13:05:13Z

CI MESSAGE: [1997445]: BUILD STARTED

dali-automaton · 2021-01-20T15:10:55Z

CI MESSAGE: [1997445]: BUILD PASSED

wonrzrzeczny added 2 commits January 11, 2021 20:13

Fix crop_shape usage bugs in bbox_crop.cc

98742f8

Signed-off-by: Kacper Kluk <kk406184@students.mimuw.edu.pl>

Update copyright notice year in bbox_crop.cc

ea11f0f

Signed-off-by: Kacper Kluk <kk406184@students.mimuw.edu.pl>

JanuszL reviewed Jan 12, 2021

View reviewed changes

dali/operators/image/crop/bbox_crop.cc Outdated Show resolved Hide resolved

Add tests verifying fixed cases

5a7c597

Signed-off-by: Kacper Kluk <kk406184@students.mimuw.edu.pl>

jantonguirao assigned jantonguirao and JanuszL Jan 13, 2021

JanuszL reviewed Jan 13, 2021

View reviewed changes

jantonguirao reviewed Jan 14, 2021

View reviewed changes

wonrzrzeczny added 2 commits January 14, 2021 10:21

Change TYPE_SWITCH type name in bbox_crop.cc

6e7d162

Signed-off-by: Kacper Kluk <kk406184@students.mimuw.edu.pl>

Revert output dtype selection based on arguments

dab0ef3

Signed-off-by: Kacper Kluk <kk406184@students.mimuw.edu.pl>

JanuszL approved these changes Jan 19, 2021

View reviewed changes

Add note to RandomBBoxCrop documentation

bf64f56

Signed-off-by: Kacper Kluk <kk406184@students.mimuw.edu.pl>

JanuszL approved these changes Jan 20, 2021

View reviewed changes

jantonguirao approved these changes Jan 20, 2021

View reviewed changes

awolant merged commit 548cc7e into NVIDIA:master Jan 21, 2021

wonrzrzeczny deleted the bboxcrop_fixes branch January 21, 2021 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RandomBBoxCrop errors while using crop_shape argument #2605

Fix RandomBBoxCrop errors while using crop_shape argument #2605

wonrzrzeczny commented Jan 12, 2021

JanuszL commented Jan 13, 2021

dali-automaton commented Jan 13, 2021

dali-automaton commented Jan 13, 2021

JanuszL Jan 13, 2021

wonrzrzeczny Jan 13, 2021

JanuszL Jan 13, 2021

wonrzrzeczny Jan 13, 2021

wonrzrzeczny Jan 13, 2021

wonrzrzeczny Jan 13, 2021

jantonguirao Jan 14, 2021

jantonguirao Jan 18, 2021

wonrzrzeczny Jan 19, 2021

jantonguirao Jan 14, 2021

wonrzrzeczny Jan 14, 2021

jantonguirao Jan 14, 2021

wonrzrzeczny Jan 14, 2021

jantonguirao Jan 14, 2021

JanuszL commented Jan 19, 2021

dali-automaton commented Jan 19, 2021

dali-automaton commented Jan 19, 2021

JanuszL commented Jan 20, 2021

dali-automaton commented Jan 20, 2021

dali-automaton commented Jan 20, 2021

	TYPE_SWITCH(output_dtype, type2id, type, (int, float), (
	TYPE_SWITCH(output_dtype, type2id, T, (int, float), (

Fix RandomBBoxCrop errors while using crop_shape argument #2605

Fix RandomBBoxCrop errors while using crop_shape argument #2605

Conversation

wonrzrzeczny commented Jan 12, 2021

Why we need this PR?

What happened in this PR?

JanuszL commented Jan 13, 2021

dali-automaton commented Jan 13, 2021

dali-automaton commented Jan 13, 2021

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

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 commented Jan 19, 2021

dali-automaton commented Jan 19, 2021

dali-automaton commented Jan 19, 2021

JanuszL commented Jan 20, 2021

dali-automaton commented Jan 20, 2021

dali-automaton commented Jan 20, 2021