Add debayer operator #4495

stiepan · 2022-12-05T17:50:03Z

Add debayer operator
Signed-off-by: Kamil Tokarski ktokarski@nvidia.com

Category:

New feature (non-breaking change which adds functionality)

Description:

Adds a new operator based on the npp_debayer_kernel
Operator accepts gpu input (bayered images/videos), the algorithm and one of four possible bayer patterns.
The only supported algorithm as for now is "bilinear_npp". It is not simply bilinear, because npp does extra "chroma correlation" when inferring green values for blue and red based pixels. I imagine, in the future we may support both: bilinear and bilinear_npp.
The bayer pattern specification can be done per-sample and per-frame. For this reason, following @mzient suggestion, the pattern is specified through "blue_position" parameter as 2d coordinate of the blue color within bayer 2x2 tile. It is unambigious although a bit weird. However, support for tensor strings is almost non-existent, while using enums is also a bit akward as it requires reinterperet operator along the way.

Additional information:

Affected modules and functionalities:

New operator, new python tests. Existing funcionalities are unaffected.

Key points relevant for the review:

Around 70% of this PR are tests and documentation. It is a small PR. :)

Tests:

Checklist

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: DEBAYER.02, 03, 04, 07, 08, 09, 11, 12, 13.

JIRA TASK: DALI-3075

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> Add docs Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> NPP baseline tests Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> Fix lint issues Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> Better algorithm description in the docs Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> Revert formatting in npp helper filer Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> Add debayer calls to stub generator Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> Include uin16_t tests Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> Test single-channel 3dim inputs, adjust error messages, prepare seq baseline Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> Read video helper Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> Use pair of ints instead of enum, add cpp tests Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com> Simplfy the tests sligthly Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan · 2022-12-05T19:55:53Z

!build

dali-automaton · 2022-12-05T20:00:52Z

CI MESSAGE: [6685417]: BUILD STARTED

dali-automaton · 2022-12-06T00:15:36Z

CI MESSAGE: [6685417]: BUILD FAILED

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan · 2022-12-06T11:00:43Z

!build

dali-automaton · 2022-12-06T11:05:18Z

CI MESSAGE: [6692025]: BUILD STARTED

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan · 2022-12-06T15:11:37Z

!build

dali-automaton · 2022-12-06T15:27:57Z

CI MESSAGE: [6694079]: BUILD STARTED

klecki

Still reading tests, comments for implementation

klecki · 2022-12-06T14:43:46Z

dali/operators/image/color/debayer.h

+constexpr static const char *algArgName = "algorithm";
+constexpr static const char *bluePosArgName = "blue_position";


Suggested change

constexpr static const char *algArgName = "algorithm";

constexpr static const char *bluePosArgName = "blue_position";

constexpr static const char *kAlgArgName = "algorithm";

constexpr static const char *kBluePosArgName = "blue_position";

I think this k is required by the coding guide. I am also never sure if it's better to use those constants or maybe just stick with strings. I was thinking about alternative once, but didn't find anything that would allow to have both validation and not introduce need for additional definitions or obfuscation.

klecki · 2022-12-06T14:47:41Z

dali/operators/image/color/debayer.h

+                      sample_idx, " has shape ", input_shapes[sample_idx],
+                      ", which, assuming HWC layout, implies ", num_channels, " channels. ",
+                      "If you are trying to process video or sequences, please make sure to set "
+                      "input's layout to `FHW`. You can use `fn.reshape` to set proper layout."));


Can we do it without assumptions, just check what is the layout and error out accordingly?

Btw, can we access the original layout in Sequence Operator?

If you don't provide any layout but pass 3d tensor in hope of processing sequence of bayered images, you likely will fail here. This is for such a situation. There layout won't tell us much.

As to the SequenceOperator, kind of. You have original layouts in ShouldExpand(ws) overrides, the idea was that if you'd like to report layout to the user, you can do that there and then forget about it. There is also IsExpanding() method to tell if that's the sequence case or not.

Oh, this kinda boils down to the fact, that we don't know if we were running with empty layout if executor set it to a default.

klecki · 2022-12-06T14:58:29Z

dali/operators/image/color/debayer.h

+          blue_position.size(), " values."));
+  int y = blue_position[0];
+  int x = blue_position[1];
+  int bayer_enum_val = 2 * y + x;


Hmm, (-2, 4) would work fine.

Yeah, the current check is a bit liberal, just to prevent "overflowing" the enum. Do you think we need the strict version to check it elementwise?

I think so, it's easier to go in the other direction later.

klecki · 2022-12-06T15:40:05Z

dali/operators/image/color/debayer.h

