New issue
Advanced search Search tips

Issue 760351 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 760348

Blocking:
issue 754872



Sign in to add a comment

Add I420 format option to CopyOutputRequest

Project Member Reported by m...@chromium.org, Aug 29 2017

Issue description

This is required for moving tab capture into VIZ. The idea is to move YUV/I420 image format conversion into the copy request execution pipeline (i.e., in GLRenderer/SoftwareRenderer), adding an option to request this in viz::CopyOutputRequest.

This is follow-up to track an action item from this code review: https://chromium-review.googlesource.com/c/chromium/src/+/637003/6

 
 

Comment 1 by m...@chromium.org, Sep 21 2017

Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 21 2017

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

commit edb2ce24a97f7624c4783977982a292efe33bb2b
Author: Yuri Wiitala <miu@chromium.org>
Date: Thu Sep 21 19:51:17 2017

Lazy-init GLHelperReadbackSupport to eliminate sync calls at startup.

When a GLHelper instance was created, it also constructed a
GLHelperReadbackSupport instance. However, in doing so, there were
certain calls being made on the GL context that were synchronous, which
is: 1) bad to do during init time; and 2) really bad to do if the client
code will never use the results anyway. Therefore, this change simply
lazy-creates the GLHelperReadbackSupport instance upon first use.

Bug:  760348 ,  760351 
Change-Id: I44e63a71331f31b6d88305bb00f29af9928a722e
Reviewed-on: https://chromium-review.googlesource.com/676945
Reviewed-by: Xiangjun Zhang <xjz@chromium.org>
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503540}
[modify] https://crrev.com/edb2ce24a97f7624c4783977982a292efe33bb2b/components/viz/common/gl_helper.cc
[modify] https://crrev.com/edb2ce24a97f7624c4783977982a292efe33bb2b/components/viz/common/gl_helper.h

Project Member

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

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

commit 9e59e6960ddaffb6b83ab23153c5434dd60fc286
Author: Yuri Wiitala <miu@chromium.org>
Date: Mon Oct 02 22:54:31 2017

GLHelper: Split non-readback part of ReadbackYUV into I420Converter.

In a soon-upcoming change, GLRenderer will want more fine-grained
control over the read-back process. Therefore, this change splits the
impl that reads-back RGBA textures as Y+U+V planes in system memory:
It introduces a I420Converter, which is just the impl that converts RGBA
textures into Y+U+V textures. Then, the existing ReadbackYUV retains its
full functionality, but now uses I420Converter for the everything that
happens before the read-back.

Testing note: All new/changed code is implicitly tested by existing unit
tests because ReadbackYUV depends on the new I420Converter. :)

Bug:  760351 
Change-Id: I9cea7f110b1eb415b8b7e72d66b24391447978d8
Reviewed-on: https://chromium-review.googlesource.com/693381
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Reviewed-by: Xiangjun Zhang <xjz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#505832}
[modify] https://crrev.com/9e59e6960ddaffb6b83ab23153c5434dd60fc286/components/viz/common/gl_helper.cc
[modify] https://crrev.com/9e59e6960ddaffb6b83ab23153c5434dd60fc286/components/viz/common/gl_helper.h
[modify] https://crrev.com/9e59e6960ddaffb6b83ab23153c5434dd60fc286/components/viz/common/gl_helper_scaling.cc

Is this finished?

Comment 5 by m...@chromium.org, Oct 13 2017

WIP. Prob a week or two more work.
Labels: -M-63 M-64

Comment 7 by m...@chromium.org, Oct 19 2017

Blockedon: 760348
Just blocked on a couple small things left to do in crbug 760348.

The code for this feature is actually code-complete now. I just had to stop and go back and fix some showstopper issues in the GLHelper scaler implementation that this feature depends on. Then, I'll wrap-up testing and put up a code review. ETA 3 days for that.
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 25 2017

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

commit bab63555df3d1c31bfb96f14c8dae96ab773f65c
Author: Yuri Wiitala <miu@chromium.org>
Date: Wed Oct 25 00:19:37 2017

GLRendererCopier: Use optimal readback format.

Queries whether the GPU+driver internally use BGRA byte ordering and, if
so, uses that for readback. Otherwise, use the standard RGBA byte order.
This optimization prevents the need for extra memcpy/swizzling when
CopyOutputRequests produce SkBitmap results.

This change is also a prerequisite for a soon-upcoming change to add
I420 format readback to CopyOutputRequests.

Bug:  760351 , 770422 
Change-Id: I718401750470241e4188dfee2406867fa7293899
Reviewed-on: https://chromium-review.googlesource.com/731869
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511317}
[modify] https://crrev.com/bab63555df3d1c31bfb96f14c8dae96ab773f65c/components/viz/service/display/gl_renderer_copier.cc
[modify] https://crrev.com/bab63555df3d1c31bfb96f14c8dae96ab773f65c/components/viz/service/display/gl_renderer_copier.h
[modify] https://crrev.com/bab63555df3d1c31bfb96f14c8dae96ab773f65c/components/viz/service/display/gl_renderer_copier_unittest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Oct 26 2017

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

