Use BrightnessContrast as implementation of Brightness and Contrast ops #2981

banasraf · 2021-05-21T15:08:20Z

Signed-off-by: Rafal Banas.Rafal97@gmail.com

Why we need this PR?

Brightness and Contrast operators are now implemented as ColorTwist which less performant than BrightnessContrast.

What happened in this PR?

What solution was applied:
Brightness and Contrast are now implemented as BrightnessContrast with limited args.
Affected modules and functionalities:
Brightness and Contrast were moved from color_twist to brightness_contrast.
Key points relevant for the review:
brightness_contrast.cc/h
Validation and testing:
I added new python tests for BrightnessContrast.
Documentation (including examples):
Changes in operators documentation.

JIRA TASK: DALI-2104

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf · 2021-05-21T15:08:26Z

!build

dali-automaton · 2021-05-21T15:12:52Z

CI MESSAGE: [2396121]: BUILD STARTED

dali-automaton · 2021-05-21T16:34:24Z

CI MESSAGE: [2396121]: BUILD FAILED

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf · 2021-05-24T16:16:08Z

!build

dali-automaton · 2021-05-24T16:22:25Z

CI MESSAGE: [2402733]: BUILD STARTED

JanuszL · 2021-05-24T17:24:48Z

dali/operators/image/color/brightness_contrast.h

+    if (this->spec_.ArgumentDefined("brightness_shift")) {
+      this->GetPerSampleArgument(brightness_shift_, "brightness_shift", ws, curr_batch_size);
+    } else {
+      brightness_shift_ = std::vector<float>(curr_batch_size, 0);


Shouldn't it just get a default value from schema? Why we need this here?

Because getting an arg that is not present in the schema (e.g. it's Contrast op and we fetch for brightness) results in exception.

Can we have one constant that defines this default value here and in the schema?

I tend to disagree. The code with 0 and 1 is easier to understand.

I would agree that it doesn't make much sense to name those constants, but in this case it connects the default values in the schema with the ones here, so I think it useful.

JanuszL · 2021-05-24T17:26:11Z

dali/test/python/test_operator_brightness_contrast.py

+  for device in ['cpu', 'gpu']:
+    pipe1 = bricon_ref_pipe(ri1, 0.4, device, batch_size=batch_size)
+    pipe2 = bricon_pipe(ri2, 0.4, device, batch_size=batch_size)
+    yield compare_pipelines, pipe1, pipe2, batch_size, n_iters


Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf · 2021-05-25T09:21:46Z

!build

dali-automaton · 2021-05-25T09:27:13Z

CI MESSAGE: [2405630]: BUILD STARTED

dali-automaton · 2021-05-25T10:44:50Z

CI MESSAGE: [2405630]: BUILD PASSED

jantonguirao · 2021-05-25T16:32:12Z

dali/operators/image/color/brightness_contrast.cc

+If not set, the input type is used.)code", DALI_NO_TYPE);
+
+DALI_SCHEMA(Contrast)
+    .DocStr(R"code(Changes the color contrast of the image.)code")


color contrast -> color (?)

Also, I'd keep the formula here, for completeness

Suggested change

.DocStr(R"code(Changes the color contrast of the image.)code")

.DocStr(R"code(Adjusts the contrast of the image.)code")

jantonguirao · 2021-05-25T16:32:49Z

dali/operators/image/color/brightness_contrast.cc

-
-This operator can also change the type of data.)code")
+DALI_SCHEMA(Brightness)
+    .DocStr(R"code(Changes the brightness of the image.)code")


I guess we should keep the full explanation with the formula, adjusted for this case.

jantonguirao · 2021-05-25T16:33:47Z

dali/operators/image/color/brightness_contrast.h

