Mitigate the OS file-max limit in the VideoReader #1659

JanuszL · 2020-01-14T19:53:31Z

uses the custom read-write-seek libav API that allows providing custom accessing function. This function opens the required file on demand. The previously accessed file is closed automatically.
it is faster than calling avformat_open_input after each file reopen
add test to check if DALI can handle more than 1024 video files at once

Signed-off-by: Janusz Lisiecki jlisiecki@nvidia.com

Why we need this PR?

Pick one, remove the rest

It adds new feature needed because of VideoReader can easily hit the OS file-max limit of opened files (usually 1024)

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

What solution was applied:
Uses the custom read-write-seek libav API that allows providing custom accessing function. This function opens the required file on demand. The previously accessed file is closed automatically.
It is faster than calling avformat_open_input after each file reopen
Affected modules and functionalities:
VideoReader
Key points relevant for the review:
get_or_open_file function
Validation and testing:
Test is added
Documentation (including examples):
NA

Related to #1651

JIRA TASK: []

JanuszL · 2020-01-14T19:53:38Z

!build

dali-automaton · 2020-01-14T19:55:15Z

CI MESSAGE: [1072512]: BUILD STARTED

dali-automaton · 2020-01-14T20:49:57Z

CI MESSAGE: [1072512]: BUILD PASSED

JanuszL · 2020-01-14T23:53:10Z

dali/operators/reader/loader/video_loader.h

