Fix HEVC video handling #3247

JanuszL · 2021-08-12T09:54:17Z

it seems that preceding frames may appear after the following
keyframe so the logic that stops sending frames to the decoder
assuming that each keyframe is IDR doesn't work anymore. Improve
the logic to stop after seeing the next keyframe as it should be
very unlikely to have a frame from two keyframes behind
adds HEVC test

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

Description

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactoring (Redesign of existing code that doesn't affect functionality)
Other (e.g. Documentation, Tests, Configuration)

What happened in this PR

it seems that preceding frames may appear after the following
keyframe so the logic that stops sending frames to the decoder
assuming that each keyframe is IDR doesn't work anymore. Improve
the logic to stop after seeing the next keyframe as it should be
very unlikely to have a frame from two keyframes behind

Additional information

Affected modules and functionalities:
- video_loader.cc
- video_reader_op_test.cc

Key points relevant for the review:
- NA

Checklist

Tests

Documentation

DALI team only

Requirements

Implements new requirements
Affects existing requirements
N/A

REQ IDs: N/A

Relates to #3243
JIRA TASK: DALI-2243

dali-automaton · 2021-08-12T10:01:48Z

CI MESSAGE: [2749122]: BUILD STARTED

dali-automaton · 2021-08-12T10:10:59Z

CI MESSAGE: [2749122]: BUILD FAILED

dali-automaton · 2021-08-12T10:14:39Z

CI MESSAGE: [2749188]: BUILD STARTED

dali-automaton · 2021-08-12T11:26:30Z

CI MESSAGE: [2749188]: BUILD PASSED

JanuszL · 2021-08-12T12:15:41Z

NVIDIA/DALI_extra#65 is a prerequisite.

klecki

I think some counter how many key frames we've seen and the required number as a constant would feel more natural.

Also, doesn't this impact performance? Shouldn't we do this based on the format that we are dealing with?

JanuszL · 2021-08-13T18:08:10Z

I think some counter how many key frames we've seen and the required number as a constant would feel more natural.

I can add it.

Also, doesn't this impact performance?

It may but I don't see any other way to fix that.

Shouldn't we do this based on the format that we are dealing with?

Probably but I don't have sufficient knowledge which one needs that.

JanuszL · 2021-08-13T18:12:27Z

Anyway done.

dali-automaton · 2021-08-13T18:14:15Z

CI MESSAGE: [2760006]: BUILD STARTED

klecki

I'm not sure if I understand this correctly. Is the req.count the distance between keyframes? If so, there is a bug and we can simplify.

klecki · 2021-08-13T18:16:42Z

dali/operators/reader/loader/video_loader.cc

@@ -533,6 +533,11 @@ void VideoLoader::read_file() {
    bool is_first_frame = true;
    bool key = false;
    bool seek_must_succeed = false;
+    // how many key frames following the last requested frames we need to see so far


Suggested change

// how many key frames following the last requested frames we need to see so far

// how many key frames following the last requested frames we saw so far

klecki · 2021-08-13T18:17:35Z

dali/operators/reader/loader/video_loader.cc

+    int key_frames_count = 0;
+    // how many key frames following the last requested frames we need to see before we stop
+    // feeding the decoder
+    const int key_frames_treshold = 1;


IMO we want to see two, so I would probably write here 2, and use greater equal, but it's a nitpick:

Suggested change

const int key_frames_treshold = 1;

const int key_frames_treshold = 2;

klecki · 2021-08-13T18:24:22Z

dali/operators/reader/loader/video_loader.cc

@@ -585,10 +590,16 @@ void VideoLoader::read_file() {
          if (!is_first_frame) {
            nonkey_frame_count = 0;
            if (frame > req.frame + req.count) {


Shouldn't we check for:

Suggested change

if (frame > req.frame + req.count) {

if (frame > req.frame + (1 + key_frames_count) * req.count) {

or something?
I mean, if key frame is every req.count (just guessing), we need to see the multiple of the distance to detect key frame.

No, keyframe is the video stream property and it doesn't have to be equally placed in the video. req.count is how many frames we need to decode to between first and the last frame in the sequence.

klecki · 2021-08-13T18:25:36Z

dali/operators/reader/loader/video_loader.cc

            if (frame > req.frame + req.count) {
-              // Found a key frame past the requested range. We can stop searching
+              // Found a second key frame past the requested range. We can stop searching
              // (If there were missing frames in the range they won't be found after
              // the next key frame)
-              break;
+              // in case HEVC it seems that preceding frames can appear after a given key frame
+              // but rather not after the next one
+              if (key_frames_count > key_frames_treshold) {
+                key_frames_count = 0;
+                break;
+              }
+              ++key_frames_count;
            }


If my comment is right, this can be replaced with (note the 2):

const int key_frames_treshold = 2; if (frame > req.frame + key_frames_treshold * req.count) { break; }

Which is probably simpler.

key_frames_treshold * req.count doesn't make any sense. req.count has nothing to do with keyframes.

JanuszL · 2021-08-13T18:37:19Z

I'm not sure if I understand this correctly. Is the req.count the distance between keyframes? If so, there is a bug and we can simplify.

It is the number of frames between the first and the last in the sequence including stride.

dali-automaton · 2021-08-13T19:06:43Z

CI MESSAGE: [2760331]: BUILD STARTED

dali-automaton · 2021-08-13T19:54:13Z

CI MESSAGE: [2760006]: BUILD PASSED

dali-automaton · 2021-08-13T20:25:23Z

CI MESSAGE: [2760331]: BUILD PASSED

mzient · 2021-08-16T14:36:40Z

dali/operators/reader/loader/video_loader.cc

+    int key_frames_count = 0;
+    // how many key frames following the last requested frames we need to see before we stop
+    // feeding the decoder
+    const int key_frames_treshold = 2;


Can we infer this from the file info somehow? I think that there are relatively few formats that support bidirectional prediction - for others, that's just waste of time.
Even in case of HEVC, I think that you don't need the whole interval between keyframes - just from the keyframe to the requested frame (in forward prediction) and from the requested frame to the keyframe (in reverse prediction) - but determining which keyframe is relevant might be non-trivial.

I can add check only for HEVC but I'm not aware of any simple way to determine what kind of prediction is used in a given stream - I afraid it would require parsing headers/stream on our own what is much beyond the scope of this task.

Even in case of HEVC, I think that you don't need the whole interval between keyframes - just from the keyframe to the requested frame (in forward prediction) and from the requested frame to the keyframe (in reverse prediction) - but determining which keyframe is relevant might be non-trivial.

In some cases we don't get any meaningful timestamp from the frame we read from the stream. So it may not work at all.

mzient · 2021-08-16T14:37:49Z

Please rename the PR from "Improve" to "Fix" - after all, before this PR it just doesn't work for some valid HEVC videos.

- it seems that preceding frames may appear after the following keyframe so the logic that stops sending frames to the decoder assuming that each keyframe is IDR doesn't work anymore. Improve the logic to stop after seeing the next keyframe as it should be very unlikely to have a frame from two keyframes behind - adds HEVC test Signed-off-by: Janusz Lisiecki <jlisiecki@nvidia.com>

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

dali-automaton · 2021-08-30T23:22:23Z

CI MESSAGE: [2877236]: BUILD STARTED

dali-automaton · 2021-08-31T02:19:35Z

CI MESSAGE: [2877236]: BUILD PASSED

JanuszL mentioned this pull request Aug 12, 2021

readers.video hangs when reading HEVC #3243

Closed

JanuszL force-pushed the improve_hevc_handling branch from 105197c to fef4d38 Compare August 12, 2021 10:13

jantonguirao assigned klecki and awolant Aug 13, 2021

klecki reviewed Aug 13, 2021

View reviewed changes

mzient reviewed Aug 16, 2021

View reviewed changes

JanuszL changed the title ~~Improve HEVC video handling~~ Fix HEVC video handling Aug 30, 2021

JanuszL added 2 commits August 30, 2021 09:41

Review fixes

688d17f

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

JanuszL force-pushed the improve_hevc_handling branch from 588719e to 2c68549 Compare August 30, 2021 07:41

Review fixes

0f4f103

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

JanuszL force-pushed the improve_hevc_handling branch from 2c68549 to a8f7624 Compare August 30, 2021 07:43

mzient approved these changes Aug 30, 2021

View reviewed changes

klecki approved these changes Aug 30, 2021

View reviewed changes

JanuszL force-pushed the improve_hevc_handling branch from a8f7624 to 4963631 Compare August 30, 2021 23:12

Update DALI_EXTRA_VERSION

f2b47d0

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

JanuszL force-pushed the improve_hevc_handling branch from 4963631 to f2b47d0 Compare August 30, 2021 23:12

JanuszL merged commit e2ebfe7 into NVIDIA:main Aug 31, 2021

JanuszL deleted the improve_hevc_handling branch August 31, 2021 08:01

JanuszL added the important-fix Fixes an important issue in the software or development environment. label Sep 29, 2021

JanuszL mentioned this pull request Oct 6, 2022

Training hangs on specific video #4316

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HEVC video handling #3247

Fix HEVC video handling #3247

JanuszL commented Aug 12, 2021 •

edited

Loading

dali-automaton commented Aug 12, 2021

dali-automaton commented Aug 12, 2021

dali-automaton commented Aug 12, 2021

dali-automaton commented Aug 12, 2021

JanuszL commented Aug 12, 2021

klecki left a comment

JanuszL commented Aug 13, 2021

JanuszL commented Aug 13, 2021

dali-automaton commented Aug 13, 2021

klecki left a comment

klecki Aug 13, 2021

klecki Aug 13, 2021

klecki Aug 13, 2021

JanuszL Aug 13, 2021

klecki Aug 13, 2021

JanuszL Aug 13, 2021

JanuszL commented Aug 13, 2021

dali-automaton commented Aug 13, 2021

dali-automaton commented Aug 13, 2021

dali-automaton commented Aug 13, 2021

mzient Aug 16, 2021

JanuszL Aug 30, 2021

mzient commented Aug 16, 2021

dali-automaton commented Aug 30, 2021

dali-automaton commented Aug 31, 2021

	// how many key frames following the last requested frames we need to see so far
	// how many key frames following the last requested frames we saw so far

	const int key_frames_treshold = 1;
	const int key_frames_treshold = 2;

	if (frame > req.frame + req.count) {
	if (frame > req.frame + (1 + key_frames_count) * req.count) {

Fix HEVC video handling #3247

Fix HEVC video handling #3247

Conversation

JanuszL commented Aug 12, 2021 • edited Loading

Description

What happened in this PR

Additional information

Checklist

Tests

Documentation

DALI team only

Requirements

dali-automaton commented Aug 12, 2021

dali-automaton commented Aug 12, 2021

dali-automaton commented Aug 12, 2021

dali-automaton commented Aug 12, 2021

JanuszL commented Aug 12, 2021

klecki left a comment

Choose a reason for hiding this comment

JanuszL commented Aug 13, 2021

JanuszL commented Aug 13, 2021

dali-automaton commented Aug 13, 2021

klecki left a 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 Aug 13, 2021

dali-automaton commented Aug 13, 2021

dali-automaton commented Aug 13, 2021

dali-automaton commented Aug 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzient commented Aug 16, 2021

dali-automaton commented Aug 30, 2021

dali-automaton commented Aug 31, 2021

JanuszL commented Aug 12, 2021 •

edited

Loading