New issue
Advanced search Search tips

Issue 758350 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Nov 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocking:
issue 757605



Sign in to add a comment

Implement op folding with draw-with-alpha for oop rasterization

Project Member Reported by enne@chromium.org, Aug 23 2017

Issue description

Currently the behavior in PaintOpBuffer to fold together multiple ops is not very replicatable for transporting those ops to the gpu process.

The plan is to do this folding ahead of time on the client side so that the gpu process itself doesn't have to bother with this.  It'd be nice as a bonus to share the same code with PaintOpBuffer::playback, possibly by adding some helper class that returns a stream of (potentially folded) ops instead of just rastering directly "with flags".
 
Project Member

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

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

commit 2097e969be56cf280787eebfc15af9073998c9a5
Author: Adrienne Walker <enne@chromium.org>
Date: Tue Oct 24 21:37:18 2017

cc: Use recursion instead of iterator for oop raster

The flattening iterator makes some incorrect assumptions.  In
particular, it does not insert saves and restores correctly to prevent
DrawRecord ops from having state-changing commands escape a DrawRecord.
Also the flattening iterator ignores ctm, and so SetMatrix ops have
the wrong values through oop raster.

https://chromium-review.googlesource.com/c/chromium/src/+/716712 is an
alternate approach to this patch, which rewrites this recursion as an
iterator, but it tightly couples iter internals with usage and is a lot
more complicated than the recursion that it is trying to abstract.
So, therefore, this patch turns oop raster back into recursion.

https://chromium-review.googlesource.com/c/chromium/src/+/713439 is the
followup which abstracts the alpha folding logic in a way that can be
reused by this patch

Bug:  758350 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I025c4cb68b452a27cc198a735e219e196a11dd75
Reviewed-on: https://chromium-review.googlesource.com/726979
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511269}
[modify] https://crrev.com/2097e969be56cf280787eebfc15af9073998c9a5/cc/paint/paint_op_buffer.cc
[modify] https://crrev.com/2097e969be56cf280787eebfc15af9073998c9a5/cc/paint/paint_op_buffer.h
[modify] https://crrev.com/2097e969be56cf280787eebfc15af9073998c9a5/cc/paint/paint_op_buffer_unittest.cc
[modify] https://crrev.com/2097e969be56cf280787eebfc15af9073998c9a5/gpu/command_buffer/client/gles2_implementation.cc

Project Member

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

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

commit 7786300c3ba9ffbc00ccad5ee558675a9df0c094
Author: Adrienne Walker <enne@chromium.org>
Date: Wed Oct 25 17:16:13 2017

cc: Wrap alpha folding raster logic into an iterator

The goal is to let this logic be reused for serialization.

Bug:  758350 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I8ccb8a636a7ecce6231464a207f0878767d410a9
Reviewed-on: https://chromium-review.googlesource.com/713439
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Khushal <khushalsagar@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511497}
[modify] https://crrev.com/7786300c3ba9ffbc00ccad5ee558675a9df0c094/cc/paint/paint_op_buffer.cc
[modify] https://crrev.com/7786300c3ba9ffbc00ccad5ee558675a9df0c094/cc/paint/paint_op_buffer.h
[modify] https://crrev.com/7786300c3ba9ffbc00ccad5ee558675a9df0c094/cc/paint/paint_op_buffer_unittest.cc

Components: Internals>Compositing
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 31 2017

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

commit bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed
Author: Adrienne Walker <enne@chromium.org>
Date: Tue Oct 31 21:58:15 2017

oop: Fix flashing by moving preamble to gles2_implemntation

The https://chromium-review.googlesource.com/726979 patch caused some
bugs with --enable-oop-rasterization was on.  In particular, by
adding saves and restores, it wrapped all the "preamble" logic for the
first RasterCHROMIUM with the setup logic in a save/restore.  This
caused any partial raster tile to be incorrect and caused flashing.

This patch moves the preamble logic into the RasterCHROMIUM call itself,
addressing a TODO in SerializeHelper.  This is needed for the future
anyway so that the underlying tracking SkCanvas can have the correct
state for image decode querying.

By moving the preamble logic, saves/restores can be added to real
DrawRecords and not the fake preamble DrawRecord and the flashing bug
is also fixed.

Bug:  758350 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I64b3ee698e7712e55ed0170d1d77b62d8a87b72a
Reviewed-on: https://chromium-review.googlesource.com/745523
Commit-Queue: enne <enne@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#512969}
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/cc/raster/gpu_raster_buffer_provider.cc
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/cc/raster/raster_source.h
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/gpu/command_buffer/client/gles2_c_lib_autogen.h
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/gpu/command_buffer/client/gles2_implementation.cc
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/gpu/command_buffer/client/gles2_implementation_autogen.h
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/gpu/command_buffer/client/gles2_interface_autogen.h
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/gpu/command_buffer/client/gles2_interface_stub_autogen.h
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/gpu/command_buffer/client/gles2_interface_stub_impl_autogen.h
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/gpu/command_buffer/client/gles2_trace_implementation_autogen.h
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/gpu/command_buffer/client/gles2_trace_implementation_impl_autogen.h
[modify] https://crrev.com/bc5cd82eff89c73415aaf537ecbb2f809d9ee2ed/gpu/command_buffer/cmd_buffer_functions.txt

Comment 5 by enne@chromium.org, Nov 17 2017

Owner: khushals...@chromium.org
Status: Assigned (was: Available)
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 20 2017

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

commit 92f669180c3fa4c7190817870ead6155e12fcc78
Author: Khushal <khushalsagar@chromium.org>
Date: Mon Nov 20 10:08:50 2017

cc: Implement alpha folding during serialization.

Add an option to override flags during serialization and use it do
alpha folding for SaveLayer/Draw/Restore during serialization. The
overridden flags will also be used to replace images in shaders.

R=enne@chromium.org

Bug:  758350 
Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel
Change-Id: Iff0a9fd81bd9dee9b932dc95e194208e34dc4069
Reviewed-on: https://chromium-review.googlesource.com/777844
Commit-Queue: Khushal <khushalsagar@chromium.org>
Reviewed-by: enne <enne@chromium.org>
Cr-Commit-Position: refs/heads/master@{#517788}
[modify] https://crrev.com/92f669180c3fa4c7190817870ead6155e12fcc78/cc/paint/paint_op_buffer.cc
[modify] https://crrev.com/92f669180c3fa4c7190817870ead6155e12fcc78/cc/paint/paint_op_buffer.h
[modify] https://crrev.com/92f669180c3fa4c7190817870ead6155e12fcc78/cc/paint/paint_op_buffer_serializer.cc
[modify] https://crrev.com/92f669180c3fa4c7190817870ead6155e12fcc78/cc/paint/paint_op_buffer_serializer.h
[modify] https://crrev.com/92f669180c3fa4c7190817870ead6155e12fcc78/cc/paint/paint_op_buffer_unittest.cc

Status: Fixed (was: Assigned)

Sign in to add a comment