Can repro on Scarlet

I misunderstood this as a regression, but probably screenshot never worked
on RK because ARM Malis use ASTC compressed format, which is what is 
misinterpreted as ARGB8888.
https://www.khronos.org/registry/OpenGL/extensions/OES/OES_texture_compression_astc.txt

Joe, do you have an idea what to do here? 

Rockchip has a way to read back (Sec 3.3.8 "Write back" of the TRM) the
contents being displayed on the screen but wiring it wouldn't be trivial
(and there isn't a user space exposure of this). 

Also Intel seems to work now because KMS/DRM is going to hocus-pocus
reconcile whatever the internal format with a linear read-back but that
isn't guaranteed to stay the same. (maybe dcastagna@/hoegsberg@ can say
more about this?).

Currently the screenshot tool looks at fbs in KMS state. Unfortunately KMS does not expose modifiers, and even if it did, we wouldn't know how to interpret certain format/modifier configuration.
Currently the screenshot tool incorrectly assumes RGB linear.

We discussed this briefly with Kristian and Sean.
We believe a good alternative to get the scanned out content is to use the write back connectors in KMS: https://01.org/linuxgraphics/gfx-docs/drm/gpu/drm-kms.html#writeback-connectors


Rockchip display controller supports write back, but we'd need to implement it in the driver.

It turns out that this is also tracked by http://b/111620036.

This is also causing breakage in new Crostini tests as well.

I really like Daniel's write back connector as a fix for this. I am wondering if this is available on all the platforms with this trouble?  I am working on some blockers right now, but I can look into this some more next week. 

Yes, I think fixing CTS/dEQP failures is more important.

BTW how does Chrome take a screenshot? Can we do the same way?

I actually wrote the code that does screen mirroring for ARC++ by having Chrome do the capture. The relevant code is here:

https://cs.chromium.org/chromium/src/chrome/browser/chromeos/arc/screen_capture/arc_screen_capture_session.cc?dr=CSs&sq=package:chromium&g=0

But it's getting a graphics buffer from Android and then rendering into that on the Chrome side (but that buffer can still be mapped for access)....so it's probably not going to be useful for a standalone program like screenshot.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/d793df3847aafa03dbc881ba533872a1b6e73926

commit d793df3847aafa03dbc881ba533872a1b6e73926
Author: Daniel Erat <derat@chromium.org>
Date: Wed Sep 26 17:31:58 2018

tasts-tests: Update graphics.Screenshot dependency.

Make the graphics.Screenshot test depend on a new
"screenshot" feature. It formerly depended on
"display_backlight" to exclude systems without internal
displays, but the new feature also excludes RK3399 systems
(on which the screenshot tool produces garbled images).

BUG=chromium:880597
TEST=graphics.Screenshot is skipped on kevin but still runs
     on caroline
CQ-DEPEND=Ief46119fe68cf064bdd3171217e58f13147b8758

Change-Id: Iab50f58767061092baa062d61f5ad8636aa2be01
Reviewed-on: https://chromium-review.googlesource.com/1244397
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jeffrey Kardatzke <jkardatzke@google.com>

[modify] https://crrev.com/d793df3847aafa03dbc881ba533872a1b6e73926/src/chromiumos/tast/local/bundles/cros/graphics/screenshot.go

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast/+/86c64375bfb495b6b97219d7ffcd91692a316765

commit 86c64375bfb495b6b97219d7ffcd91692a316765
Author: Daniel Erat <derat@chromium.org>
Date: Wed Sep 26 17:31:58 2018

tast: Add "screenshot" dependency.

Add a "screenshot" feature that can be used to skip testing
on devices that don't have internal displays or that are
based on RK3399.

BUG=chromium:880597
TEST=verified that "tast run" reports screenshot feature
CQ-DEPEND=I05cff49141a798c750a932dd43b079c6ce9be6e6

Change-Id: Ief46119fe68cf064bdd3171217e58f13147b8758
Reviewed-on: https://chromium-review.googlesource.com/1244417
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jeffrey Kardatzke <jkardatzke@google.com>

[modify] https://crrev.com/86c64375bfb495b6b97219d7ffcd91692a316765/src/chromiumos/cmd/local_test_runner/main.go
[modify] https://crrev.com/86c64375bfb495b6b97219d7ffcd91692a316765/docs/test_dependencies.md

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/e77c3588259b68d68825eae4117b515a4c4d66dc

commit e77c3588259b68d68825eae4117b515a4c4d66dc
Author: Daniel Erat <derat@chromium.org>
Date: Wed Sep 26 17:31:57 2018

tast-use-flags: Add rk3399 to IUSE.

The screenshot command saves incorrect images on
RK3399-based devices due to a compressed format being
misinterpreted as ARGB8888.

Make tast-use-flags capture the rk3399 USE flag so we can
add a "screenshot" dependency that checks that rk3399 is
unset.

BUG=chromium:880597
TEST=emerged tast-use-flags

Change-Id: I05cff49141a798c750a932dd43b079c6ce9be6e6
Reviewed-on: https://chromium-review.googlesource.com/1244058
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Jeffrey Kardatzke <jkardatzke@google.com>

