Revert "Use glUnpremultiplyAndDitherCopyCHROMIUM in GPU Raster"

This reverts commit fa5503b. Reason for revert: Causing visual glitches on non-low-end webview (issue 816356). Also causing a larger than expected memory regression (issue 815270). Will fix and re-land. Original change's description: > Use glUnpremultiplyAndDitherCopyCHROMIUM in GPU Raster > > This change addresses banding issues by using the newly added > glUnpremultiplyAndDitherCopyCHROMIUM. > > In cases where we previously rasterized into a RGBA4444 target, we > instead rasterize into a temporary RGBA8888 target, then do an > unpremultiply/dither copy into the RGBA4444 target. > > This means that tiles may be either unpremultiplied or premultiplied, so > we need a way to pipe this information to the GLRenderer so it can > blend correctly. This change introduces a is_premultiplied property to > ContentDrawQuadBase to handle this. > > Bug: 789153 > Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel > Change-Id: I8bb2af2a94934ca7fe79a180d65bff253163c98c > Reviewed-on: https://chromium-review.googlesource.com/899866 > Reviewed-by: enne <enne@chromium.org> > Reviewed-by: Ken Buchanan <kenrb@chromium.org> > Commit-Queue: Eric Karl <ericrk@chromium.org> > Cr-Commit-Position: refs/heads/master@{#538686} TBR=kenrb@chromium.org,enne@chromium.org,ericrk@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 789153, 815270, 816356 Change-Id: I4faf63e575471c6830fcb5475b35870cf436c398 Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.android:android_optional_gpu_tests_rel Reviewed-on: https://chromium-review.googlesource.com/937883 Reviewed-by: Eric Karl <ericrk@chromium.org> Commit-Queue: Eric Karl <ericrk@chromium.org> Cr-Commit-Position: refs/heads/master@{#539255}
qweri0p · Feb 26, 2018 · bc529af · bc529af
1 parent 70d15d5
commit bc529af
Show file tree

Hide file tree

Showing 41 changed files with 80 additions and 296 deletions.
diff --git a/cc/ipc/cc_param_traits_unittest.cc b/cc/ipc/cc_param_traits_unittest.cc
@@ -405,10 +405,10 @@ TEST_F(CCParamTraitsTest, AllQuads) {
   pass_cmp->CopyFromAndAppendDrawQuad(texture_in);
 
   TileDrawQuad* tile_in = pass_in->CreateAndAppendDrawQuad<TileDrawQuad>();
-  tile_in->SetAll(
-      shared_state3_in, arbitrary_rect2, arbitrary_rect1_inside_rect2,
-      arbitrary_bool1, arbitrary_resourceid3, arbitrary_rectf1, arbitrary_size1,
-      arbitrary_bool2, arbitrary_bool3, arbitrary_bool4, arbitrary_bool5);
+  tile_in->SetAll(shared_state3_in, arbitrary_rect2,
+                  arbitrary_rect1_inside_rect2, arbitrary_bool1,
+                  arbitrary_resourceid3, arbitrary_rectf1, arbitrary_size1,
+                  arbitrary_bool2, arbitrary_bool3, arbitrary_bool4);
   pass_cmp->CopyFromAndAppendDrawQuad(tile_in);
 
   YUVVideoDrawQuad* yuvvideo_in =

diff --git a/cc/ipc/cc_serialization_perftest.cc b/cc/ipc/cc_serialization_perftest.cc
@@ -361,11 +361,10 @@ class CCSerializationPerfTest : public testing::Test {
           arbitrary_blend_mode2, arbitrary_context_id2);
       for (uint32_t j = 0; j < 6; ++j) {
         auto* tile_in = pass_in->CreateAndAppendDrawQuad<viz::TileDrawQuad>();
-        tile_in->SetAll(shared_state2_in, arbitrary_rect2,
-                        arbitrary_rect1_inside_rect2, arbitrary_bool1,
-                        arbitrary_resourceid3, arbitrary_rectf1,
-                        arbitrary_size1, arbitrary_bool2, arbitrary_bool3,
-                        arbitrary_bool4, arbitrary_bool5);
+        tile_in->SetAll(
+            shared_state2_in, arbitrary_rect2, arbitrary_rect1_inside_rect2,
+            arbitrary_bool1, arbitrary_resourceid3, arbitrary_rectf1,
+            arbitrary_size1, arbitrary_bool2, arbitrary_bool3, arbitrary_bool4);
       }
     }
 

diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_impl.cc
@@ -470,7 +470,7 @@ void PictureLayerImpl::AppendQuads(viz::RenderPass* render_pass,
               offset_visible_geometry_rect, needs_blending,
               draw_info.resource_id_for_export(), texture_rect,
               draw_info.resource_size(), draw_info.contents_swizzled(),
-              draw_info.is_premultiplied(), nearest_neighbor_,
+              nearest_neighbor_,
               !layer_tree_impl()->settings().enable_edge_anti_aliasing);
           ValidateQuadResources(quad);
           has_draw_quad = true;

diff --git a/cc/layers/render_surface_unittest.cc b/cc/layers/render_surface_unittest.cc
@@ -68,7 +68,7 @@ class FakePictureLayerImplForRenderSurfaceTest : public FakePictureLayerImpl {
     for (const auto& rect : quad_rects_) {
       auto* quad = render_pass->CreateAndAppendDrawQuad<viz::TileDrawQuad>();
       quad->SetNew(shared_quad_state, rect, rect, needs_blending, 0,
-                   gfx::RectF(rect), bounds(), false, false, false, false);
+                   gfx::RectF(rect), bounds(), false, false, false);
     }
   }
 

diff --git a/cc/raster/bitmap_raster_buffer_provider.cc b/cc/raster/bitmap_raster_buffer_provider.cc
@@ -114,11 +114,6 @@ bool BitmapRasterBufferProvider::IsResourceSwizzleRequired(
   return ResourceFormatRequiresSwizzle(viz::RGBA_8888);
 }
 
-bool BitmapRasterBufferProvider::IsResourcePremultiplied(
-    bool must_support_alpha) const {
-  return true;
-}
-
 bool BitmapRasterBufferProvider::CanPartialRasterIntoProvidedResource() const {
   return true;
 }

