Repository navigation
Estimate a video decoder's work from the decoding order in its index - #6559
jantonguirao wants to merge 3 commits into
Conversation
Planning decode work across lanes needs an estimate of the pictures a decoder decodes for a sample. A frame costs every picture up to its own in decoding order (B-frames), clips with an edit list decode a hidden lead-in before their first frame, a seek can land a GOP before the keyframe, and padding past the last frame reopens the file. The frame index now records, per frame, the position of its packet in decoding order (counting packets hidden by an edit list) and, for the frames SeekFrame seeks to, where a seek really starts decoding (asking the demuxer only for streams with hidden packets). Both decoders count the pictures they decode. FramesDecoderBase::AddDecodeWork mirrors SeekFrame (Reset, kMinimumSeekLeap, same keyframe, backward seek) to estimate the pictures decoded and the reopenings, PositionAt gives the position of a decoder from its next frame, and HandleBoundary is static so that frames can be mapped without a decoder. Tests check the estimate against real decoding on the CPU (exact) and GPU (within one picture). Nothing that affects the output changes. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Demuxer-specific seek behavior and CPU/NVDEC work estimates need final human validation with the real-stream tests.
No open issues 🎉
What changed in this PR
Adds a decoding-work model for the upcoming video reader decode-lane planner. The model accounts for decoding order, hidden packets, seek landing points, and decoder reopenings; it does not yet affect scheduling or decoded output.
Changes:
- Records decoding positions and probes seek landing points for streams with hidden packets.
- Adds work-estimation helpers and CPU/GPU decoded-picture counters.
- Adds tests comparing estimates with actual decoding, including reordered frames, hidden lead-ins, seeks, and padding.
| File | Description |
|---|---|
dali/operators/video/frames_decoder_test.cc |
Tests index metadata and estimated work against real decoding. |
dali/operators/video/frames_decoder_gpu.cc |
Counts pictures submitted to NVDEC. |
dali/operators/video/frames_decoder_cpu.cc |
Counts packets submitted to FFmpeg. |
dali/operators/video/frames_decoder_base.h |
Declares index metadata, estimation APIs, counters, and shared seek threshold. |
dali/operators/video/frames_decoder_base.cc |
Populates decoding metadata, probes seek landings, and implements work estimation. |
Review effort: Balanced
 [Medium impact] The PR appears safe to merge; both previous findings are fixed.
