Could this actually be related to  crbug.com/687290 ?

+kylechar, any ozone changes recently that might affect mirroring?

I'm not aware of anything that changed in Ozone DRM. rjkroege@ would have a better idea than me though.

I can repro this on peach_pit too on 58.0.3028.0 / 9190.0.0. 
This needs to be bisected.

I can't reproduce this on link 59.0.3042.0 just FYI.

dcastagna@: you are the last person to have mutated the planes code afaik. Any ideas?

Bisecting ...

Bisect ended. Culprit CL is https://codereview.chromium.org/2626413002. Assigning to owner.

The feature introduced with https://codereview.chromium.org/2626413002 is Windows specific and turned off by default so it isn't clear how it could affect this.

It is possible that I somehow overlooked a Chrome OS specific codepath when integrating this in gpu_process_transport_factory.cc in which case the fix should be trivial.

Other that gpu_process_transport_factory.cc the rest of the change is either in WIN_OS only parts of the code or in a new code that shouldn't run unless D3DVsync feature is enabled.

Could you provide a bit more details because I am not familiar with development and testing on Chrome OS.

1) How to repro? Should I just connect an external monitor to ChromeBook?

2) Anyone who understands Display Mirror Mode code, especially gpu_process_transport_factory.cc? What should I look for? SoftwareOutputDeviceOzone?

Connect an external display to ChromeOS, and change its options in "chrome://settings/display" from "Extended" to "Mirrored".

I'm not familiar with that code, but the GpuVSyncBeginFrameSource and related changes might be the cause.

GpuVSyncBeginFrameSource is a part of the new feature. It is used only on Windows when D3DVsync feature is enabled. 

But it looks like I've unintentionally removed one line from gpu_process_transport_factory.cc:
line 565: data->display_output_surface = display_output_surface.get();

Likely that was a result of a bad merge resolution. I'll work on a patch to put this back.

That CL also breaks unified desktop mode.

Is there any way to test the fix on Windows?

Not that I know of. Send me your CL and I can validate it for you locally.

Here is the pending fix: https://codereview.chromium.org/2764633003

Your fix works! Thanks!

 Issue 696048  has been merged into this issue.

Can we get this into 58?

https://chromium.googlesource.com/chromium/src/+/165005de11ad4207b3003a42a3ce41f4ab797e67

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

commit cf279ea7eee857ebdfe71e06df71894674ce8107
Author: Stanislav Chiknavaryan <stanisc@chromium.org>
Date: Wed Mar 22 17:26:44 2017

Fixing Display Mirror Mode issue

As a part of https://codereview.chromium.org/2626413002 I've
inadvertently removed an unrelated line of code that sets
display_output_surface field on per-compositor data.

See line 565 here:
https://codereview.chromium.org/2626413002/diff/120001/content/browser/compositor/gpu_process_transport_factory.cc

This change puts the removed line of code back.

Thanks to afakhry@ for verifying the fix!

BUG= 702713 

Review-Url: https://codereview.chromium.org/2764633003
Cr-Commit-Position: refs/heads/master@{#458587}
(cherry picked from commit 165005de11ad4207b3003a42a3ce41f4ab797e67)

Review-Url: https://codereview.chromium.org/2764153003 .
Cr-Commit-Position: refs/branch-heads/3029@{#359}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/cf279ea7eee857ebdfe71e06df71894674ce8107/content/browser/compositor/gpu_process_transport_factory.cc

Still able to reproduce the issue in Chrome OS 9334.20.0, 58.0.3029.36 on Caroline. 

Mirror mode is still not working in 9334.23.0,58.0.3029.39. 

Does it work on latest Canary? I wonder if this is an issue with the merge.

It's reproducible in ToT: 9406.0.0/59.0.3051.3 too. 

Mouse cursor is seen on external screen, however there is no cursor streaking (trail is left visible). 

Hmm... But afakhry@ tried my fix and it was working for him - see comment #18.

What about the rest of the display. Is it blank as stated in repro steps above? 


I double checked my change line by line and with the exception of the accidentally removed line that I've already fixed I can't see what else could be wrong. 
Anyone of Chrome OS side familiar with Mirroring / Unified Display modes, could you take a look at my changes in https://codereview.chromium.org/2626413002 and https://codereview.chromium.org/2764633003?

I think the only changes that could be possibly relevant are those in gpu_process_transport_factory.cc.

Is it possible that your original change removed the synthetic begin frame ticking from the unified surface?

oshima@: wdyt?

rjikroege@, what is that surface type (class) in that case?

Before my change there are 3 possibilities:
1) When MusBrowserCompositorOutputSurface is created (line 543):
    begin_frame_source - comes from mus_output_surface
    synthetic_begin_frame_source - ends up being reset (nullptr)
2) Otherwise if disable_display_vsync is false:
    synthetic_begin_frame_source is DelayBasedBeginFrameSource
    begin_frame_source - the same as synthetic_begin_frame_source
