cc: Use gpu raster in HUD.

cc/layers/heads_up_display_layer_impl.cc

Index: cc/layers/heads_up_display_layer_impl.cc
diff --git a/cc/layers/heads_up_display_layer_impl.cc b/cc/layers/heads_up_display_layer_impl.cc
index 7340f184c1fa53d818fa604e93095d21eaef0947..cb81cc99b10d5dc2d1b65045d54d092655964f2c 100644
--- a/cc/layers/heads_up_display_layer_impl.cc
+++ b/cc/layers/heads_up_display_layer_impl.cc
@@ -18,10 +18,12 @@
 #include "cc/debug/debug_colors.h"
 #include "cc/output/begin_frame_args.h"
 #include "cc/quads/texture_draw_quad.h"
+#include "cc/raster/scoped_gpu_raster.h"
 #include "cc/resources/memory_history.h"
 #include "cc/trees/frame_rate_counter.h"
 #include "cc/trees/layer_tree_host_impl.h"
 #include "cc/trees/layer_tree_impl.h"
+#include "gpu/command_buffer/client/gles2_interface.h"
 #include "skia/ext/platform_canvas.h"
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkPaint.h"
@@ -95,9 +97,9 @@ void HeadsUpDisplayLayerImpl::AcquireResource(
   }
 
   auto resource = base::MakeUnique<ScopedResource>(resource_provider);
-  resource->Allocate(
-      internal_content_bounds_, ResourceProvider::TEXTURE_HINT_IMMUTABLE,
-      resource_provider->best_texture_format(), gfx::ColorSpace());
+  resource->Allocate(internal_content_bounds_,
+                     ResourceProvider::TEXTURE_HINT_IMMUTABLE_FRAMEBUFFER,
+                     RGBA_8888, gfx::ColorSpace());

[danakj, 2017/05/01 22:57:32]

do you want best_render_buffer_format() instead of best_texture_format()? that
is what we return here:
https://cs.chromium.org/chromium/src/cc/raster/gpu_raster_buffer_provider.cc?...

[sohan, 2017/05/03 11:06:34]

Acknowledged.

   resources_.push_back(std::move(resource));
 }
 
