-
Notifications
You must be signed in to change notification settings - Fork 53
Vulkan validation layer and decoder errors and assert fixes #199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e2c160b
d7a1818
287aba8
c5086da
5e75e7f
5d0059e
3ef4fd1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -227,11 +227,12 @@ VkResult VkVideoEncoderAV1::ProcessDpb(VkSharedBaseObj<VkVideoEncodeFrameInfo>& | |
| assert(dpbIndx >= 0); | ||
|
|
||
| m_dpbAV1->ConfigureRefBufUpdate(pFrameInfo->bShownKeyFrameOrSwitch, pFrameInfo->bShowExistingFrame, frameUpdateType); | ||
| assert(pFrameInfo->frameEncodeEncodeOrderNum <= std::numeric_limits<uint32_t>::max()); | ||
| m_dpbAV1->InvalidateStaleReferenceFrames(static_cast<uint32_t>(pFrameInfo->frameEncodeEncodeOrderNum), pFrameInfo->picOrderCntVal, &m_stateAV1.m_sequenceHeader); | ||
| pFrameInfo->stdPictureInfo.refresh_frame_flags = (uint8_t)m_dpbAV1->GetRefreshFrameFlags(pFrameInfo->bShownKeyFrameOrSwitch, pFrameInfo->bShowExistingFrame); | ||
|
|
||
| // For show existing frame, skip stale reference invalidation and return early. | ||
| // Show existing frames don't have a valid frameEncodeEncodeOrderNum since they | ||
| // don't represent a new encoded frame - they just display a previously encoded one. | ||
| if (pFrameInfo->bShowExistingFrame) { | ||
| pFrameInfo->stdPictureInfo.refresh_frame_flags = (uint8_t)m_dpbAV1->GetRefreshFrameFlags(pFrameInfo->bShownKeyFrameOrSwitch, pFrameInfo->bShowExistingFrame); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Adding this line is fine, but updating
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for looking at the changes, @krajunv! Can you please be more specific on the change you want me to make?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This line doesn’t affect the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, the refresh_frame_flags is not present in the bitstream when show_existing_frame=1. But we would still want to keep this line because: Vulkan API validation - The driver may still validate the struct fields even if it doesn't encode them. Setting it to 0 ensures no validation warnings about unexpected non-zero values. Internal DPB tracking - While the driver won't write it to the bitstream, it may still use the value internally to verify no reference buffer updates are being requested. Defensive coding - If a future Vulkan implementation or validation layer starts checking this field, having it correctly set to 0 prevents issues. Bottom line: It's technically unnecessary for bitstream generation, but harmless and follows the principle of keeping the struct consistent with what the encode operation semantically means (no reference updates). Do you have a strong objection on this, @krajunv? |
||
| m_dpbAV1->DpbPictureEnd(dpbIndx, encodeFrameInfo->setupImageResource/*nullptr*/, &m_stateAV1.m_sequenceHeader, | ||
| pFrameInfo->bShowExistingFrame, pFrameInfo->bShownKeyFrameOrSwitch, | ||
| pFrameInfo->stdPictureInfo.flags.error_resilient_mode, | ||
|
|
@@ -240,6 +241,10 @@ VkResult VkVideoEncoderAV1::ProcessDpb(VkSharedBaseObj<VkVideoEncodeFrameInfo>& | |
| return VK_SUCCESS; | ||
| } | ||
|
|
||
| assert(pFrameInfo->frameEncodeEncodeOrderNum <= std::numeric_limits<uint32_t>::max()); | ||
| m_dpbAV1->InvalidateStaleReferenceFrames(static_cast<uint32_t>(pFrameInfo->frameEncodeEncodeOrderNum), pFrameInfo->picOrderCntVal, &m_stateAV1.m_sequenceHeader); | ||
| pFrameInfo->stdPictureInfo.refresh_frame_flags = (uint8_t)m_dpbAV1->GetRefreshFrameFlags(pFrameInfo->bShownKeyFrameOrSwitch, pFrameInfo->bShowExistingFrame); | ||
|
|
||
| // setup recon picture (pSetupReferenceSlot) | ||
| bool success = m_dpbImagePool->GetAvailableImage(encodeFrameInfo->setupImageResource, | ||
| VK_IMAGE_LAYOUT_VIDEO_ENCODE_DPB_KHR); | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a new problem. I did not see any issue when encoding. Either it was holding correct state or initialized to some non-negative value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that the assert() was added with commit#1a883255 and I think this check uncovered the issue in debug mode.
Your change looks good to me.