3) All other cases:
    synthetic_begin_frame_source is BackToBackBeginFrameSource
    begin_frame_source - the same as synthetic_begin_frame_source
    
With https://codereview.chromium.org/2626413002 IsGpuVSyncSignalSupported() always returns false on all platforms except OS_WIN.
For CromeOS that still leaves us with 3 cases:
1) When MusBrowserCompositorOutputSurface is created (line 553):
    begin_frame_source - comes from mus_output_surface
    synthetic_begin_frame_source - never set because begin_frame_source isn't nullptr
    gpu_vsync_begin_frame_source - also remains unset
2) Otherwise if disable_display_vsync is false:
    synthetic_begin_frame_source is DelayBasedBeginFrameSource
    begin_frame_source - the same as synthetic_begin_frame_source
    gpu_vsync_begin_frame_source - remains unset
3) All other cases:
    synthetic_begin_frame_source is BackToBackBeginFrameSource
    begin_frame_source - the same as synthetic_begin_frame_source  
    gpu_vsync_begin_frame_source - remains unset

Also I should mention that I tried to reproduce this issue on my corp chromebook yesterday by connecting it to an external HP display. I switched chromebook to Dev Channel and got build 58.0.3029.31, which is even before my attempt to fix this bug. However I did't see any issue with either extending to the external monitor or the mirroring mode. Does it depend on hardware?

I can still repro this issue, however the mouse cursor shows without the trails in the external display. Something else might have happened after your fix.

I can repro at ToT on caroline.

Note: looking at logs, I can see too many of the following GL errors:

[23941:23941:0328/103402.003663:ERROR:gles2_cmd_decoder.cc(14479)] [.DisplayCompositor-0x155e48bbba00]GL ERROR :GL_INVALID_VALUE : glCopyTexSubImage2D: bad dimensions. 

[23941:23941:0328/103402.010397:ERROR:logger.cc(46)] Too many GL errors, not reporting any more for this context. use --disable-gl-error-limit to see all errors.

Could this change be related? It landed on the same day with my fix.
https://codereview.chromium.org/2762873002

Sounds like a bisect is needed.

Many ARM devices can't do hardware mirroring and so a cc::Reflector instances does it for them.

The GL error suggests that one of the output surfaces is missing maybe?

fsamuel@: do you think that your CL might have contributed?

I don't think my CL is related. It should only affect Mus.

Could anybody please help investigating this? I am not setup to build/test for CromeOS so there isn't much I can do to make more progress with this.

As an experiment we could try to revert my change, although considering comments #34 and #18 I doubt this is still related to my change.

Here is the patch to revert it if anyone has any cycles to try to apply it locally and verify if the issue is still there:
https://codereview.chromium.org/2779303002/

@39: here are instructions for building Chrome OS: https://www.chromium.org/chromium-os/developer-guide

Here are instructions for building chrome for Chrome OS:
https://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chromium-browser

Let me know if you have questions!

@40: As a Chrome Windows dev I don't currently have a Linux workstation which is one of prerequisites. I have a ChromeBook but this bug doesn't repro on it with the official Dev 58.0.3029.31 build.

I'll received your message and I will try to help you with this bug later today. Sorry, I'm just working on other things at the moment.

+halliwell@

Here is another change between in 58.0.3020.0 - 58.0.3021.0 range that looks like it could be related. In particular it contains a change related to CopyTextureSubImage call - could that be causing the "bad dimensions" errors mentioned in comment #34.

