New issue
Advanced search Search tips

Issue 826633 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug

Blocking:
issue 730660
issue 787099



Sign in to add a comment

VizDisplayCompositor makes the Omnibox entries disappear

Reported by kurtext...@gmail.com, Mar 28 2018

Issue description

Chrome Version       : 67.0.3381.1
OS Version: 10.0
URLs (if applicable) : https://vsynctester.com
Other browsers tested:
  Add OK or FAIL after other browsers where you have tested this issue:
     Safari: OK
    Firefox: OK
    IE/Edge: OK
    (Chrome without --enable-viz): OK

What steps will reproduce the problem?
1. Go to chrome://flags/#enable-viz-display-compositor
2. Enable #enable-viz-display-compositor (and restart the Browser)
3. Enter anything in the omnibox

What is the expected result?
You can see search results, history etc. below the omnibox

What happens instead of that?
The omnibox results don't appear

Please provide any additional information below. Attach a screenshot if
possible.

UserAgentString: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3381.1 Safari/537.36



 
Components: UI>Browser>Omnibox
Owner: kylec...@chromium.org
Status: Assigned (was: Unconfirmed)
Cc: sadrul@chromium.org
This seems like it's Windows only. I can reproduce on Windows Canary but not Linux ToT.
Cc: danakj@chromium.org
Status: Started (was: Assigned)
On Windows the omnibox popup is it's own top level HWND/surface that forces software compositing. Something is broken with software compositing here because the popup shows up if switched to use GPU compositing.
Blocking: 787099
Labels: -Pri-3 M-68 Pri-1
Summary: VizDisplayCompositor makes the Omnibox entries disappear (was: VIZ makes the Omnibox entries disappear)
I've tracked down where the problem occurs. It has to do with how SoftwareOutputDeviceWin draws to the omnibox popup HWND. SoftwareOutputDeviceWin::is_hwnd_composited_ is true for the omnibox and that changes how the EndPaint() step works. UpdateLayeredWindow() fails here:

https://cs.chromium.org/chromium/src/components/viz/service/display_embedder/software_output_device_win.cc?l=179&rcl=5d11ee41754c0190b1d22374bde5e521ed1d7814

GetLastError() returns ERROR_ACCESS_DENIED 0x5. If the GPU sandbox is disabled with --no-sandbox then the omnibox will appear. 
Cc: sunn...@chromium.org vmi...@chromium.org
The code for |is_hwnd_composited_| was added in response to  https://crbug.com/241146  where the browser window wasn't drawing correctly with software compositing.

In SoftwareOutputDeviceWin |is_hwnd_composited_| is set true when the HWND has a property that says it can be transparent. As far as I can tell, that property is only set for the omnibox popup and tooltips. According to sadrul the omnibox popup HWND size can be larger than the number of items drawn, so maybe that's why it's transparent.

The window property for transparency is based on this check:
https://cs.chromium.org/chromium/src/ui/views/widget/widget_hwnd_utils.cc?l=156&rcl=c7b67fae0c3f257e3c292ab5d97e3c96c87f6d4b 

Always setting |is_hwnd_composited_| false seems to work fine on my Windows 10 test machine. I wonder if we could just get rid of that code path? It was added in response to a bug five years ago and a lot has changed. I'm not sure if Windows 7/8 needs it or if there are other potential problems.

I'm also wondering if we can let the GPU process call UpdateLayeredWindow() somehow. I don't really understand how the GPU process sandbox works to know why/how that system call is blocked. Do we whitelist system calls?

+sunnyps/vmiura is there someone with more Windows graphics knowledge who can help here?
Cc: tsepez@chromium.org
+tsepez for if we can allow calls to Windows UpdateLayeredWindow() from the GPU process? It's blocked by the GPU sandbox currently.
I talked with tsepez@ and forshaw@ about why calls to UpdateLayeredWindow() were failing. The HWND is created/owned by the browser process. It's likely UIPI blocks the UpdateLayerWindow() and SetWindowLong() calls to that HWND from the GPU process.


The likely solution is a nested HWND created in the GPU process, parented to the browser process HWND, similar to how GPU compositing works. I'll try that.
Project Member

