Use GPU Convert in nvJPEG decoder #4247

staniewzki · 2022-09-09T13:17:03Z

Category:

New feature (non-breaking change which adds functionality)

Description:

This PR adds usage of GPU Convert in nvJPEG decoder. This enables the decoder to output data types other than uint8_t, and support orientation adjustment.

During my work on this PR me and @szkarpinski found out that a lot of the orientation-related code in imgcodec is bugged, and this PR fixes all the previous mistakes.

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: N/A

jantonguirao · 2022-09-27T12:51:24Z

dali/imgcodec/decoders/nvjpeg/nvjpeg.cc

    if (roi.use_roi()) {
      ctx.shape[0] = roi.shape()[0];
      ctx.shape[1] = roi.shape()[1];
    }
-    DecodeJpegSample(*in, out.mutable_data<uint8_t>(), opts, ctx);
+
+    // Synchronizing on the acces to immediate_buffer


Suggested change

// Synchronizing on the acces to immediate_buffer

// Synchronizing on the access to immediate_buffer

jantonguirao · 2022-09-27T12:53:59Z

dali/imgcodec/decoders/nvjpeg/nvjpeg.cc

+    auto& intermediate_buffer = ctx.resources.intermediate_buffer;
+    CUDA_CALL(cudaEventSynchronize(ctx.resources.decode_event));


Open question: Do we need to synchronize always or only when we actually use intermediate_buffer?

Only when the buffer is actually used. So I guess here It could be conditional, and later on when some other resources are used we can sync again? The second sync would be a no-op, right?

