Adds `enable_frame_num` to the experimental video reader #5628

JanuszL · 2024-09-09T14:30:16Z

adds an ability to output frame numbers in the experimental video
reader
this allows making VideoReaderDecoderCpuTest.RandomShuffle_* test
independent from the hardcoding of the expected frame order (in case
random generator implementation changes)

Category:

New feature (non-breaking change which adds functionality)

Description:

adds an ability to output frame numbers in the experimental video
reader
this allows making VideoReaderDecoderCpuTest.RandomShuffle_* test
independent from the hardcoding of the expected frame order (in case
random generator implementation changes)
needed for Moves to manylinux_2_28 #5608 as the random generator output changes with the manylinux change

Additional information:

Affected modules and functionalities:

experimental video reader

Key points relevant for the review:

NA

Tests:

Checklist

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: RDVID.13

JIRA TASK: N/A

- adds an ability to output frame numbers in the experimental video reader - this allows making VideoReaderDecoderCpuTest.RandomShuffle_* test independent from the hardcoding of the expected frame order (in case random generator implementation changes) Signed-off-by: Janusz Lisiecki <[email protected]>

JanuszL · 2024-09-09T14:30:22Z

!build

dali-automaton · 2024-09-09T14:35:52Z

CI MESSAGE: [18262573]: BUILD STARTED

dali-automaton · 2024-09-09T16:15:10Z

CI MESSAGE: [18262564]: BUILD FAILED

dali-automaton · 2024-09-09T16:15:20Z

CI MESSAGE: [18262573]: BUILD FAILED

mzient · 2024-09-10T09:31:30Z

dali/operators/reader/loader/video/video_loader_decoder_base.h

  int label_;
+  int first_frame_;


Since VideoSample already has a non-trivial constructor, some initialization (e.g. to invalid values, like -1) would be nice.

Signed-off-by: Janusz Lisiecki <[email protected]>

dali-automaton · 2024-09-10T10:04:34Z

CI MESSAGE: [18262573]: BUILD PASSED

mzient · 2024-09-10T10:11:05Z

dali/operators/reader/video_reader_decoder_cpu_op.cc

+  int num_outputs = 1;
+  if (spec.HasArgument("labels")) num_outputs++;
+  bool enable_frame_num = spec.GetArgument<bool>("enable_frame_num");
+  if (enable_frame_num) num_outputs++;
+  return num_outputs;


Perhaps clearer (?) Your call.

Suggested change

int num_outputs = 1;

if (spec.HasArgument("labels")) num_outputs++;

bool enable_frame_num = spec.GetArgument<bool>("enable_frame_num");

if (enable_frame_num) num_outputs++;

return num_outputs;

bool has_label_output = spec.HasArgument("labels");

bool has_frame_num_output = spec.GetArgument<bool>("enable_frame_num");

return 1 + has_label_output + has_frame_num_output;

mzient · 2024-09-10T10:12:09Z

dali/operators/reader/video_reader_decoder_cpu_op.cc

+      R"code(If set, returns the first frame number in the decoded sequence
+as a separate output.)code",


Suggested change

R"code(If set, returns the first frame number in the decoded sequence

as a separate output.)code",

R"code(If set, returns the index of the first frame in the decoded sequence

as an additional output.)code",

mzient · 2024-09-10T10:13:27Z

dali/operators/reader/video_reader_decoder_cpu_op.cc

@@ -68,6 +81,10 @@ even in the variable frame rate scenario.)code")
  .AddArg("sequence_length",
      R"code(Frames to load per sequence.)code",
      DALI_INT32)
