New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 867187 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 11
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows , Chrome , Mac
Pri: 2
Type: Task



Sign in to add a comment

Enable disk shader caching for OOPR on desktop.

Project Member Reported by khushals...@chromium.org, Jul 24

Issue description

Skia limits program binary support to OpenGL 4.1 on desktop (see https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/gl/GrGLCaps.cpp?sq=package:chromium&q=GrGLCaps.cpp&g=0&l=596), which will not work with our current setup for gl bindings since that is limited to 3.2 (https://cs.chromium.org/chromium/src/ui/gl/init/create_gr_gl_interface.cc?l=61).

The most appropriate fix for this is to probably to have skia also consider the available extensions before veto-ing this on older versions, similar to what the program cache does in chromium (https://cs.chromium.org/chromium/src/gpu/command_buffer/service/memory_program_cache.cc?g=0&l=220)
 
We can also consider adding missing 4.1 entrypoints to the Chrome bindings. The current version limit was selected based on available entrypoints, but the size concern for new entrypoints is mostly relevant for Android and probably not for desktop.
Only one entry point was missing based on what skia currently queries for (https://chromium-review.googlesource.com/c/chromium/src/+/1198530). This is with me running oop-r with 4.1 and adding entry points as I discovered failures during GrGLInterface validation.

I'm not sure if this is very future proof, or correct. And whether I need to audit and set up all possible entry points supported with 4.1. 
Owner: khushals...@chromium.org
Status: Fixed (was: Available)
Fixed with this change: https://chromium-review.googlesource.com/c/chromium/src/+/1198530.
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 5

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

commit c2df980a117b4402aecd7eec2da09fd0d71b4d9f
Author: Khushal <khushalsagar@chromium.org>
Date: Tue Sep 04 17:15:23 2018

ui/gl: Set up bindings for GrGLInterface to support GL 4.1.

Add missing function pointers and GL entry points to support using
GL 4.1 with skia. The primary motivation for this is to enable disk
caching of shaders on desktop with OOP raster, since skia requires
atleast GL 4.1 to enable this.

R=kbr@chromium.org

Bug:  867187 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Ibb373fc96845286e3385d611b628524c58444ea0
Reviewed-on: https://chromium-review.googlesource.com/1198530
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#588560}
[modify] https://crrev.com/c2df980a117b4402aecd7eec2da09fd0d71b4d9f/ui/gl/generate_bindings.py
[modify] https://crrev.com/c2df980a117b4402aecd7eec2da09fd0d71b4d9f/ui/gl/gl_bindings_api_autogen_gl.h
[modify] https://crrev.com/c2df980a117b4402aecd7eec2da09fd0d71b4d9f/ui/gl/gl_bindings_autogen_gl.cc
[modify] https://crrev.com/c2df980a117b4402aecd7eec2da09fd0d71b4d9f/ui/gl/gl_bindings_autogen_gl.h
[modify] https://crrev.com/c2df980a117b4402aecd7eec2da09fd0d71b4d9f/ui/gl/gl_bindings_autogen_mock.cc
[modify] https://crrev.com/c2df980a117b4402aecd7eec2da09fd0d71b4d9f/ui/gl/gl_bindings_autogen_mock.h
[modify] https://crrev.com/c2df980a117b4402aecd7eec2da09fd0d71b4d9f/ui/gl/gl_mock_autogen_gl.h
[modify] https://crrev.com/c2df980a117b4402aecd7eec2da09fd0d71b4d9f/ui/gl/gl_stub_autogen_gl.h
[modify] https://crrev.com/c2df980a117b4402aecd7eec2da09fd0d71b4d9f/ui/gl/init/create_gr_gl_interface.cc

Labels: Merge-Request-70
Status: Assigned (was: Fixed)
There are some regressions in input latency for OOP-R finch experiment[1] on ChromeOS which could likely be related to the absence of disk shader caching.

The regression is in doing the GPU swap for the first scroll event. This is a known regression with OOP because skia's work for a single tile can't be pre-empted and shaders generally tend to be the most expensive part of that raster work. Its worthwhile to merge this change to M70.

[1]: https://uma.googleplex.com/variations?sid=0ad151cfab219c2a8335b13beeab850f
Labels: -Merge-Request-70 Merge-Approved-70
branch:3538
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 11

Labels: -merge-approved-70 merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cfd6f5f2636b33872982a7ce1bc351c0930231ca

commit cfd6f5f2636b33872982a7ce1bc351c0930231ca
Author: Khushal <khushalsagar@chromium.org>
Date: Tue Sep 11 20:00:42 2018

ui/gl: Set up bindings for GrGLInterface to support GL 4.1.

Add missing function pointers and GL entry points to support using
GL 4.1 with skia. The primary motivation for this is to enable disk
caching of shaders on desktop with OOP raster, since skia requires
atleast GL 4.1 to enable this.

R=​kbr@chromium.org

Bug:  867187 
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: Ibb373fc96845286e3385d611b628524c58444ea0
Reviewed-on: https://chromium-review.googlesource.com/1198530
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: Kenneth Russell <kbr@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#588560}(cherry picked from commit c2df980a117b4402aecd7eec2da09fd0d71b4d9f)
Reviewed-on: https://chromium-review.googlesource.com/1220032
Reviewed-by: Khushal <khushalsagar@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#288}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/cfd6f5f2636b33872982a7ce1bc351c0930231ca/ui/gl/generate_bindings.py
[modify] https://crrev.com/cfd6f5f2636b33872982a7ce1bc351c0930231ca/ui/gl/gl_bindings_api_autogen_gl.h
[modify] https://crrev.com/cfd6f5f2636b33872982a7ce1bc351c0930231ca/ui/gl/gl_bindings_autogen_gl.cc
[modify] https://crrev.com/cfd6f5f2636b33872982a7ce1bc351c0930231ca/ui/gl/gl_bindings_autogen_gl.h
[modify] https://crrev.com/cfd6f5f2636b33872982a7ce1bc351c0930231ca/ui/gl/gl_bindings_autogen_mock.cc
[modify] https://crrev.com/cfd6f5f2636b33872982a7ce1bc351c0930231ca/ui/gl/gl_bindings_autogen_mock.h
[modify] https://crrev.com/cfd6f5f2636b33872982a7ce1bc351c0930231ca/ui/gl/gl_mock_autogen_gl.h
[modify] https://crrev.com/cfd6f5f2636b33872982a7ce1bc351c0930231ca/ui/gl/gl_stub_autogen_gl.h
[modify] https://crrev.com/cfd6f5f2636b33872982a7ce1bc351c0930231ca/ui/gl/init/create_gr_gl_interface.cc

Status: Fixed (was: Assigned)

Sign in to add a comment