Video reader resize #2097

awolant · 2020-07-09T08:43:44Z

Why we need this PR?

It adds new feature needed because of asks from multiple sources for video resize

What happened in this PR?

What solution was applied:
Create new operator VideoReaderResize that combines reading and resizing the videos
Affected modules and functionalities:
Code around video reader and resize
Key points relevant for the review:
VideoReaderResize, refactoring of VideoReader
Validation and testing:
Added Python tests to compare with existing VideoReader and Resize ops
Documentation (including examples):
Spec for new op has docs, inherits docs from existing ops as well

JIRA TASK: [Use DALI-681]

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-07-09T08:44:06Z

!build

dali-automaton · 2020-07-09T08:45:52Z

CI MESSAGE: [1455408]: BUILD STARTED

dali-automaton · 2020-07-09T08:50:31Z

CI MESSAGE: [1455429]: BUILD STARTED

dali-automaton · 2020-07-09T08:54:07Z

CI MESSAGE: [1455408]: BUILD FAILED

dali-automaton · 2020-07-09T08:54:42Z

CI MESSAGE: [1455429]: BUILD FAILED

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-07-09T09:00:02Z

!build

dali-automaton · 2020-07-09T09:07:27Z

CI MESSAGE: [1455477]: BUILD STARTED

awolant · 2020-07-09T09:30:21Z

!build

dali-automaton · 2020-07-09T09:35:44Z

CI MESSAGE: [1455540]: BUILD STARTED

dali-automaton · 2020-07-09T11:29:17Z

CI MESSAGE: [1455477]: BUILD FAILED

JanuszL · 2020-07-09T11:36:37Z

dali/operators/reader/nvdecoder/sequencewrapper.h

+    }
+
+    frames.set_type(sequence.type());
+    frames.Resize(shape);


Maybe you can use