+  .AddOptionalArg("enable_frame_num",


Suggested change

.AddOptionalArg("enable_frame_num",

.AddOptionalArg("enable_frame_idx",

?

I want to keep it consistent with the video reader .

I understand and agree - nonetheless, it's unfortunate.

mzient · 2024-09-10T10:15:08Z

dali/operators/reader/video_reader_decoder_op_test.cc

@@ -168,6 +172,15 @@ void VideoReaderDecoderBaseTest::RunShuffleTest<dali::CPUBackend>() {
    RunShuffleTestImpl<dali::CPUBackend>("cpu", dali::CPU_ONLY_DEVICE_ID);
 }

+template<>
+int VideoReaderDecoderBaseTest::GetFrameNo(


Suggested change

int VideoReaderDecoderBaseTest::GetFrameNo(

int VideoReaderDecoderBaseTest::GetFrameIdx(

...if going with the idx suggestion.

mzient · 2024-09-10T10:15:36Z

dali/operators/reader/video_reader_decoder_op_test.cc

@@ -181,6 +194,15 @@ void VideoReaderDecoderBaseTest::RunShuffleTest<dali::GPUBackend>() {
    RunShuffleTestImpl<dali::GPUBackend>("gpu", 0);
 }

+template<>
+int VideoReaderDecoderBaseTest::GetFrameNo(


Suggested change

int VideoReaderDecoderBaseTest::GetFrameNo(

int VideoReaderDecoderBaseTest::GetFrameIdx(

mzient · 2024-09-10T10:15:52Z

dali/operators/reader/video_reader_decoder_op_test.cc

@@ -40,6 +40,9 @@ class VideoReaderDecoderBaseTest : public VideoTestBase {
  virtual void AssertFrame(
    int frame_id, const uint8_t *frame, TestVideo &ground_truth) = 0;

+  template<typename Backend>
+  int GetFrameNo(dali::TensorList<Backend> &device_frame_no);


Suggested change

int GetFrameNo(dali::TensorList<Backend> &device_frame_no);

int GetFrameIdx(dali::TensorList<Backend> &device_frame_idx);

mzient · 2024-09-10T10:15:59Z

dali/operators/reader/video_reader_decoder_gpu_op.h

@@ -35,6 +35,7 @@ class VideoReaderDecoderGpu : public DataReader<GPUBackend, VideoSampleGpu, Vide

 private:
  bool has_labels_ = false;
+  bool has_frame_no_ = false;


Suggested change

bool has_frame_no_ = false;

bool has_frame_idx_ = false;

Signed-off-by: Janusz Lisiecki <[email protected]>

JanuszL · 2024-09-10T10:43:41Z

!build

dali-automaton · 2024-09-10T12:15:21Z

CI MESSAGE: [18290753]: BUILD FAILED

dali-automaton · 2024-09-10T14:37:49Z

CI MESSAGE: [18290753]: BUILD STARTED

Signed-off-by: Janusz Lisiecki <[email protected]>

JanuszL · 2024-09-10T18:21:46Z

!build

dali-automaton · 2024-09-10T18:26:39Z

CI MESSAGE: [18302585]: BUILD STARTED

dali-automaton · 2024-09-10T20:17:12Z

CI MESSAGE: [18302585]: BUILD PASSED

mzient reviewed Sep 10, 2024

View reviewed changes

Review fix

f621fe6

Signed-off-by: Janusz Lisiecki <[email protected]>

mzient self-assigned this Sep 10, 2024

mzient reviewed Sep 10, 2024

View reviewed changes

dali-automaton assigned jantonguirao Sep 10, 2024

mzient reviewed Sep 10, 2024

View reviewed changes

Review fixes

ec7fa68

Signed-off-by: Janusz Lisiecki <[email protected]>

JanuszL force-pushed the frame_no_exp_reader branch from e2afd19 to ec7fa68 Compare September 10, 2024 10:37

More review fixed

b969f7b

Signed-off-by: Janusz Lisiecki <[email protected]>

mzient approved these changes Sep 10, 2024

View reviewed changes

jantonguirao approved these changes Sep 10, 2024

View reviewed changes

Fix

a8d08cb

Signed-off-by: Janusz Lisiecki <[email protected]>

JanuszL merged commit 4b1833a into NVIDIA:main Sep 11, 2024
7 checks passed

JanuszL deleted the frame_no_exp_reader branch September 11, 2024 05:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds `enable_frame_num` to the experimental video reader #5628

Adds `enable_frame_num` to the experimental video reader #5628

JanuszL commented Sep 9, 2024 •

edited

Loading

JanuszL commented Sep 9, 2024

dali-automaton commented Sep 9, 2024

dali-automaton commented Sep 9, 2024

dali-automaton commented Sep 9, 2024

mzient Sep 10, 2024

JanuszL Sep 10, 2024

dali-automaton commented Sep 10, 2024

mzient Sep 10, 2024

JanuszL Sep 10, 2024

mzient Sep 10, 2024

JanuszL Sep 10, 2024

mzient Sep 10, 2024

JanuszL Sep 10, 2024

mzient Sep 10, 2024

mzient Sep 10, 2024

JanuszL Sep 10, 2024

mzient Sep 10, 2024

JanuszL Sep 10, 2024

mzient Sep 10, 2024

JanuszL Sep 10, 2024

mzient Sep 10, 2024

JanuszL Sep 10, 2024

JanuszL commented Sep 10, 2024

dali-automaton commented Sep 10, 2024

dali-automaton commented Sep 10, 2024

JanuszL commented Sep 10, 2024

dali-automaton commented Sep 10, 2024

dali-automaton commented Sep 10, 2024

		R"code(If set, returns the first frame number in the decoded sequence
		as a separate output.)code",

	.AddOptionalArg("enable_frame_num",
	.AddOptionalArg("enable_frame_idx",

	int VideoReaderDecoderBaseTest::GetFrameNo(
	int VideoReaderDecoderBaseTest::GetFrameIdx(

	int GetFrameNo(dali::TensorList<Backend> &device_frame_no);
	int GetFrameIdx(dali::TensorList<Backend> &device_frame_idx);

Adds enable_frame_num to the experimental video reader #5628

Adds enable_frame_num to the experimental video reader #5628

Conversation

JanuszL commented Sep 9, 2024 • edited Loading

Category:

Description:

Additional information:

Affected modules and functionalities:

Key points relevant for the review:

Tests:

Checklist

Documentation

DALI team only

Requirements

JanuszL commented Sep 9, 2024

dali-automaton commented Sep 9, 2024

dali-automaton commented Sep 9, 2024

dali-automaton commented Sep 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dali-automaton commented Sep 10, 2024

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

JanuszL commented Sep 10, 2024

dali-automaton commented Sep 10, 2024

dali-automaton commented Sep 10, 2024

JanuszL commented Sep 10, 2024

dali-automaton commented Sep 10, 2024

dali-automaton commented Sep 10, 2024

Adds `enable_frame_num` to the experimental video reader #5628

Adds `enable_frame_num` to the experimental video reader #5628

JanuszL commented Sep 9, 2024 •

edited

Loading