https://codereview.chromium.org/2699173002

That's a fairly mechanical refactoring, but sure ... always possible there's a mistake :)  I also am not set up to reproduce, sounds like it needs someone who can do that to help take a look?

The CL you landed @ https://bugs.chromium.org/p/chromium/issues/detail?id=702713#c9 should have fixed the issue because I tested it myself. You should be looking for something else that landed after that.

I tried your revert CL at https://codereview.chromium.org/2779303002/ and it didn't help. Something else must have landed afterwards. Yet another bisect is needed! :/

Comparing the build @ your fix CL commit and its parent commit on all kevin (ARM device), Samus (x86), and Caroline (x86), I found the following:

- On Kevin (ARM), your CL fixed the issue completely!

- On Caroline and Samus (x86), your CL only removed the trailing mouse cursors from the external display, but everything else is still black.

I think I tested your CL (earlier when you asked me to confirm) on a peach-pit, which is also an ARM-device.

Does this give some insight? I have no idea what's going on. Anything that needs to be done in x86 specifically?

@45: afakhry@, thank you very much for testing this patch! I'll look for other changes that landed around https://chromium.googlesource.com/chromium/src/+/165005de11ad4207b3003a42a3ce41f4ab797e67

Please take a look at #46, I think we posted at the exact same moment. Just making sure you won't miss it. :)

A quote from #37: "Many ARM devices can't do hardware mirroring and so a cc::Reflector instances does it for them."

My patch fixed the code that passes display_output_surface to the reflector. So that should be explaining why it is fixed on on ARM.

For x86 I suspect there is another issue that was hidden behind the initial bug. The errors in #34 indicate there are "bad dimensions." errors in glCopyTexSubImage2D. 

That must be triggered by the following code in GLES2DecoderImpl::DoCopyTexSubImage2D:

  if (!texture->GetLevelType(target, level, &type, &internal_format) ||
      !texture->ValidForTexture(
          target, level, xoffset, yoffset, 0, width, height, 1)) {
    LOCAL_SET_GL_ERROR(GL_INVALID_VALUE, func_name, "bad dimensions.");
    return;
  }

Both GetLevelType and ValidForTexture look very simple - just a bunch of checks and no actual GL calls. So this looks like passing invalid parameters to DoCopyTexSubImage2D or something like that.

I did a quick bisect on Caroline around halliwell's CL following stanisc's suspicion in comment#43. Turns out he was right. This CL is the one that broke it on X86 devices: https://codereview.chromium.org/2699173002.

Assigning to halliwell@.

Which of these surface types is used (I don't know anything about Caroline)?

Do you have a Samus? It should be the same issue there too.

I don't have any ChromeOS devices.  Which of the surface types in the review are used in this case?

halliwell: partial swap buffers is disabled on ARM (crbug.com/705290) while it is enabled on other devices.

I wonder if your CL didn't break anything intel specific but it broke the partial update path instead.

I'll grab a device tomorrow and debug this.

Ok, spent today getting hardware and putting a build on it.  I can reproduce and confirm that my CL causes the bug.  Will get to the bottom of it on Monday.

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

commit 08ae4cdea9398a15871e80126e6f2fbd47a4fa9b
Author: halliwell <halliwell@chromium.org>
Date: Mon Apr 03 22:56:45 2017

Fix screen mirroring on Chrome OS (ReflectorTexture partial swap)

Copying texture subimage requires the texture to already be
defined, so ensure ReflectorTexture performs a full copy before any
partial copies.

Bug was caused by https://codereview.chromium.org/2699173002/, since
partial swap with subrect == surface size was previously implicitly
treated as a full swap.

BUG= 702713 

Review-Url: https://codereview.chromium.org/2797443003
Cr-Commit-Position: refs/heads/master@{#461568}

[modify] https://crrev.com/08ae4cdea9398a15871e80126e6f2fbd47a4fa9b/content/browser/compositor/gpu_browser_compositor_output_surface.cc
[modify] https://crrev.com/08ae4cdea9398a15871e80126e6f2fbd47a4fa9b/content/browser/compositor/gpu_browser_compositor_output_surface.h

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Verified in 9334.40.0/58.0.3029.66 on Caroline.