Per frame brightness contrast #3937

stiepan · 2022-05-26T22:23:49Z

Category:

New feature (non-breaking change which adds functionality)

Description:

Adds per-frame parameters support to color-manipulating operators.
Makes contrast_center parameter per-sample (and per-frame too) for the sake of completeness.
Adds unfloded_views_range utility to simplify handling of frame-like inputs in an operator when SequenceOperator may be overkill or cannot be used (either because unfolding of arbitrary layout is needed or broadcasting all arguments to match an unfolded sequence-like input may be unnecessary).

Additional information:

Affected modules and functionalities:

color_twist (hue, hsv, saturation, color_twist)
brightness_contrast (brightness_contrast, brightness, contrast)

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: CTSTOPS.05, BRICON.06

JIRA TASK: DALI-2804

stiepan · 2022-05-26T22:25:34Z

!build

dali-automaton · 2022-05-26T22:30:02Z

CI MESSAGE: [4943793]: BUILD STARTED

dali-automaton · 2022-05-26T22:35:38Z

CI MESSAGE: [4943793]: BUILD FAILED

stiepan · 2022-05-26T22:41:58Z

!build

dali-automaton · 2022-05-26T22:45:03Z

CI MESSAGE: [4944005]: BUILD STARTED

dali-automaton · 2022-05-26T23:53:03Z

CI MESSAGE: [4944005]: BUILD PASSED

mzient · 2022-05-27T09:35:39Z

dali/pipeline/operator/sequence_utils.h

