Rotation into Video Layer + Content Transform

cc/layers/video_layer_impl.cc

Index: cc/layers/video_layer_impl.cc
diff --git a/cc/layers/video_layer_impl.cc b/cc/layers/video_layer_impl.cc
index 7b4234c870cc023429578a807918edc4a9739bdf..a91592e16ad85625ed6c94e9644bcd87aed872be 100644
--- a/cc/layers/video_layer_impl.cc
+++ b/cc/layers/video_layer_impl.cc
@@ -37,8 +37,8 @@ scoped_ptr<VideoLayerImpl> VideoLayerImpl::Create(
 }
 
 VideoLayerImpl::VideoLayerImpl(LayerTreeImpl* tree_impl, int id)
-    : LayerImpl(tree_impl, id),
-      frame_(NULL) {}
+    : LayerImpl(tree_impl, id), frame_(NULL) {
+}
 
 VideoLayerImpl::~VideoLayerImpl() {
   if (!provider_client_impl_->Stopped()) {
@@ -55,7 +55,9 @@ VideoLayerImpl::~VideoLayerImpl() {
 
 scoped_ptr<LayerImpl> VideoLayerImpl::CreateLayerImpl(
     LayerTreeImpl* tree_impl) {
-  return scoped_ptr<LayerImpl>(new VideoLayerImpl(tree_impl, id()));
+  VideoLayerImpl* impl = new VideoLayerImpl(tree_impl, id());
+  impl->set_video_rotation(video_rotation_);

[enne (OOO), 2014/07/21 23:42:49]

Properties should get pushed in push properties, not in this create constructor.

[suderman, 2014/07/22 00:05:20]

ditto

+  return scoped_ptr<LayerImpl>(impl);
 }
 
 void VideoLayerImpl::PushPropertiesTo(LayerImpl* layer) {
@@ -134,16 +136,44 @@ void VideoLayerImpl::AppendQuads(
       render_pass->CreateAndAppendSharedQuadState();
   PopulateSharedQuadState(shared_quad_state);
 
+  gfx::Transform transform = draw_transform();

[enne (OOO), 2014/07/22 21:10:39]

I think you also need to move the draw transform out of AppendQuads.  It is too
late in the pipeline.  DamageTracker::ExtendDamageForLayer uses layer transform
and bounds, and will not take this rotation into consideration.

I think the right place to do this would actually be to do this during
VideoLayer::PushPropertiesTo, and clobber the transform that's set on the
VideoLayerImpl with this modified version after calling the
Layer::PushProperties base class method.  Then, none of the code that deals with
LayerImpl has to know anything about this rotation and can just treat it as a
transformed layer (which it already knows how to do).

Additionally, you will not need a NoteLayerPropertyChanged since that'll happen
automatically as the layer is created for the first time.

[suderman, 2014/07/22 21:48:16]

I'll move the transform into push properties and (along with the rotation
setting) and check functionality.

[suderman, 2014/07/22 22:53:59]

Unfortunately draw_transform is not set via PushProperties and is not set when
PushProperties is executed. If I move the rotation transform to PushProperties
it is wiped out before append quads.

[enne (OOO), 2014/07/22 22:55:37]

Sorry, I was trying to suggest that you modify LayerImpl::transform_.

+  transform.Scale(content_bounds().width(), content_bounds().height());
+
+  switch (video_rotation_) {
+    case media::VIDEO_ROTATION_90:
+      transform.Rotate(90.0);
+      transform.Translate(0.0, -1.0);
+      break;
+    case media::VIDEO_ROTATION_180:
+      transform.Rotate(180.0);
+      transform.Translate(-1.0, -1.0);
+      break;
+    case media::VIDEO_ROTATION_270:
+      transform.Rotate(270.0);
+      transform.Translate(-1.0, 0);
+    case media::VIDEO_ROTATION_0:
+      break;
+  }
+
+  shared_quad_state->SetAll(transform,
+                            content_bounds(),
+                            visible_content_rect(),

[enne (OOO), 2014/07/21 23:42:49]

This seems a bit wrong.  Quad rects and visible content rects are in the space
of content bounds.  You can't change quad rects to be 1x1 but not change content
bounds or visible content rect without breaking some assumptions.

It seems like you could do this without the scale, if you just translated by
-width or -height rather than -1?

[suderman, 2014/07/22 00:05:20]

Previous version used width/height translation w/o scaling however it was found
to be confusing due to 90/270 degree rotations swapping width/height. We could
change to use a 1,1 content/visible rect to match or use the width/height
translation. I have alternated between these before so I am willing to use
either once there is a final say.

+                            clip_rect(),
+                            is_clipped(),
+                            draw_opacity(),
+                            blend_mode(),
+                            sorting_context_id());
+
   AppendDebugBorderQuad(
       render_pass, content_bounds(), shared_quad_state, append_quads_data);
 
-  gfx::Rect quad_rect(content_bounds());
+  gfx::Rect quad_rect(1, 1);
   gfx::Rect opaque_rect(contents_opaque() ? quad_rect : gfx::Rect());
   gfx::Rect visible_rect = frame_->visible_rect();
   gfx::Size coded_size = frame_->coded_size();
 
-  gfx::Rect visible_quad_rect = occlusion_tracker.UnoccludedContentRect(
-      quad_rect, draw_properties().target_space_transform);
+  gfx::Rect visible_quad_rect =
+      occlusion_tracker.UnoccludedContentRect(quad_rect, transform);
   if (visible_quad_rect.IsEmpty())
     return;