ShareData(void *ptr, size_t bytes, const TensorListShape<> &shape, const TypeInfo &type = {}```?

dali-automaton · 2020-07-09T11:38:48Z

CI MESSAGE: [1455540]: BUILD FAILED

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-07-09T15:23:07Z

build!

awolant · 2020-07-09T16:29:07Z

!build

dali-automaton · 2020-07-09T16:35:37Z

CI MESSAGE: [1456327]: BUILD STARTED

dali-automaton · 2020-07-09T17:57:07Z

CI MESSAGE: [1456327]: BUILD FAILED

JanuszL · 2020-07-09T20:22:46Z

dali/operators/reader/video_reader_resize_op.h

+
+namespace dali {
+
+class VideoReaderResize : public VideoReader, protected ResizeAttr, protected ResizeBase {


Inheritance or composition?
I don't think ti should inherit from ResizeAttr, not sure about ResizeBase either.

This is how Resize is built, so I wanted to follow this pattern. If we want to change, let's change both, in separate PR maybe?

JanuszL · 2020-07-09T20:27:56Z

dali/operators/reader/video_reader_resize_op.h

+    output.Resize(output_shape);
+  }
+
+  void ShareSingleOutput(int data_idx, TensorList<GPUBackend> &batch_output,


Suggested change

void ShareSingleOutput(int data_idx, TensorList<GPUBackend> &batch_output,

void SequenceToTensorList(int data_idx, TensorList<GPUBackend> &batch_output,

I'm not sure if ShareSingleOutput tells what it does.

JanuszL · 2020-07-09T20:33:36Z

dali/test/python/test_operator_video_reader_resize.py

+        for sample in range(batch_size):
+            yield [sequences_out[sample]]
+
+    gt_pipeline = dali.pipeline.Pipeline(


Maybe you can use ElementExtract to extract all frames from a sequence as a separate batches and call resize on it instead of going through ExternalSource?

Maybe I'm mistaken, but I think that writing a pipeline with ElementExtract that would be robust to changes of batch_size and sequence_length throughout the tests might be somewhat involved. You need to adjust number of outputs and calls to Resize depending on sequence_length. If you feel strongly about it, I can do it, but maybe we can leave it as it is?

I think it would depend only on the sequence_length, and you can do it in the loop. So you can compare two pipelines:
VideoReader+seq_lenElementExtract+seq_lenResize vs VideoesizeReader+seq_len*ElementExtract. But it is just an idea. Let us wait for another opinion.

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-07-09T21:08:20Z

!build

dali-automaton · 2020-07-09T21:43:16Z

CI MESSAGE: [1457164]: BUILD STARTED

dali-automaton · 2020-07-09T23:44:17Z

CI MESSAGE: [1457164]: BUILD FAILED

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-07-10T11:40:51Z

!build

dali-automaton · 2020-07-10T11:45:57Z

CI MESSAGE: [1459098]: BUILD STARTED

JanuszL · 2020-07-10T12:06:04Z

dali/test/python/test_video_reader_resize.py

+            for i in range(video_length):
+                resized_frames[i] = self.resize(resized_frames[i])


I think it should do:

Suggested change

for i in range(video_length):

resized_frames[i] = self.resize(resized_frames[i])

resized_frames = self.resize(resized_frames)

dali/test/python/test_video_reader_resize.py

Signed-off-by: Albert Wolant <awolant@nvidia.com>

dali-automaton · 2020-07-10T12:55:54Z

CI MESSAGE: [1459098]: BUILD FAILED

jantonguirao · 2020-07-10T12:43:31Z

dali/operators/image/resize/resize_crop_mirror.h

@@ -35,6 +35,14 @@ enum t_idInfo : uint32_t {
  t_mirrorVert
 };

+struct TransformMeta {
+    int H, W, C;


nitpick: non-standard indentation (we are using 2 spaces)

jantonguirao · 2020-07-10T12:54:52Z

dali/operators/reader/nvdecoder/sequencewrapper.h

+  void share_frames(TensorList<GPUBackend> &frames) {
+    void *current_sequence = sequence.raw_mutable_data();
+
+    TensorListShape<> shape;


You can:

auto shape = TensorListShape<>::make_uniform(count, frame_shape());

jantonguirao · 2020-07-10T12:56:04Z

dali/operators/image/resize/resize_crop_mirror.h

+struct TransformMeta {
+    int H, W, C;
+    int rsz_h, rsz_w;
+    std::pair<int, int> crop;


I'm not sure what this crop pair refers to. Sizes? assuming 0 anchors? I'd rather have a CropWindow instance here but you can dismiss this change as out-of-scope if you consider it so

I wanted to change existing resize related code as little as possible, so maybe let's leave it for now. AFAIK it will be heavily changed soon.

jantonguirao · 2020-07-10T12:58:13Z

dali/operators/reader/video_reader_resize_op.h

+  inline ~VideoReaderResize() override = default;
+
+ protected:
+  void SetupSharedSampleParams(DeviceWorkspace &ws) override {}


jantonguirao · 2020-07-10T13:00:30Z

dali/operators/reader/video_reader_resize_op.h

+      TensorList<GPUBackend> &video_output,
+      SequenceWrapper &prefetched_video,
+      DeviceWorkspace &ws) override {
+    std::fill_n(


Suggested change

std::fill_n(

auto params = detail::GetResamplingParams(...);

for (auto& entry : resample_params_)

entry = params;

is it equivalent?

I think so. STL version is more readable for me though, so I would like to leave it as it is, if that is ok with you?

jantonguirao · 2020-07-10T13:06:23Z

dali/test/python/test_video_reader_resize.py

+                frame = sample[frame_id]
+                gt_frame = gt_batch[frame_id].at(sample_id)
+
+                if gt_frame.shape == frame.shape:


Why not:

assert (gt_frame.shape == frame.shape), "Shapes are not equal: {} != {}".format( gt_frame.shape, frame.shape) assert (gt_frame == frame).all(), "Images are not equal"

Done. I did an if to be able to set the breakpoint inside it and look at stuff while working on it.

jantonguirao · 2020-07-10T13:06:39Z

dali/test/python/test_video_reader_resize.py

+        batch, = pipeline.run()
+        batch = batch.as_cpu()
+        gt_batch = list(gt_pipeline.run())
+


Maybe:

gt_batch = [out.as_cpu() for out in gt_pipeline.run()]

jantonguirao · 2020-07-10T13:07:03Z

dali/test/python/test_video_reader_resize.py

+    return pipeline
+
+
+def compare_video_resize_pipelines(pipeline, gt_pipeline, batch_size, video_length, iterations=16):


can't you use compare_pipelines from test_utils.py?

Not that straight forward, because gt_pipeline has different layout. It returns sequence_length outputs where n-th outputs has n-th frame for all videos.

jantonguirao · 2020-07-10T13:08:22Z

dali/test/python/test_video_reader_resize.py

+def test_video_resize(batch_size=2):
+    for vp in video_reader_params:
+        for rp in resize_params:
+            yield run_for_params, batch_size, vp, rp


how is printed when you run with nosetest?

This runs 14 separate tests. All info from vp and rp is printed. Maybe not supper pretty, but it's all there, so I guess it's fine.

banasraf · 2020-07-10T13:40:10Z

dali/operators/reader/video_reader_op.h

-      label_output->set_type(TypeInfo::Create<int>());
-      label_output->Resize(label_shape_);
+      label_output_ = &ws.Output<GPUBackend>(output_index++);
+      label_output_->set_type(TypeInfo::Create<int>());


Suggested change

label_output_->set_type(TypeInfo::Create<int>());

label_output_->set_type(TypeTable::GetTypeInfo(TypeTable::GetTypeID<int>()));

It's rather better to use type table than create a new type info.

BTW, TypeTable should definitely have a method to obtain a TypeInfo from a static type (basically those two methods in one).

Done, added this method as: TypeTable::GetTypeInfoFromStatic<int>()

banasraf · 2020-07-10T13:41:27Z

dali/operators/reader/video_reader_op.h

-        frame_num_output->set_type(TypeInfo::Create<int>());
-        frame_num_output->Resize(frame_num_shape_);
+        frame_num_output_ = &ws.Output<GPUBackend>(output_index++);
+        frame_num_output_->set_type(TypeInfo::Create<int>());


Suggested change

frame_num_output_->set_type(TypeInfo::Create<int>());

frame_num_output_->set_type(TypeTable::GetTypeInfo(TypeTable::GetTypeID<int>()));

banasraf · 2020-07-10T13:41:48Z

dali/operators/reader/video_reader_op.h

-        timestamp_output->set_type(TypeInfo::Create<double>());
-        timestamp_output->Resize(timestamp_shape_);
+        timestamp_output_ = &ws.Output<GPUBackend>(output_index++);
+        timestamp_output_->set_type(TypeInfo::Create<double>());


Suggested change

timestamp_output_->set_type(TypeInfo::Create<double>());

timestamp_output_->set_type(TypeTable::GetTypeInfo(TypeTable::GetTypeID<double>()));

banasraf · 2020-07-10T14:03:08Z

dali/operators/reader/video_reader_op.h

    if (dtype_ == DALI_FLOAT) {
-      tl_sequence_output.set_type(TypeInfo::Create<float>());
+      output.set_type(TypeInfo::Create<float>());


here, also GetTypeInfo

banasraf · 2020-07-10T14:03:25Z

dali/operators/reader/video_reader_op.h

    } else {  // dtype_ == DALI_UINT8
-      tl_sequence_output.set_type(TypeInfo::Create<uint8>());
+      output.set_type(TypeInfo::Create<uint8>());


and here, GetTypeInfo

banasraf

Ok, with minor comments

dali-automaton · 2020-07-10T14:42:08Z

CI MESSAGE: [1459098]: BUILD PASSED

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant · 2020-07-10T14:57:04Z

!build

dali-automaton · 2020-07-10T15:05:48Z

CI MESSAGE: [1459445]: BUILD STARTED

dali-automaton · 2020-07-10T17:03:28Z

CI MESSAGE: [1459445]: BUILD PASSED

awolant added 7 commits July 2, 2020 16:09

First working version

39f5b2f

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Resize parameters

1b1d9a6

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Extract VideoReaderResize

3bd4095

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Working per video resize

6beabc8

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Refactored

233e1c5

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Fix lint

448bf0c

Signed-off-by: Albert Wolant <awolant@nvidia.com>

More refactoring

4a99081

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant changed the title ~~Video reader resize~~ [WIP] Video reader resize Jul 9, 2020

Fix lint, again

b0f1b0f

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant changed the title ~~[WIP] Video reader resize~~ Video reader resize Jul 9, 2020

awolant requested a review from a team July 9, 2020 09:31

JanuszL reviewed Jul 9, 2020

View reviewed changes

awolant added 2 commits July 9, 2020 13:58

Merge remote-tracking branch 'nvidia/master' into video_reader_resize

e8ea62f

Review, more tests

e7d5513

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant force-pushed the video_reader_resize branch from 99db416 to e7d5513 Compare July 9, 2020 15:18

JanuszL reviewed Jul 9, 2020

View reviewed changes

Fix for review

820ae28

Signed-off-by: Albert Wolant <awolant@nvidia.com>

awolant added 2 commits July 10, 2020 08:45

Rename test

c3c395c

Signed-off-by: Albert Wolant <awolant@nvidia.com>

Change test to ElementExtract

107f38f

Signed-off-by: Albert Wolant <awolant@nvidia.com>

JanuszL reviewed Jul 10, 2020

View reviewed changes

dali/test/python/test_video_reader_resize.py Show resolved Hide resolved

More review fixes

50158a6

Signed-off-by: Albert Wolant <awolant@nvidia.com>

JanuszL approved these changes Jul 10, 2020

View reviewed changes

jantonguirao reviewed Jul 10, 2020

View reviewed changes

banasraf reviewed Jul 10, 2020

View reviewed changes

banasraf approved these changes Jul 10, 2020

View reviewed changes

More review fixes

60ea8d6

Signed-off-by: Albert Wolant <awolant@nvidia.com>

jantonguirao approved these changes Jul 10, 2020

View reviewed changes

awolant merged commit ada6f49 into NVIDIA:master Jul 10, 2020


		namespace dali {

		class VideoReaderResize : public VideoReader, protected ResizeAttr, protected ResizeBase {

	void ShareSingleOutput(int data_idx, TensorList<GPUBackend> &batch_output,
	void SequenceToTensorList(int data_idx, TensorList<GPUBackend> &batch_output,

		for i in range(video_length):
		resized_frames[i] = self.resize(resized_frames[i])

	for i in range(video_length):
	resized_frames[i] = self.resize(resized_frames[i])
	resized_frames = self.resize(resized_frames)

-    std::fill_n(
+  auto params = detail::GetResamplingParams(...);
+  for (auto& entry : resample_params_)
+    entry = params;

		return pipeline


		def compare_video_resize_pipelines(pipeline, gt_pipeline, batch_size, video_length, iterations=16):

	label_output_->set_type(TypeInfo::Create<int>());
	label_output_->set_type(TypeTable::GetTypeInfo(TypeTable::GetTypeID<int>()));

	frame_num_output_->set_type(TypeInfo::Create<int>());
	frame_num_output_->set_type(TypeTable::GetTypeInfo(TypeTable::GetTypeID<int>()));

	timestamp_output_->set_type(TypeInfo::Create<double>());
	timestamp_output_->set_type(TypeTable::GetTypeInfo(TypeTable::GetTypeID<double>()));

Video reader resize #2097

Video reader resize #2097

Conversation

awolant commented Jul 9, 2020 • edited Loading

Why we need this PR?

What happened in this PR?

awolant commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

awolant commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

awolant commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Jul 9, 2020

awolant commented Jul 9, 2020

awolant commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

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

awolant commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

dali-automaton commented Jul 9, 2020

awolant commented Jul 10, 2020

dali-automaton commented Jul 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Jul 10, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

banasraf Jul 10, 2020 • 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

banasraf left a comment

Choose a reason for hiding this comment

dali-automaton commented Jul 10, 2020

awolant commented Jul 10, 2020

dali-automaton commented Jul 10, 2020

dali-automaton commented Jul 10, 2020

awolant commented Jul 9, 2020 •

edited

Loading

banasraf Jul 10, 2020 •

edited

Loading