@@ -295,6 +297,8 @@ class VideoLoader : public Loader<GPUBackend, SequenceWrapper> {

  volatile bool stop_;
  std::vector<file_meta> file_info_;
+
+  constexpr static size_t MAX_OPEN_FILE = 200;


This is an educated guess limit, but maybe it should be just 1/5 of /proc/sys/fs/file-max or have some global counter. What if user opens files in the same process from python? Or spawns more than one VideoReader?
Any suggestion?

Maybe we can have it as an argument to the reader so that the user has control of it. Also, I'd help with testing this

Maybe recently_opened_ and open_files_ should be global to all VideoLoaders - so the limit is global as well. If we have it per operator then I see someone creating multiple VideoReader with the limits that sum up above the OS limit and that leads to a crash.

On the other hand we need to have some locking and this can influence the overall perf.

jantonguirao · 2020-01-15T10:00:14Z

dali/operators/reader/loader/video_loader.h

@@ -295,6 +297,8 @@ class VideoLoader : public Loader<GPUBackend, SequenceWrapper> {

  volatile bool stop_;
  std::vector<file_meta> file_info_;
+
+  constexpr static size_t MAX_OPEN_FILE = 200;


Maybe we can have it as an argument to the reader so that the user has control of it. Also, I'd help with testing this

jantonguirao · 2020-01-15T10:01:06Z

dali/test/python/test_video_pipeline.py

@@ -117,3 +121,13 @@ def test_multi_gpu_video_pipeline():
    for p in pipes:
        p.build()
        p.run()
+
+# checks if the VideoReader can handle more than OS max open file limit of opened files at once
+def test_plenty_video_files():


Suggested change

def test_plenty_video_files():

def test_plenty_of_video_files():

jantonguirao · 2020-01-15T10:02:44Z

dali/test/python/test_video_pipeline.py

+# checks if the VideoReader can handle more than OS max open file limit of opened files at once
+def test_plenty_video_files():
+    # make sure that there is one sequence per video file
+    pipe = VideoPipe(batch_size=BATCH_SIZE, data=PLENTY_VIDEO_FILES, step=1000000, sequence_length=1)


if you incorporate my suggestion of having a configurable max number of open video files then you could test this easily by setting a small number

Not really. This step is set to make sure we just read one frame per each video as we just want to traverse through all videos. Having a configurable max number won't help either as the real limit is the OS. All we need to test if the limit set inside DALI is smaller than the OS one it works (and it doesn't matter if it is OS limit - 1 or just 1).

jantonguirao · 2020-01-15T10:04:47Z

dali/operators/reader/loader/video_loader.h

@@ -295,6 +297,8 @@ class VideoLoader : public Loader<GPUBackend, SequenceWrapper> {

  volatile bool stop_;
  std::vector<file_meta> file_info_;
+
+  constexpr static size_t MAX_OPEN_FILE = 200;


Suggested change

constexpr static size_t MAX_OPEN_FILE = 200;

constexpr static size_t kMaxOpenFiles = 200;

jantonguirao · 2020-01-15T10:07:04Z

dali/operators/reader/loader/video_loader.cc

+    recently_opened_.push_back(filename);
+    if (recently_opened_.size() > MAX_OPEN_FILE) {
+      // close the oldest file
+      auto& file_to_close = open_files_[recently_opened_.front()];


Shouldn't you remove the entry from open_files_ as well?

No. It is enough to file_to_close.open = false.

mzient · 2020-01-15T11:51:47Z

dali/operators/reader/loader/video_loader.cc

@@ -24,7 +24,8 @@
 #include <limits>
 #include <sstream>

-inline int gcd(int a, int b) {
+
+static inline int gcd(int a, int b) {


Why? An inline function has internal linkage, no need for static.

- introduces LRU to close opened VideoFiles just to prevent hitting the OS file-max limit - when the number of the opened files crosses the threshold the oldest one is closed Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2020-01-17T17:50:44Z

!build

dali-automaton · 2020-01-17T17:55:27Z

CI MESSAGE: [1078926]: BUILD STARTED

dali-automaton · 2020-01-17T17:59:32Z

CI MESSAGE: [1078926]: BUILD FAILED

mzient · 2020-01-17T18:39:16Z

dali/operators/reader/loader/video_loader.cc

+  if (ret == 0) {
+    return ftell(file_data->file_stream);
+  } else {
+    return ret;


This is ambiguous - the standard doesn't say that the value returned on error is negative. Perhaps we should throw?

It is not the standard. It is what FFmpeg expects, and expects some error code which is < 0.

I meant that fseek does not guarantee a negative value on error - just nonzero. You probably should return -1 or -abs(err) to make sure the value is <0.

mzient · 2020-01-17T18:50:19Z

dali/operators/reader/loader/video_loader.h

+    long int file_position;
+    std::string filename;
+  };
+  VideoFileData file_desc;


Nitpick: why not VideoFileDesc or file_data?

mzient · 2020-01-17T18:51:01Z

dali/operators/reader/loader/video_loader.h

@@ -295,6 +304,8 @@ class VideoLoader : public Loader<GPUBackend, SequenceWrapper> {

  volatile bool stop_;
  std::vector<file_meta> file_info_;
+
+  constexpr static size_t kMaxOpenFiles = 200;


mzient · 2020-01-17T18:53:19Z

qa/TL0_videoreader_test/test.sh

+  #create dummy files for max_opened_file_test
+  for i in {0..1400};
+  do
+      ln -s ../video_files/${split[0]}_0.${split[1]} many_video_files/${split[0]}_$i.${split[1]}


Can we create these in /tmp perhaps?

JanuszL · 2020-01-18T03:19:51Z

!build

dali-automaton · 2020-01-18T03:25:33Z

CI MESSAGE: [1079892]: BUILD STARTED

dali-automaton · 2020-01-18T04:15:51Z

CI MESSAGE: [1079892]: BUILD PASSED

mzient · 2020-01-23T08:31:13Z

dali/operators/reader/loader/video_loader.cc

    LOG_LINE << "Opening file " << filename << std::endl;

-    AVFormatContext* raw_fmt_ctx = nullptr;
-    int ret = avformat_open_input(&raw_fmt_ctx, filename.c_str(), NULL, NULL);
+    AVFormatContext* raw_fmt_ctx = avformat_alloc_context();


I'd create a temporary unique pointer with make_unique_av here to avoid leaking resources on failure.

mzient · 2020-01-23T08:31:25Z

dali/operators/reader/loader/video_loader.cc

+    size_t avio_ctx_buffer_size = 4096;
+    uint8_t *avio_ctx_buffer = static_cast<uint8_t *>(av_malloc(avio_ctx_buffer_size));
+    if (!avio_ctx_buffer) {
+      DALI_FAIL("Failed to allocate avio_ctx_buffer");


raw_fmt_ctx is leaked here.

mzient · 2020-01-23T08:31:42Z

dali/operators/reader/loader/video_loader.cc

+      DALI_FAIL("Failed to allocate avio_alloc_context");
+    }
+    raw_fmt_ctx->pb = avio_ctx;
+    int ret = avformat_open_input(&raw_fmt_ctx, NULL, NULL, NULL);
    if (ret < 0) {
      DALI_FAIL(std::string("Could not open file ") + filename + " because of " + av_err2str(ret));


...and also here.

No, avformat_open_input frees avio_ctx_buffer in case of failure.

mzient · 2020-01-23T08:47:54Z

dali/operators/reader/loader/video_loader.h

@@ -73,7 +74,7 @@ std::vector<dali::file_meta> get_file_label_pair(const std::string& path,
 }  // namespace filesystem

 struct OpenFile {
-  bool open = false;
+  bool loaded = false;


I don't like this new name. The file is not open - sure, but much less is it loaded. Perhaps we could use one of the other fields to determine if the descriptor is empty?

bool empty() const noexcept { return file_desc.filename.empty(); }

mzient · 2020-01-23T08:58:35Z

dali/operators/reader/loader/video_loader.h

@@ -73,7 +74,7 @@ std::vector<dali::file_meta> get_file_label_pair(const std::string& path,
 }  // namespace filesystem

 struct OpenFile {


I'd rename this to VideoFile or similar, since it no longer represents an open file.

mzient · 2020-01-23T09:00:48Z

dali/operators/reader/loader/video_loader.h

@@ -95,6 +96,13 @@ struct OpenFile {
  AVCodecContext* codec;
 #endif
  av_unique_ptr<AVFormatContext> fmt_ctx_;
+
+  struct VideoFileDesc {
+    FILE *file_stream;


I suggest adding a destructor which would close those streams - and a close function. It may be either here (in which case I'd suggest making this struct standalone) or in the enclosing class.

dali/operators/reader/loader/video_loader.cc

jantonguirao · 2020-01-23T09:55:03Z

dali/operators/reader/loader/video_loader.cc

+  // open file if needed
+  if (!file_data->file_stream) {
+    file_data->file_stream = fopen(file_data->filename.c_str(), "r");
+    if (!file_data->file_stream) {


This could be a DALI_ENFORCE

jantonguirao · 2020-01-23T09:55:24Z

dali/operators/reader/loader/video_loader.cc

+    }
+    size_t avio_ctx_buffer_size = 4096;
+    uint8_t *avio_ctx_buffer = static_cast<uint8_t *>(av_malloc(avio_ctx_buffer_size));
+    if (!avio_ctx_buffer) {


could be a DALI_ENFORCE

jantonguirao · 2020-01-23T09:55:31Z

dali/operators/reader/loader/video_loader.cc

-    AVFormatContext* raw_fmt_ctx = nullptr;
-    int ret = avformat_open_input(&raw_fmt_ctx, filename.c_str(), NULL, NULL);
+    AVFormatContext* raw_fmt_ctx = avformat_alloc_context();
+    if (!raw_fmt_ctx) {


could be a DALI_ENFORCE

jantonguirao · 2020-01-23T09:55:50Z

dali/operators/reader/loader/video_loader.cc

+      DALI_FAIL("Failed to allocate avio_alloc_context");
+    }
+    raw_fmt_ctx->pb = avio_ctx;
+    int ret = avformat_open_input(&raw_fmt_ctx, NULL, NULL, NULL);
    if (ret < 0) {


JanuszL · 2020-01-23T13:32:26Z

!build

dali-automaton · 2020-01-23T13:35:53Z

CI MESSAGE: [1087535]: BUILD STARTED

dali-automaton · 2020-01-23T15:37:15Z

CI MESSAGE: [1087535]: BUILD PASSED

mzient · 2020-01-23T15:56:42Z

dali/operators/reader/loader/video_loader.h

-struct OpenFile {
-  bool open = false;
+struct VideoFileDesc {
+  FILE *file_stream;


Suggested change

FILE *file_stream;

FILE *file_stream = nullptr;

JanuszL · 2020-01-23T16:04:02Z

!build

dali-automaton · 2020-01-23T16:05:25Z

CI MESSAGE: [1087720]: BUILD STARTED

dali-automaton · 2020-01-23T16:54:18Z

CI MESSAGE: [1087720]: BUILD PASSED

jantonguirao · 2020-01-27T11:15:01Z

dali/operators/reader/loader/video_loader.cc

+  // open file if needed
+  if (!file_data->file_stream) {
+    file_data->file_stream = fopen(file_data->filename.c_str(), "r");
+    DALI_ENFORCE(file_data->file_stream, std::string("Could not open file ") + file_data->filename);


nitpick: you could use make_string("Could not open the file: ", file_data->filename)

jantonguirao · 2020-01-27T11:16:12Z

dali/operators/reader/loader/video_loader.cc

+    DALI_ENFORCE(raw_fmt_ctx, "Failed to allocate avformat_alloc_context");
+
+    size_t avio_ctx_buffer_size = 4096;
+    uint8_t *avio_ctx_buffer = static_cast<uint8_t *>(av_malloc(avio_ctx_buffer_size));


would be good to wrap this one with a unique_ptr

- uses the custom read-write-seek libav API that allows providing custom accessing function. This function opens the required file on demand. The previously accessed file is closed automatically. - it is faster than calling avformat_open_input after each file reopen - adds test to check if DALI can handle more than 1024 video files at once - moves generated video test files into /tmp/ dir Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

JanuszL · 2020-01-27T12:50:14Z

!build

dali-automaton · 2020-01-27T12:55:34Z

CI MESSAGE: [1093065]: BUILD STARTED

dali-automaton · 2020-01-27T14:40:59Z

CI MESSAGE: [1093065]: BUILD PASSED

JanuszL requested review from jantonguirao, szalpal, klecki, mzient and banasraf January 14, 2020 19:53

JanuszL commented Jan 14, 2020

View reviewed changes

jantonguirao reviewed Jan 15, 2020

View reviewed changes

mzient reviewed Jan 15, 2020

View reviewed changes

Mitigate the OS file-max limit in the VideoReader

d44ef02

- introduces LRU to close opened VideoFiles just to prevent hitting the OS file-max limit - when the number of the opened files crosses the threshold the oldest one is closed Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

mzient reviewed Jan 17, 2020

View reviewed changes

JanuszL mentioned this pull request Jan 21, 2020

ops.VideoReader: Too Many Opened Files #1651

Closed

mzient reviewed Jan 23, 2020

View reviewed changes

jantonguirao approved these changes Jan 23, 2020

View reviewed changes

jantonguirao self-requested a review January 23, 2020 10:01

mzient reviewed Jan 23, 2020

View reviewed changes

mzient approved these changes Jan 23, 2020

View reviewed changes

jantonguirao approved these changes Jan 27, 2020

View reviewed changes

JanuszL merged commit 2b06c0f into NVIDIA:master Jan 27, 2020

JanuszL deleted the mitigate_vid_file_limit branch January 27, 2020 15:07

JanuszL mentioned this pull request Mar 12, 2020

Trouble loading UCF-101 using the pip-installed version of dali #1811

Closed

	def test_plenty_video_files():
	def test_plenty_of_video_files():

	constexpr static size_t MAX_OPEN_FILE = 200;
	constexpr static size_t kMaxOpenFiles = 200;

		@@ -73,7 +74,7 @@ std::vector<dali::file_meta> get_file_label_pair(const std::string& path,
		} // namespace filesystem

		struct OpenFile {

Mitigate the OS file-max limit in the VideoReader #1659

Mitigate the OS file-max limit in the VideoReader #1659

Conversation

JanuszL commented Jan 14, 2020 • edited Loading

Why we need this PR?

What happened in this PR?

JanuszL commented Jan 14, 2020

dali-automaton commented Jan 14, 2020

dali-automaton commented Jan 14, 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

JanuszL commented Jan 17, 2020

dali-automaton commented Jan 17, 2020

dali-automaton commented Jan 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient Jan 23, 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

JanuszL commented Jan 18, 2020

dali-automaton commented Jan 18, 2020

dali-automaton commented Jan 18, 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

mzient Jan 23, 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

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 Jan 23, 2020

dali-automaton commented Jan 23, 2020

dali-automaton commented Jan 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JanuszL commented Jan 23, 2020

dali-automaton commented Jan 23, 2020

dali-automaton commented Jan 23, 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

JanuszL commented Jan 27, 2020

dali-automaton commented Jan 27, 2020

dali-automaton commented Jan 27, 2020

JanuszL commented Jan 14, 2020 •

edited

Loading

mzient Jan 23, 2020 •

edited

Loading

mzient Jan 23, 2020 •

edited

Loading