The synchronization would have to happen almost in the same place anyways (#4292), I don't think it makes any difference.

mzient · 2022-09-27T12:55:14Z

dali/imgcodec/decoders/nvjpeg/nvjpeg.cc

    if (roi.use_roi()) {
      ctx.shape[0] = roi.shape()[0];
      ctx.shape[1] = roi.shape()[1];
    }
-    DecodeJpegSample(*in, out.mutable_data<uint8_t>(), opts, ctx);
+
+    // Synchronizing on the acces to immediate_buffer


Suggested change

// Synchronizing on the acces to immediate_buffer

// Synchronizing on the access to itermediate_buffer

?

dali/imgcodec/util/output_shape.h

mzient · 2022-09-28T12:20:23Z

dali/imgcodec/decoders/nvjpeg/nvjpeg.cc

      ctx.shape[0] = roi.shape()[0];
      ctx.shape[1] = roi.shape()[1];


We need to use un-rotated size here - (as we do in ParseJpegSample). This is most certainly a bug and needs a follow-up work (this PR or otherwise).

mzient

OKayish - but I'd rather not merge it before #4224
Also, the orientation support with ROI looks broken.

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

dali-automaton · 2022-09-30T06:44:23Z

CI MESSAGE: [6053230]: BUILD STARTED

dali-automaton · 2022-09-30T08:21:11Z

CI MESSAGE: [6053230]: BUILD FAILED

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

staniewzki · 2022-09-30T09:40:40Z

!build

dali-automaton · 2022-09-30T09:45:11Z

CI MESSAGE: [6054356]: BUILD STARTED

dali-automaton · 2022-09-30T11:06:58Z

CI MESSAGE: [6054356]: BUILD FAILED

dali-automaton · 2022-09-30T11:20:16Z

CI MESSAGE: [6054356]: BUILD PASSED

mzient · 2022-09-30T11:39:57Z

dali/imgcodec/util/output_shape.cc

+    roi.begin[idx] = info.shape[idx ^ swap_xy] - roi.begin[idx];
+    roi.end[idx] = info.shape[idx ^ swap_xy] - roi.end[idx];


Well.... this will only work for exactly 2 dimensions. If we had, say, MJPEG sequence, we'd have an extra outermost dimension and the index xoring hack would break.
Having said that - let's merge it as is and improve in a follow-up.

mzient · 2022-09-30T11:41:39Z

dali/imgcodec/util/output_shape.h

-      extent = roi.end[d];
-    if (d < roi.begin.size())
-      extent -= roi.begin[d];
+    if (roi) {


This check seems redundant...

I will double check it, I think I had some error without it. But yes, seems redundant.

Okay, I think I know why I did this, but that was wrong.

mzient · 2022-09-30T11:42:11Z

dali/imgcodec/util/output_shape.h

@@ -21,7 +21,7 @@ namespace dali {
 namespace imgcodec {

 template <typename OutShape>
-void OutputShape(OutShape &&out_shape,
+void DLL_PUBLIC OutputShape(OutShape &&out_shape,


I don't understand it. A template shouldn't require external linkage - unless you're doing something very wrong.

Well I don't fully understand as well, I though that you told me to do that 😛 Reverting this then.

staniewzki mentioned this pull request Sep 22, 2022

Add JPEG orientation reference data NVIDIA/DALI_extra#106

Merged

staniewzki changed the title ~~Support other output data types in nvJPEG decoder~~ Support other output data types and orientation adjustment in nvJPEG decoder Sep 22, 2022

staniewzki changed the title ~~Support other output data types and orientation adjustment in nvJPEG decoder~~ Use GPU Convert in nvJPEG decoder Sep 22, 2022

staniewzki force-pushed the nvjpeg-convert branch 3 times, most recently from 8080811 to 043d0e4 Compare September 27, 2022 11:58

staniewzki marked this pull request as ready for review September 27, 2022 11:59

jantonguirao reviewed Sep 27, 2022

View reviewed changes

mzient reviewed Sep 27, 2022

View reviewed changes

jantonguirao reviewed Sep 27, 2022

View reviewed changes

dali/imgcodec/util/output_shape.h Outdated Show resolved Hide resolved

jantonguirao approved these changes Sep 27, 2022

View reviewed changes

jantonguirao assigned jantonguirao and mzient Sep 27, 2022

jantonguirao approved these changes Sep 28, 2022

View reviewed changes

mzient reviewed Sep 28, 2022

View reviewed changes

mzient approved these changes Sep 28, 2022

View reviewed changes

staniewzki added 5 commits September 28, 2022 16:07

Using GPU Convert in nvjpeg

9bac1ba

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

Adjust orientation

b5b80b7

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

Linter fixes

0b8b7b1

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

Remove debug function

759a854

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

Review changes

85022e4

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

mzient force-pushed the nvjpeg-convert branch from 0e3b37a to 85022e4 Compare September 28, 2022 14:09

mzient and others added 3 commits September 28, 2022 16:10

Fix after rebase.

70fd513

Signed-off-by: Michal Zientkiewicz <michalz@nvidia.com>

Orientation fixes

ca28e5c

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

Adjusting ROI to orientation

54d28ef

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

staniewzki requested review from mzient and jantonguirao September 29, 2022 15:14

jantonguirao approved these changes Sep 30, 2022

View reviewed changes

Bump DALI_EXTRA_VERSION

1897450

Signed-off-by: Michał Staniewski <m.staniewzki@gmail.com>

mzient reviewed Sep 30, 2022

View reviewed changes

mzient approved these changes Sep 30, 2022

View reviewed changes

staniewzki merged commit 256c021 into NVIDIA:main Sep 30, 2022

staniewzki mentioned this pull request Sep 30, 2022

Fix nvJPEG decoder synchronization #4292

Closed

18 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use GPU Convert in nvJPEG decoder #4247

Use GPU Convert in nvJPEG decoder #4247

staniewzki commented Sep 9, 2022 •

edited

jantonguirao Sep 27, 2022

jantonguirao Sep 27, 2022

staniewzki Sep 27, 2022 •

edited

staniewzki Sep 27, 2022 •

edited

mzient Sep 27, 2022

mzient Sep 28, 2022

mzient left a comment

dali-automaton commented Sep 30, 2022

dali-automaton commented Sep 30, 2022

staniewzki commented Sep 30, 2022

dali-automaton commented Sep 30, 2022

dali-automaton commented Sep 30, 2022

dali-automaton commented Sep 30, 2022

mzient Sep 30, 2022

mzient Sep 30, 2022

staniewzki Sep 30, 2022

staniewzki Sep 30, 2022

mzient Sep 30, 2022

staniewzki Sep 30, 2022

	// Synchronizing on the acces to immediate_buffer
	// Synchronizing on the access to immediate_buffer

		auto& intermediate_buffer = ctx.resources.intermediate_buffer;
		CUDA_CALL(cudaEventSynchronize(ctx.resources.decode_event));

	// Synchronizing on the acces to immediate_buffer
	// Synchronizing on the access to itermediate_buffer

		ctx.shape[0] = roi.shape()[0];
		ctx.shape[1] = roi.shape()[1];

		roi.begin[idx] = info.shape[idx ^ swap_xy] - roi.begin[idx];
		roi.end[idx] = info.shape[idx ^ swap_xy] - roi.end[idx];

Use GPU Convert in nvJPEG decoder #4247

Use GPU Convert in nvJPEG decoder #4247

Conversation

staniewzki commented Sep 9, 2022 • edited

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staniewzki Sep 27, 2022 • edited

Choose a reason for hiding this comment

staniewzki Sep 27, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient left a comment

Choose a reason for hiding this comment

dali-automaton commented Sep 30, 2022

dali-automaton commented Sep 30, 2022

staniewzki commented Sep 30, 2022

dali-automaton commented Sep 30, 2022

dali-automaton commented Sep 30, 2022

dali-automaton commented Sep 30, 2022

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

staniewzki commented Sep 9, 2022 •

edited

staniewzki Sep 27, 2022 •

edited

staniewzki Sep 27, 2022 •

edited