[rename] https://crrev.com/e77c3588259b68d68825eae4117b515a4c4d66dc/chromeos-base/tast-use-flags/tast-use-flags-0.0.1-r8.ebuild
[modify] https://crrev.com/e77c3588259b68d68825eae4117b515a4c4d66dc/chromeos-base/tast-use-flags/tast-use-flags-0.0.1.ebuild

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

commit 2b323e1e757bfbc5f62bea7bad11744fd688eacc
Author: Jeffrey Kardatzke <jkardatzke@google.com>
Date: Fri Sep 28 19:12:09 2018

Add autotest private API for taking a screenshot

This is a temporary solution to taking screenshots for tast-tests that
will work properly on ARM devices as well as deal with hardware
overlays. Longer term the plan is to implement this via the DRM API with
a write-back connector and then use that from the CLI screenshot
program.

BUG= chromium:849438 ,chromium:880597, chromium:887016 
TEST=tast run graphics.Screenshot (after updating it to use this)

Change-Id: I3919822599338e4786c715d7b0419742417b83c2
Reviewed-on: https://chromium-review.googlesource.com/1247181
Commit-Queue: Jeffrey Kardatzke <jkardatzke@google.com>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Reviewed-by: Achuith Bhandarkar <achuith@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595173}
[modify] https://crrev.com/2b323e1e757bfbc5f62bea7bad11744fd688eacc/chrome/browser/chromeos/extensions/autotest_private/DEPS
[modify] https://crrev.com/2b323e1e757bfbc5f62bea7bad11744fd688eacc/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.cc
[modify] https://crrev.com/2b323e1e757bfbc5f62bea7bad11744fd688eacc/chrome/browser/chromeos/extensions/autotest_private/autotest_private_api.h
[modify] https://crrev.com/2b323e1e757bfbc5f62bea7bad11744fd688eacc/chrome/common/extensions/api/autotest_private.idl
[modify] https://crrev.com/2b323e1e757bfbc5f62bea7bad11744fd688eacc/chrome/test/data/extensions/api_test/autotest_private/test.js
[modify] https://crrev.com/2b323e1e757bfbc5f62bea7bad11744fd688eacc/extensions/browser/extension_function_histogram_value.h
[modify] https://crrev.com/2b323e1e757bfbc5f62bea7bad11744fd688eacc/tools/metrics/histograms/enums.xml

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/tast-tests/+/7938385ddbb8683775b47fb2c6efc4f6696c4706

commit 7938385ddbb8683775b47fb2c6efc4f6696c4706
Author: Jeffrey Kardatzke <jkardatzke@google.com>
Date: Tue Oct 02 12:19:19 2018

tast-tests: Use autotest API for screenshots

This is a temporary solution until the CLI screenshot program is updated
to work with ARM devices and also handle hardware overlays.

BUG= chromium:849438 ,chromium:880597, chromium:887016 
TEST=tast run graphics.ScreenshotChrome graphics.ScreenshotCLI
vm.CrostiniStartEverything

Change-Id: Idb908b55097370158a45737a425a896414ea4e93
Reviewed-on: https://chromium-review.googlesource.com/1246690
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Jeffrey Kardatzke <jkardatzke@google.com>
Reviewed-by: Jeffrey Kardatzke <jkardatzke@google.com>

[modify] https://crrev.com/7938385ddbb8683775b47fb2c6efc4f6696c4706/src/chromiumos/tast/local/bundles/cros/vm/crostini_start_everything.go
[modify] https://crrev.com/7938385ddbb8683775b47fb2c6efc4f6696c4706/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_launcher_app.go
[modify] https://crrev.com/7938385ddbb8683775b47fb2c6efc4f6696c4706/src/chromiumos/tast/local/bundles/cros/vm/subtest/verify_app_from_terminal.go
[modify] https://crrev.com/7938385ddbb8683775b47fb2c6efc4f6696c4706/src/chromiumos/tast/local/screenshot/screenshot.go
[add] https://crrev.com/7938385ddbb8683775b47fb2c6efc4f6696c4706/src/chromiumos/tast/local/bundles/cros/graphics/screenshot_cli.go
[rename] https://crrev.com/7938385ddbb8683775b47fb2c6efc4f6696c4706/src/chromiumos/tast/local/bundles/cros/graphics/sshot/sshot.go
[add] https://crrev.com/7938385ddbb8683775b47fb2c6efc4f6696c4706/src/chromiumos/tast/local/bundles/cros/graphics/screenshot_chrome.go

Related to  issue 887016 

#16: What's the connection?  https://crbug.com/887016  also affects eve -- is the point that the fix discussed here would also fix that issue?

FWIW, it seems that '/usr/local/sbin/screenshot' command cannot capture hw-overlayed windows.
I did some tests with ARC++ windows, and I see a black screen.
Not sure whether fixing hw-overlay screenshots is outside the scope of this bug. A simple workaround is to inject Ctrl+F4 (or Ctrl+F5) and let Chrome take the screenshot.

Deleted: hw_overlay_screenshot.png 1.0 MB

	hw_overlay_screenshot.png 1.0 MB View Download

That's correct, the screenshot command can't capture overlays. We could consider an improvement to make that work. But it's a separate bug from the AFBC decompression inability, so we should open a separate ticket.

marcheu@ done: https://crbug.com/914890

This issue has an owner, a component and a priority, but is still listed as untriaged or unconfirmed. By definition, this bug is triaged. Changing status to "assigned". Please reach out to me if you disagree with how I've done this.