diff --git a/cc/raster/bitmap_raster_buffer_provider.h b/cc/raster/bitmap_raster_buffer_provider.h
@@ -39,7 +39,6 @@ class CC_EXPORT BitmapRasterBufferProvider : public RasterBufferProvider {
   void Flush() override;
   viz::ResourceFormat GetResourceFormat(bool must_support_alpha) const override;
   bool IsResourceSwizzleRequired(bool must_support_alpha) const override;
-  bool IsResourcePremultiplied(bool must_support_alpha) const override;
   bool CanPartialRasterIntoProvidedResource() const override;
   bool IsResourceReadyToDraw(
       const ResourcePool::InUsePoolResource& resource) const override;

diff --git a/cc/raster/gpu_raster_buffer_provider.cc b/cc/raster/gpu_raster_buffer_provider.cc
@@ -28,7 +28,6 @@
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "gpu/command_buffer/client/raster_interface.h"
 #include "gpu/command_buffer/common/gpu_memory_buffer_support.h"
-#include "skia/ext/texture_handle.h"
 #include "third_party/skia/include/core/SkMultiPictureDraw.h"
 #include "third_party/skia/include/core/SkPictureRecorder.h"
 #include "third_party/skia/include/core/SkSurface.h"
@@ -39,54 +38,6 @@
 namespace cc {
 namespace {
 
-class ScopedSkSurfaceForUnpremultiplyAndDither {
- public:
-  ScopedSkSurfaceForUnpremultiplyAndDither(
-      viz::RasterContextProvider* context_provider,
-      const gfx::Rect& playback_rect,
-      const gfx::Rect& raster_full_rect,
-      GLuint texture_id,
-      const gfx::Size& size,
-      bool use_distance_field_text,
-      bool can_use_lcd_text,
-      int msaa_sample_count)
-      : context_provider_(context_provider),
-        texture_id_(texture_id),
-        offset_(playback_rect.OffsetFromOrigin() -
-                raster_full_rect.OffsetFromOrigin()),
-        size_(playback_rect.size()) {
-    // Allocate a 32-bit surface for raster. We will copy from that into our
-    // actual surface in destruction.
-    SkImageInfo n32Info =
-        SkImageInfo::MakeN32Premul(size.width(), size.height());
-    SkSurfaceProps surface_props =
-        LayerTreeResourceProvider::ScopedSkSurface::ComputeSurfaceProps(
-            use_distance_field_text, can_use_lcd_text);
-    surface_ = SkSurface::MakeRenderTarget(
-        context_provider->GrContext(), SkBudgeted::kYes, n32Info,
-        msaa_sample_count, kTopLeft_GrSurfaceOrigin, &surface_props);
-  }
-
-  ~ScopedSkSurfaceForUnpremultiplyAndDither() {
-    GrBackendObject handle =
-        surface_->getTextureHandle(SkSurface::kFlushRead_BackendHandleAccess);
-    const GrGLTextureInfo* info =
-        skia::GrBackendObjectToGrGLTextureInfo(handle);
-    context_provider_->RasterInterface()->UnpremultiplyAndDitherCopyCHROMIUM(
-        info->fID, texture_id_, offset_.x(), offset_.y(), size_.width(),
-        size_.height());
-  }
-
-  SkSurface* surface() { return surface_.get(); }
-
- private:
-  viz::RasterContextProvider* context_provider_;
-  GLuint texture_id_;
-  gfx::Vector2d offset_;
-  gfx::Size size_;
-  sk_sp<SkSurface> surface_;
-};
-
 static void RasterizeSourceOOP(
     const RasterSource* raster_source,
     bool resource_has_previous_content,
@@ -134,9 +85,6 @@ static void RasterizeSourceOOP(
                      raster_source->requires_clear());
   ri->EndRasterCHROMIUM();
 
-  // TODO(ericrk): Handle unpremultiply+dither for 4444 cases.
-  // https://crbug.com/789153
-
   ri->DeleteTextures(1, &texture_id);
 }
 
@@ -177,8 +125,7 @@ static void RasterizeSource(
     const RasterSource::PlaybackSettings& playback_settings,
     viz::RasterContextProvider* context_provider,
     bool use_distance_field_text,
-    int msaa_sample_count,
-    bool unpremultiply_and_dither) {
+    int msaa_sample_count) {
   ScopedGrContextAccess gr_context_access(context_provider);
 
   gpu::raster::RasterInterface* ri = context_provider->RasterInterface();
@@ -192,23 +139,12 @@ static void RasterizeSource(
   }
 
   {
-    base::Optional<LayerTreeResourceProvider::ScopedSkSurface> scoped_surface;
-    base::Optional<ScopedSkSurfaceForUnpremultiplyAndDither>
-        scoped_dither_surface;
-    SkSurface* surface;
-    if (!unpremultiply_and_dither) {
-      scoped_surface.emplace(context_provider->GrContext(), texture_id,
-                             texture_target, resource_size, resource_format,
-                             use_distance_field_text,
-                             playback_settings.use_lcd_text, msaa_sample_count);
-      surface = scoped_surface->surface();
-    } else {
-      scoped_dither_surface.emplace(
-          context_provider, playback_rect, raster_full_rect, texture_id,
-          resource_size, use_distance_field_text,
-          playback_settings.use_lcd_text, msaa_sample_count);
-      surface = scoped_dither_surface->surface();
-    }
+    LayerTreeResourceProvider::ScopedSkSurface scoped_surface(
+        context_provider->GrContext(), texture_id, texture_target,
+        resource_size, resource_format, use_distance_field_text,
+        playback_settings.use_lcd_text, msaa_sample_count);
+
+    SkSurface* surface = scoped_surface.surface();
 
     // Allocating an SkSurface will fail after a lost context.  Pretend we
     // rasterized, as the contents of the resource don't matter anymore.
@@ -232,15 +168,6 @@ static void RasterizeSource(
   ri->DeleteTextures(1, &texture_id);
 }
 
-bool ShouldUnpremultiplyAndDitherResource(viz::ResourceFormat format) {
-  switch (format) {
-    case viz::RGBA_4444:
-      return true;
-    default:
-      return false;
-  }
-}
-
 }  // namespace
 
 // Subclass for InUsePoolResource that holds ownership of a gpu-rastered backing
@@ -414,12 +341,6 @@ bool GpuRasterBufferProvider::IsResourceSwizzleRequired(
   return false;
 }
 
-bool GpuRasterBufferProvider::IsResourcePremultiplied(
-    bool must_support_alpha) const {
-  return !ShouldUnpremultiplyAndDitherResource(
-      GetResourceFormat(must_support_alpha));
-}
-
 bool GpuRasterBufferProvider::CanPartialRasterIntoProvidedResource() const {
   // Partial raster doesn't support MSAA, as the MSAA resolve is unaware of clip
   // rects.
@@ -529,8 +450,7 @@ gpu::SyncToken GpuRasterBufferProvider::PlaybackOnWorkerThread(
                     texture_storage_allocated, resource_size, resource_format,
                     color_space, raster_full_rect, playback_rect, transform,
                     playback_settings, worker_context_provider_,
-                    use_distance_field_text_, msaa_sample_count_,
-                    ShouldUnpremultiplyAndDitherResource(resource_format));
+                    use_distance_field_text_, msaa_sample_count_);
   }
 
   // Generate sync token for cross context synchronization.

diff --git a/cc/raster/gpu_raster_buffer_provider.h b/cc/raster/gpu_raster_buffer_provider.h
@@ -39,7 +39,6 @@ class CC_EXPORT GpuRasterBufferProvider : public RasterBufferProvider {
   void Flush() override;
   viz::ResourceFormat GetResourceFormat(bool must_support_alpha) const override;
   bool IsResourceSwizzleRequired(bool must_support_alpha) const override;
-  bool IsResourcePremultiplied(bool must_support_alpha) const override;
   bool CanPartialRasterIntoProvidedResource() const override;
   bool IsResourceReadyToDraw(
       const ResourcePool::InUsePoolResource& resource) const override;

diff --git a/cc/raster/one_copy_raster_buffer_provider.cc b/cc/raster/one_copy_raster_buffer_provider.cc
@@ -225,13 +225,6 @@ bool OneCopyRasterBufferProvider::IsResourceSwizzleRequired(
   return ResourceFormatRequiresSwizzle(GetResourceFormat(must_support_alpha));
 }
 
-bool OneCopyRasterBufferProvider::IsResourcePremultiplied(
-    bool must_support_alpha) const {
-  // TODO(ericrk): Handle unpremultiply/dither in one-copy case as well.
-  // https://crbug.com/789153
-  return true;
-}
-
 bool OneCopyRasterBufferProvider::CanPartialRasterIntoProvidedResource() const {
   // While OneCopyRasterBufferProvider has an internal partial raster
   // implementation, it cannot directly partial raster into the externally

diff --git a/cc/raster/one_copy_raster_buffer_provider.h b/cc/raster/one_copy_raster_buffer_provider.h
@@ -44,7 +44,6 @@ class CC_EXPORT OneCopyRasterBufferProvider : public RasterBufferProvider {
   void Flush() override;
   viz::ResourceFormat GetResourceFormat(bool must_support_alpha) const override;
   bool IsResourceSwizzleRequired(bool must_support_alpha) const override;
-  bool IsResourcePremultiplied(bool must_support_alpha) const override;
   bool CanPartialRasterIntoProvidedResource() const override;
   bool IsResourceReadyToDraw(
       const ResourcePool::InUsePoolResource& resource) const override;

diff --git a/cc/raster/raster_buffer_provider.h b/cc/raster/raster_buffer_provider.h
@@ -62,9 +62,6 @@ class CC_EXPORT RasterBufferProvider {
   // Determine if the resource requires swizzling.
   virtual bool IsResourceSwizzleRequired(bool must_support_alpha) const = 0;
 
-  // Determines if the resource is premultiplied.
-  virtual bool IsResourcePremultiplied(bool must_support_alpha) const = 0;
-
   // Determine if the RasterBufferProvider can handle partial raster into
   // the Resource provided in AcquireBufferForRaster.
   virtual bool CanPartialRasterIntoProvidedResource() const = 0;

diff --git a/cc/raster/zero_copy_raster_buffer_provider.cc b/cc/raster/zero_copy_raster_buffer_provider.cc
@@ -250,11 +250,6 @@ bool ZeroCopyRasterBufferProvider::IsResourceSwizzleRequired(
   return ResourceFormatRequiresSwizzle(GetResourceFormat(must_support_alpha));
 }
 
-bool ZeroCopyRasterBufferProvider::IsResourcePremultiplied(
-    bool must_support_alpha) const {
-  return true;
-}
-
 bool ZeroCopyRasterBufferProvider::CanPartialRasterIntoProvidedResource()
     const {
   return false;

diff --git a/cc/raster/zero_copy_raster_buffer_provider.h b/cc/raster/zero_copy_raster_buffer_provider.h
@@ -42,7 +42,6 @@ class CC_EXPORT ZeroCopyRasterBufferProvider : public RasterBufferProvider {
   void Flush() override;
   viz::ResourceFormat GetResourceFormat(bool must_support_alpha) const override;
   bool IsResourceSwizzleRequired(bool must_support_alpha) const override;
-  bool IsResourcePremultiplied(bool must_support_alpha) const override;
   bool CanPartialRasterIntoProvidedResource() const override;
   bool IsResourceReadyToDraw(
       const ResourcePool::InUsePoolResource& resource) const override;

diff --git a/cc/resources/layer_tree_resource_provider.cc b/cc/resources/layer_tree_resource_provider.cc
@@ -996,21 +996,6 @@ LayerTreeResourceProvider::ScopedSkSurface::ScopedSkSurface(
   texture_info.fFormat = TextureStorageFormat(format);
   GrBackendTexture backend_texture(size.width(), size.height(),
                                    GrMipMapped::kNo, texture_info);
-  SkSurfaceProps surface_props =
-      ComputeSurfaceProps(use_distance_field_text, can_use_lcd_text);
-  surface_ = SkSurface::MakeFromBackendTextureAsRenderTarget(
-      gr_context, backend_texture, kTopLeft_GrSurfaceOrigin, msaa_sample_count,
-      ResourceFormatToClosestSkColorType(format), nullptr, &surface_props);
-}
-
-LayerTreeResourceProvider::ScopedSkSurface::~ScopedSkSurface() {
-  if (surface_)
-    surface_->prepareForExternalIO();
-}
-
-SkSurfaceProps LayerTreeResourceProvider::ScopedSkSurface::ComputeSurfaceProps(
-    bool use_distance_field_text,
-    bool can_use_lcd_text) {
   uint32_t flags =
       use_distance_field_text ? SkSurfaceProps::kUseDistanceFieldFonts_Flag : 0;
   // Use unknown pixel geometry to disable LCD text.
@@ -1020,7 +1005,14 @@ SkSurfaceProps LayerTreeResourceProvider::ScopedSkSurface::ComputeSurfaceProps(
     surface_props =
         SkSurfaceProps(flags, SkSurfaceProps::kLegacyFontHost_InitType);
   }
-  return surface_props;
+  surface_ = SkSurface::MakeFromBackendTextureAsRenderTarget(
+      gr_context, backend_texture, kTopLeft_GrSurfaceOrigin, msaa_sample_count,
+      ResourceFormatToClosestSkColorType(format), nullptr, &surface_props);
+}
+
+LayerTreeResourceProvider::ScopedSkSurface::~ScopedSkSurface() {
+  if (surface_)
+    surface_->prepareForExternalIO();
 }
 
 void LayerTreeResourceProvider::ValidateResource(viz::ResourceId id) const {

diff --git a/cc/resources/layer_tree_resource_provider.h b/cc/resources/layer_tree_resource_provider.h
@@ -292,9 +292,6 @@ class CC_EXPORT LayerTreeResourceProvider : public ResourceProvider {
 
     SkSurface* surface() const { return surface_.get(); }
 
-    static SkSurfaceProps ComputeSurfaceProps(bool use_distance_field_text,
-                                              bool can_use_lcd_text);
-
    private:
     sk_sp<SkSurface> surface_;
 

diff --git a/cc/test/fake_raster_buffer_provider.cc b/cc/test/fake_raster_buffer_provider.cc
@@ -44,11 +44,6 @@ bool FakeRasterBufferProviderImpl::IsResourceSwizzleRequired(
   return ResourceFormatRequiresSwizzle(GetResourceFormat(must_support_alpha));
 }
 
-bool FakeRasterBufferProviderImpl::IsResourcePremultiplied(
-    bool must_support_alpha) const {
-  return true;
-}
-
 bool FakeRasterBufferProviderImpl::CanPartialRasterIntoProvidedResource()
     const {
   return true;

diff --git a/cc/test/fake_raster_buffer_provider.h b/cc/test/fake_raster_buffer_provider.h
@@ -23,7 +23,6 @@ class FakeRasterBufferProviderImpl : public RasterBufferProvider {
   void Flush() override;
   viz::ResourceFormat GetResourceFormat(bool must_support_alpha) const override;
   bool IsResourceSwizzleRequired(bool must_support_alpha) const override;
-  bool IsResourcePremultiplied(bool must_support_alpha) const override;
   bool CanPartialRasterIntoProvidedResource() const override;
   bool IsResourceReadyToDraw(
       const ResourcePool::InUsePoolResource& resource) const override;