[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 in one piece.

Problem. FramesDecoderGpu::HandlePictureDisplay maps the NVDEC output surface of each displayed picture, launches the color space conversion on the decoder's stream, and then calls cudaStreamSynchronize before it unmaps the surface. So for every picture the GPU decoder converts, the host thread stops and waits for the conversion kernel to finish before it can hand the next packet to the parser and NVDEC. Decoding and conversion are serialized with host round trips in between, and the decoding thread is idle for the duration of every conversion.

This affects every user of the GPU video decoder (FramesDecoderGpu): fn.experimental.readers.video with device="gpu", and fn.experimental.decoders.video and fn.experimental.inputs.video with device="mixed". The cost grows with the number of pictures converted (long samples, high resolution) and it gets in the way of overlapping the work of several decoders, which the decode lanes of #6563 and #6564 rely on.

Solution. The wait is moved to the point where it is actually needed: right before NVDEC may overwrite the surface.

• After the conversion is launched, the surface stays mapped and a CUDAEvent (conversion_done_) is recorded on the stream behind it.
• The new FramesDecoderGpu::ReleaseMappedFrame waits for that event and unmaps the surface. It is called right before the next cuvidDecodePicture and in Flush.
• The destructor, which already synchronizes the stream (since Sync the stream before a GPU video decoder frees its frame buffers #6556), then unmaps a surface that is still mapped itself (without throwing), on the decoder's own device via DeviceGuard, before the decoder goes back to the NVDEC cache. The destructor may run on a thread where another device is current, hence the device guard (NVDECLease gets a device_id() accessor for that).
• NVDEC is therefore still used only after every earlier conversion is done, and at most one surface per decoder is mapped at a time.
• The mapped surface is tracked as soon as cuvidMapVideoFrame returns, so a conversion that throws after the map (e.g. an unsupported output type) no longer leaks the mapping: before, cuvidUnmapVideoFrame was simply skipped in that case. If the conversion failed and the event was not recorded, the release waits for the whole stream instead.
• The move constructor of FramesDecoderGpu is deleted (it was defaulted): the parser calls back into the object, and it may now hold a mapped surface.

Because the decoder no longer waits for its GPU work, its users now wait for its stream themselves, so that asynchronous errors (conversions, copies) are still reported at the sample that caused them, rather than in the decoder's destructor:

• fn.experimental.decoders.video (VideoDecoderBase): after each sample, before its decoder is reset;
• fn.experimental.readers.video (VideoReaderDecoder): after each sample is decoded;
• fn.experimental.inputs.video (VideoInput): at the end of each Run, on the decoder's own stream (exposed by the new FramesDecoderGpu::stream()), so the frames are complete even if ws.stream() is a different stream.

This keeps one wait per sample (or per Run) instead of one per picture. The decoded frames do not change.

For tests, a process-wide counter of mapped surfaces is added: frame_dec_gpu_impl::NVDECFramesMapped().

Depends on #6556, #6559 and #6540. Their commits are on this branch until they are merged; please review only this PR's own commit:

• Do not wait on the host for every picture the GPU video decoder converts

The other commits belong to those PRs: Sync the stream before a GPU video decoder frees its frame buffers (#6556), Estimate a video decoder's work from the decoding order in its index (#6559), and the #6540 commits (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).

Additional information:

Affected modules and functionalities:

• dali/operators/video/frames_decoder_gpu.h/.cc: deferred unmap (ReleaseMappedFrame, mapped_frame_, conversion_done_, conversion_recorded_), unmap in Flush and the destructor, stream() accessor, NVDECLease::device_id(), frame_dec_gpu_impl::NVDECFramesMapped(), deleted move constructor.
• dali/operators/video/decoder/video_decoder_base.h: stream sync after each GPU sample (fn.experimental.decoders.video).
• dali/operators/video/reader/video_reader_decoder_op.cc: stream sync after each GPU sample (fn.experimental.readers.video).
• dali/operators/video/input/video_input.h: sync of the decoder's stream at the end of each GPU Run (fn.experimental.inputs.video).
• dali/operators/video/frames_decoder_test.cc: new tests.

Key points relevant for the review:

• Correctness of the surface lifetime: every path that can make NVDEC write a surface again (ProcessPictureDecode, Flush) and the destructor (before the decoder returns to NVDECCache) must release the mapped surface first, and the release must wait for the conversion that reads it.
• The destructor: it syncs the stream, then unmaps under a DeviceGuard for the decoder's device; the unmap does not throw. A failed unmap is left counted in NVDECFramesMapped, so that tests can see it.
• The fallback to cudaStreamSynchronize when the conversion failed before the event was recorded (conversion_recorded_ == false).
• The new per-sample / per-Run stream syncs in the three operators: they are what keeps asynchronous errors attributed to the right sample now that the decoder does not sync per picture.

Tests:

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

New GTests in dali/operators/video/frames_decoder_test.cc. Except for the last one, they hold up the decoder's stream with a host function (DelayStream) so that conversions are still pending when the decoder decodes the next picture, seeks, flushes or is destroyed, and compare the frames with those decoded without the delay on the default stream (the first two also with the ground truth):

• FramesDecoderGpuTest.ConversionPendingAtNextPicture: reading in a row, seeking forward and back, reading the last frame, restarting from the beginning and leaping across a short gap;
• FramesDecoderGpuTest.ConversionPendingAtNextPictureNoIndex: the same from memory without an index (frames go through the decoder's frame buffer);
• FramesDecoderGpuTest.DecoderReturnedToCacheIsClean: decoders destroyed with a pending conversion return a clean decoder to the NVDEC cache, which the next decoder on another stream reuses (checked with NVDECDecodersCreated);
• FramesDecoderGpuTest.DecodersDestroyedInAnotherThread: decoders running in their own threads and streams are destroyed from another thread;
• FramesDecoderGpuTest.SurfaceUnmappedAfterFailedConversion: a conversion that throws after the map does not leave the surface mapped once the decoder is destroyed.

All of them also check, through NVDECFramesMapped, that at most one surface per decoder is mapped and none is left mapped afterwards.

FramesDecoderGpuTest.DecoderCacheReusedForCroppedVideo (from #6540) only builds its file path with make_string now; its checks are unchanged.

Existing tests that cover the change:

• frames_decoder_test.cc: FramesDecoderGpuTest.*
• dali/test/python/test_video_reader.py and dali/test/python/test_video_pipeline.py: GPU fn.experimental.readers.video
• dali/test/python/decoder/test_video.py: GPU fn.experimental.decoders.video
• dali/test/python/input/test_video.py: GPU fn.experimental.inputs.video

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.