Comment 8 by bugdroid1@chromium.org, Apr 20 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6435a49cb07e8ad0e88900a431d6033fa5467fac

commit 6435a49cb07e8ad0e88900a431d6033fa5467fac
Author: kylechar <kylechar@chromium.org>
Date: Fri Apr 20 20:52:35 2018

Add parameter to force |is_hwnd_composited_| false.

Add an parameter to SoftwareOutputDeviceWin to always set
|is_hwnd_composited_| to false. This parameter is only set true when
running OOP-D (eg. VizDisplayCompositor feature) so it should have no
effect on normal canary users.

The reason for this CL is that the calls to SetWindowLong() and
UpdateLayeredWindow() don't work in the GPU process. The other drawing
path works correctly. Certain top level windows, for example the omnibox
popup or the URL tooltip, use this path and don't show up with OOP-D.

Not using the layered window drawing path works properly on Windows 10.
I'm not totally sure how it will effect Windows 7 and Vista, hence this
patch to make it possible to test with canary. This should be reverted
after testing.

Bug:  826633 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: If632d25c7111161e2a84caae8dc7b13d65f9ff51
Reviewed-on: https://chromium-review.googlesource.com/1022123
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#552464}
[modify] https://crrev.com/6435a49cb07e8ad0e88900a431d6033fa5467fac/components/viz/service/display_embedder/gpu_display_provider.cc
[modify] https://crrev.com/6435a49cb07e8ad0e88900a431d6033fa5467fac/components/viz/service/display_embedder/software_output_device_win.cc
[modify] https://crrev.com/6435a49cb07e8ad0e88900a431d6033fa5467fac/components/viz/service/display_embedder/software_output_device_win.h

Labels: TE-Verified-68.0.3404.0 TE-Verified-M68
Able to reproduce the issue on win-10 using chrome reported version #67.0.3381.1.

Verified the fix on Win-10 using Chrome version #68.0.3404.0 as per the comment #0.
Attaching screen cast for reference.
Observed that search results, history etc. appeared below the omnibox.
Hence, the fix is working as expected. 
Adding the verified labels.

Thanks...!!
826633.mp4
411 KB View Download
Blocking: 730660
I tested the change on Windows 7 as well. Everything works with aero on/off and gpu/software compositing as far as I can tell. I'll just try and remove the UpdateLayeredWindow() path.
Another update after getting some feedback from sunnyps. He mentioned that WS_EX_LAYERED was the only way to do transparency if DWM/aero is disabled. I did some more testing and I guess I didn't actually disable DWM/aero when testing originally. I tried again with DWM disabled on Windows 7 and there is a black area under the omnibox popup. Looking closer there is a drop shadow.

So this is a bit of a problem:

1. Layered windows are needed on Windows Vista and 7 with DWM disabled for the transparency effects.
2. We can't call UpdateLayeredWindow() from the GPU process because of the GPU sandbox. 
3. WS_EX_LAYERED is incompatible with child HWNDs on Windows Vista and 7. Excerpt from MSDN:
> Windows 8:  The WS_EX_LAYERED style is supported for top-level windows and child windows. Previous Windows versions support WS_EX_LAYERED only for top-level windows.
So creating a child window in the GPU process is probably also not an option.

Cc: piman@chromium.org
So on Win8+ we do want to use a child window, right?

Is this why the UI forced off gpu compositing for some things before we started doing it for performance/startup reasons..

On Win7, what's the best we can do for resize with SW? You mentioned you see the content repeating into the newly exposed area? Can we control what windows will draw into there? Or respond to WM_PAINT or WM_EXPOSED (I made that up) or something to show a ui background color instead?
I think for Windows 8+ we want to use a child window in the GPU process and we don't need to use WS_EX_LAYERED / UpdateLayeredWindow(). The child window makes repainting on resize nicer. DWM is always used and that just supports transparency. On Windows Vista and 7 with DWM/aero on I think we can do the same.

Windows Vista and & with DWM/aero off is the special case. Reading through some documentation I think we can still use layered windows and draw to them with the normal drawing API. Basically the following:
1. Call SetWindowLong(WS_EX_LAYERED) on the transparent window in the browser.
2. Call SetLayeredWindowAttributes() to set the blending information in the browser.
3 Draw to the Window using skia::CopyHDC() from the GPU process.
Wait, no I don't think that will work. SetLayeredWindowAttributes() doesn't take a BLENDFUNCTION where we can specify per pixel alpha the same way UpdateLayeredWindow() does.