+ public:
+  CombinedRange(Range &&range, Ranges &&...ranges)
+      : ranges_{std::make_tuple(std::forward<Range>(range), std::forward<Ranges>(ranges)...)} {
+    assert(((range.NumSlices() == ranges.NumSlices()) && ...));


Fold expressions are a C++17 feature - this will break debug builds with NVCC 10.2.

Suggested change

assert(((range.NumSlices() == ranges.NumSlices()) && ...));

#if __cplusplus >= 201703L

assert(((range.NumSlices() == ranges.NumSlices()) && ...));

#endif

done, I disabled the whole CombinedRange, as the std::apply is also part of c++17

jantonguirao · 2022-05-27T10:33:29Z

dali/operators/image/color/brightness_contrast.cc

+    auto in_view = view<const InputType, ndim>(input[sample_id]);
+    auto out_view = view<OutputType, ndim>(output[sample_id]);


It's cheaper to get the view to the whole batch and then index:

auto in_view_batch = view<const InputType, ndim>(input); auto out_view_batch = view<OutputType, ndim>(output); for (int sample_id = 0; sample_id < num_samples; sample_id++) { auto in_view = in_view_batch[sample_id]; auto out_view = out_view_batch[sample_id];

jantonguirao · 2022-05-27T10:39:58Z

dali/operators/image/color/color_twist.cc

+    auto in_view = view<const InputType, ndim>(input[i]);
+    auto out_view = view<OutputType, ndim>(output[i]);


here as well. View first, index later is cheaper

Thanks, done!

jantonguirao · 2022-05-27T10:47:46Z

dali/test/python/test_operator_brightness_contrast.py

+          for (has_3_dims, use_const_contr_center) in rng.sample([
+              (b1, b2) for b1 in [True, False] for b2 in [True, False]], 2):


I guess this is to limit the number of combinations? Just asking, do we have any risks of missing some important combination?

Yes, it is just for this reason. I don't think so, that's why I left it to (pseudo) chance. :)

jantonguirao · 2022-05-27T10:56:08Z

dali/pipeline/operator/sequence_utils.h

+  using ValueType = std::tuple<typename Range::ValueType, typename Ranges::ValueType...>;
+
+  ValueType operator[](IndexType idx) const {
+    return std::apply([&idx](const auto &...ranges) { return std::make_tuple(ranges[idx]...); },


nitpick:

Suggested change

return std::apply([&idx](const auto &...ranges) { return std::make_tuple(ranges[idx]...); },

return std::apply([idx](const auto &...ranges) { return std::make_tuple(ranges[idx]...); },

would do I think

jantonguirao · 2022-05-27T10:57:01Z

dali/pipeline/operator/sequence_utils.h

+namespace dali {
+namespace sequence_utils {
+
+template <typename Storage, typename T, int ndim, int ndims_to_unfold>


Can you add some docstrings? Looking at this file alone is not enough to understand its purpose. I think

jantonguirao · 2022-05-27T10:57:39Z

dali/pipeline/operator/sequence_utils.h

+}
+
+template <typename Range, typename... Ranges>
+class CombinedRange {


Can you also add some docstrings here?

stiepan · 2022-05-29T21:00:33Z

!build

stiepan · 2022-05-29T21:01:10Z

dali/pipeline/operator/sequence_shape.h

-template <typename Range>
-class RangeIterator {
-  using IndexType = typename Range::IndexType;
-  using ValueType = typename Range::ValueType;
-
- public:
-  RangeIterator(const Range &range, IndexType idx) : range_{range}, idx_{idx} {}
-
-  ValueType operator*() const {
-    return range_[idx_];
-  }
-
-  bool operator==(const RangeIterator &other) const {
-    return idx_ == other.idx_;
-  }
-
-  bool operator!=(const RangeIterator &other) const {
-    return !(*this == other);
-  }
-
-  void operator++() {
-    ++idx_;
-  }
-
- private:
-  const Range &range_;
-  IndexType idx_;
-};
-


Moved to sequence_utils.h

dali-automaton · 2022-05-29T21:04:55Z

CI MESSAGE: [4962954]: BUILD FAILED

dali-automaton · 2022-05-29T21:06:53Z

CI MESSAGE: [4963005]: BUILD STARTED

dali-automaton · 2022-05-29T21:14:27Z

CI MESSAGE: [4963005]: BUILD FAILED

stiepan · 2022-05-30T07:42:04Z

!build

dali-automaton · 2022-05-30T07:45:07Z

CI MESSAGE: [4965549]: BUILD STARTED

dali-automaton · 2022-05-30T08:58:41Z

CI MESSAGE: [4965549]: BUILD FAILED

dali-automaton · 2022-05-30T09:10:56Z

CI MESSAGE: [4965549]: BUILD PASSED

klecki

Some comments

klecki · 2022-05-30T10:41:35Z

dali/pipeline/data/sequence_utils.h

+  }
+
+  bool operator==(const RangeIterator &other) const {
+    return idx_ == other.idx_;


I know this is a copy, but can two iterators that "point to" different ranges be equal if the point at the same index?

I think we can keep it as is for efficiency and assert that the collection is the same - it's a coding error and we can say it's UB to compare iterators from different collections.

klecki · 2022-05-30T10:50:57Z

dali/pipeline/data/sequence_utils.h

+          assert(view_.shape.size() >= ndims_to_unfold);
+          return volume(view_.shape.first(ndims_to_unfold));
+        }()},
+        slice_shape_{view_.shape.last(out_ndim)},


Does last work with DynamicDimensions?

klecki · 2022-05-30T11:33:50Z

dali/pipeline/data/sequence_utils.h

+template <int ndims_to_unfold, typename... Storages, typename... Ts, int... ndims>
+CombinedRange<UnfoldedViewRange<Storages, Ts, ndims, ndims_to_unfold>...> unfolded_views_range(
+    const TensorView<Storages, Ts, ndims> &...views) {
+  return combine_ranges(unfolded_view_range<ndims_to_unfold>(views)...);


Any error checking for the equal volume of unfolded dimensions or we just #yolo?

There an assert in combine ranges for equal length of the zipped ranges. I could simply change it to DALI_ENFORCE, but as I am using it in the context of video processing operators, where the number of frames is set by the operator to match the one from the input, it seems superfluous. We don't really check that if doing it manually. I stated in the docs the volumes must be equal, there is an assert, my opinion would be to check it manually before calling the utility if that is not something that follows from the usage logic. But maybe we prefer being on safe side and one extra == check per each sample in batch is fair price to pay, I won't argue if you insist.

klecki · 2022-05-30T13:02:47Z

dali/operators/image/color/brightness_contrast.cc

@@ -31,12 +32,12 @@ This operator can also change the type of data.)code")
    .NumOutput(1)
    .AddOptionalArg("brightness",
                    "Brightness mutliplier.",
-                    kDefaultBrightness, true)
+                    kDefaultBrightness, true, true)


General remark, how about adding a dedicated enum for the PerFrame option rather than having the growing true, true chain. Ofc in another PR.

Actually, I was thinking about flags.

klecki · 2022-05-30T13:12:18Z

dali/operators/image/color/brightness_contrast.cc


  using Kernel = kernels::MultiplyAddCpu<OutputType, InputType, 3>;
-  kernel_manager_.Initialize<Kernel>();
+  kernel_manager_.template Resize<Kernel>(1);


In the original implementation it was max_batch_size_ here.

It was, but then the operator called all the samples with the kernel instance of idx 0.

klecki · 2022-05-30T13:21:25Z

dali/test/python/test_utils.py

@@ -626,3 +626,16 @@ def gen():
 def as_array(tensor):
    import_numpy()
    return np.array(tensor.as_cpu() if isinstance(tensor, TensorGPU) else tensor)
+
+
+def python_function(*inputs, function, **kwargs):


What is this supposed to be doing? Can you add a docstring? The name is pretty generic.

klecki · 2022-05-30T13:21:53Z

dali/test/python/test_operator_color_twist.py

+    video_test_cases = [
+        (fn.hue, {}, [ArgCb("hue", hue, True)]),
+        (fn.saturation, {}, [ArgCb("saturation", saturation, True)]),
+        (fn.hsv, {}, [ArgCb("hue", hue, True), ArgCb(


stiepan · 2022-06-01T18:57:19Z

!build

dali-automaton · 2022-06-01T19:00:05Z

CI MESSAGE: [4987616]: BUILD STARTED

dali-automaton · 2022-06-01T20:26:20Z

CI MESSAGE: [4987616]: BUILD FAILED

dali-automaton · 2022-06-01T21:07:07Z

CI MESSAGE: [4987616]: BUILD PASSED

stiepan · 2022-06-06T11:12:52Z

!build

dali-automaton · 2022-06-06T11:15:28Z

CI MESSAGE: [5023066]: BUILD STARTED

dali-automaton · 2022-06-06T12:22:48Z

CI MESSAGE: [5023066]: BUILD PASSED

Signed-off-by: Kamil Tokarski <[email protected]>

…ur ops Signed-off-by: Kamil Tokarski <[email protected]>

Signed-off-by: Kamil Tokarski <[email protected]>

… to data dir Signed-off-by: Kamil Tokarski <[email protected]>

Signed-off-by: Kamil Tokarski <[email protected]>

…sorvector Signed-off-by: Kamil Tokarski <[email protected]>

Signed-off-by: Kamil Tokarski <[email protected]>

stiepan · 2022-06-14T12:51:55Z

!build

dali-automaton · 2022-06-14T12:55:13Z

CI MESSAGE: [5081203]: BUILD STARTED

dali-automaton · 2022-06-14T14:11:11Z

CI MESSAGE: [5081203]: BUILD PASSED

Signed-off-by: Kamil Tokarski <[email protected]>

stiepan · 2022-06-14T14:27:32Z

!build

dali-automaton · 2022-06-14T14:30:08Z

CI MESSAGE: [5081892]: BUILD STARTED

dali-automaton · 2022-06-14T15:52:02Z

CI MESSAGE: [5081892]: BUILD PASSED

stiepan marked this pull request as ready for review May 27, 2022 09:12

mzient reviewed May 27, 2022

View reviewed changes

jantonguirao assigned jantonguirao and klecki May 27, 2022

jantonguirao approved these changes May 27, 2022

View reviewed changes

stiepan commented May 29, 2022

View reviewed changes

klecki reviewed May 30, 2022

View reviewed changes

stiepan force-pushed the per_frame_brightness_contrast branch from 86efd70 to ad0b697 Compare June 1, 2022 18:53

stiepan force-pushed the per_frame_brightness_contrast branch from ad0b697 to ba96ce9 Compare June 6, 2022 08:45

stiepan requested a review from klecki June 6, 2022 13:00

klecki approved these changes Jun 13, 2022

View reviewed changes

stiepan added 13 commits June 14, 2022 14:11

Add per-sample contrast-center to bright_contrast op

e89d8f1

Signed-off-by: Kamil Tokarski <[email protected]>

Derive from SequenceOp, make per-sample args support per-frame inputs

a11c827

Signed-off-by: Kamil Tokarski <[email protected]>

Color twist per-frame operator

ea57a8b

Signed-off-by: Kamil Tokarski <[email protected]>

Introduce range utility over unfolded tensor views and use it in colo…

f3547b4

…ur ops Signed-off-by: Kamil Tokarski <[email protected]>

Fix linter issues

2dab152

Signed-off-by: Kamil Tokarski <[email protected]>

Move rangeiterator to sequenceutils, move sequenceutils from operator…

762894e

… to data dir Signed-off-by: Kamil Tokarski <[email protected]>

Add docs (and change int capture from reference to value)

52d2602

Signed-off-by: Kamil Tokarski <[email protected]>

Make tensorview over a whole list first, not over each particular ten…

f226b50

…sorvector Signed-off-by: Kamil Tokarski <[email protected]>

Add missing endline in test_utils.py

acdabb0

Signed-off-by: Kamil Tokarski <[email protected]>

Fix dynamicdimension unfold, add docs

52eb1cb

Signed-off-by: Kamil Tokarski <[email protected]>

Add unfolding cc tests

3a83b41

Signed-off-by: Kamil Tokarski <[email protected]>

Fix misspelled test name

144e89c

Signed-off-by: Kamil Tokarski <[email protected]>

Test linitng

f3762f5

Signed-off-by: Kamil Tokarski <[email protected]>

stiepan force-pushed the per_frame_brightness_contrast branch from ba96ce9 to f3762f5 Compare June 14, 2022 12:45

Fix operator brigthness contrast video test suite

ca006a8

Signed-off-by: Kamil Tokarski <[email protected]>

stiepan merged commit e687379 into NVIDIA:main Jun 14, 2022

JanuszL mentioned this pull request Jan 11, 2023

DALI 2022 roadmap #3774

Closed

		auto in_view = view<const InputType, ndim>(input[sample_id]);
		auto out_view = view<OutputType, ndim>(output[sample_id]);

		for (has_3_dims, use_const_contr_center) in rng.sample([
		(b1, b2) for b1 in [True, False] for b2 in [True, False]], 2):

	return std::apply([&idx](const auto &...ranges) { return std::make_tuple(ranges[idx]...); },
	return std::apply([idx](const auto &...ranges) { return std::make_tuple(ranges[idx]...); },

Per frame brightness contrast #3937

Per frame brightness contrast #3937

Conversation

stiepan commented May 26, 2022 • edited Loading

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Checklist

Tests

Documentation

DALI team only

Requirements

stiepan commented May 26, 2022

dali-automaton commented May 26, 2022

dali-automaton commented May 26, 2022

stiepan commented May 26, 2022

dali-automaton commented May 26, 2022

dali-automaton commented May 26, 2022

mzient May 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stiepan commented May 29, 2022

Choose a reason for hiding this comment

dali-automaton commented May 29, 2022

dali-automaton commented May 29, 2022

dali-automaton commented May 29, 2022

stiepan commented May 30, 2022

dali-automaton commented May 30, 2022

dali-automaton commented May 30, 2022

dali-automaton commented May 30, 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

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

stiepan commented Jun 1, 2022

dali-automaton commented Jun 1, 2022

dali-automaton commented Jun 1, 2022

dali-automaton commented Jun 1, 2022

stiepan commented Jun 6, 2022

dali-automaton commented Jun 6, 2022

dali-automaton commented Jun 6, 2022

stiepan commented Jun 14, 2022

dali-automaton commented Jun 14, 2022

dali-automaton commented Jun 14, 2022

stiepan commented Jun 14, 2022

dali-automaton commented Jun 14, 2022

dali-automaton commented Jun 14, 2022

stiepan commented May 26, 2022 •

edited

Loading

mzient May 27, 2022 •

edited

Loading