-    this->GetPerSampleArgument(brightness_, "brightness", ws, curr_batch_size);
-    this->GetPerSampleArgument(brightness_shift_, "brightness_shift", ws, curr_batch_size);
-    this->GetPerSampleArgument(contrast_, "contrast", ws, curr_batch_size);
+    if (this->spec_.ArgumentDefined("brightness")) {


perhaps out of scope, but consider using the ArgValue utility. The code is much simpler to write this way.

jantonguirao · 2021-05-25T16:35:40Z

dali/test/python/test_operator_brightness_contrast.py

+         fn.random.uniform(range=[0.4, 0.6], seed=123)
+
+
+@pipeline_def(num_threads=12, device_id=0, seed=1234)


that's quite a high number of threads. Maybe 4?

dali/test/python/test_operator_brightness_contrast.py

jantonguirao · 2021-05-25T16:38:01Z

dali/test/python/test_operator_brightness_contrast.py

+  ri2 = RandomDataIterator(batch_size, shape=(512, 256, 3), dtype=np.float32)
+  for device in ['cpu', 'gpu']:
+    pipe1 = bri_and_con_pipe(ri1, 0.4, device, batch_size=batch_size)
+    pipe2 = bricon_pipe(ri2, 0.4, device, batch_size=batch_size)


can you put that 0.4 in a variable so that it is easier to understand what it is?

dali/test/python/test_operator_brightness_contrast.py

mzient · 2021-05-26T06:12:03Z

dali/test/python/test_operator_brightness_contrast.py

+  return fn.random.uniform(range=[0.5, 2.0], seed=123), \
+         fn.random.uniform(range=[0.5, 2.0], seed=123), \
+         fn.random.uniform(range=[0.4, 0.6], seed=123)


Suggested change

return fn.random.uniform(range=[0.5, 2.0], seed=123), \

fn.random.uniform(range=[0.5, 2.0], seed=123), \

fn.random.uniform(range=[0.4, 0.6], seed=123)

return fn.random.uniform(range=[0.0, 5.0], seed=123), \

fn.random.uniform(range=[0.0, 5.0], seed=123), \

fn.random.uniform(range=[-1,0, 1.0], seed=123)

mzient · 2021-05-26T06:12:51Z

dali/test/python/test_operator_brightness_contrast.py

+
+
+def bricon_ref(input, brightness, brightness_shift, contrast, contrast_center):
+  return brightness_shift + brightness * (contrast_center + contrast * (input - contrast_center))


Where is the output's dynamic range?

Suggested change

return brightness_shift + brightness * (contrast_center + contrast * (input - contrast_center))

return brightness_shift * output_range + brightness * (contrast_center + contrast * (input - contrast_center))

Extended tests to check different input/output types

mzient · 2021-05-26T06:13:53Z

dali/test/python/test_operator_brightness_contrast.py

+  ri1 = RandomDataIterator(batch_size, shape=(512, 256, 3), dtype=np.float32)
+  ri2 = RandomDataIterator(batch_size, shape=(512, 256, 3), dtype=np.float32)


The operator's output depends on the type (type's dynamic range is used) - please include this fact in the test.

Extended tests to check different input/output types

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf · 2021-06-08T11:14:37Z

dali/operators/image/color/brightness_contrast.h