Comment 17 by piman@chromium.org, Apr 25 2018

Because the backing for SoftwareOutputDeviceWin is in shared memory already, could we do something like write the pixels in the viz process, and then send an IPC to the browser which would create DIB/DC and call UpdateLayeredWindow() ?
piman: Yes, I think that's probably the fallback solution if nothing else works. Is it already in SharedMemory though? If we're using UpdateLayeredWindow() that means |backing_| is null. skia::CreatePlatformCanvasWithSharedSection() has nullptr |shared_section| passed in as a result. I need to read the skia code closer to figure out what happens after that.

Another solution that comes to mind, which isn't very workable, is to run the display compositor on a compositor thread in the browser process. That would be fine without SwiftShader, mojo doesn't care where that thread is, but it's messy if we have GPU process for SwiftShader that doesn't launch a compositor thread vs a GPU process for GPU acceleration that does.
Not sure if you considered this, but SetLayeredWindowAttributes allows setting a color key for transparency. However the color key is specified as a COLORREF which is RGB only (0x00RRGGBB DWORD). I don't think the window HDC has an alpha channel, so 0xFFRRGGBB is same as 0x00RRGGBB, and we can't use 0x00000000 as the color key because that would make black transparent.
sunnyps: My understanding is that color key won't work for things like drop shadows where we need partial transparency on a per pixel basis?
#20 yes, it won't work for those cases

I guess best bet is to do what piman suggested in #17
Project Member

Comment 22 by bugdroid1@chromium.org, May 4 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ad2b06d3735780ef0b1889f77557a390ea27acd

commit 2ad2b06d3735780ef0b1889f77557a390ea27acd
Author: kylechar <kylechar@chromium.org>
Date: Fri May 04 15:09:51 2018

Don't use layered windows on Windows 8+.

This CL stops using layered windows on Windows 8 and higher. Layered
window provide a way to add transparency effects on HWNDs. This is a
legacy API and DWM natively supports transparency. DWM will always be
there on Windows 8 and higher, so layered windows aren't needed.

Layered windows present GPU sandbox problems with OOP-D. Layered windows
also can't be partial updated, the entire window needs to be updated at
once.

Bug:  826633 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I6d0ab96392ee603b3b6ed680114f9e8a43c6134f
Reviewed-on: https://chromium-review.googlesource.com/1024874
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556059}
[modify] https://crrev.com/2ad2b06d3735780ef0b1889f77557a390ea27acd/components/viz/service/display_embedder/gpu_display_provider.cc
[modify] https://crrev.com/2ad2b06d3735780ef0b1889f77557a390ea27acd/components/viz/service/display_embedder/software_output_device_win.cc
[modify] https://crrev.com/2ad2b06d3735780ef0b1889f77557a390ea27acd/components/viz/service/display_embedder/software_output_device_win.h

Labels: TE-Verified-68.0.3423.0
Able to reproduce the issue on Win-10 & 8 using chrome reported version #67.0.3381.1.

Tested the issue on Windows 10 & 8 using Chrome version #64.0.3323.0 as per the issue mentioned in the original comment. Observed that issue works as intended (Observed that search results, history etc. appeared below the omnibox when flag - #enable-viz-display-compositor is enabled). Hence adding the TE verified label. Attached the screen cast for reference.

Thanks!
826633.mp4
1.2 MB View Download
Project Member

Comment 24 by bugdroid1@chromium.org, May 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1477d0b90fee2412597c604332b034e310fc98e1

commit 1477d0b90fee2412597c604332b034e310fc98e1
Author: kylechar <kylechar@chromium.org>
Date: Tue May 08 14:29:43 2018

Split SoftwareOutputDeviceWin class.

SoftwareOutputDeviceWin has two code paths to draw currently. With
OOP-D there will additional implementations needed. Instead of
shoehorning everything into one class, split into two separate classes
and provide a function to get the correct one.

This doesn't really reduce code now but it will make it simpler to add
implementations for OOP-D and to remove implementations in the future
after OOP-D is launched.

