cc: Use gpu raster in HUD.

cc/layers/heads_up_display_layer_impl.cc

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/layers/heads_up_display_layer_impl.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 #include <vector>
 
 #include "base/numerics/safe_conversions.h"
 #include "base/stl_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/trace_event_argument.h"
 #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"
 #include "third_party/skia/include/core/SkPath.h"
 #include "third_party/skia/include/core/SkTypeface.h"
 #include "third_party/skia/include/effects/SkColorMatrixFilter.h"
 #include "third_party/skia/include/effects/SkGradientShader.h"
 #include "ui/gfx/geometry/point.h"
 #include "ui/gfx/geometry/size.h"
 #include "ui/gfx/geometry/size_conversions.h"
@@ skipping 53 matching lines @@
 void HeadsUpDisplayLayerImpl::AcquireResource(
     ResourceProvider* resource_provider) {
   for (auto& resource : resources_) {
     if (!resource_provider->InUseByConsumer(resource->id())) {
       resource.swap(resources_.back());
       return;
     }
   }
 
   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));
 }
 
 void HeadsUpDisplayLayerImpl::ReleaseUnmatchedSizeResources(
     ResourceProvider* resource_provider) {
   base::EraseIf(resources_,
                 [this](const std::unique_ptr<ScopedResource>& resource) {
                   return internal_content_bounds_ != resource->size();
                 });
 }
@@ skipping 39 matching lines @@
       render_pass->CreateAndAppendDrawQuad<TextureDrawQuad>();
   quad->SetNew(shared_quad_state, quad_rect, opaque_rect, visible_quad_rect,
                resources_.back()->id(), premultiplied_alpha, uv_top_left,
                uv_bottom_right, SK_ColorTRANSPARENT, vertex_opacity, flipped,
                nearest_neighbor, false);
   ValidateQuadResources(quad);
 }
 
 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);
+  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);
 
-  if (canvas_size.width() != internal_content_bounds_.width() ||
-      canvas_size.height() != internal_content_bounds_.height() ||
-      !hud_surface_) {
-    TRACE_EVENT0("cc", "ResizeHudCanvas");
-
-    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();
 
   {
     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_);
 
     DrawHudContents(hud_surface_->getCanvas());
 
     hud_surface_->getCanvas()->restore();
   }
 
   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() {
   resources_.clear();
 }
 
 gfx::Rect HeadsUpDisplayLayerImpl::GetEnclosingRectInTargetSpace() const {
   DCHECK_GT(internal_contents_scale_, 0.f);
   return GetScaledEnclosingRectInTargetSpace(internal_contents_scale_);
 }
@@ skipping 582 matching lines @@
   return "cc::HeadsUpDisplayLayerImpl";
 }
 
 void HeadsUpDisplayLayerImpl::AsValueInto(
     base::trace_event::TracedValue* dict) const {
   LayerImpl::AsValueInto(dict);
   dict->SetString("layer_name", "Heads Up Display Layer");
 }
 
 }  // namespace cc