{CUDA Decoding} Destroy existing NVDEC decoder before recreating on sequence re-signal - #261
Closed
YLouWashU wants to merge 2 commits into
Closed
{CUDA Decoding} Destroy existing NVDEC decoder before recreating on sequence re-signal#261YLouWashU wants to merge 2 commits into
YLouWashU wants to merge 2 commits into
Conversation
…cs (facebookresearch#260) Summary: Adds a Linux build-only CI job (`build-nvcodec`) to VRS `build-and-test.yml` that installs `nv-codec-headers`, configures with `-DBUILD_WITH_XPRS=ON -DENABLE_NVCODEC=ON`, and builds. GitHub-hosted runners have no NVIDIA GPU, so this compiles the NVDEC path -- which needs only `nv-codec-headers`, not a GPU or CUDA toolkit -- to catch OSS build breakage. GPU decode is validated manually on a GPU machine. Also documents `nv-codec-headers` as an optional dependency for GPU-accelerated H.265 decoding in the README. Part of the GPU-accelerated H.265 decoding rollout. Lands after the xprs CUDA decoder files are synced to the GitHub VRS mirror (configerator cconf change), since the NVCODEC build needs those sources. Reviewed By: georges-berenger Differential Revision: D110227873
…equence re-signal Summary: `NvDecoder::HandleVideoSequence` is the NVDEC parser's sequence callback and, as its own header doc notes, can fire again mid-stream on a new sequence header or format change. The body unconditionally called `cuvidCreateDecoder(&_decoder, ...)`, so on any re-invocation it overwrote `_decoder` with a fresh handle without destroying the previous one — leaking the old `CUvideodecoder` and leaving the parser driving a stale/duplicated decoder. This is the "returning stale decoder" issue raised in review on D103253728. Fix: guard the create call. When `_decoder` is already non-null, `cuvidDestroyDecoder` it (and null it, so a subsequent create failure can't double-free in the destructor) before creating the replacement. Destroy-before-recreate is chosen over `cuvidReconfigureDecoder` for correctness and simplicity: Aria streams have constant format so re-signal is rare, making the recreate cost negligible; reconfigure (an in-place perf optimization bounded by the original ulMaxWidth/Height) can be a later change if a re-signaling workload ever needs it. Follow-up to D103253728. Reviewed By: georges-berenger Differential Revision: D113848493
meta-cla
Bot
added
the
CLA Signed
Contributor
|
@YLouWashU has exported this pull request. If you are a Meta employee, you can view the originating Diff in D113848493. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary:
NvDecoder::HandleVideoSequenceis the NVDEC parser's sequence callback and, as its own header doc notes, can fire again mid-stream on a new sequence header or format change. The body unconditionally calledcuvidCreateDecoder(&_decoder, ...), so on any re-invocation it overwrote_decoderwith a fresh handle without destroying the previous one — leaking the oldCUvideodecoderand leaving the parser driving a stale/duplicated decoder. This is the "returning stale decoder" issue raised in review on D103253728.Fix: guard the create call. When
_decoderis already non-null,cuvidDestroyDecoderit (and null it, so a subsequent create failure can't double-free in the destructor) before creating the replacement. Destroy-before-recreate is chosen overcuvidReconfigureDecoderfor correctness and simplicity: Aria streams have constant format so re-signal is rare, making the recreate cost negligible; reconfigure (an in-place perf optimization bounded by the original ulMaxWidth/Height) can be a later change if a re-signaling workload ever needs it.Follow-up to D103253728.
Reviewed By: georges-berenger
Differential Revision: D113848493