+      pattern_.resize(batch_size, static_pattern_);
+    } else {
+      const auto &tv = ws.ArgumentInput(debayer::bluePosArgName);
+      auto blue_positions_view = view<const int, 1>(tv);


Just checking, I think we have something that asserts dimensionality here, so it would error out with a bit of a cryptic message, if the user provides us with for example 2D-input. And we don't strictly require 1D data, just two elements exactly?

Yep, we require two elements. And the message will be cryptic. So your suggestion is to go with dynamic extent and simply .num_elements() == 2?

This is one solution if you want to be more flexible.

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

dali-automaton · 2022-12-06T16:35:43Z

CI MESSAGE: [6692025]: BUILD FAILED

…equiresemnts for the tensor input Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

dali-automaton · 2022-12-06T18:27:53Z

CI MESSAGE: [6694079]: BUILD PASSED

stiepan · 2022-12-07T13:29:05Z

!build

dali-automaton · 2022-12-07T13:35:15Z

CI MESSAGE: [6704750]: BUILD STARTED

dali-automaton · 2022-12-07T15:58:45Z

CI MESSAGE: [6704750]: BUILD PASSED

banasraf · 2022-12-07T16:46:04Z

dali/operators/image/color/debayer_gpu.cc

+      return view<const InOutT, 2>(input);
+    }
+    assert(([&]() {
+      for (int sample_idx = 0; sample_idx < in_shape.num_samples(); sample_idx++) {


assert(std::all_of(in_shape.begin(), in_shape.end(), [](auto &shape) { return shape[2] == 1; }));

nitpick

Unfortunately, AFAIK, there is no begin/end in neither TL nor TLS class.

stiepan added 3 commits December 5, 2022 18:48

Test video inputs

d6baab8

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Include an example in the docs

5eb315a

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Add variable batch size/cpu only tests

f17d16c

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

jantonguirao assigned banasraf and jantonguirao Dec 6, 2022

klecki assigned klecki and unassigned jantonguirao Dec 6, 2022

Setup scipy for variable batch size tests

6149743

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

klecki reviewed Dec 6, 2022

View reviewed changes

Follow kOnstant convention

1507c78

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

stiepan added 2 commits December 6, 2022 17:51

More strict validation of blue position, less strict dimensionality r…

9095a00

…equiresemnts for the tensor input Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

Fix typos in test comments, add npp docs link

7dc82ed

Signed-off-by: Kamil Tokarski <ktokarski@nvidia.com>

klecki approved these changes Dec 7, 2022

View reviewed changes

banasraf approved these changes Dec 7, 2022

View reviewed changes

stiepan merged commit 80ee597 into NVIDIA:main Dec 7, 2022

JanuszL mentioned this pull request Jan 11, 2023

DALI 2022 roadmap #3774

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add debayer operator #4495

Add debayer operator #4495

stiepan commented Dec 5, 2022 •

edited

Loading

stiepan commented Dec 5, 2022

dali-automaton commented Dec 5, 2022

dali-automaton commented Dec 6, 2022

stiepan commented Dec 6, 2022

dali-automaton commented Dec 6, 2022

stiepan commented Dec 6, 2022

dali-automaton commented Dec 6, 2022

klecki left a comment

klecki Dec 6, 2022

stiepan Dec 6, 2022

klecki Dec 6, 2022

stiepan Dec 6, 2022 •

edited

Loading

stiepan Dec 6, 2022

klecki Dec 6, 2022

stiepan Dec 6, 2022

klecki Dec 6, 2022

stiepan Dec 6, 2022

klecki Dec 6, 2022

stiepan Dec 6, 2022

klecki Dec 6, 2022

stiepan Dec 6, 2022

klecki Dec 6, 2022

stiepan Dec 6, 2022

dali-automaton commented Dec 6, 2022

dali-automaton commented Dec 6, 2022

stiepan commented Dec 7, 2022

dali-automaton commented Dec 7, 2022

dali-automaton commented Dec 7, 2022

banasraf Dec 7, 2022

stiepan Dec 7, 2022

		constexpr static const char *algArgName = "algorithm";
		constexpr static const char *bluePosArgName = "blue_position";

Add debayer operator #4495

Add debayer operator #4495

Conversation

stiepan commented Dec 5, 2022 • edited Loading

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

stiepan commented Dec 5, 2022

dali-automaton commented Dec 5, 2022

dali-automaton commented Dec 6, 2022

stiepan commented Dec 6, 2022

dali-automaton commented Dec 6, 2022

stiepan commented Dec 6, 2022

dali-automaton commented Dec 6, 2022

klecki left a 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

stiepan Dec 6, 2022 • 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

dali-automaton commented Dec 6, 2022

dali-automaton commented Dec 6, 2022

stiepan commented Dec 7, 2022

dali-automaton commented Dec 7, 2022

dali-automaton commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stiepan commented Dec 5, 2022 •

edited

Loading

stiepan Dec 6, 2022 •

edited

Loading