Imgcodec module boilerplate (interfaces/placeholders/basic logic) #4029

jantonguirao · 2022-06-30T11:14:18Z

Signed-off-by: Joaquin Anton janton@nvidia.com
Co-authored-by: Michał Zientkiewicz mzient@gmail.com

Category:

New feature

Description:

It starts a new library libdali_imgcodec, to be used to hold all the image decoding infrastructure, that we will use later to re-implement our image decoder operators.
The new architecture is meant to be easy to extend, and to take care of the differences between our image decoding implementations (ROI support, type conversion, CPU/GPU, etc)
This is an initial code drop, which will be followed up with an actual implementation

Additional information:

Affected modules and functionalities:

New module

Key points relevant for the review:

Interfaces, overall design

Tests:

Checklist

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

JIRA TASK: DALI-2734

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao · 2022-06-30T11:40:31Z

!build

dali-automaton · 2022-06-30T11:45:02Z

CI MESSAGE: [5221126]: BUILD STARTED

dali-automaton · 2022-06-30T11:49:50Z

CI MESSAGE: [5221126]: BUILD FAILED

dali/operators/decoder/nvjpeg/nvjpeg_decoder_decoupled_api.h

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao · 2022-06-30T12:05:47Z

!build

dali-automaton · 2022-06-30T12:09:57Z

CI MESSAGE: [5221299]: BUILD STARTED

dali-automaton · 2022-06-30T13:30:47Z

CI MESSAGE: [5221299]: BUILD PASSED

Signed-off-by: Joaquin Anton <janton@nvidia.com>

mzient · 2022-07-01T10:05:51Z

dali/imgcodec/dali_imgcodec_test.cc

@@ -0,0 +1,42 @@
+// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


Copy-paste; nothing to review here.

mzient · 2022-07-01T10:06:38Z

include/dali/imgcodec/image_format.h

@@ -0,0 +1,98 @@
+// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


This is an important file

mzient · 2022-07-01T10:06:42Z

include/dali/imgcodec/image_codec.h

@@ -0,0 +1,153 @@
+// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


This is an important file

mzient · 2022-07-01T10:06:50Z

include/dali/imgcodec/image_source.h

@@ -0,0 +1,94 @@
+// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


This is an important file

Signed-off-by: Joaquin Anton <janton@nvidia.com>

awolant · 2022-07-04T10:22:31Z

qa/TL0_cpu_only/test_nofw.sh

+    "dali_operator_test.bin" \
+    "dali_imgcodec_test.bin"


This change was not added to Xavier tests? Was it intentional for some reason?

Missed that one by accident. Good catch

awolant · 2022-07-04T10:53:26Z

include/dali/imgcodec/image_format.h

