[Contextual Search] Add support for crushed sprites and animate the search provider icon

chrome/browser/android/compositor/layer/crushed_sprite_layer.cc

+// Copyright 2015 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 "chrome/browser/android/compositor/layer/crushed_sprite_layer.h"
+
+#include "cc/layers/layer.h"
+#include "cc/layers/ui_resource_layer.h"
+#include "content/public/browser/android/compositor.h"
+#include "ui/android/resources/crushed_sprite_resource.h"
+#include "ui/android/resources/resource_manager.h"
+#include "ui/gfx/canvas.h"
+#include "ui/gfx/skia_util.h"
+
+namespace chrome {
+namespace android {
+
+// static
+scoped_refptr<CrushedSpriteLayer> CrushedSpriteLayer::Create(
+    ui::ResourceManager* resource_manager) {
+  return make_scoped_refptr(new CrushedSpriteLayer(resource_manager));
+}
+
+scoped_refptr<cc::Layer> CrushedSpriteLayer::layer() {
+  return layer_;
+}
+
+void CrushedSpriteLayer::SetCrushedSpriteResource(
+    scoped_refptr<ui::CrushedSpriteResource> resource) {
+  resource_ = resource;

[David Trainor- moved to gerrit, 2015/10/15 21:04:57]

Ideally we wouldn't hold onto the resource here.  I think the general rule is
the ResourceManager owns them.  I see you got around it with struct wrapper, but
I'm wondering if there's a better way to manage that data at a higher level. 
Ie:  The caller of these methods drives whether or not this retains the
resource.

The tricky part would be dropping the resource from the manager at the right
time (other people could want it).  But for now you could add a method like
releaseCrushedSpriteResource() or something...  Not sure what's happening with
the Java memory but I'll take a look in a bit :).

ie in contextual_search_layer:
if (layer_->IsLastFrame() && current_frame == layer_->GetFrame()) {
  resource_manager_->ReleaseCrushedSpriteResource(magicid);
} else if (current_frame != layer_->GetFrame()) {
  resource = resource_manager_->GetCrushedSpriteResource(magicid);
  // This doesn't actually hold onto the data, but uses it during this draw. 
Can check some internal id or something to see if it's a totally different type
of crushed sprite resource from last time this was called and reset internal
state.
  layer_->SetFrame(resource, current_frame);
}

[Theresa, 2015/10/24 00:06:45]

I got rid of this method and just pass the resource in to DrawSpriteFrame. See
response to other comment wrt to releasing the crushed sprite from memory in
contextual_search_layer.

+}
+
+void CrushedSpriteLayer::DrawSpriteFrame(int sprite_frame) {

[David Trainor- moved to gerrit, 2015/10/15 21:04:57]

Should you clamp sprite_frame between 0 and the max frame?

[Theresa, 2015/10/24 00:06:45]

Done.

+  if (sprite_frame == 0 || sprite_frame != previous_frame_) {
+    // Reset the previous_frame if the animation is being re-run.
+    if (previous_frame_ > sprite_frame) {
+      previous_frame_ = 0;
+    }
+
+    // If this is the last frame and a bitmap for it has been cached with the
+    // resource, set the bitmap on layer_ and return.
+    if (sprite_frame == resource_->GetFrameCount() - 1 &&
+        !resource_->GetBitmapForLastFrame().empty()) {

[David Trainor- moved to gerrit, 2015/10/15 21:04:57]

Do we need this?  Can't we rely on the sprite_frame to always be the last one so
we skip all of this code?

[Theresa, 2015/10/24 00:06:44]

I think you mean rely on previous_frame_bitmap to be the last one? (sprite_frame
is whatever frame we're currently drawing)

I implemented it this way because I thought the resource could/would outlive the
layer. Also, the resource could be shared between multiple layers, whereas the
previous_frame_bitmap is not.

I looked into this more and I think the contextual search layer only gets
created once then lives in perpetuity until Chrome is closed, so for the current
implementation, using previous_frame_bitmap instead of caching the bitmap for
the last frame with the resource would work/perform equally well.

[David Trainor- moved to gerrit, 2015/10/27 15:14:47]

I see your point.  I'd be okay with sharing it.  Something to fix though,
calling layer_->SetBitmap() builds a new internal ScopedUIResource IIRC.  That
will build a new texture, which means we're not sharing the bitmap on the GPU. 
Can we save the last frame bitmap to a UI resource internally in
CrushedSpriteResource?

[Theresa, 2015/10/27 19:48:02]

Done.

+      layer_->SetBitmap(resource_->GetBitmapForLastFrame());
+      return;
+    }
+
+    // Reload the source bitmap if necessary.
+    if (resource_->BitmapHasBeenEvictedFromMemory()) {

[David Trainor- moved to gerrit, 2015/10/15 21:04:57]

See above.  Can probably simplify this a lot and remove the inter-dependencies. 
Push control logic to the caller who knows about how this layer will be used?

[Theresa, 2015/10/24 00:06:45]

I put the logic for evicting the sprite sheet from memory/using the cached
bitmap for the last frame/reloading the crushed sprite if necessary here because
I think (hope) it will be useful for more than just the search provider icon
animation.

By default, I think we don't want crushed sprite sheets hanging around in memory
after the animation is finished. But I would be okay with moving this logic and
the above logic to contextual_search_layer.cc since nobody else uses this layer
yet.

[David Trainor- moved to gerrit, 2015/10/27 15:14:47]

Won't this cause reloads during animations if two crushed sprite layers are
drawing?

[Theresa, 2015/10/27 19:48:02]

As discussed offline, yes, it could cause unnecessary reloads but it's unlikely
that two layers will use the same crushed sprite resource. I moved the reloading
logic to the ResourceManager.

+      resource_manager_->ReloadCrushedSpriteResource(
+          resource_->GetBitmapResourceId());
+    }
+
+    // Set up an SkCanvas backed by an SkBitmap to draw into.
+    SkBitmap* bitmap = new SkBitmap();
+    bitmap->allocN32Pixels(resource_->GetSpriteSize(),
+                           resource_->GetSpriteSize());
+    SkCanvas* canvas = new SkCanvas(*bitmap);

[David Trainor- moved to gerrit, 2015/10/15 21:04:57]

Leaking canvas and bitmap.  Use:

// Don't worry, it copies it but doesn't copy the pixel ref.
SkBitmap bitmap;

// We want canvas to go away when we're done painting.
skia::RefPtr<SkCanvas> = skia::AdoptRef(new SkCanavs(bitmap));

[Theresa, 2015/10/24 00:06:44]

Done.

+
+    // If this isn't the first or last frame, draw the previous frame(s).
+    // Note(twellington): This assumes that the last frame in the crushed sprite
+    // animation does not require any previous frames drawn before it. This code
+    // needs to be updated if crushed sprites are added for which this
+    // assumption does not hold.
+    if (sprite_frame != 0 && sprite_frame != resource_->GetFrameCount() - 1) {
+      // Draw the previous frame.
+      canvas->drawBitmap(previous_frame_bitmap_, 0, 0, nullptr);

[David Trainor- moved to gerrit, 2015/10/15 21:04:57]

So... UIResourceBitmap expects an immututable bitmap.  I forget why that is
(probably due to assumptions that the UIResourceBitmap always represents the
actual SkBitmap).  If we can change that, the following makes sense.  Otherwise,
ignore (but it would be a good straight forward optimization):

An interesting thing to try would be to back the canvas with the
previous_frame_bitmap_'s canvas and see if you can just use that instead of
redrawing it :).

Actually, I would probably always back the frame with the previous frame's
bitmap (if that works).  Then in the cases where you can't use the previous
frame I would clear it with canvas_->clear(SkColor).  That will skip the bitmap
allocation you do every frame.  I would check that UIResourceLayer is okay with
you changing the contents of the bitmap it holds... and still call SetBitmap()
or something to let it know it's invalid.

[Theresa, 2015/10/24 00:06:45]

I'm not sure why it has to be immutable (it just has a comment saying it must be
then does a bunch of DCHECK's to make sure it is). git blame says
powei@chormium.org wrote that code.. any idea on who that is (ldap)?

[David Trainor- moved to gerrit, 2015/10/27 15:14:47]

I'm not sure if he still works here.  I would imagine it's because we want the
GL texture to always represent the bitmap and we can't guarantee that if it's
immutable.

[Theresa, 2015/10/27 19:48:02]

Acknowledged.

+
+      // Draw any skipped frames.
+      for (int i = previous_frame_ + 1; i < sprite_frame; ++i) {
+        DrawRectanglesForFrame(i, canvas);
+      }
+    }
+
+    // Draw the current frame.
+    DrawRectanglesForFrame(sprite_frame, canvas);
+
+    // Set the bitmap on layer_.
+    bitmap->setImmutable();
+    layer_->SetBitmap(*bitmap);
+
+    // Cache the bitmap on the resource if this is the last frame.
+    if (sprite_frame == resource_->GetFrameCount() - 1) {

[David Trainor- moved to gerrit, 2015/10/15 21:04:57]

See above, probably don't need.

[Theresa, 2015/10/27 19:48:02]

I left the bitmap memory ejection here but moved the reloading logic to
ResourceManager->GetCrushedSpriteResource

+      resource_->SetBitmapForLastFrame(*bitmap);
+    }
+
+    // Update previous_frame_* variables.
+    previous_frame_bitmap_ = *bitmap;
+    previous_frame_ = sprite_frame;
+  }
+}
+
+void CrushedSpriteLayer::DrawRectanglesForFrame(int frame, SkCanvas* canvas) {

[David Trainor- moved to gerrit, 2015/10/15 21:04:57]

skia::RefPtr<SkCanvas>

[Theresa, 2015/10/24 00:06:45]

Done.

+  ui::CrushedSpriteResource::FrameSrcDstRects src_dst_rects =
+       resource_->GetRectanglesForFrame(frame);
+  for (auto rect : src_dst_rects) {

[David Trainor- moved to gerrit, 2015/10/15 21:04:57]

auto& so we don't copy the object?

[Theresa, 2015/10/24 00:06:44]

Done.

+    canvas->drawBitmapRect(resource_->GetBitmap(),
+                           gfx::RectToSkRect(rect.first),
+                           gfx::RectToSkRect(rect.second),
+                           nullptr);
+   }
+}
+
+CrushedSpriteLayer::CrushedSpriteLayer(ui::ResourceManager* resource_manager)
+    : resource_manager_(resource_manager),
+      layer_(
+          cc::UIResourceLayer::Create(content::Compositor::LayerSettings())),
+      previous_frame_(0) {

[David Trainor- moved to gerrit, 2015/10/15 21:04:57]

-1 to say that we haven't drawn the first frame?

[Theresa, 2015/10/24 00:06:44]

Done.

+  layer_->SetIsDrawable(true);
+}
+
+
+CrushedSpriteLayer::~CrushedSpriteLayer() {
+}
+
+}  // namespace android
+}  // namespace chrome