@@ -157,23 +159,29 @@ void HeadsUpDisplayLayerImpl::AppendQuads(
 
 void HeadsUpDisplayLayerImpl::UpdateHudTexture(
     DrawMode draw_mode,
-    ResourceProvider* resource_provider) {
+    ResourceProvider* resource_provider,
+    ContextProvider* context_provider) {

[danakj, 2017/05/01 22:57:32]

What happens with --disable-gpu where the context provider is null?

[sohan, 2017/05/03 11:06:34]

I hope we do not want the SW raster path anymore, i am making it early return if
cxt provider is not there. is it ok ?

[danakj, 2017/05/03 14:44:51]

That would mean you can't have a HUD when using software compositing, so no
that's not really good.

[sohan, 2017/05/03 14:50:30]

OK, so we keep the old SW raster code path too?

[danakj, 2017/05/03 14:52:08]

Yes, we need it when there is no GPU.

[sohan, 2017/05/04 12:03:57]

Done.

   if (draw_mode == DRAW_MODE_RESOURCELESS_SOFTWARE || !resources_.back()->id())
     return;
 
-  SkISize canvas_size;
-  if (hud_surface_)
-    canvas_size = hud_surface_->getCanvas()->getBaseLayerSize();
-  else
-    canvas_size.set(0, 0);
-
-  if (canvas_size.width() != internal_content_bounds_.width() ||
-      canvas_size.height() != internal_content_bounds_.height() ||
-      !hud_surface_) {
-    TRACE_EVENT0("cc", "ResizeHudCanvas");
+  ContextProvider::ScopedContext scoped_context(context_provider);
+  gpu::gles2::GLES2Interface* gl = scoped_context.ContextGL();
+  DCHECK(gl);
+  ScopedGpuRaster gpu_raster(context_provider);
+  ResourceProvider::ScopedWriteLockGL lock(resource_provider,
+                                           resources_.back()->id(), false);

[danakj, 2017/05/01 22:57:32]

Can you comment that you pass false here because youre using the compositor
context, not the raster context? Also use a temp var to give the "false" a name

[sohan, 2017/05/03 11:06:34]

Done.

+  gfx::Rect raster_rect(internal_content_bounds_);

[danakj, 2017/05/01 22:57:32]

you could remove this var and go directly to the SkIRect

[sohan, 2017/05/03 11:06:35]

Done.

+  SkIRect raster_bounds = gfx::RectToSkIRect(raster_rect);
 
-    hud_surface_ = SkSurface::MakeRasterN32Premul(
-        internal_content_bounds_.width(), internal_content_bounds_.height());
+  {
+    TRACE_EVENT0("cc", "CreateHudCanvas");
+    if (hud_surface_) {
+      hud_surface_->prepareForExternalIO();

[danakj, 2017/05/01 22:57:32]

Do you need this since this is the single owner of hud_surface_?

[sohan, 2017/05/03 11:06:35]

as we remove the surface handling based on your comment later, this resetting is
no longer reqd.

+      hud_surface_.reset();
+    }
+    ResourceProvider::ScopedSkSurfaceProvider scoped_surface(
+        context_provider, &lock, false, false, false, 0);

[danakj, 2017/05/01 22:57:32]

again comment to explain the false and give these literals names via temp vars
please

[sohan, 2017/05/03 11:06:35]

Done.

+    hud_surface_ = scoped_surface.sk_sp_surface();

[danakj, 2017/05/01 22:57:32]

why do you keep the surface alive for gpu raster? it's used to draw into the
texture but doesnt control the lifetime of it. basically it is a means to get an
SkCanvas for gpu raster.

I suggest you figure out what surface you want to use, pull out the SkCanvas
into a variable based on that, then use that below instead of
hud_surface_->getCanvas(), so that code doesn't have to care about the surface.

[sohan, 2017/05/03 11:06:35]

Done.

   }
 
   UpdateHudContents();
@@ -182,6 +190,8 @@ void HeadsUpDisplayLayerImpl::UpdateHudTexture(
     TRACE_EVENT0("cc", "DrawHudContents");
     hud_surface_->getCanvas()->clear(SkColorSetARGB(0, 0, 0, 0));
     hud_surface_->getCanvas()->save();
+    hud_surface_->getCanvas()->translate(-raster_rect.x(), -raster_rect.y());

[danakj, 2017/05/01 22:57:32]

can you point me how would x and y be not 0 here?

[sohan, 2017/05/03 11:06:35]

Yes, this was not needed, i tried to use the same canvas calls as SW raster
before. Removed.

+    hud_surface_->getCanvas()->clipRect(SkRect::MakeFromIRect(raster_bounds));

[danakj, 2017/05/01 22:57:32]

how come the clip?

[sohan, 2017/05/03 11:06:35]

Same as above.

     hud_surface_->getCanvas()->scale(internal_contents_scale_,
                                      internal_contents_scale_);
 
@@ -191,14 +201,15 @@ void HeadsUpDisplayLayerImpl::UpdateHudTexture(
   }
 
   TRACE_EVENT0("cc", "UploadHudTexture");
-  SkPixmap pixmap;
-  hud_surface_->peekPixels(&pixmap);
-  DCHECK(pixmap.addr());
-  DCHECK(pixmap.info().colorType() == kN32_SkColorType);
-  resource_provider->CopyToResource(resources_.back()->id(),
-                                    static_cast<const uint8_t*>(pixmap.addr()),
-                                    internal_content_bounds_);
-  resource_provider->GenerateSyncTokenForResource(resources_.back()->id());
+
+  const uint64_t fence = gl->InsertFenceSyncCHROMIUM();
+
+  gl->OrderingBarrierCHROMIUM();
+
+  gpu::SyncToken sync_token;
+  gl->GenUnverifiedSyncTokenCHROMIUM(fence, sync_token.GetData());

[danakj, 2017/05/01 22:57:33]

This stuff is more heavy then you need if !async_worker_context, which is
equivalent to what you have here right?
https://cs.chromium.org/chromium/src/cc/raster/gpu_raster_buffer_provider.cc?...

[sohan, 2017/05/03 11:06:34]

I am using GenSyncToken in latest PS, i hope this is what you wanted, because we
shouldnt remove sync tokens all together. right?

+  lock.set_sync_token(sync_token);
+  lock.set_synchronized(true);
 }
 
 void HeadsUpDisplayLayerImpl::ReleaseResources() {