@@ -84,16 +86,32 @@ class BrightnessContrastOp : public Operator<Backend> {
    // out = (brightness_shift * brightness_range +
    //        brightness * (contrast_center - contrast * contrast_center)) +
    //        brightness * contrast * in
+    float norm_brightness = (brightness * brightness_contrast::FullRange<OutputType>()) /


This is the bugfix. It adds normalization to the output range.

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

JanuszL · 2021-06-08T11:33:35Z

dali/operators/image/color/brightness_contrast.cc

+    .DocStr(R"code(Adjusts the brightness of the images based on the following formula::
+    out = brightness_shift * output_range + brightness * in
+Where output_range is 1 for float outputs or the maximum positive value for integral types.


Suggested change

.DocStr(R"code(Adjusts the brightness of the images based on the following formula::

out = brightness_shift * output_range + brightness * in

Where output_range is 1 for float outputs or the maximum positive value for integral types.

.DocStr(R"code(Adjusts the brightness of the images based on the following formula::

out = brightness_shift * output_range + brightness * in

Where output_range is 1 for float outputs or the maximum positive value for integral types.

You need an empty line between formula and the comment

JanuszL · 2021-06-08T11:33:51Z

dali/operators/image/color/brightness_contrast.cc

+    .DocStr(R"code(Adjusts the contrast of the images based on the following formula::
+    out = grey + contrast * (in - grey)
+Where:


Suggested change

.DocStr(R"code(Adjusts the contrast of the images based on the following formula::

out = grey + contrast * (in - grey)

Where:

.DocStr(R"code(Adjusts the contrast of the images based on the following formula::

out = grey + contrast * (in - grey)

Where:

Similar

JanuszL · 2021-06-08T11:38:09Z

dali/test/python/test_operator_brightness_contrast.py

+    clipped = np.round(clipped)
+  return clipped.astype(out_dtype)
+
+def dali_to_np_type(dtype):


Maybe you can use dali_type_to_np ?

JanuszL

Please adjust Copyright notices as well.

jantonguirao · 2021-06-08T11:18:13Z

dali/operators/image/color/brightness_contrast.cc

-	The output_range is 1 for float outputs or the maximum positive value for integral types.
-	Grey denotes the value of 0.5 for ``float``, 128 for ``uint8``, and 16384 for ``int16``, and so on.
+DALI_SCHEMA(Brightness)
+    .DocStr(R"code(Adjusts the brightness of the images based on the following formula::


This won't work well with the operators table, the first sentence should be short to fit into the table:

Suggested change

.DocStr(R"code(Adjusts the brightness of the images based on the following formula::

.DocStr(R"code(Adjusts the brightness of images.

The brightness is adjusted based on the following formula::

jantonguirao · 2021-06-08T11:19:22Z

dali/operators/image/color/brightness_contrast.cc

+If not set, the input type is used.)code", DALI_NO_TYPE);
+
+DALI_SCHEMA(Contrast)
+    .DocStr(R"code(Adjusts the contrast of the images based on the following formula::


Suggested change

.DocStr(R"code(Adjusts the contrast of the images based on the following formula::

.DocStr(R"code(Adjusts the contrast of images.

The contrast is adjusted based on the following formula::

jantonguirao · 2021-06-08T11:20:28Z

dali/operators/image/color/brightness_contrast.cc

+Where:
+
+-	The output_range is 1 for float outputs or the maximum positive value for integral types.
+-	Grey denotes the value of 0.5 for ``float``, 128 for ``uint8``, and 16384 for ``int16``, and so on.


Suggestion: rename grey to "midrange", "halfrange" or similar.

Actually, there is an argument contrast_center that is used is this value, so I renamed gray to contrast_center. It has its own description so I removed this information from here.

jantonguirao · 2021-06-08T11:21:17Z

dali/operators/image/color/brightness_contrast.cc

+DALI_SCHEMA(BrightnessContrast)
+    .AddParent("Brightness")
+    .AddParent("Contrast")
+    .DocStr(R"code(Adjusts the brightness and contrast of the images based on the following


Suggested change

.DocStr(R"code(Adjusts the brightness and contrast of the images based on the following

.DocStr(R"code(Adjusts the brightness and contrast the images.

The brightness and contrast is adjusted based on the following

mzient · 2021-06-08T13:02:22Z

dali/operators/image/color/brightness_contrast.h

  }

  void AcquireArguments(const workspace_t<Backend> &ws) {
    auto curr_batch_size = ws.GetInputBatchSize(0);
    if (this->spec_.ArgumentDefined("brightness")) {
      this->GetPerSampleArgument(brightness_, "brightness", ws, curr_batch_size);
    } else {
-      brightness_ = std::vector<float>(curr_batch_size, 1.f);
+      brightness_ = std::vector<float>(curr_batch_size, kDefaultBrightness);


1 and 0 are not considered magic numbers - and it's much easier to understand the code when you see a default argument of multiplication or addition without having to look up the remote definition.

mzient · 2021-06-08T13:02:51Z

dali/operators/image/color/brightness_contrast.h

    }

    if (this->spec_.ArgumentDefined("brightness_shift")) {
      this->GetPerSampleArgument(brightness_shift_, "brightness_shift", ws, curr_batch_size);
    } else {
-      brightness_shift_ = std::vector<float>(curr_batch_size, 0);
+      brightness_shift_ = std::vector<float>(curr_batch_size, kDefaultBrightnessShift);


mzient · 2021-06-08T13:03:02Z

dali/operators/image/color/brightness_contrast.h

    }

    if (this->spec_.ArgumentDefined("contrast")) {
      this->GetPerSampleArgument(contrast_, "contrast", ws, curr_batch_size);
    } else {
-      contrast_ = std::vector<float>(curr_batch_size, 1.f);
+      contrast_ = std::vector<float>(curr_batch_size, kDefaultContrast);


...and again.

mzient · 2021-06-08T13:05:29Z

dali/operators/image/color/brightness_contrast.h

+const float kDefaultBrightness = 1.f;
+const float kDefaultBrightnessShift = 0;
+const float kDefaultContrast = 1.f;


Please don't. It's much easier to understand the purpose of having a multiplier of 1 or an addend of 0 than to look for definition somewhere. This also may create a misconception that these values are somehow arbitrarily chosen, when in fact they are mathematically fixed.

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

JanuszL · 2021-06-08T18:00:13Z

dali/test/python/test_operator_brightness_contrast.py

+  inp = fn.brightness(inp, brightness=brightness, brightness_shift=brightness_shift, dtype=dtype)
+  return inp


Suggested change

inp = fn.brightness(inp, brightness=brightness, brightness_shift=brightness_shift, dtype=dtype)

return inp

out = fn.brightness(inp, brightness=brightness, brightness_shift=brightness_shift, dtype=dtype)

return out

And similar in other functions.
Just a suggestion.

Changed to directly returning

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf · 2021-06-09T07:16:37Z

!build

dali-automaton · 2021-06-09T07:22:13Z

CI MESSAGE: [2455714]: BUILD STARTED

dali-automaton · 2021-06-09T07:51:47Z

CI MESSAGE: [2455714]: BUILD FAILED

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf · 2021-06-09T08:18:50Z

!build

dali-automaton · 2021-06-09T08:21:39Z

CI MESSAGE: [2455858]: BUILD STARTED

dali-automaton · 2021-06-09T09:46:59Z

CI MESSAGE: [2455858]: BUILD FAILED

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf · 2021-06-10T15:12:39Z

!build

dali-automaton · 2021-06-10T15:16:41Z

CI MESSAGE: [2461351]: BUILD STARTED

dali-automaton · 2021-06-10T20:43:20Z

CI MESSAGE: [2461351]: BUILD PASSED

Use BrightnessContrast as implementation of Brightness and Contrast ops

b99a3da

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

Fix arguments fetching. Add tests

51a4593

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf changed the title ~~[WiP] Use BrightnessContrast as implementation of Brightness and Contrast ops~~ Use BrightnessContrast as implementation of Brightness and Contrast ops May 24, 2021

JanuszL reviewed May 24, 2021

View reviewed changes

JanuszL approved these changes May 24, 2021

View reviewed changes

JanuszL self-assigned this May 24, 2021

newline

8c6f51d

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

jantonguirao self-assigned this May 25, 2021

jantonguirao reviewed May 25, 2021

View reviewed changes

mzient reviewed May 26, 2021

View reviewed changes

Bug fix.

09c59ea

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf commented Jun 8, 2021

View reviewed changes

remove debug print

87c2f4c

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

JanuszL reviewed Jun 8, 2021

View reviewed changes

jantonguirao reviewed Jun 8, 2021

View reviewed changes

mzient reviewed Jun 8, 2021

View reviewed changes

Review fixes + test fix

644857b

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

banasraf force-pushed the use-bricon-in-bri-and-con branch from 39c37ce to 644857b Compare June 8, 2021 14:05

jantonguirao approved these changes Jun 8, 2021

View reviewed changes

JanuszL reviewed Jun 8, 2021

View reviewed changes

test refactoring

931a970

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

JanuszL approved these changes Jun 8, 2021

View reviewed changes

fix compilation

b4485b4

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

Fix tests

782fd52

Signed-off-by: Rafal <Banas.Rafal97@gmail.com>

JanuszL approved these changes Jun 10, 2021

View reviewed changes

banasraf merged commit 2293d33 into NVIDIA:main Jun 11, 2021

	.DocStr(R"code(Changes the color contrast of the image.)code")
	.DocStr(R"code(Adjusts the contrast of the image.)code")

		fn.random.uniform(range=[0.4, 0.6], seed=123)


		@pipeline_def(num_threads=12, device_id=0, seed=1234)



		def bricon_ref(input, brightness, brightness_shift, contrast, contrast_center):
		return brightness_shift + brightness * (contrast_center + contrast * (input - contrast_center))

	return brightness_shift + brightness * (contrast_center + contrast * (input - contrast_center))
	return brightness_shift * output_range + brightness * (contrast_center + contrast * (input - contrast_center))

		ri1 = RandomDataIterator(batch_size, shape=(512, 256, 3), dtype=np.float32)
		ri2 = RandomDataIterator(batch_size, shape=(512, 256, 3), dtype=np.float32)

-    .DocStr(R"code(Adjusts the brightness of the images based on the following formula::
+    .DocStr(R"code(Adjusts the brightness of images.
+The brightness is adjusted based on the following formula::

-    .DocStr(R"code(Adjusts the contrast of the images based on the following formula::
+    .DocStr(R"code(Adjusts the contrast of images.
+The contrast is adjusted based on the following formula::

-    .DocStr(R"code(Adjusts the brightness and contrast of the images based on the following
+    .DocStr(R"code(Adjusts the brightness and contrast the images.
+The brightness and contrast is adjusted based on the following

		inp = fn.brightness(inp, brightness=brightness, brightness_shift=brightness_shift, dtype=dtype)
		return inp

Use BrightnessContrast as implementation of Brightness and Contrast ops #2981

Use BrightnessContrast as implementation of Brightness and Contrast ops #2981

Conversation

banasraf commented May 21, 2021 • edited Loading

Why we need this PR?

What happened in this PR?

banasraf commented May 21, 2021

dali-automaton commented May 21, 2021

dali-automaton commented May 21, 2021

banasraf commented May 24, 2021

dali-automaton commented May 24, 2021

Choose a reason for hiding this comment

banasraf May 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Jun 8, 2021 • 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

banasraf commented May 25, 2021

dali-automaton commented May 25, 2021

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

mzient May 26, 2021 • 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

banasraf Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

JanuszL Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanuszL Jun 8, 2021 • 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

JanuszL 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

mzient Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banasraf commented Jun 9, 2021

dali-automaton commented Jun 9, 2021

dali-automaton commented Jun 9, 2021

banasraf commented Jun 9, 2021

dali-automaton commented Jun 9, 2021

dali-automaton commented Jun 9, 2021

banasraf commented Jun 10, 2021

dali-automaton commented Jun 10, 2021

dali-automaton commented Jun 10, 2021

banasraf commented May 21, 2021 •

edited

Loading

banasraf May 24, 2021 •

edited

Loading

mzient Jun 8, 2021 •

edited

Loading

mzient May 26, 2021 •

edited

Loading

banasraf Jun 8, 2021 •

edited

Loading

JanuszL Jun 8, 2021 •

edited

Loading

JanuszL Jun 8, 2021 •

edited

Loading

mzient Jun 8, 2021 •

edited

Loading

mzient Jun 8, 2021 •

edited

Loading