+  std::vector<ImageCodec*> codec_ptrs_;
+};
+
+class DLL_PUBLIC ImageFormatRegistry {


Based on what I understand this is sort of an "entry point" to this API. Can you share a bit on how this will be used by the operators? If there are any adapters or additional layers of abstraction necessary to use it in the operator maybe missing functionality should be included here?

It's not exactly the entry point - there must be something on a higher level, but we're still considering whether that something should be a part of this library or left to the operator.

I meant entry point to the part that is in this PR.

I am asking about this because I am a bit afraid that this design is incomplete with regards on how it will be integrated with DALI. This will probably cause some, maybe significant changes, when this integration eventually comes.

I think it would be good to have that figured out before we spend some significant effort into implementing it for particular formats.

awolant · 2022-07-04T10:56:22Z

include/dali/imgcodec/image_format.h

+  } orientation;
+};
+
+class ImageParser {


Since this interface is supposed to be implemented by others I think it requires documentation.

awolant · 2022-07-04T11:21:50Z

include/dali/imgcodec/image_codec.h

+  }
+};
+
+class DLL_PUBLIC ImageCodec {


As we discussed I think using the word "codec" in this context might be misleading. As far as I understand this is something that can give you an object that can be used to decode images encoded with some particular method.

I'll rename to Decoder

awolant · 2022-07-04T11:32:14Z

include/dali/imgcodec/image_format.h

+   * @param image
+   * @return ImageFormat*
+   */
+  ImageFormat* GetImageFormat(ImageSource *image) const;


As I understand, we get ImageSource from the reader, then we look here for matching ImageFormat. We do so by using CanParse method of a parser associated with that ImageFormat. Is this correct?
And this is done, because ImageSource is basically raw data with some small additions.

I think that more often than not, we know the format of a file based on its extension or some other external information (like DL dataset description). Having this, there is no need to dynamically check, which parser matches the ImageSource and by extension what is the format.
I get that in general, maybe we do not now the format in the first place and we need this but I think that based on our use cases this is rather an exceptional situation. Is this assumption correct?

Are there any plans to include something like that? Or maybe parsers can depend on ImageSource.SourceInfo for some additional context.

I am asking because I am a bit afraid that it will not be easy to write those CanParse functions and this method of finding a format relies on them. Especially with many formats and matching format being at the end of the list, we might see some performance penalties, if the CanParse methods are slow.

This will be basically Cut/Paste (with slight modifications) from functions like CheckIsJPEG, CheckIsPNG, etc which we already have. The biggest difference is that the new functions will work with ImageSource rather than raw memory.

Especially with many formats and matching format being at the end of the list, we might see some performance penalties, if the CanParse methods are slow.

We already do that - and sometimes more than once, as is the case of peek_image_shape.

Yeah, so the question is, do we want to replicate this approach or not?

Since this whole thing is being redone from scratch this is the time to question assumptions made before, I think.

Regarding CanParse. You need to check if the header of the file is the one you are looking for (reading the first few bytes). I don't think there's much point in "knowing" what the file format is, because you can have a cat.jpg that is in fact a TIFF (we do have those in ImageNet). It's not until you read the header of the file that you know the format of the file.

I get that your concern is that most of the time, when a file is called "cat.jpg" is in fact a JPEG, and we could use that as a hint to try the JPEG parser first. I am not sure this is justified, when we are basically just checking one or two bytes in the header. It's equivalent to checking few bytes in the filename.

Thanks for the clarification.

I agree that generally there is not much overhead to check this. Maybe I'm just a bit skewed by the issues from video space where the only 100% solid way to check something is to decode it :)

awolant · 2022-07-04T11:33:09Z

include/dali/imgcodec/image_codec.h

+  }
+};
+
+class DLL_PUBLIC ImageCodec {


Could you explain how these will be created and kept during the pipeline or program lifetime? Is there any registry or something?

Yes, see ImageFormatRegistry. You register formats and you register codecs for those formats. The ImageCodec is used as a factory to create ImageCodecInstance. There will likely be like a codec instance cache on a higher level abstraction that will be used by the ops. This will be part of the next PR.

// Note: We will have a single format registry for DALI ImageFormatRegistry registry; ImageFormat jpeg("jpeg", std::make_shared<JpegParser>()); jpeg.RegisterCodec(std::make_shared<JpegCodec>(), 0.0f); // Inside a particular OP. We might want to abstract the logic of the cache to a separate entity (next PR) auto format = registry.GetImageFormat(image_source); for (auto codec : format.Codecs()) { if (...) { // for example limiting to host mem codecs if (codec instance in cache) { // use codec instance } else { codec_instance = codec.Create(...) // cache codec instance // use codec instance } } }

Yeah, I was asking about where ImageCodec comes from.

awolant · 2022-07-04T11:38:48Z

include/dali/imgcodec/image_source.h

+/**
+ * @brief A source of data from image parsers and codecs
+ */
+class DLL_PUBLIC ImageSource {


Can you explain a bit how ImageSource will be created? By the readers? Or output from the readers will be wrapped in the decoder?

At least initially the decoder will wrap the memory into an ImageSource by calling FromHostMem. In case of fused reader-decoders, we will likely employ other methods (most likely FromFilename).

They are created by those From* functions.

A decoder op will do FromHostMem(tensor_data, tensor_vol, tensor_source_info)

A reader op will do FromFilename(image_filename)

You mean regular reader will do FromFilename?

I meant a future fused reader that produces decoded images, as we are planning to have (for partial reading purposes)

include/dali/imgcodec/image_format.h

Signed-off-by: Joaquin Anton <janton@nvidia.com>

include/dali/imgcodec/image_format.h

Signed-off-by: Joaquin Anton <janton@nvidia.com>

banasraf · 2022-07-05T11:28:32Z

include/dali/imgcodec/image_format.h

+   * @param image
+   * @return ImageFormat*
+   */
+  ImageFormat* GetImageFormat(ImageSource *image) const;


Suggested change

ImageFormat* GetImageFormat(ImageSource *image) const;

const ImageFormat* GetImageFormat(ImageSource *image) const;

banasraf · 2022-07-05T11:34:16Z

include/dali/imgcodec/image_codec.h

@@ -0,0 +1,153 @@
+// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


Should filename be changed to image_decoder.h?

banasraf · 2022-07-05T11:36:11Z

include/dali/imgcodec/image_codec.h

+  /**
+   * @brief Creates an instance of a codec
+   *
+   * Note: For codecs that carry no state, this may just increase reference count on a singleton.


I guess with the naming ImageDecoder instead of ImageCodec, the name codec no longer has any meaning.

Suggested change

* Note: For codecs that carry no state, this may just increase reference count on a singleton.

* Note: For decoders that carry no state, this may just increase reference count on a singleton.

You might want to grep for the word codec in the PR.

stiepan · 2022-07-05T11:41:40Z

include/dali/imgcodec/image_source.h

+  /**
+   * @brief Creates an image source from data in device memory
+   */
+  static ImageSource FromDeviceMem(const void *mem, size_t size, std::string source_info = "");


I get it is not supported now, but out of curiosity: do we want to pass the dev mem without some synchronization context or is it just the ops job and we don't want it here?

That's an excellent suggestion. I think we could use AccessOrder here.

include/dali/imgcodec/image_codec.h

include/dali/imgcodec/image_format.h

Signed-off-by: Joaquin Anton <janton@nvidia.com>

mzient · 2022-07-05T14:12:17Z

dali/imgcodec/parsers/bmp.h

@@ -0,0 +1,33 @@
+


Empty line above license header?

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao · 2022-07-05T14:52:42Z

!build

dali-automaton · 2022-07-05T14:55:32Z

CI MESSAGE: [5260824]: BUILD STARTED

dali-automaton · 2022-07-05T16:10:50Z

CI MESSAGE: [5260824]: BUILD PASSED

Imgcodec module boilerplate (interfaces/placeholders/basic logic)

f4a6257

Signed-off-by: Joaquin Anton <janton@nvidia.com>

mzient reviewed Jun 30, 2022

View reviewed changes

dali/operators/decoder/nvjpeg/nvjpeg_decoder_decoupled_api.h Outdated Show resolved Hide resolved

Fix lint and revert unnecessary changes

829a268

Signed-off-by: Joaquin Anton <janton@nvidia.com>

Remove Convert prototype for now

c20b1ef

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao assigned banasraf, awolant and stiepan Jul 1, 2022

mzient reviewed Jul 1, 2022

View reviewed changes

Document image_source.h

c9530e9

Signed-off-by: Joaquin Anton <janton@nvidia.com>

awolant reviewed Jul 4, 2022

View reviewed changes

stiepan reviewed Jul 5, 2022

View reviewed changes

include/dali/imgcodec/image_format.h Outdated Show resolved Hide resolved

Code review fixes

e550226

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao force-pushed the imgcodec_boilerplate branch from 3158a75 to e550226 Compare July 5, 2022 09:03

stiepan reviewed Jul 5, 2022

View reviewed changes

include/dali/imgcodec/image_format.h Outdated Show resolved Hide resolved

More code review fixes

7d09862

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao force-pushed the imgcodec_boilerplate branch from 81f5318 to 7d09862 Compare July 5, 2022 10:34

banasraf reviewed Jul 5, 2022

View reviewed changes

banasraf approved these changes Jul 5, 2022

View reviewed changes

stiepan reviewed Jul 5, 2022

View reviewed changes

include/dali/imgcodec/image_codec.h Outdated Show resolved Hide resolved

awolant approved these changes Jul 5, 2022

View reviewed changes

stiepan reviewed Jul 5, 2022

View reviewed changes

include/dali/imgcodec/image_format.h Show resolved Hide resolved

stiepan approved these changes Jul 5, 2022

View reviewed changes

jantonguirao added 2 commits July 5, 2022 15:23

More code review fixes

9e7857b

Signed-off-by: Joaquin Anton <janton@nvidia.com>

device id and access order to FromDeviceMem

67f2d10

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao force-pushed the imgcodec_boilerplate branch from 24f78dc to 67f2d10 Compare July 5, 2022 14:11

mzient reviewed Jul 5, 2022

View reviewed changes

dali/imgcodec/parsers/bmp.h Outdated

@@ -0,0 +1,33 @@

Copy link

Contributor

mzient Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty line above license header?

Fix bmp.h

f9902c8

Signed-off-by: Joaquin Anton <janton@nvidia.com>

jantonguirao merged commit 10961ec into NVIDIA:main Jul 5, 2022

		@@ -0,0 +1,42 @@
		// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

		@@ -0,0 +1,98 @@
		// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

		@@ -0,0 +1,153 @@
		// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

		@@ -0,0 +1,94 @@
		// Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. All rights reserved.

	ImageFormat* GetImageFormat(ImageSource *image) const;
	const ImageFormat* GetImageFormat(ImageSource *image) const;

	* Note: For codecs that carry no state, this may just increase reference count on a singleton.
	* Note: For decoders that carry no state, this may just increase reference count on a singleton.

Imgcodec module boilerplate (interfaces/placeholders/basic logic) #4029

Imgcodec module boilerplate (interfaces/placeholders/basic logic) #4029

Conversation

jantonguirao commented Jun 30, 2022 • edited

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

jantonguirao commented Jun 30, 2022

dali-automaton commented Jun 30, 2022

dali-automaton commented Jun 30, 2022

jantonguirao commented Jun 30, 2022

dali-automaton commented Jun 30, 2022

dali-automaton commented Jun 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

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 Jul 4, 2022 • edited

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

jantonguirao Jul 5, 2022 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jantonguirao commented Jul 5, 2022

dali-automaton commented Jul 5, 2022

dali-automaton commented Jul 5, 2022

jantonguirao commented Jun 30, 2022 •

edited

mzient Jul 4, 2022 •

edited

jantonguirao Jul 5, 2022 •

edited