There is shared base class with some simple logic and DCHECKs. There are
then two implementations, one that draws directly the HWND and one that
draws using layered window APIs.

Bug:  826633 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Ic7819261125e776ab26ce664cac47994e234948d
Reviewed-on: https://chromium-review.googlesource.com/1044291
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556792}
[modify] https://crrev.com/1477d0b90fee2412597c604332b034e310fc98e1/components/viz/service/BUILD.gn
[modify] https://crrev.com/1477d0b90fee2412597c604332b034e310fc98e1/components/viz/service/display_embedder/gpu_display_provider.cc
[modify] https://crrev.com/1477d0b90fee2412597c604332b034e310fc98e1/components/viz/service/display_embedder/gpu_display_provider.h
[add] https://crrev.com/1477d0b90fee2412597c604332b034e310fc98e1/components/viz/service/display_embedder/output_device_backing.cc
[add] https://crrev.com/1477d0b90fee2412597c604332b034e310fc98e1/components/viz/service/display_embedder/output_device_backing.h
[add] https://crrev.com/1477d0b90fee2412597c604332b034e310fc98e1/components/viz/service/display_embedder/output_device_backing_unittest.cc
[modify] https://crrev.com/1477d0b90fee2412597c604332b034e310fc98e1/components/viz/service/display_embedder/software_output_device_win.cc
[modify] https://crrev.com/1477d0b90fee2412597c604332b034e310fc98e1/components/viz/service/display_embedder/software_output_device_win.h
[modify] https://crrev.com/1477d0b90fee2412597c604332b034e310fc98e1/content/browser/compositor/gpu_process_transport_factory.cc

Project Member

Comment 25 by bugdroid1@chromium.org, May 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c4882e89e58af55e374072fcee6e103a3b15bdc0

commit c4882e89e58af55e374072fcee6e103a3b15bdc0
Author: Findit <findit-for-me@appspot.gserviceaccount.com>
Date: Tue May 08 16:10:18 2018

Revert "Split SoftwareOutputDeviceWin class."

This reverts commit 1477d0b90fee2412597c604332b034e310fc98e1.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 556792 as the
culprit for failures in the build cycles as shown on:
https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzE0NzdkMGI5MGZlZTI0MTI1OTdjNjA0MzMyYjAzNGUzMTBmYzk4ZTEM

Sample Failed Build: https://ci.chromium.org/buildbot/chromium.win/Win%207%20Tests%20x64%20%281%29/38084

Sample Failed Step: viz_unittests

Original change's description:
> Split SoftwareOutputDeviceWin class.
> 
> SoftwareOutputDeviceWin has two code paths to draw currently. With
> OOP-D there will additional implementations needed. Instead of
> shoehorning everything into one class, split into two separate classes
> and provide a function to get the correct one.
> 
> This doesn't really reduce code now but it will make it simpler to add
> implementations for OOP-D and to remove implementations in the future
> after OOP-D is launched.
> 
> There is shared base class with some simple logic and DCHECKs. There are
> then two implementations, one that draws directly the HWND and one that
> draws using layered window APIs.
> 
> Bug:  826633 
> Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
> Change-Id: Ic7819261125e776ab26ce664cac47994e234948d
> Reviewed-on: https://chromium-review.googlesource.com/1044291
> Commit-Queue: kylechar <kylechar@chromium.org>
> Reviewed-by: danakj <danakj@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#556792}

