Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2171283004:
Remove asserts on scissor size in Vulkan (Closed)
DescriptionRemove asserts on scissor size in Vulkan
These are asserts are firing from a recent change to our scissor code.
Since these asserts were added, the Vulkan spec has been updated to no
longer require the scissor is insides the bounds of the image, just that
x + width does not overflow.
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2171283004
Committed: https://skia.googlesource.com/skia/+/0e72e9ee3b0201f05e5d74917b17c8a427b14d9f
Patch Set 1 #
Total comments: 1
Patch Set 2 : Clamp to x,y to 0 #Messages
Total messages: 20 (7 generated)
Description was changed from ========== Remove asserts on scissor size in Vulkan These are asserts are firing from a recent change to our scissor code. Since these asserts were added, the Vulkan spec has been updated to no longer require the scissor is insides the bounds of the image, just that x + width does not overflow. BUG=skia: ========== to ========== Remove asserts on scissor size in Vulkan These are asserts are firing from a recent change to our scissor code. Since these asserts were added, the Vulkan spec has been updated to no longer require the scissor is insides the bounds of the image, just that x + width does not overflow. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2171283004 ==========
egdaniel@google.com changed reviewers: + bsalomon@google.com, csmartdalton@google.com
The CQ bit was checked by egdaniel@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
cc'ing Sheriff Joe
lgtm
https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp File src/gpu/vk/GrVkPipeline.cpp (left): https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp... src/gpu/vk/GrVkPipeline.cpp:511: SkASSERT(scissor.offset.y + scissor.extent.height <= (uint32_t)target.height()); Ah, looks my change gave you grief. I also removed a similar assert in GL: https://codereview.chromium.org/2160093002/diff/140001/src/gpu/gl/GrGLIRect.h In GL, the only requirement is that width and height are non-negative. Should we also remove the asserts that x,y >= 0? (I'd be surprised if vulkan allowed you to overstep the right edge, but not the left.)
On 2016/07/22 20:54:48, csmartdalton wrote: > https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp > File src/gpu/vk/GrVkPipeline.cpp (left): > > https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp... > src/gpu/vk/GrVkPipeline.cpp:511: SkASSERT(scissor.offset.y + > scissor.extent.height <= (uint32_t)target.height()); > Ah, looks my change gave you grief. I also removed a similar assert in GL: > > https://codereview.chromium.org/2160093002/diff/140001/src/gpu/gl/GrGLIRect.h > > In GL, the only requirement is that width and height are non-negative. Should we > also remove the asserts that x,y >= 0? (I'd be surprised if vulkan allowed you > to overstep the right edge, but not the left.) That is still required, from spec "The x and y members of offset must be greater than or equal to 0"
On 2016/07/22 20:56:44, egdaniel wrote: > On 2016/07/22 20:54:48, csmartdalton wrote: > > https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp > > File src/gpu/vk/GrVkPipeline.cpp (left): > > > > > https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp... > > src/gpu/vk/GrVkPipeline.cpp:511: SkASSERT(scissor.offset.y + > > scissor.extent.height <= (uint32_t)target.height()); > > Ah, looks my change gave you grief. I also removed a similar assert in GL: > > > > https://codereview.chromium.org/2160093002/diff/140001/src/gpu/gl/GrGLIRect.h > > > > In GL, the only requirement is that width and height are non-negative. Should > we > > also remove the asserts that x,y >= 0? (I'd be surprised if vulkan allowed you > > to overstep the right edge, but not the left.) > > That is still required, from spec "The x and y members of offset must be greater > than or equal to 0" Oh ok. Is there code then that crops the scissor rect to [0,0]? In GL I started running into negative offsets.
The CQ bit was unchecked by egdaniel@google.com
On 2016/07/22 21:01:41, csmartdalton wrote: > On 2016/07/22 20:56:44, egdaniel wrote: > > On 2016/07/22 20:54:48, csmartdalton wrote: > > > > https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp > > > File src/gpu/vk/GrVkPipeline.cpp (left): > > > > > > > > > https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp... > > > src/gpu/vk/GrVkPipeline.cpp:511: SkASSERT(scissor.offset.y + > > > scissor.extent.height <= (uint32_t)target.height()); > > > Ah, looks my change gave you grief. I also removed a similar assert in GL: > > > > > > > https://codereview.chromium.org/2160093002/diff/140001/src/gpu/gl/GrGLIRect.h > > > > > > In GL, the only requirement is that width and height are non-negative. > Should > > we > > > also remove the asserts that x,y >= 0? (I'd be surprised if vulkan allowed > you > > > to overstep the right edge, but not the left.) > > > > That is still required, from spec "The x and y members of offset must be > greater > > than or equal to 0" > > Oh ok. Is there code then that crops the scissor rect to [0,0]? In GL I started > running into negative offsets. I don't think so, I guess we'll need to add that in Vulkan then
On 2016/07/22 21:05:05, egdaniel wrote: > On 2016/07/22 21:01:41, csmartdalton wrote: > > On 2016/07/22 20:56:44, egdaniel wrote: > > > On 2016/07/22 20:54:48, csmartdalton wrote: > > > > > > https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp > > > > File src/gpu/vk/GrVkPipeline.cpp (left): > > > > > > > > > > > > > > https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp... > > > > src/gpu/vk/GrVkPipeline.cpp:511: SkASSERT(scissor.offset.y + > > > > scissor.extent.height <= (uint32_t)target.height()); > > > > Ah, looks my change gave you grief. I also removed a similar assert in GL: > > > > > > > > > > https://codereview.chromium.org/2160093002/diff/140001/src/gpu/gl/GrGLIRect.h > > > > > > > > In GL, the only requirement is that width and height are non-negative. > > Should > > > we > > > > also remove the asserts that x,y >= 0? (I'd be surprised if vulkan allowed > > you > > > > to overstep the right edge, but not the left.) > > > > > > That is still required, from spec "The x and y members of offset must be > > greater > > > than or equal to 0" > > > > Oh ok. Is there code then that crops the scissor rect to [0,0]? In GL I > started > > running into negative offsets. > > I don't think so, I guess we'll need to add that in Vulkan then Do you think this is to allow the HW to use unsigned or is it possibly a spec bug?
On 2016/07/22 21:07:24, bsalomon wrote: > On 2016/07/22 21:05:05, egdaniel wrote: > > On 2016/07/22 21:01:41, csmartdalton wrote: > > > On 2016/07/22 20:56:44, egdaniel wrote: > > > > On 2016/07/22 20:54:48, csmartdalton wrote: > > > > > > > > > https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp > > > > > File src/gpu/vk/GrVkPipeline.cpp (left): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2171283004/diff/1/src/gpu/vk/GrVkPipeline.cpp... > > > > > src/gpu/vk/GrVkPipeline.cpp:511: SkASSERT(scissor.offset.y + > > > > > scissor.extent.height <= (uint32_t)target.height()); > > > > > Ah, looks my change gave you grief. I also removed a similar assert in > GL: > > > > > > > > > > > > > > https://codereview.chromium.org/2160093002/diff/140001/src/gpu/gl/GrGLIRect.h > > > > > > > > > > In GL, the only requirement is that width and height are non-negative. > > > Should > > > > we > > > > > also remove the asserts that x,y >= 0? (I'd be surprised if vulkan > allowed > > > you > > > > > to overstep the right edge, but not the left.) > > > > > > > > That is still required, from spec "The x and y members of offset must be > > > greater > > > > than or equal to 0" > > > > > > Oh ok. Is there code then that crops the scissor rect to [0,0]? In GL I > > started > > > running into negative offsets. > > > > I don't think so, I guess we'll need to add that in Vulkan then > > Do you think this is to allow the HW to use unsigned or is it possibly a spec > bug? Don't know.
added x y clamp
The CQ bit was checked by egdaniel@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com Link to the patchset: https://codereview.chromium.org/2171283004/#ps20001 (title: "Clamp to x,y to 0")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Remove asserts on scissor size in Vulkan These are asserts are firing from a recent change to our scissor code. Since these asserts were added, the Vulkan spec has been updated to no longer require the scissor is insides the bounds of the image, just that x + width does not overflow. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2171283004 ========== to ========== Remove asserts on scissor size in Vulkan These are asserts are firing from a recent change to our scissor code. Since these asserts were added, the Vulkan spec has been updated to no longer require the scissor is insides the bounds of the image, just that x + width does not overflow. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2171283004 Committed: https://skia.googlesource.com/skia/+/0e72e9ee3b0201f05e5d74917b17c8a427b14d9f ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://skia.googlesource.com/skia/+/0e72e9ee3b0201f05e5d74917b17c8a427b14d9f |
