[jantonguirao]

Category:

Refactoring (Redesign of existing code that doesn't affect functionality)

Description:

Split out of #6545 (decode lanes for fn.experimental.readers.video), which was too big to review as a whole. This PR covers one concern only: the GPU video decoder no longer maps, converts or keeps a frame buffer for pictures that are never read. It also skips decoding H.264 non-reference pictures that are never read.

Depends on: #6561, which itself builds on #6556, #6559 and #6540. Until those are merged, their commits are also on this branch. This PR's own change is only the last commit, "Do not map, convert nor decode video pictures that are never read". The other commits belong to other PRs:

• "Do not wait on the host for every picture the GPU video decoder converts": Do not wait on the host for every picture the GPU video decoder converts #6561
• "Estimate a video decoder's work from the decoding order in its index": Estimate a video decoder's work from the decoding order in its index #6559
• "Sync the stream before a GPU video decoder frees its frame buffers": Sync the stream before a GPU video decoder frees its frame buffers #6556
• "Fix NVDEC decoder cache never matching cropped videos", "Count created NVDEC decoders in the cache reuse test", "Explain the NVDEC decoder cache reuse condition" and the merge commit that brings them in: Fix NVDEC decoder cache never matching cropped videos #6540

The problem

FramesDecoderGpu has to decode every picture between a keyframe and the frames it returns, because later pictures may refer to them. Until now it also did everything else for each of those pictures. When a picture was not returned right away, it was mapped from the NVDEC output surface and color-converted into a frame buffer. That buffer was device memory the size of one output frame. Many of these pictures are never read with their data:

• Pictures hidden by an edit list. These come before the first visible frame, and there can be hundreds of them. One of the Sintel test clips has 188, and before this change the decoder held more than 188 frame buffers to read its first frames.
• Frames skipped on the way to the frames to read. Examples are the frames between those of a strided sample (stride > 1), and the frames between a keyframe and the target of a seek.
• Frames shown before the next frame to read that are already buffered because of decoding order.

For each such picture, the decoder mapped an NVDEC surface and launched a color conversion into a frame buffer, and then threw the result away. This used GPU time and device memory. With an edit-list lead-in, the frame buffer pool also grew to one buffer per hidden picture. This affects the GPU backends of fn.experimental.readers.video, fn.experimental.decoders.video and fn.experimental.inputs.video, which all use FramesDecoderGpu. The effect is largest for strided sampling, for seeks into long GOPs, and for videos with an edit-list lead-in.

The change

• The decoder now knows which frames it reads next (FramesDecoderBase::frames_to_read_):

  • DecodeFrames sets it to the frames of the sample it has yet to read.
  • SeekFrame sets it to the frame it seeks to, unless DecodeFrames has already set it.
  • It is cleared on scope exit.

• FramesDecoderGpu::CanBeReadWithData(pts) decides whether a buffered frame can still be read. The index is sorted by timestamp (SetIndex now asserts this). A frame can be read only if:

  • its timestamp is in the index at or after the next frame, and
  • it is one of the frames to read next, or it comes after the last of them, or the frames to read next are not known. A frame after the last one is kept because a later DecodeFrames call on the same decoder may read it.

  Without an index, every frame can be read, as before.

• In HandlePictureDisplay, a buffered frame that cannot be read keeps its slot in the buffer, but only its timestamp is kept (BufferedFrame::has_data_ == false). It is neither mapped nor converted.

• Frame buffers are attached lazily (AttachFrameBuffer). A slot reuses the buffer of an empty slot or of a timestamp-only slot. Only if there is none is a new buffer allocated. Neither parser initialization nor growing the buffer (FindEmptySlot) allocates a frame buffer for every slot any more.

• In ProcessPictureDecode, an H.264 non-reference frame picture (ref_pic_flag == 0, not a field picture) that is not read with its data (IsPictureReadWithData) is not decoded at all. No other picture refers to it, so only its timestamp is kept for display order. These pictures are counted by SkippedPictures(). FramesDecoderBase::ParsedPictures() (decoded plus skipped) is now the number that the AddDecodeWork estimate from Estimate a video decoder's work from the decoding order in its index #6559 matches. Other codecs, and reference or field pictures, are always decoded.

• Reading a frame from the buffer that kept only its timestamp fails with an internal error, instead of returning a frame whose data was never written.

The frames returned do not change. The tests compare them against a decoder that maps every picture.

Additional information:

Affected modules and functionalities:

• dali/operators/video/frames_decoder_base.{h,cc}:
  • frames_to_read_, set by DecodeFramesImpl and SeekFrame;
  • skipped_pictures_ and ParsedPictures();
  • an assertion in SetIndex that the index is sorted by timestamp.
• dali/operators/video/frames_decoder_gpu.{h,cc}:
  • BufferedFrame::has_data_;
  • CanBeReadWithData, IsPictureReadWithData and AttachFrameBuffer;
  • lazy frame buffer allocation;
  • the H.264 non-reference skip in ProcessPictureDecode;
  • the internal-error check in ReadNextFrameWithIndex;
  • test and diagnostic accessors MappedPictures(), SkippedPictures(), NumFrameBuffers() and SetBufferedFrameDataForTesting.
• dali/operators/video/frames_decoder_test.cc: new tests, and the decode-work checks now use ParsedPictures().

Users: the GPU backends of fn.experimental.readers.video, fn.experimental.decoders.video and fn.experimental.inputs.video. The CPU decoder is not changed.

Key points relevant for the review:

• CanBeReadWithData must never say "no" to a frame that is read later. If it did, that frame would have no data. This PR turns that case into an internal error (ReadNextFrameWithIndex), not a garbage frame. Please check the reasoning in its comment:
  • Until a flush, buffered frames are only looked up for frames at or after next_frame_idx_.
  • Frames after the last one in frames_to_read_ are always kept.
• frames_to_read_ lifetime. It is a span into a local SmallVector in DecodeFramesImpl, and is cleared by AtScopeExit. SeekFrame only sets it, and clears it, when DecodeFrames has not set it already.
• The non-reference skip. The timestamp of the picture being decoded is taken as piped_pts_.front(), or current_pts_ when nothing is piped. This is the same timestamp HandlePictureDisplay will assign. IsPictureReadWithData makes the same decision HandlePictureDisplay makes: the picture is returned now (current_copy_to_output_), or buffered with its data. The skip is limited to H.264 frame pictures with ref_pic_flag == 0.
• AttachFrameBuffer buffer reuse. It takes the buffer of an empty or timestamp-only slot. Any pending copy out of that buffer and the conversion into it both run on stream_, so they stay ordered.
• AddDecodeWork (from Estimate a video decoder's work from the decoding order in its index #6559) still counts skipped non-reference pictures as work. This is exact in parsed pictures (ParsedPictures), but it over-estimates NVDEC work. Weighting them would need a reference flag per index entry, so it is left for a follow-up.

Tests:

• Existing tests apply
• New tests added
  • Python tests
  • GTests
  • Benchmark
  • Other
• N/A

New GTests in frames_decoder_test.cc:

• FramesDecoderGpuTest.PicturesNotReadAreNotMapped uses a clip with 188 pictures hidden by an edit list. It reads frames in a row, with stride 3, and from frame 30. It checks that:
  • the frames match a decoder that maps every picture;
  • no more than 10 frame buffers are held (versus more than 188 when every picture is mapped);
  • at least 80 hidden non-reference pictures are skipped;
  • at most a few pictures beyond the frames read are mapped.
• FramesDecoderGpuTest.SameFramesAsMappingAll covers Sintel clips, VFR H.264 and (when supported) HEVC and VFR HEVC. It compares the frames, and the total pictures parsed, with a decoder that maps every picture, for:
  • strided samples;
  • duplicate frames;
  • REFLECT_101/CLAMP/CONSTANT padding;
  • seeks;
  • consecutive and overlapping samples on one decoder (as a decode lane from Decode the video reader's batches with up to num_threads decode lanes #6563 decodes them);
  • AvSeekFrame to a frame that is not a keyframe.
• FramesDecoderGpuTest.ReadingFrameNotMappedFails checks that reading a frame whose data was not kept raises the internal error and does not return a garbage frame.

Changed: the decode-work helpers used by FramesDecoderTest_CpuOnlyTests.DecodeWork, FramesDecoderGpuTest.DecodeWork and FramesDecoderGpuTest.DecodeWorkFromPosition now compare the estimate with ParsedPictures().

Existing tests that cover the change:

• all FramesDecoderGpuTest.* tests, e.g. ConstantFrameRate*, VariableFrameRate*, InMemory*, *NoIndex, RawH264 and RawH265;
• dali/test/python/test_video_reader.py, dali/test/python/decoder/test_video.py, dali/test/python/input/test_video.py and dali/test/python/test_video_pipeline.py.

Checklist

Documentation

• Existing documentation applies
• Documentation updated
  • Docstring
  • Doxygen
  • RST
  • Jupyter
  • Other
• N/A

DALI team only

Requirements

• Implements new requirements
• Affects existing requirements
• N/A

REQ IDs: N/A

JIRA TASK: DALI-4935

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.