commit 21865dc7acb7dfb02e7bd182e438cc620b7084bc
Author: Yuri Wiitala <miu@chromium.org>
Date: Thu Oct 26 23:37:41 2017

Add CopyOutputRequest::set_result_selection().

Adds a new property to CopyOutputRequests that allows for selecting a
portion of the result from the result (post-scaled) coordinate space.

Motivations:

1. While working on adding I420 format support to copy requests, it
became apparent that resolving certain coordinate calculations required
extra back-and-forth rect transformations with some weird edge cases.
The extra math goes away when GL/SoftwareRenderer can resolve alignment
issues entirely in the scaled coordinate space.

2. Simplifies the usage of copy requests from the perspective of client-
side code when using scaled output to patch changes to whole images (see
 crbug.com/775740  for discussion).

3. Allows for simpler integration with the GLHelperScaling impl. A
future optimization will be able to use the implementation's "minimal
sampling rect" utility to improve performance.

Bug:  760348 ,  760351 
Change-Id: I1df527514a7b11119d2a68b21f05aa74c3bdd765
Reviewed-on: https://chromium-review.googlesource.com/736429
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512015}
[modify] https://crrev.com/21865dc7acb7dfb02e7bd182e438cc620b7084bc/components/viz/common/frame_sinks/copy_output_request.h
[modify] https://crrev.com/21865dc7acb7dfb02e7bd182e438cc620b7084bc/components/viz/service/display/copy_output_scaling_pixeltest.cc
[modify] https://crrev.com/21865dc7acb7dfb02e7bd182e438cc620b7084bc/components/viz/service/display/gl_renderer.cc
[modify] https://crrev.com/21865dc7acb7dfb02e7bd182e438cc620b7084bc/components/viz/service/display/gl_renderer_copier.cc
[modify] https://crrev.com/21865dc7acb7dfb02e7bd182e438cc620b7084bc/components/viz/service/display/gl_renderer_copier.h
[modify] https://crrev.com/21865dc7acb7dfb02e7bd182e438cc620b7084bc/components/viz/service/display/gl_renderer_copier_pixeltest.cc
[modify] https://crrev.com/21865dc7acb7dfb02e7bd182e438cc620b7084bc/components/viz/service/display/gl_renderer_copier_unittest.cc
[modify] https://crrev.com/21865dc7acb7dfb02e7bd182e438cc620b7084bc/components/viz/service/display/software_renderer.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Nov 8 2017

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

commit 8e41ce2231c7ecf48773ad0a187deff854241c87
Author: Yuri Wiitala <miu@chromium.org>
Date: Wed Nov 08 00:58:50 2017

CopyOutputRequest: Add I420 result format and implementation.

Adds a new I420_PLANES result format to CopyOutputRequests to support
video screen capture use cases. Since this is meant for exclusive use by
the soon-upcoming viz::FrameSinkVideoCapturer implementation, these
requests are not meant to cross mojo service boundaries.

I420_PLANES results are optimized for a single-copy into the final
memory location, whether they are generated from GLRendererCopier or
SoftwareRenderer:

1. For GLRenderer: The GLHelper I420Converter is used to planarize a
source texture. From that, the Y+U+V planes are downloaded from the GPU
via a single pixel buffer object.

2. For SoftwareRenderer: Very little implementation in the renderer
itself has been changed. Instead, CopyOutputResult provides a default
ReadI420Planes() method that converts directly from the SkBitmap
representation.

Bug:  760351 
Change-Id: I1df495e1a1a3d2aee7c543514e391c2e3e1d8f1a
Reviewed-on: https://chromium-review.googlesource.com/742151
Commit-Queue: Yuri Wiitala <miu@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514686}
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/common/BUILD.gn
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/common/frame_sinks/DEPS
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/common/frame_sinks/copy_output_request.cc
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/common/frame_sinks/copy_output_request.h
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/common/frame_sinks/copy_output_result.cc
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/common/frame_sinks/copy_output_result.h
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/common/gl_helper.h
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/service/BUILD.gn
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/service/display/DEPS
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/service/display/copy_output_scaling_pixeltest.cc
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/service/display/gl_renderer_copier.cc
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/service/display/gl_renderer_copier.h
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/service/display/gl_renderer_copier_unittest.cc
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/components/viz/service/display/software_renderer.cc
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/content/browser/compositor/surface_utils.cc
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/services/viz/public/cpp/compositing/copy_output_result_struct_traits.cc
[modify] https://crrev.com/8e41ce2231c7ecf48773ad0a187deff854241c87/services/viz/public/cpp/compositing/copy_output_result_struct_traits.h

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

Status: Fixed (was: Started)
Components: -Internals>MUS Internals>Services>WindowService

Sign in to add a comment