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

Issue 775740 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

GLHelper scaling support for "patching" for performance

Project Member Reported by m...@chromium.org, Oct 17 2017

Issue description

For the CopyOutputRequest execution use cases, GLHelper scaling should be able to support two methods of scaling just subsets of an entire original texture, by allowing the client to do either of the following:

1. Given the same source texture, but different output rects.

2. Given a smaller source texture (with overscan accounted for), and with an output rect having an upper-left corner of (0,0).

This would allow for a huge performance boost to tab/desktop capture from the compositor, since the entire pipeline would be able to do far less work to generate a new frame that hasn't changed much from the prior frame.

#1 is a matter of doing the right rect calculations throughout the multiple stages of the scaler impl.

#2 is much more difficult, as this would require some of the scaler shader programs to be modified to operate on fractional coordinates (e.g., the bicubic scaler seems to assume whole-numbered source pixel coordinates, causing offset errors in the results).
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 19 2017

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

commit 904e66900a801d6aa98d969c0913456628c0010c
Author: Yuri Wiitala <miu@chromium.org>
Date: Thu Oct 19 03:13:44 2017

GLHelperScaling fixes and initial "patching" performance support.

Fixes a number of existing bugs when requesting scaling of a subset of
the entire output (e.g., when the output rect does not have an origin of
0,0): 1) Corrected overscan calculations; 2) Math fixes, including
proper accounting for vertical flipping; 3) Cleaned-up some confusing
variable naming.

Added a plethora of unit testing for use of: 1) the source texture
re-positioning offset; and 2) output rects with non-0,0 origins.

TODO: Currently, scaling using a source texture re-positioning offset
having non-whole-numbered coordinates is broken. This is documented and
guarded with DCHECKs for safety. There is not yet a use case for this,
but the issues will be addressed in a later change.

Bug:  775740 
Change-Id: Iaa783a5e9efa37f16abbf2a4a8660cf843efb5c7
Reviewed-on: https://chromium-review.googlesource.com/724426
Reviewed-by: Xiangjun Zhang <xjz@chromium.org>
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509985}
[modify] https://crrev.com/904e66900a801d6aa98d969c0913456628c0010c/components/viz/common/gl_helper.cc
[modify] https://crrev.com/904e66900a801d6aa98d969c0913456628c0010c/components/viz/common/gl_helper.h
[modify] https://crrev.com/904e66900a801d6aa98d969c0913456628c0010c/components/viz/common/gl_helper_benchmark.cc
[modify] https://crrev.com/904e66900a801d6aa98d969c0913456628c0010c/components/viz/common/gl_helper_scaling.cc
[modify] https://crrev.com/904e66900a801d6aa98d969c0913456628c0010c/components/viz/common/gl_helper_unittest.cc
[modify] https://crrev.com/904e66900a801d6aa98d969c0913456628c0010c/components/viz/service/display/gl_renderer_copier.cc

Comment 2 Deleted

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 23 2017

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

commit 43ebdaa507565a9abae4ce04b51e8fc7cd53c9f3
Author: Yuri Wiitala <miu@chromium.org>
Date: Mon Oct 23 22:18:02 2017

GLHelper: Split vertical-flip into the two separate things it means.

This changes splits the GLHelper scaler's "vertical flip" mode into two
separate things: 1) Whether the source texture content is vertically
flipped (for the source rect math), and 2) Whether the scaler's output
should be vertically flipped (to prevent the need to reverse the row
order in a later readback operation).

Bug:  760348 ,  775740 
Change-Id: Ieabfa0a6ec151ba47470677b45518604c7a80a26
Reviewed-on: https://chromium-review.googlesource.com/731766
Reviewed-by: Xiangjun Zhang <xjz@chromium.org>
Reviewed-by: Fady Samuel <fsamuel@chromium.org>
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#510938}
[modify] https://crrev.com/43ebdaa507565a9abae4ce04b51e8fc7cd53c9f3/components/viz/common/gl_helper.cc
[modify] https://crrev.com/43ebdaa507565a9abae4ce04b51e8fc7cd53c9f3/components/viz/common/gl_helper.h
[modify] https://crrev.com/43ebdaa507565a9abae4ce04b51e8fc7cd53c9f3/components/viz/common/gl_helper_benchmark.cc
[modify] https://crrev.com/43ebdaa507565a9abae4ce04b51e8fc7cd53c9f3/components/viz/common/gl_helper_scaling.cc
[modify] https://crrev.com/43ebdaa507565a9abae4ce04b51e8fc7cd53c9f3/components/viz/common/gl_helper_scaling.h
[modify] https://crrev.com/43ebdaa507565a9abae4ce04b51e8fc7cd53c9f3/components/viz/common/gl_helper_unittest.cc
[modify] https://crrev.com/43ebdaa507565a9abae4ce04b51e8fc7cd53c9f3/components/viz/service/display/gl_renderer_copier.cc
[modify] https://crrev.com/43ebdaa507565a9abae4ce04b51e8fc7cd53c9f3/content/browser/media/capture/aura_window_capture_machine.cc
[modify] https://crrev.com/43ebdaa507565a9abae4ce04b51e8fc7cd53c9f3/content/browser/renderer_host/delegated_frame_host.cc

Comment 4 by m...@chromium.org, Nov 8 2017

Labels: -Pri-2 Pri-3
At this point, most of the work is done. However, one lingering issue should be addressed at some point: "some of the scaler shader programs to be modified to operate on fractional coordinates (e.g., the bicubic scaler seems to assume whole-numbered source pixel coordinates, causing offset errors in the results)."

Since what was immediately needed is done, and the rest is lower priority, I'm downgrading the priority of this bug.

Comment 5 by m...@chromium.org, Feb 1 2018

Components: Internals>Media>Capture

Comment 6 by m...@chromium.org, Feb 1 2018

Components: -Blink>GetUserMedia>Tab

Comment 7 by m...@chromium.org, Feb 7 2018

Components: Internals>Media>ScreenCapture

Comment 8 by m...@chromium.org, Feb 7 2018

Components: -Internals>Media>Capture
Status: Fixed (was: Started)
This was addressed in the new viz::GLScaler replacement.
Labels: Hotlist-DesktopUIChecked
*** UI Mass Triage ***

Sign in to add a comment