Passthrough command decoder doesn't handle direct composition draw offset |
|||||
Issue descriptionUnder direct composition, the default framebuffer is either a swap chain or dcomp surface depending on whether overlays are being used or not. When we use a dcomp surface, we have to do all drawing in between BeginDraw (called by SetDrawRectangle command) and EndDraw (called by SwapBuffers command). BeginDraw takes a damage rect and returns a surface pointer and draw offset. We have to draw within the damage rect but offset all drawing commands by the returned offset. We implement this in validating decoder by adjusting viewport and scissor on every glViewport/Scissor call and when switching framebuffers (see GetBoundFramebufferDrawOffset function). Passthrough decoder does not support draw offset. This can manifest as a benign one row and column of black pixels (noticed on my workstation when returned offset is 1x1) to serious drawing corruption. This cannot be implemented easily in passthrough decoder except by caching the viewport and scissor state which breaks the "passthrough" nature of the decoder. piman@ suggested implementing an ANGLE EGL extension that allows specifying the draw offset when we create the pbuffer surface for the dcomp surface. The multiview extension does something similar but the offset is specified when binding the fbo I think? Note: We need do adjust viewport and scissor at the very least when switching framebuffers because viewport and scissor state is independent of fbo state. We might want to adjust BlitFramebuffer and ReadPixels too which we don't do on validating decoder today, and it somehow works? P1 because this is breaks direct composition which is needed for power savings on Windows. I'd be happy to own this if ANGLE folks could provide some guidance.
,
Oct 23
We could emulate this in the passthrough decoder, glViewport and glScissor calls are not in the critical performance path. Implementing as an EGL extension would also be fine. Even better if the pbuffer has the same draw offset for it's whole lifetime and we can specify it upfront. If the validating/raster decoder only need this feature on top of direct composition we can then use the extension instead of doing the offsets in the decoders. The only problems I see are around how much of the API should respect the offsets, should it affect things like glReadPixels, glCopyTexImage, etc? It's possible to handle this but more work. Happy to explain how this would be implemented in ANGLE if you want to add it yourself.
,
Oct 23
AFAIUI it does need to handle glReadPixels & glCopyTex(Sub)Image2D, which we use for readbacks and some compositing workloads (e.g. filters)
,
Oct 23
Let's start with scissor and viewport because that's what validating implements right now. The dcomp surface pbuffer will have the same draw offset for its entire lifetime. We recreate the pbuffer on each frame. We'll need to adjust viewport and scissor on framebuffer switch, and that's the part that's difficult with passthrough decoder. Geoff, can you provide some pointers for implementing this as an EGL extension so I can start prototyping it?
,
Oct 24
Sure, We'll probably need two new enums for X and Y offset to pass to eglCreatePbufferFromClientBuffer, our unused range of EGL enums is currently 0x3481-0x348F. Add an entry in DisplayExtensions: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/Caps.h?l=678 Expose the extension string: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/Caps.cpp?l=1363. Enable the extension for D3D11 here: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/renderer/d3d/d3d11/Renderer11.cpp?l=1141 Validate any new parameters here: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/validationEGL.cpp?l=1291 You can unpack the offsets and store them in SurfaceD3D: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/renderer/d3d/SurfaceD3D.h Trigger re-sync of the scissor/viewport by invalidating them when the draw framebuffer binding changes: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/renderer/d3d/d3d11/StateManager11.cpp?l=1009 Apply the offset viewport for drawing here: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/renderer/d3d/d3d11/StateManager11.cpp?l=1313 and scissor here: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/renderer/d3d/d3d11/StateManager11.cpp?l=1284 You can cast the framebuffer to a Framebuffer11 which can query the surface for offsets That should be enough for equivalent functionality to the validating decoder. ReadPixels can be done by modifying the read area in Framebuffer11: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/renderer/d3d/d3d11/Framebuffer11.cpp?l=250 Blit too: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/renderer/d3d/d3d11/Framebuffer11.cpp?l=277 CopyTex(Sub)Image may be a bit more complicated but it's done in the TextureD3D classes: https://cs.chromium.org/chromium/src/third_party/angle/src/libANGLE/renderer/d3d/TextureD3D.cpp
,
Oct 25
FWIW this bug causes flashing at small window sizes: https://screenshot.googleplex.com/5cbLJkN7qbW Maggie noticed this first when testing underlays, but I can confirm this happens with passthrough and doesn't happen with validating decoder. I presume Windows uses a different surface when a window is smaller than a certain size, and the draw offsets are arbitrary (not just 0,0 or 1,1).
,
Oct 25
Note: I should say that it happens when either width or height are below a threshold, not the size.
,
Oct 25
Great job pin point the issue of this flash. This is a blocker for passthrough command buffer. Geoff, can you take this? Sunny already has a lot on his place.
,
Oct 25
Issue 885191 has been merged into this issue.
,
Oct 30
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ff9a0b7dc3f5fed73f1b62ef39cab1ecb264e4b3 commit ff9a0b7dc3f5fed73f1b62ef39cab1ecb264e4b3 Author: Geoff Lang <geofflang@chromium.org> Date: Tue Oct 30 15:37:51 2018 Respect surface draw offset in the passthrough cmd decoder. Offset the scissor and viewports based on the current surface's draw offset. This is needed for small direct composition surfaces. BUG= 898001 Change-Id: I9869a5513ff728f77c70b35aba8757c969b53adc Reviewed-on: https://chromium-review.googlesource.com/c/1301955 Commit-Queue: Geoff Lang <geofflang@chromium.org> Reviewed-by: Zhenyao Mo <zmo@chromium.org> Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org> Cr-Commit-Position: refs/heads/master@{#603910} [modify] https://crrev.com/ff9a0b7dc3f5fed73f1b62ef39cab1ecb264e4b3/gpu/command_buffer/service/gles2_cmd_decoder_passthrough.cc [modify] https://crrev.com/ff9a0b7dc3f5fed73f1b62ef39cab1ecb264e4b3/gpu/command_buffer/service/gles2_cmd_decoder_passthrough.h [modify] https://crrev.com/ff9a0b7dc3f5fed73f1b62ef39cab1ecb264e4b3/gpu/command_buffer/service/gles2_cmd_decoder_passthrough_doers.cc
,
Dec 3
Closing this as fixed, will file a followup bug for implementing this as an extension in ANGLE.
,
Dec 3
|
|||||
►
Sign in to add a comment |
|||||
Comment 1 by sunn...@chromium.org
, Oct 23Status: Assigned (was: Available)