|
|
!build |
|
CI MESSAGE: [77048284]: BUILD STARTED |
AddDecodeWork ignores frames out of range (a constant frame from HandleBoundary is copied, not decoded) instead of indexing past the index. PositionAt returns -1 after the last frame, as NextFrameIdx does, and also counts the leading pictures of an open GOP, decoded after its keyframe. BuildIndex does not ask the demuxer where seeks land in all-intra streams, where it would read the whole file again. The docs say what a decoded picture is on each decoder. Tests: exact estimates on hand-built indices (B-frames, a hidden lead-in, a seek landing before its keyframe, an open GOP, out-of-range frames, no decoding order); the reopenings are checked against a new Reopens() counter; the GPU slack only lets a decoder be ahead, not behind; the seek landing of files without hidden packets is checked against the demuxer; "across a keyframe" runs before the far jump, so that most files have a keyframe ahead. Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
CI MESSAGE: [77048284]: BUILD FAILED |
…r names Signed-off-by: Joaquin Anton Guirao <janton@nvidia.com>
|
!build |
|
CI MESSAGE: [77287961]: BUILD STARTED |
|
CI MESSAGE: [77287961]: BUILD PASSED |
| static inline int64_t PacketKey(const AVPacket &packet) { | ||
| if (packet.pos >= 0) | ||
| return packet.pos; | ||
| return packet.dts != AV_NOPTS_VALUE ? packet.dts : packet.pts; |
There was a problem hiding this comment.
[Nit] PacketKey returns a file position when pos >= 0 and a timestamp otherwise, and both go into the same unordered_map<int64_t, int>. If a stream has some packets without pos, a dts can equal an unrelated packet's byte offset and match the wrong sync packet. emplace also silently keeps the first packet if two key packets share a key. It is unlikely for mov, but a std::pair<bool, int64_t> key (or two maps) removes the ambiguity.
| // Counted before the negative timestamps are skipped: hidden packets are decoded too | ||
| entry.decode_idx = decode_idx++; | ||
| if (packet->flags & AV_PKT_FLAG_KEY) | ||
| sync_packets.emplace(PacketKey(*packet), entry.decode_idx); |
There was a problem hiding this comment.
[Minor] sync_packets is filled for every key packet of every file, but it is read only when ask_demuxer is true (hidden packets and not all-intra). In an all-intra stream every packet is a key packet, so this allocates one hash node per frame and drops the map unused.
Suggestion: collect (PacketKey, decode_idx) in a std::vector (cheap push_back) and build the lookup only if ask_demuxer turns out true, or sort that vector and use std::lower_bound for the landing lookup.
| entry.seek_decode_idx = entry.decode_idx; | ||
| if (!ask_demuxer) | ||
| continue; | ||
| if (av_seek_frame(ctx_, stream_id_, entry.pts, AVSEEK_FLAG_BACKWARD) < 0) { |
There was a problem hiding this comment.
[Question] For files with a hidden lead-in, BuildIndex now does one av_seek_frame plus one packet read per GOP. BuildIndex runs at reader initialization for every file, and at this point nothing consumes seek_decode_idx (the planner is #6563). It takes a long init-time cost on a large dataset of such files. A seek lands at an arbitrary file offset, so this is random I/O on top of the sequential scan above it.
Is building an index in lazily an option here?
| if (first > 0) | ||
| first = index[first - 1].last_keyframe_id; | ||
| int decoded = -1; | ||
| for (int i = first; i < next_frame; i++) |
There was a problem hiding this comment.
[Question] PositionAt scans up to two GOPs and AddDecodeWork scans from the next frame to the target, so both are O(GOP length) per call. With a long-GOP file (e.g. a single keyframe and 100k frames) this is O(n) per call. If the planner calls them per sample per candidate lane, that adds up (batch size × lanes × GOP length per batch). Is that acceptable for the planned use, or should the max decode index within a GOP be precomputed?
| // A frame is returned once the pictures up to its own (in decoding order) are decoded, so the | ||
| // decoder decodes up to the furthest picture of the frames it reads, skipped ones included | ||
| int start = pos.decoded; | ||
| for (int i = pos.next_frame; i <= frame; i++) |
There was a problem hiding this comment.
Same as previous comment in line 641
| int decode_idx = -1; | ||
| // For a frame that SeekFrame seeks to (the last keyframe of some frame): the decoding position | ||
| // of the packet where a seek to its timestamp lands (see AvSeekFrame); -1 otherwise or if unknown | ||
| int seek_decode_idx = -1; |
There was a problem hiding this comment.
[Minor] IndexEntry grows from 16 to 24 bytes (pts 8 + last_keyframe_id 4 + two bools = 14, padded to 16; with decode_idx the next int starts at offset 16, so the struct is padded to 24). seek_decode_idx then fits in the remaining 4 bytes of padding, so it costs nothing extra; decode_idx is what adds the 8 bytes.
FrameIndexCache (reader/video_reader_decoder_op.cc) keeps the FrameIndex of every file for the lifetime of the process, and I did not see any eviction. So index memory goes up by 50% for all users of readers.video, including those that never use the planner. Is that growth acceptable for large datasets (e.g. 1e9 frames in total adds about 8 GB)? Staying at 16 bytes would need the two bools folded into spare bits of an int field, which may not be worth it.
| void CheckDecodeWork(FramesDecoderBase &decoder, FramesDecoderBase::DecodePosition &pos, | ||
| int64_t &surplus, const std::vector<int> &frame_ids, uint8_t *buffer, | ||
| int slack, const std::string &what) { | ||
| ASSERT_LE(frame_ids.size(), static_cast<size_t>(kMaxDecodeWorkFrames)); |
There was a problem hiding this comment.
[Nit] ASSERT_* in these void helpers only returns from the helper, not from the test, so CheckGPUDecodeWork / CheckGPUPositionAt carry on with the next path after a fatal failure, and CheckDecodeWorkScenarios after a failed ASSERT_GE(n, 40) returns while its caller continues. Calling the helpers through ASSERT_NO_FATAL_FAILURE(...) makes the test stop at the first failing file.
| const std::string &path) { | ||
| auto &index = decoder.GetIndex(); | ||
| int n = index.size(); | ||
| ASSERT_GE(n, 40); |
There was a problem hiding this comment.
Same as in line 333
| FramesDecoderCpu decoder(path); | ||
| decoder.BuildIndex(); | ||
| std::vector<uint8_t> buffer(kMaxDecodeWorkFrames * decoder.FrameSize()); | ||
| CheckDecodeWorkScenarios(decoder, buffer.data(), 0, path); |
There was a problem hiding this comment.
[Question] The PR description says "On the CPU the estimate must be exact", but this test runs only on VP9 files, which have no reordering and no hidden packets. decoded_pictures_ on the CPU counts packets sent to avcodec_send_packet. For H.264/HEVC with B-frames, libavcodec may need packets beyond a frame's own decoding position before it returns that frame (reorder delay), so the count could be higher than AddDecodeWork estimates. Is the CPU model meant to cover those streams? If so, a CPU test on test_1.mp4 (H.264) or a Sintel clip would confirm it; if not, a note on AddDecodeWork saying the CPU estimate is exact only without reordering would help.
I did not run this, so it may well hold.
Category:
New feature (non-breaking change which adds functionality)
Description:
This PR is split out of #6545 (decode lanes for
fn.experimental.readers.video). It adds the model of a video decoder's work that the decode-lane planner (#6563) uses to split a batch between lanes. On its own it changes no output: nothing outside the tests calls the new functions yet. It is based on main and does not depend on the other split PRs.Why it is needed. To spread a batch over several decoders, the reader has to know what each group of samples costs. Counting frames is not enough, because what a decoder really does for a sample depends on the stream:
SeekFrameseeks at all depends on how far ahead the frame is and on its keyframe. Once a decoder has returned the last frame of the file (e.g. for a sample padded past the end), its next seek resets it, which reopens the file.A planner that ignores this puts the wrong amount of work on each lane, and the batch waits for the most loaded one.
What changes.
IndexEntry::decode_idx: the position of its packet in decoding order, counting all the packets of the stream, including those hidden by an edit list.SeekFrameseeks to (the last keyframe of some frame),IndexEntry::seek_decode_idxrecords where a seek to it really starts decoding. If the stream has no hidden packets, this is the keyframe itself. OtherwiseBuildIndexasks the demuxer (av_seek_frameand a read of the first packet of the video stream, per seek target), because the demuxer's rule is not exposed. Packets are matched by file position, or by timestamp when the position is unknown. If the landing packet is not a known sync packet, or the demuxer cannot seek, the keyframe itself is recorded. The demuxer is asked only for streams with hidden packets that are not all-intra: an all-intra stream would get read a second time, and there a seek lands on the keyframe anyway.FrameIndex::DecodeIdxandFrameIndex::SeekDecodeIdxreturn these values. They fall back to display order and to the frame itself when the index has no decoding order (e.g. a hand-built index).FramesDecoderBase::AddDecodeWork(static) mirrorsSeekFrameandReadNextFrame:Resetat the end of the file,kMinimumSeekLeap, decoding through within the same keyframe, and seeking backward. It adds the pictures decoded to return a frame from a givenDecodePosition, and the number of reopenings, to aDecodeWork, then advances the position. A frame out of range (e.g. a constant frame fromHandleBoundary) is not decoded and adds nothing, as inDecodeFramesImpl.FramesDecoderBase::PositionAt(static) estimates where a decoder is after reading the frames up tonext_frame - 1in a row, including the leading pictures of an open GOP (decoded after its keyframe); past the last frame it returns-1, asNextFrameIdxdoes. A decoder that the reader keeps between batches can be costed from its next frame this way.MINIMUM_SEEK_LEAP = 10inSeekFrame, becomes the publicFramesDecoderBase::kMinimumSeekLeap, so thatAddDecodeWorkandSeekFrameshare it.HandleBoundaryis now static, so that sample frames can be mapped without a decoder.DecodedPictures()): packets sent to FFmpeg on the CPU,cuvidDecodePicturecalls on the GPU. The tests use this counter to check the estimate against real decoding. A packet that holds several pictures (a VP9 superframe with a hidden frame, the two fields of an interlaced frame) counts once in the estimate but more than once on the GPU; this is documented. AReopens()counter, incremented inReset(), lets the tests check the estimated reopenings too.Who is affected. Apart from incrementing the decoded-picture counter, the only runtime change is in
BuildIndex, used byfn.experimental.readers.videoand byfn.experimental.decoders.videowithbuild_index=True. For streams with packets hidden by an edit list, it now makes one extra demuxer seek and packet read per seek target. For other streams it only records the decoding position of each packet. Decoded frames do not change.Additional information:
Affected modules and functionalities:
dali/operators/video/frames_decoder_base.{h,cc}:IndexEntry::decode_idx,IndexEntry::seek_decode_idx,FrameIndex::DecodeIdx,FrameIndex::SeekDecodeIdx;BuildIndexfills them in;DecodePosition,DecodeWork,PositionAt,AddDecodeWork;kMinimumSeekLeapreplaces the localMINIMUM_SEEK_LEAPinSeekFrame;HandleBoundaryis static;DecodedPictures(),Reopens()and thedecoded_pictures_,reopens_counters.dali/operators/video/frames_decoder_cpu.cc,dali/operators/video/frames_decoder_gpu.cc: count decoded pictures.dali/operators/video/frames_decoder_test.cc: new tests.Key points relevant for the review:
AddDecodeWorkmust stay in sync withSeekFrame. Both have comments pointing to each other. An estimate that is off costs planning quality only, never correctness, because decoding still goes throughSeekFrame. The tests replayDecodeFrameson real files and compare.BuildIndex. It is limited to streams with hidden packets, and it stops ifav_seek_framefails (stream not seekable).BuildIndexcallsReset()afterwards, as before, so the decoder state is unchanged.kGpuPictureSlack), never behind it: a sample may cost less than estimated only by the surplus carried over from the samples before it. On the CPU the estimate must be exact.av_seek_frame, after which every seek resets the decoder (documented onAddDecodeWork).Tests:
frames_decoder_test.cc: the existingFramesDecoderTest_CpuOnlyTests.*andFramesDecoderGpuTest.*tests (indexing, seeking, decoding), anddali/test/python/decoder/test_video.pyfor both operators (including the padding modes that go throughHandleBoundary, e.g.test_video_decoder_frame_indices,test_enable_frame_num_sequence_non_constant_padding). Output must not change.frames_decoder_test.cc:FramesDecoderDecodeWorkTest.*: exact estimates on hand-built indices, with no decoder: B-frames with a hidden lead-in, a seek that lands before its keyframe, an open GOP, frames out of range, and an index without decoding order.PositionAt(k)must equal the position after reading the frames beforekin a row.FramesDecoderTest_CpuOnlyTests.IndexDecodeOrder,FramesDecoderGpuTest.IndexDecodeOrder: in files without hidden packets (VP9, and H.264 on the GPU),decode_idxis a permutation of the decoding positions, every keyframe is a seek target, every seek target hasseek_decode_idx == decode_idx, and other frames have none. SinceBuildIndexrecords this without asking the demuxer, the test asks a demuxer of its own where each seek lands. On the CPU (VP9 only),decode_idxalso equals the frame index, as VP9 has no reordering.FramesDecoderGpuTest.IndexDecodeOrderLeadIn: Sintel clips with a hidden lead-in. Covers a long lead-in (188 hidden pictures), clips where only the IDR is hidden, a seek to a shown IDR that lands on the hidden one, and seeks that land on the shown IDRs.FramesDecoderTest_CpuOnlyTests.DecodeWork(exact) andFramesDecoderGpuTest.DecodeWork(within one picture): sample sequences that take every path ofSeekFrame(decoding through, a short leap, a far jump, across a keyframe, backward, duplicates, the last frame, past the end, edge padding). Each one comparesAddDecodeWorkwith the pictures the decoder really decodes, the estimated reopenings withReopens(), and the predicted next frame withNextFrameIdx(). The CPU test runs on VP9 files (CFR and VFR); the GPU test on H.264 (with and without lead-in), VP9 and, if supported, HEVC.FramesDecoderGpuTest.DecodeWorkFromPositionand the CPU part ofDecodeWork: reading every frame in a row, each frame is costed withPositionAt+AddDecodeWork. In total, the decoder is never behind the estimate and at most the slack ahead of it. On the GPU, some frames must cost nothing (B-frames already decoded).FramesDecoderGpuTest.DISABLED_DecodeWorkAllSintelClips,FramesDecoderGpuTest.DISABLED_DecodeWorkFromPositionAllSintelClips: the same on every Sintel clip. They are disabled by default because they are slow; run them with--gtest_also_run_disabled_testsafter changingSeekFrameorAddDecodeWork.Checklist
Documentation
DALI team only
Requirements
REQ IDs: N/A
JIRA TASK: DALI-4935