Change-Id: Id9dfcb6f3fb2999f307ca9058e274cac7c827f93
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  826633 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Reviewed-on: https://chromium-review.googlesource.com/1050405
Cr-Commit-Position: refs/heads/master@{#556814}
[modify] https://crrev.com/c4882e89e58af55e374072fcee6e103a3b15bdc0/components/viz/service/BUILD.gn
[modify] https://crrev.com/c4882e89e58af55e374072fcee6e103a3b15bdc0/components/viz/service/display_embedder/gpu_display_provider.cc
[modify] https://crrev.com/c4882e89e58af55e374072fcee6e103a3b15bdc0/components/viz/service/display_embedder/gpu_display_provider.h
[delete] https://crrev.com/a6d3b2c20dd8ab040c9a002b50511171ad74a8d3/components/viz/service/display_embedder/output_device_backing.cc
[delete] https://crrev.com/a6d3b2c20dd8ab040c9a002b50511171ad74a8d3/components/viz/service/display_embedder/output_device_backing.h
[delete] https://crrev.com/a6d3b2c20dd8ab040c9a002b50511171ad74a8d3/components/viz/service/display_embedder/output_device_backing_unittest.cc
[modify] https://crrev.com/c4882e89e58af55e374072fcee6e103a3b15bdc0/components/viz/service/display_embedder/software_output_device_win.cc
[modify] https://crrev.com/c4882e89e58af55e374072fcee6e103a3b15bdc0/components/viz/service/display_embedder/software_output_device_win.h
[modify] https://crrev.com/c4882e89e58af55e374072fcee6e103a3b15bdc0/content/browser/compositor/gpu_process_transport_factory.cc

Project Member

Comment 26 by bugdroid1@chromium.org, May 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/65efc1a4346974c84b5600d5468e2ffb03d12a97

commit 65efc1a4346974c84b5600d5468e2ffb03d12a97
Author: kylechar <kylechar@chromium.org>
Date: Tue May 08 21:00:12 2018

RELAND: Split SoftwareOutputDeviceWin class.

SoftwareOutputDeviceWin has two code paths to draw currently. With
OOP-D there will additional implementations needed. Instead of
shoehorning everything into one class, split into two separate classes
and provide a function to get the correct one.

This doesn't really reduce code now but it will make it simpler to add
implementations for OOP-D and to remove implementations in the future
after OOP-D is launched.

There is shared base class with some simple logic and DCHECKs. There are
then two implementations, one that draws directly the HWND and one that
draws using layered window APIs.

Landed in https://crrev.com/c/1044291 originally. I had logic inside of
a DCHECK which of course never runs with DCHECKs off. The tests relying
on that logic failed as a result.

Bug:  826633 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: Ic982aed50963b45cb0bb92a86da7dc9fbc0038ba
Reviewed-on: https://chromium-review.googlesource.com/1050608
Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556956}
[modify] https://crrev.com/65efc1a4346974c84b5600d5468e2ffb03d12a97/components/viz/service/BUILD.gn
[modify] https://crrev.com/65efc1a4346974c84b5600d5468e2ffb03d12a97/components/viz/service/display_embedder/gpu_display_provider.cc
[modify] https://crrev.com/65efc1a4346974c84b5600d5468e2ffb03d12a97/components/viz/service/display_embedder/gpu_display_provider.h
[add] https://crrev.com/65efc1a4346974c84b5600d5468e2ffb03d12a97/components/viz/service/display_embedder/output_device_backing.cc
[add] https://crrev.com/65efc1a4346974c84b5600d5468e2ffb03d12a97/components/viz/service/display_embedder/output_device_backing.h
[add] https://crrev.com/65efc1a4346974c84b5600d5468e2ffb03d12a97/components/viz/service/display_embedder/output_device_backing_unittest.cc
[modify] https://crrev.com/65efc1a4346974c84b5600d5468e2ffb03d12a97/components/viz/service/display_embedder/software_output_device_win.cc
[modify] https://crrev.com/65efc1a4346974c84b5600d5468e2ffb03d12a97/components/viz/service/display_embedder/software_output_device_win.h
[modify] https://crrev.com/65efc1a4346974c84b5600d5468e2ffb03d12a97/content/browser/compositor/gpu_process_transport_factory.cc

Project Member

Comment 27 by bugdroid1@chromium.org, May 11 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/45d37513595436da35fc22f373d4d082e92ae8e2

commit 45d37513595436da35fc22f373d4d082e92ae8e2
Author: kylechar <kylechar@chromium.org>
Date: Fri May 11 16:57:31 2018

Change swap ack with software compositing.

This CL moves the responsibility for acknowledging that swap has
occurred from SoftwareOutputSurface to SoftwareOutputDevice. This is to
allow handling asynchronous swap better. This CL should have no change
in behaviour but a follow up CL will add an overridden implementation of
SoftwareOutputDevice::OnSwapBuffers().

