Video resize #2164

mzient · 2020-07-28T23:27:52Z

Why we need this PR?

Pick one, remove the rest

It adds new feature needed because we want video, channel-first (and 3D) Resize
Refactoring to allow different resize types and dimensionalities

What happened in this PR?

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

What solution was applied:
- Almost complete rewrite of Resize operators
Affected modules and functionalities:
- Resize operator family
Key points relevant for the review:
- ??
Validation and testing:
- Existing tests for regressions
- ResizeAttr tests for new parameterizations
- Python tests?
Documentation (including examples):
- Separate PR

JIRA TASK: DALI-1077 DALI-1537 DALI-1538

JanuszL · 2020-07-30T21:51:47Z

dali/operators/image/resize/random_resized_crop.h

 #include "dali/operators/image/crop/random_crop_attr.h"
 #include "dali/kernels/imgproc/resample/params.h"

 namespace dali {

 template <typename Backend>
 class RandomResizedCrop : public Operator<Backend>
-                        , protected ResizeBase
-                        , protected RandomCropAttr {
+                        , protected ResamplingFilterAttr


Do we need to inherit from ResamplingFilterAttr?

No, we can have it via composition. Other operators are already refactored.

JanuszL · 2020-07-30T21:55:16Z

dali/operators/image/resize/resampling_attr.cc

+      DALI_INTERP_LINEAR, true)
+  .AddOptionalArg("min_filter", "Filter used when scaling down",
+      DALI_INTERP_LINEAR, true)
+  .AddOptionalArg<DALIDataType>("dtype", "Output data type; must be same as input type of `float`. "


I don't think I understand.

should be "or float". Typo.

JanuszL · 2020-07-30T22:00:43Z

dali/operators/image/resize/resampling_attr.cc

+void ResamplingFilterAttr::PrepareFilterParams(
+      const OpSpec &spec, const ArgumentWorkspace &ws, int num_samples) {
+  if (num_samples < 0)
+    num_samples = spec.GetArgument<int>("batch_size");


Maybe we can slowly stop using "batch_size" at all? Just asking.

👍 I'll see what I can do. It's just a fallback anyway.

JanuszL · 2020-07-30T22:30:02Z

dali/operators/image/resize/resize.cc

 }

 template <>
-void Resize<CPUBackend>::RunImpl(SampleWorkspace &ws) {
+void Resize<CPUBackend>::RunImpl(HostWorkspace &ws) {
  const int thread_idx = ws.thread_idx();


JanuszL · 2020-07-30T22:30:40Z

dali/operators/image/resize/resize.cc

+    const auto &input_shape = input.shape();
+    auto &attr_out = ws.OutputRef<CPUBackend>(1);
+    const auto &attr_shape = attr_out.shape();
+    assert(attr_shape.num_samples() == input_shape.num_samples() && attr_shape.sample_dim() == 1 &&


Can you format in the same way as in L88

JanuszL · 2020-07-30T22:36:34Z

dali/test/python/test_video_reader_resize.py

@@ -150,3 +150,7 @@ def test_video_resize(batch_size=2):
    for vp in video_reader_params:
        for rp in resize_params:
            yield run_for_params, batch_size, vp, rp
+
+if __name__ == "__main__":


Bad idea. It quickly becomes obsolete. If you really must you can try to query this module for all available functions and run all that falls into the default nosetests regexp - https://nose.readthedocs.io/en/latest/usage.html#cmdoption-m

It's here because nosetests launches new processes and defeats debugging attempts (and does other ugly things that have to be disabled with "no-smoke" and "don't-catch-fire" kind of flags).

Then query this module for all available functions and run all that falls into the default nosetests regexp - https://nose.readthedocs.io/en/latest/usage.html#cmdoption-m sounds like a way to do.

working on it...

To be honest, I'm not sure about this. With how it is implemented now, we lose all of the nose functionality and it's still impractical to debug single test. In my opinion, this should be implemented either with this official API from nose or by your IDE plugin/setup (e.g. plugin for VS Code). And in both cases it does not belong with this PR, I think. I'm not even sure, if things like "how you debug unit tests" should be part of the code base. After all, we don't include other launch/debug configs, IDEs settings etc.

+1 for official API - but I'd still prefer to have a way to run the tests with fewer levels of indirections.
I think I agree that this solution is not good after all - while it catches all tests, it's not what I really want here. I'll remove it and revert the changes to test_utils.
Regarding the plugin - I think it doesn't launch GDB, but a python debugger (?).

dali/pipeline/operator/operator.h

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

- Refactoring - ResizeAttr and ResamplingFilterAttr now used by composition - Bugfixes - Add possibility to launch test without nosetests. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali/operators/image/resize/resize_attr.cc

JanuszL · 2020-07-31T11:51:35Z

dali/operators/image/resize/resize_attr.cc

+          out_size[d] = in_size[d] * final_scale;
+          scale[d] = final_scale;
+        } else if (std::abs(scale[d]) != final_scale) {
+          scale[d] = std::copysign(final_scale, scale[d]);


Can it be moved outside of the if - else?

And do we need this if-else at all?

dali/operators/image/resize/resize_attr.cc

dali/pipeline/operator/common.h

dali/operators/image/resize/resize_base.cc

mzient · 2020-08-05T20:41:51Z

!build

dali-automaton · 2020-08-05T20:45:58Z

CI MESSAGE: [1523577]: BUILD STARTED

dali-automaton · 2020-08-05T22:20:14Z

CI MESSAGE: [1523577]: BUILD FAILED

jantonguirao · 2020-08-06T06:14:38Z

dali/operators/image/resize/resize.h

+                 const TensorListShape<> &orig_shape) const {
+    int N = orig_shape.num_samples();
+    int D = NumSpatialDims();
+    assert(orig_shape.sample_dim() == D);


aren't there non-spatial dimensions?

Not tested, apparently :\

Test added.

jantonguirao · 2020-08-06T06:18:58Z

dali/operators/image/resize/resize_attr.cc

+                    output image is smaller than specified - e.g. 640x480 image with desired output
+                    size of 1920x1080 will actually produce 1920x1440 output.
+
+  This argument is mutually exclusive with ``resize_longer`` and ``resize_shorter``)", "default")


I don't understand why this argument is mutually exclusive with resize_longer and resize_shorter. IMO the resize mode should be compatible with those as well

jantonguirao · 2020-08-06T06:21:02Z

dali/operators/image/resize/resize_attr.cc

+  .AddOptionalArg("resize_shorter", R"(The length of the shorter dimension of the resized image.
+This option is mutually exclusive with ``resize_longer`` and explicit size arguments
+The op will keep the aspect ratio of the original image.
+This option is equivalent to specifying the same size for all dimensions and ``mode="not_smaller"``.


Can you elaborate on why you are assuming mode="not_smaller" here? I understood that resize_shorter is equivalent to resize_x, given that x is the shorter dimension (for example).

Actually... it's not. At least it's not implemented this way. resize_shorter=S is implemented as:

resize(size=S, # broadcast to all dims mode="not_smaller")

So it cannot be combined with any other mode, because it implies the mode.
I see no real-life use case when one would want to resize the shorter/longer edge and keep the others or combine it with other modes.

I assumed that the whole reason for having resize_shorter/resize_longer is to make the image large enough to be able to make square crops with that size. If we extend this to rectangular crops, we end up with rectangular size
and mode "not_smaller".

jantonguirao · 2020-08-06T06:24:29Z

dali/operators/image/resize/resize_attr.cc

+must be specified together with ``roi_start``. The coordinates follow the tensor shape order
+(same as ``size``). The coordinates can be either absolute (in pixels, the default) or
+relative (0..1), depending on the value of ``relative_roi`` argument. If a RoI origin is greater
+than RoI end, the region is flipped.))", nullptr, true)


Suggested change

than RoI end, the region is flipped.))", nullptr, true)

than RoI end in any dimension, the resulting image will be flipped (mirrored) in that dimension.))", nullptr, true)