The main change is that instead of SoftwareOutputSurface holding a
TaskRunner the SoftwareOutputDevice does. Only the macOS
SoftwareOutputDevice needs a specific TaskRunner provided. All other
SoftwareOutputDevice implementations just grab the current one on
construction.

Bug:  826633 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I97f487db90fe18f4182d2210539f61f2cb694c6a
Reviewed-on: https://chromium-review.googlesource.com/1055476
Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557910}
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/components/viz/service/display/software_output_device.cc
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/components/viz/service/display/software_output_device.h
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/components/viz/service/display_embedder/gpu_display_provider.cc
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/components/viz/service/display_embedder/software_output_device_mac.cc
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/components/viz/service/display_embedder/software_output_device_mac.h
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/components/viz/service/display_embedder/software_output_device_mac_unittest.mm
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/components/viz/service/display_embedder/software_output_surface.cc
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/components/viz/service/display_embedder/software_output_surface.h
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/content/browser/compositor/gpu_process_transport_factory.cc
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/content/browser/compositor/gpu_process_transport_factory.h
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/content/browser/compositor/software_browser_compositor_output_surface.cc
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/content/browser/compositor/software_browser_compositor_output_surface.h
[modify] https://crrev.com/45d37513595436da35fc22f373d4d082e92ae8e2/content/browser/compositor/software_browser_compositor_output_surface_unittest.cc

Project Member

Comment 28 by bugdroid1@chromium.org, May 14 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ba2f0e599a479592e5065a6b94b22794e7bf012b

commit ba2f0e599a479592e5065a6b94b22794e7bf012b
Author: kylechar <kylechar@chromium.org>
Date: Mon May 14 21:17:24 2018

viz: Make UpdateLayeredWindow() work with OOP-D.

This CL adds a SoftwareOutputDevice implementation for Windows software
compositing to a layered window. The layered window API is required to
support transparency when DWM is disabled. Calling UpdateLayeredWindow()
is also blocked by the GPU sandbox.

To work around this SoftwareOutputDeviceWinProxy in the GPU process
draws into a shared memory buffer and sends an IPC to the browser
process. LayeredWindowUpdaterImpl receives the IPC and calls
UpdateLayeredWindow().

Having the browser process draw into an HWND with pixels produced in the
GPU process shouldn't be a security issue. The GPU process can already
draw into the HWND via other APIs. Drawing is clipped to the HWND size.

It's not ideal having to send an IPC from the GPU to browser process for
each draw. However, layered windows are the only way for an HWND to
guarantee it supports transparency on Windows 7. Having a child window
in the GPU process is not possible as layered windows are incompatible
with child windows for Windows 7.

HWNDs with transparent backgrounds are used infrequently and don't
update quickly so this should hopefully be performant enough. This code
is also only used for Windows 7.

Bug:  826633 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel
Change-Id: I3b2d749b75aed7789b19b3e8dfd7d0371ef6fe73
Reviewed-on: https://chromium-review.googlesource.com/1042283
Commit-Queue: kylechar <kylechar@chromium.org>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558462}
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/common/BUILD.gn
[add] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/common/display/use_layered_window.cc
[add] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/common/display/use_layered_window.h
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/host/BUILD.gn
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/host/DEPS
[add] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/host/layered_window_updater_impl.cc
[add] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/host/layered_window_updater_impl.h
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/service/display_embedder/display_provider.h
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/service/display_embedder/gpu_display_provider.cc
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/service/display_embedder/gpu_display_provider.h
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/service/display_embedder/software_output_device_win.cc
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/service/display_embedder/software_output_device_win.h
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/service/frame_sinks/frame_sink_manager_impl.cc
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/test/mock_display_client.h
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/test/test_display_provider.cc
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/components/viz/test/test_display_provider.h
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/content/browser/compositor/gpu_process_transport_factory.cc
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/content/browser/compositor/in_process_display_client.cc
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/content/browser/compositor/in_process_display_client.h
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/services/viz/privileged/interfaces/compositing/BUILD.gn
[modify] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/services/viz/privileged/interfaces/compositing/display_private.mojom
[add] https://crrev.com/ba2f0e599a479592e5065a6b94b22794e7bf012b/services/viz/privileged/interfaces/compositing/layered_window_updater.mojom

Status: Fixed (was: Started)

Sign in to add a comment