jantonguirao · 2020-08-06T06:29:07Z

dali/operators/image/resize/resize_attr.cc

+                                   int sample_idx) const {
+  in_lo.resize(spatial_ndim_);
+  in_hi.resize(spatial_ndim_);
+  static constexpr float min_size = 1e-3f;  // minimum size, in pixels


I don't understand why this value. This is not a number of pixels

It's not a number in pixels, but it is a distance in pixels. You're free to have a RoI that is not placed at pixel boundary - indeed, with bicubic or Lanczos resampling, it's going to make sense and even look good.

jantonguirao · 2020-08-06T06:52:33Z

dali/operators/image/resize/resize_attr.cc

+        }
+      }
+      if (has_max_size_) {
+        for (int d = 0; d < spatial_ndim_; d++) {


This could be extracted to ApplyMaxSize or something like that (I see that this code is repeated)

It's the threshold value. It's repeated only twice and not very long. The third application of max size is different...

jantonguirao · 2020-08-06T07:04:38Z

dali/operators/image/resize/resize_attr.cc

+
+  for (int d = 0; d < spatial_ndim_; d++) {
+    DALI_ENFORCE(in_lo[d] != in_hi[d] || requested_size[d] == 0,
+                "Cannot produce non-empty output from empty input");


print d, in_lo/in_hi, requested_size?

if the size is 0, lo and hi will always be 0.

jantonguirao · 2020-08-06T07:08:15Z

dali/operators/image/resize/resize_attr.cc

+    bool flip = out_sz < 0;
+    params.dst_size[d] = std::max(1, round_int(std::fabs(out_sz)));
+    if (flip)
+      std::swap(params.src_lo[d], params.src_hi[d]);


If lo was > hi, and you swap them here. How does the flipping work?

There are two ways to flip:

negative output size

lo > hi
The final result uses the latter, but AdjustOutputSize uses the former (hence the first swap).

jantonguirao · 2020-08-06T07:19:31Z

dali/operators/image/resize/resize_op_impl.h

+
+template <typename Backend>
+struct ResizeBase<Backend>::Impl {
+  using input_type =  typename Workspace::template input_t<Backend>::element_type;


Suggested change

using input_type = typename Workspace::template input_t<Backend>::element_type;

using InputType = typename Workspace::template input_t<Backend>::element_type;

Would read better IMO

dali/pipeline/operator/common.h

…d examples. Fix imports in test_operator_resize_seq test. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Add test for save_attrs. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient · 2020-08-06T14:21:51Z

!build

dali-automaton · 2020-08-06T14:25:51Z

CI MESSAGE: [1525664]: BUILD STARTED

jantonguirao · 2020-08-06T15:12:16Z

dali/operators/image/resize/resize_op_impl_cpu.h

+    kernels::KernelContext ctx;
+
+    for (int i = 0; i < GetNumFrames(); i++) {
+      kernels::InTensorCPU<In, frame_ndim> dummy_input;


I guess I'll redo it when dealing with 3D resize - but I want to avoid merging the changes now.

jantonguirao · 2020-08-06T15:15:58Z

dali/pipeline/operator/common_test.cc

+  vector<float> shape;
+
+  int out_d = GetShapeLikeArgument<float>(shape, spec, "size", ws, -1, -1);
+  EXPECT_EQ(out_d, D) << "Diemsnionality should match the size of the tensors in the list.";


Suggested change

EXPECT_EQ(out_d, D) << "Diemsnionality should match the size of the tensors in the list.";

EXPECT_EQ(out_d, D) << "Dimensionality should match the size of the tensors in the list.";

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

dali-automaton · 2020-08-06T16:01:52Z

CI MESSAGE: [1525664]: BUILD PASSED

Add a test for dtype. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient · 2020-08-06T17:47:54Z

!build

dali-automaton · 2020-08-06T17:51:03Z

CI MESSAGE: [1526193]: BUILD STARTED

dali-automaton · 2020-08-06T19:18:51Z

CI MESSAGE: [1526193]: BUILD FAILED

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient · 2020-08-06T19:42:45Z

!build

dali-automaton · 2020-08-06T19:45:32Z

CI MESSAGE: [1526525]: BUILD STARTED

dali-automaton · 2020-08-06T22:13:14Z

CI MESSAGE: [1526525]: BUILD FAILED

dali-automaton · 2020-08-06T23:43:22Z

CI MESSAGE: [1526525]: BUILD PASSED

dali-automaton · 2020-08-07T07:01:28Z

CI MESSAGE: [1527950]: BUILD STARTED

dali-automaton · 2020-08-07T08:39:23Z

CI MESSAGE: [1527950]: BUILD PASSED

mzient requested a review from a team July 28, 2020 23:27

JanuszL reviewed Jul 30, 2020

View reviewed changes

dali/pipeline/operator/operator.h Outdated Show resolved Hide resolved

mzient added 12 commits July 31, 2020 09:01

WIP

30695d3

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Revert resize_crop_mirror.h

191e0e8

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Further reorganization.

28435a5

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

[WIP] Nothing works, total teardown!

9dd9767

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

[WIP]

46d8187

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

[WIP]

e6d0001

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

[WIP]

24c68ef

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

[WIP] Commit before split/rebase.

0415f86

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Working?

0f37ba3

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Added more tests for ResizeAttr. Minor Fixes. VideoResize works!

aeee24e

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

VideoReaderResize working

f14ce56

- Refactoring - ResizeAttr and ResamplingFilterAttr now used by composition - Bugfixes - Add possibility to launch test without nosetests. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

Add support for channel-not-last layouts.

19bdbcc

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the VideoResize branch from 0956626 to 19bdbcc Compare July 31, 2020 08:22

JanuszL reviewed Jul 31, 2020

View reviewed changes

dali/operators/image/resize/resize_attr.cc Show resolved Hide resolved

JanuszL reviewed Jul 31, 2020

View reviewed changes

dali/operators/image/resize/resize_attr.cc Show resolved Hide resolved

JanuszL reviewed Jul 31, 2020

View reviewed changes

dali/operators/image/resize/resize_attr.cc Show resolved Hide resolved

JanuszL reviewed Jul 31, 2020

View reviewed changes

dali/operators/image/resize/resize_attr.cc Show resolved Hide resolved

JanuszL reviewed Jul 31, 2020

View reviewed changes

dali/operators/image/resize/resize_attr.cc Show resolved Hide resolved

JanuszL reviewed Jul 31, 2020

View reviewed changes

dali/pipeline/operator/common.h Show resolved Hide resolved

JanuszL reviewed Jul 31, 2020

View reviewed changes

dali/pipeline/operator/common.h Show resolved Hide resolved

JanuszL reviewed Jul 31, 2020

View reviewed changes

dali/operators/image/resize/resize_base.cc Show resolved Hide resolved

JanuszL approved these changes Aug 5, 2020

View reviewed changes

jantonguirao reviewed Aug 6, 2020

View reviewed changes

Deprecate image_type argument in Resize and remove it from tests an…

2671cd7

…d examples. Fix imports in test_operator_resize_seq test. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the VideoResize branch from 39ffba0 to b400627 Compare August 6, 2020 13:03

Fix save_attrs.

a28a2cc

Add test for save_attrs. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient force-pushed the VideoResize branch from b400627 to a28a2cc Compare August 6, 2020 13:04

Review fixes.

0fd3744

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

jantonguirao reviewed Aug 6, 2020

View reviewed changes

Typo.

e707f5f

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

awolant approved these changes Aug 6, 2020

View reviewed changes

Fix a bug in handling types.

dc6a5aa

Add a test for dtype. Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

jantonguirao approved these changes Aug 6, 2020

View reviewed changes

Skip border pixel in comparison with PIL.

b77a4eb

Signed-off-by: Michał Zientkiewicz <mzient@gmail.com>

mzient merged commit d931bca into NVIDIA:master Aug 7, 2020

	than RoI end, the region is flipped.))", nullptr, true)
	than RoI end in any dimension, the resulting image will be flipped (mirrored) in that dimension.))", nullptr, true)

	using input_type = typename Workspace::template input_t<Backend>::element_type;
	using InputType = typename Workspace::template input_t<Backend>::element_type;

	EXPECT_EQ(out_d, D) << "Diemsnionality should match the size of the tensors in the list.";
	EXPECT_EQ(out_d, D) << "Dimensionality should match the size of the tensors in the list.";

Video resize #2164

Video resize #2164

Conversation

mzient commented Jul 28, 2020 • edited Loading

Why we need this PR?

What happened in this PR?

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 Jul 30, 2020 • edited Loading

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

mzient commented Aug 5, 2020

dali-automaton commented Aug 5, 2020

dali-automaton commented Aug 5, 2020

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

mzient Aug 6, 2020 • edited Loading

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

mzient Aug 6, 2020 • edited Loading

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

mzient commented Aug 6, 2020

dali-automaton commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Aug 6, 2020

mzient commented Aug 6, 2020

dali-automaton commented Aug 6, 2020

dali-automaton commented Aug 6, 2020

mzient commented Aug 6, 2020

dali-automaton commented Aug 6, 2020

dali-automaton commented Aug 6, 2020

dali-automaton commented Aug 6, 2020

dali-automaton commented Aug 7, 2020

dali-automaton commented Aug 7, 2020

mzient commented Jul 28, 2020 •

edited

Loading

JanuszL Jul 30, 2020 •

edited

Loading

mzient Aug 6, 2020 •

edited

Loading

mzient Aug 6, 2020 •

edited

Loading