Rotation into Video Layer + Content Transform

Integrated video rotation piping into video layer {impl} and
included the transform for fixing the video orientation.

BUG=47554
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288194

[suderman, 2014-07-11 18:15:13]

Only a small change in the webmediaplayer_impl however there are more
substantial changes in the video layers. Also, quick check on the tests. They
are complete for checking the transform occurs correctly however there isn't any
large scale integration tests to make sure the rotation propagates to the video
layer.

[scherkus (not reviewing), 2014-07-11 18:54:08]

+danakj

overall seems OK but I'll defer to danakj@ for better judgement on the cc/ code

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer.cc
File cc/layers/video_layer.cc (right):

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer.cc...
cc/layers/video_layer.cc:15: VideoLayer::VideoLayer(VideoFrameProvider*
provider) : provider_(provider) {
initialize video_rotation_

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer.cc...
cc/layers/video_layer.cc:24: impl->set_video_rotation(video_rotation_);
if this is the only way to create a VideoLayerImpl and we always set the
rotation ... I wonder whether it makes more sense to have video_rotation as part
of the ctor

I'll let danakj@ weigh in

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer.h
File cc/layers/video_layer.h (right):

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer.h#...
cc/layers/video_layer.h:31: virtual void set_video_rotation(media::VideoRotation
video_rotation);
drop virtual and inline the setter as per style:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Access...

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer_im...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer_im...
cc/layers/video_layer_impl.cc:157: default:
try to avoid default where possible and instead explicitly list the cases

should anything change in the future the compiler will catch missing cases

[danakj, 2014-07-11 21:04:30]

I think you wanna add tests to layer_tree_host_unittest_video which are
integration unit tests. You can verify that your rotation makes it through the
pipeline there.

We have many subclasses of LayerTreeTest that you can use to guide you, or you
can ask if it's confusing. Basically you override functions in the TestHooks and
check things or poke things as needed.

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer.cc
File cc/layers/video_layer.cc (right):

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer.cc...
cc/layers/video_layer.cc:24: impl->set_video_rotation(video_rotation_);
This is the wrong place to do this, override PushPropertiesTo and do this set in
that place.

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer.cc...
cc/layers/video_layer.cc:44: }
This should call SetNeedsCommit();

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer_im...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer_im...
cc/layers/video_layer_impl.cc:69:
other->SetProviderClientImpl(provider_client_impl_);
pass the new rotation here, not in CreateLayerImpl

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer_im...
cc/layers/video_layer_impl.cc:139: PopulateSharedQuadState(shared_quad_state);
replace this with a call to SetAll (do this after you compute things), and pass
in the right content bounds and transform there.

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer_im...
cc/layers/video_layer_impl.cc:141: gfx::Size bounds = content_bounds();
rotated_content_bounds

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer_im...
cc/layers/video_layer_impl.cc:157: default:
On 2014/07/11 18:54:07, scherkus wrote:
> try to avoid default where possible and instead explicitly list the cases
> 
> should anything change in the future the compiler will catch missing cases

+1

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer_im...
cc/layers/video_layer_impl.cc:160:
shared_quad_state->content_to_target_transform = transform;
You want to Preconcat your transform onto the existing transform, not replace
it.

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer_im...
cc/layers/video_layer_impl.cc:357: void
VideoLayerImpl::set_video_rotation(media::VideoRotation video_rotation) {
SetVideoRotation, this isn't inline

https://codereview.chromium.org/388643002/diff/60001/cc/layers/video_layer_im...
cc/layers/video_layer_impl.cc:359: }
You need to call NoteLayerPropertyChanged()

[suderman, 2014-07-14 17:34:10]

Suggest changes have been integrated

New Content
- new test added for [layer]->[layer impl]->[layer impl] rotation propagation
- updated damage rect check to ensure it is still correct under rotatoin

Still need a rotation from [webmediaplayer_impl]->[video_layer] propagation.

[danakj, 2014-07-14 20:59:53]

So, I looked this over again, and while it appears to work to just do the
rotation in AppendQuads, I realized that it's so late in the pipeline that
things like occlusion tracking will have no idea this is going on, which means
we might think something is occluded and not draw it, when actually the video
will be rotated and it will be visible, which is bad. And properties on the
layer like visible_content_rect() will just be completely wrong.

To make this work right, we need to insert that rotation into the layer's
transform earlier.

I've been trying to think of a clean way to do this in the VideoLayer instead to
have it modify the layer's transform_ that gets pushed but it's kinda messy. And
it sounds like you've got the layer bounds actually wrong to fool the occlusion
into being right? I'm not a fan of that kind of thing.

This feels like something blink should be solving, where it knows what the
bounds of the layer are, and that it should be rotated, and sets the transform
on the weblayer appropriately. I don't want to propagate lies thru the system
that happen to cancel each other out.

[scherkus (not reviewing), 2014-07-14 23:35:18]

On 2014/07/14 20:59:53, danakj wrote:
> So, I looked this over again, and while it appears to work to just do the
> rotation in AppendQuads, I realized that it's so late in the pipeline that
> things like occlusion tracking will have no idea this is going on, which means
> we might think something is occluded and not draw it, when actually the video
> will be rotated and it will be visible, which is bad. And properties on the
> layer like visible_content_rect() will just be completely wrong.
> 
> To make this work right, we need to insert that rotation into the layer's
> transform earlier.
> 
> I've been trying to think of a clean way to do this in the VideoLayer instead
to
> have it modify the layer's transform_ that gets pushed but it's kinda messy.
And
> it sounds like you've got the layer bounds actually wrong to fool the
occlusion
> into being right? I'm not a fan of that kind of thing.
> 
> This feels like something blink should be solving, where it knows what the
> bounds of the layer are, and that it should be rotated, and sets the transform
> on the weblayer appropriately. I don't want to propagate lies thru the system
> that happen to cancel each other out.

Would that be operating on the WebLayer{Impl}, where we create it here?
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

... or perhaps plumbing this information all the way into the render tree /
RenderVideo?
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...


Having not looked too much into this... it's very possible we may have to plumb
rotation all the way into blink. I'm actually wondering if the
paint-to-webgl/canvas path is broken w/ the current approach.

suderman: can you see whether using rotated mp4s work with the following test
pages:
http://sleep.sea/demos/paint/

[suderman, 2014-07-14 23:40:25]

If there is a isue with paint to canvas, webgl, etc, I'm thinking we would have
to change how the texture is loaded.

[suderman, 2014-07-14 23:54:12]

Frame gets completely screwed up in canvas / webgl. I'm assuming we only fix
this with the final transform it only propagates correctly to drawing on screen
and not any other time.

[danakj, 2014-07-17 20:19:06]

ok since the layer bounds match the output of what layer here, I guess it can
work this way. I don't know the blink side of things well so I'll rely on you to
say that it's not reasonable to plumb this to blink and have it just set a
transform on the WebLayer.

https://codereview.chromium.org/388643002/diff/100001/cc/layers/video_layer.cc
File cc/layers/video_layer.cc (right):

https://codereview.chromium.org/388643002/diff/100001/cc/layers/video_layer.c...
cc/layers/video_layer.cc:11: scoped_refptr<VideoLayer>
VideoLayer::Create(VideoFrameProvider* provider) {
Do we need to keep 2 Create methods?

https://codereview.chromium.org/388643002/diff/100001/cc/layers/video_layer.c...
cc/layers/video_layer.cc:31: VideoLayerImpl::Create(tree_impl, id(), provider_);
Sorry to backtrack, we don't normally have properties set once at construction
time and never change. Because this is one, you could pass it along here and get
rid of the PushProperties methods and the impl-side setter method. Your
instincts were right on that. Then we don't have to worry about testing damage
tracking things and such, and less code.

https://codereview.chromium.org/388643002/diff/100001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/100001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:146: transform.Rotate(90);
90.0

https://codereview.chromium.org/388643002/diff/100001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:147: transform.Translate(0, -1.0);
0.0, etc.

(double literals to methods expecting a double)

https://codereview.chromium.org/388643002/diff/100001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:172: gfx::Rect quad_rect(1.0, 1.0);
Rect is integer, these should be ints

[suderman, 2014-07-21 17:12:17]

Layers changes made and additional browser test added. With Dana on vacation for
the next week would you be able to review Enne?

Adds logging of metadata into player.html, does not affect other tests passing
while adding logging of size information.

[enne (OOO), 2014-07-21 22:37:05]

Sorry if this rehashes previous discussions, but I'm just trying to understand
the code.  :)

[enne (OOO), 2014-07-21 22:37:14]

https://codereview.chromium.org/388643002/diff/200001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/200001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:546: gfx_rect.set_size(
Whoa, how does this do the right thing? Does the caller of this function know
that the frame is painted differently?

https://codereview.chromium.org/388643002/diff/200001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:988: video_weblayer_.reset(new
WebLayerImpl(layer));
Can you just set the transform here on the web layer instead of much later in
the pipeline in VideoLayerImpl?

[suderman, 2014-07-21 23:12:13]

Not a problem at all, it isn't the most straightforward CL.

https://codereview.chromium.org/388643002/diff/200001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/200001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:546: gfx_rect.set_size(
This is to maintain Canvas's current behavior. As blink is now provided the
rotated dimensions (for 90 and 270 degree rotations) this Paint is provided
incorrect dimensions of the frame. If this correction isn't performed frames
become stretched/squished. 

There is a future CL (https://codereview.chromium.org/402133005/) that corrects
this behavior however I wanted to minimize the contents of this CL.

https://codereview.chromium.org/388643002/diff/200001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:988: video_weblayer_.reset(new
WebLayerImpl(layer));
In order to correctly rotate the video layer it must be translated post rotation
(to correctly center the image). To perform this rotation we need the scale of
the frame that blink computes which isn't available at this point.

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

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer.cc
File cc/layers/video_layer.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer.c...
cc/layers/video_layer.cc:24: impl->set_video_rotation(video_rotation_);
This should be set during PushProperties.

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:59: impl->set_video_rotation(video_rotation_);
Properties should get pushed in push properties, not in this create constructor.

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:160: visible_content_rect(),
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?

https://codereview.chromium.org/388643002/diff/200001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/200001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:988: video_weblayer_.reset(new
WebLayerImpl(layer));
On 2014/07/21 23:12:12, suderman wrote:
> In order to correctly rotate the video layer it must be translated post
rotation
> (to correctly center the image). To perform this rotation we need the scale of
> the frame that blink computes which isn't available at this point.

And I guess Blink doesn't know at layout time that this is a video so it can't
do anything special there, either.  So, maybe cc really is the only place where
both of these pieces of information are known.  Hmm.

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

Both changes I am willing to make, however as both were changed previously when
working with Dana I want to make sure this is the final decision.

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer.cc
File cc/layers/video_layer.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer.c...
cc/layers/video_layer.cc:24: impl->set_video_rotation(video_rotation_);
On 2014/07/21 23:42:49, enne wrote:
> This should be set during PushProperties.

I am willing to make this change however it has moved in and out of push
properties previously (I was asked to add it to push properties then remove it)
so once everything else is sorted out, if we decide to move it to push
properties, I will make that change.

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:59: impl->set_video_rotation(video_rotation_);
On 2014/07/21 23:42:49, enne wrote:
> Properties should get pushed in push properties, not in this create
constructor.

ditto

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:160: visible_content_rect(),
On 2014/07/21 23:42:49, enne wrote:
> 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?

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.

[enne (OOO), 2014-07-22 21:10:40]

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer.cc
File cc/layers/video_layer.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer.c...
cc/layers/video_layer.cc:24: impl->set_video_rotation(video_rotation_);
On 2014/07/22 00:05:20, suderman wrote:
> On 2014/07/21 23:42:49, enne wrote:
> > This should be set during PushProperties.
> 
> I am willing to make this change however it has moved in and out of push
> properties previously (I was asked to add it to push properties then remove
it)
> so once everything else is sorted out, if we decide to move it to push
> properties, I will make that change.

Yeah, I disagree with what Dana said.  video_rotation either needs to be an
immutable constructor parameter or a mutable property that is pushed during push
properties.  Right now this code makes it look like a mutable property that's
set in the wrong place.

Can a video change source, reuse the same layer, and have a different rotation?

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:139: gfx::Transform transform = draw_transform();
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.

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.h (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.h:51: inline media::VideoRotation
get_video_rotation() { return video_rotation_; }
style nit: drop the get_

https://codereview.chromium.org/388643002/diff/200001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_video.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_video.cc:63: EXPECT_EQ(gfx::RectF(6.f, 6.f,
8.f, 10.f).ToString(),
Shouldn't the video be rotated here? I think this is the damage tracker issue I
mention elsewhere.

[suderman, 2014-07-22 21:48:18]

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer.cc
File cc/layers/video_layer.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer.c...
cc/layers/video_layer.cc:24: impl->set_video_rotation(video_rotation_);
If the video source changes it's metadata will still be processed by the
webmediaplayer making the rotation data set. However even though it doesn't
change moving video_rotation to push_properties groups similar data.

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:139: gfx::Transform transform = draw_transform();
I'll move the transform into push properties and (along with the rotation
setting) and check functionality.

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

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:139: gfx::Transform transform = draw_transform();
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]

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:139: gfx::Transform transform = draw_transform();
On 2014/07/22 22:53:59, suderman wrote:
> 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.

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

[suderman, 2014-07-23 21:40:46]

Rotation was moved into transform_ and all changes to video_layer_impl could be
removed. I updated the tests to check PushProperties between VideoLayer and
VideoLayerImpl. I needed to add an accessor to layer_test_common to use host_
for the layer construction. I'm not certain it is the best solution but it works
for checking the content bound and transformation matrix.

[enne (OOO), 2014-07-23 21:46:53]

Can you keep the test from cc/trees/layer_tree_host_unittest_video.cc from patch
set 11 that verifies the damage is correct when the video is rotated?

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer.cc
File cc/layers/video_layer.cc (right):

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer.c...
cc/layers/video_layer.cc:49: content_bounds =
I think content bounds shouldn't be modified here.  The bounds of the video are
still the same.  It is just rotated when drawn.

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl_unittest.cc (right):

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer_i...
cc/layers/video_layer_impl_unittest.cc:142: video_layer->SetLayerTreeHost(NULL);
Why does this need to happen? Won't the VideoLayer be destroyed before the
LayerTreeHost?

https://codereview.chromium.org/388643002/diff/260001/cc/test/layer_test_comm...
File cc/test/layer_test_common.h (right):

https://codereview.chromium.org/388643002/diff/260001/cc/test/layer_test_comm...
cc/test/layer_test_common.h:114: FakeLayerTreeHost* host() const { return
host_.get(); }
This seems fine.  Kind of surprised it wasn't needed before.

[suderman, 2014-07-23 23:27:01]

I added the test back in however it won't have rotated bounds. This is because
VideoLayer's content bounds are assumed to be correctly oriented and is swapped
and rotated during push properties to Video Layer Impl.

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer.cc
File cc/layers/video_layer.cc (right):

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer.c...
cc/layers/video_layer.cc:49: content_bounds =
The content bounds are used to construct the video layer impl's quad_rect. This
quad rect must have the dimensions unrotated. As blink is given the rotated size
to layout the page the content bounds may not reflect the unrotated size and
need to be swapped.

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl_unittest.cc (right):

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer_i...
cc/layers/video_layer_impl_unittest.cc:142: video_layer->SetLayerTreeHost(NULL);
In this case the LayerTreeHost is provided by impl's FakeLayerTreeHost. When
impl is destroyed it will destroy the FakeLayerTreeHost and when video_layer is
destroyed it will attempt to destroy it's LayerTreeHost. As it was already
destroyed by impl it will assert.

Long story short, if I didn't set LayerTreeHost to NULL LayerTreeHost would be
destroyed twice causing an assert.

[enne (OOO), 2014-07-23 23:39:26]

On 2014/07/23 23:27:01, suderman wrote:
> I added the test back in however it won't have rotated bounds. This is because
> VideoLayer's content bounds are assumed to be correctly oriented and is
swapped
> and rotated during push properties to Video Layer Impl.

Hmm, ok.

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer.cc
File cc/layers/video_layer.cc (right):

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer.c...
cc/layers/video_layer.cc:49: content_bounds =
On 2014/07/23 23:27:01, suderman wrote:
> The content bounds are used to construct the video layer impl's quad_rect.
This
> quad rect must have the dimensions unrotated. As blink is given the rotated
size
> to layout the page the content bounds may not reflect the unrotated size and
> need to be swapped.

Thanks! That was not obvious at all to me.

So, Blink doesn't know the video is rotated per se but does know the rotated
bounds?

I think you also need to update bounds() as well as content_bounds() if you're
going to do this.  Can you also leave a comment about why this has to happen?

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl_unittest.cc (right):

https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer_i...
cc/layers/video_layer_impl_unittest.cc:142: video_layer->SetLayerTreeHost(NULL);
On 2014/07/23 23:27:01, suderman wrote:
> In this case the LayerTreeHost is provided by impl's FakeLayerTreeHost. When
> impl is destroyed it will destroy the FakeLayerTreeHost and when video_layer
is
> destroyed it will attempt to destroy it's LayerTreeHost. As it was already
> destroyed by impl it will assert.
> 
> Long story short, if I didn't set LayerTreeHost to NULL LayerTreeHost would be
> destroyed twice causing an assert.

The VideoLayer has a raw/weak pointer to the LayerTreeHost, so it doesn't
destroy it.  It's also later in the scope than impl, so it should be destroyed
first unless something else is holding a ref to it?

[suderman, 2014-07-24 00:11:03]

> https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer.cc
> File cc/layers/video_layer.cc (right):
> 
>
https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer.c...
> cc/layers/video_layer.cc:49: content_bounds =
> On 2014/07/23 23:27:01, suderman wrote:
> > The content bounds are used to construct the video layer impl's quad_rect.
> This
> > quad rect must have the dimensions unrotated. As blink is given the rotated
> size
> > to layout the page the content bounds may not reflect the unrotated size and
> > need to be swapped.
> 
> Thanks! That was not obvious at all to me.
> 
> So, Blink doesn't know the video is rotated per se but does know the rotated
> bounds?

Exactly, Blink is oblivious to any special properties of
the video. The webmediaplayer tells it the amount of space
to allocated it returns the expected bounds of the video
and relative transform. On our side we use these bounds
and our rotation data to draw the video frame via the
video layer impl while correcting for rotation.

> I think you also need to update bounds() as well as content_bounds() if you're
> going to do this.  Can you also leave a comment about why this has to happen?

Done and done.

>
https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer_i...
> File cc/layers/video_layer_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/388643002/diff/260001/cc/layers/video_layer_i...
> cc/layers/video_layer_impl_unittest.cc:142:
video_layer->SetLayerTreeHost(NULL);
> On 2014/07/23 23:27:01, suderman wrote:
> > In this case the LayerTreeHost is provided by impl's FakeLayerTreeHost. When
> > impl is destroyed it will destroy the FakeLayerTreeHost and when video_layer
> is
> > destroyed it will attempt to destroy it's LayerTreeHost. As it was already
> > destroyed by impl it will assert.
> > 
> > Long story short, if I didn't set LayerTreeHost to NULL LayerTreeHost would
be
> > destroyed twice causing an assert.
> 
> The VideoLayer has a raw/weak pointer to the LayerTreeHost, so it doesn't
> destroy it.  It's also later in the scope than impl, so it should be destroyed
> first unless something else is holding a ref to it?

layer.cc:86
When layer has it's destructor called it checks whether it has a layer tree host
anymore and if I don't explicitly clear it out it asserts.

[enne (OOO), 2014-07-24 00:19:48]

> layer.cc:86
> When layer has it's destructor called it checks whether it has a layer tree
host
> anymore and if I don't explicitly clear it out it asserts.

Aha, thanks! I understand what's going on now.  I think your intuition that it
was a little weird to have to access the LayerTreeHost directly was correct.

I think this would be cleaner if you just called AddChild(video_layer) on the
LayerTreeHost's root layer.  This will set the LTH appropriately on that child
layer and when the LayerTreeHost is destroyed it will remove the child and unset
the LayerTreeHost pointer.

[suderman, 2014-07-25 17:45:42]

As the LayerTreeHost has a null root layer I had to set it as the 
root layer and it worked as expected.

[enne (OOO), 2014-07-25 17:55:13]

https://codereview.chromium.org/388643002/diff/320001/cc/trees/layer_tree_hos...
File cc/trees/layer_tree_host_unittest_video.cc (right):

https://codereview.chromium.org/388643002/diff/320001/cc/trees/layer_tree_hos...
cc/trees/layer_tree_host_unittest_video.cc:64: EXPECT_EQ(gfx::RectF(8.f, 6.f,
6.f, 6.f).ToString(),
Does this actually pass? I wouldn't expect the damage to be of size 6x6.

[danakj, 2014-07-27 11:21:13]

On Jul 22, 2014 11:10 PM, <enne@chromium.org> wrote:
>
>
>
https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer.cc
> File cc/layers/video_layer.cc (right):
>
>
https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer.c...
> cc/layers/video_layer.cc:24: impl->set_video_rotation(video_rotation_);
> On 2014/07/22 00:05:20, suderman wrote:
>>
>> On 2014/07/21 23:42:49, enne wrote:
>> > This should be set during PushProperties.
>
>
>> I am willing to make this change however it has moved in and out of
>
> push
>>
>> properties previously (I was asked to add it to push properties then
>
> remove it)
>>
>> so once everything else is sorted out, if we decide to move it to push
>> properties, I will make that change.
>
>
> Yeah, I disagree with what Dana said.  video_rotation either needs to be
> an immutable constructor parameter or a mutable property that is pushed
> during push properties.  Right now this code makes it look like a
> mutable property that's set in the wrong place.
>
> Can a video change source, reuse the same layer, and have a different
> rotation?

I thought it is an immutable property that we can set only at construction?

>
>
https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
> File cc/layers/video_layer_impl.cc (right):
>
>
https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
> cc/layers/video_layer_impl.cc:139: gfx::Transform transform =
> draw_transform();
> 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.

Because the bounds reflect the correct values of the layer regardless of
the rotation, the rotation just changes how the layer generates quads to
fill its bounds, doing it in AppendQuads had seemed OK to me.

>
https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
> File cc/layers/video_layer_impl.h (right):
>
>
https://codereview.chromium.org/388643002/diff/200001/cc/layers/video_layer_i...
> cc/layers/video_layer_impl.h:51: inline media::VideoRotation
> get_video_rotation() { return video_rotation_; }
> style nit: drop the get_
>
>
https://codereview.chromium.org/388643002/diff/200001/cc/trees/layer_tree_hos...
> File cc/trees/layer_tree_host_unittest_video.cc (right):
>
>
https://codereview.chromium.org/388643002/diff/200001/cc/trees/layer_tree_hos...
> cc/trees/layer_tree_host_unittest_video.cc:63: EXPECT_EQ(gfx::RectF(6.f,
> 6.f, 8.f, 10.f).ToString(),
> Shouldn't the video be rotated here? I think this is the damage tracker
> issue I mention elsewhere.
>
> https://codereview.chromium.org/388643002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[enne (OOO), 2014-07-28 17:35:07]

Sorry, that was my misunderstanding as to what the bounds were being passed in
as.  I don't think I would have suggested moving it where I did had I understood
that beforehand.

I'm going to stop attempting to review this and causing churn and just let
danakj finish it.

[danakj, 2014-08-05 14:40:38]

On 2014/07/28 17:35:07, enne wrote:
> Sorry, that was my misunderstanding as to what the bounds were being passed in
> as.  I don't think I would have suggested moving it where I did had I
understood
> that beforehand.
> 
> I'm going to stop attempting to review this and causing churn and just let
> danakj finish it.

Can we go back to patchset 11, more or less, though with any new test stuff you
added since. But remove the set_video_rotation() and pass it in the constructor
of VideoLayer and VideoLayerImpl.

[suderman, 2014-08-05 17:02:10]

> Can we go back to patchset 11, more or less, though with any new test stuff
you
> added since. But remove the set_video_rotation() and pass it in the
constructor
> of VideoLayer and VideoLayerImpl.

Should be possible, I may have to add an additional constructor to have
video_rotation_ default to VIDEO_ROTATION_0.

[danakj, 2014-08-05 17:15:30]

On Tue, Aug 5, 2014 at 1:02 PM, <suderman@chromium.org> wrote:

> Can we go back to patchset 11, more or less, though with any new test stuff
>>
> you
>
>> added since. But remove the set_video_rotation() and pass it in the
>>
> constructor
>
>> of VideoLayer and VideoLayerImpl.
>>
>
> Should be possible, I may have to add an additional constructor to have
> video_rotation_ default to VIDEO_ROTATION_0.
>

Or just change callers to pass 0?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[suderman, 2014-08-05 17:36:58]

On 2014/08/05 17:15:30, danakj wrote:
> On Tue, Aug 5, 2014 at 1:02 PM, <mailto:suderman@chromium.org> wrote:
> 
> > Can we go back to patchset 11, more or less, though with any new test stuff
> >>
> > you
> >
> >> added since. But remove the set_video_rotation() and pass it in the
> >>
> > constructor
> >
> >> of VideoLayer and VideoLayerImpl.
> >>
> >
> > Should be possible, I may have to add an additional constructor to have
> > video_rotation_ default to VIDEO_ROTATION_0.
> >
> 
> Or just change callers to pass 0?
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

There is a macro that depends on the constructor taking only 1 parameter
(DISALLOW_COPY_AND_ASSIGN). I'm seeing whether there is a nice way around the
issue however as videos usually require no rotation a 0 rotation default seems
to makes it feel more robust.

[danakj, 2014-08-05 17:54:18]

On 2014/08/05 17:36:58, suderman wrote:
> On 2014/08/05 17:15:30, danakj wrote:
> > On Tue, Aug 5, 2014 at 1:02 PM, <mailto:suderman@chromium.org> wrote:
> > 
> > > Can we go back to patchset 11, more or less, though with any new test
stuff
> > >>
> > > you
> > >
> > >> added since. But remove the set_video_rotation() and pass it in the
> > >>
> > > constructor
> > >
> > >> of VideoLayer and VideoLayerImpl.
> > >>
> > >
> > > Should be possible, I may have to add an additional constructor to have
> > > video_rotation_ default to VIDEO_ROTATION_0.
> > >
> > 
> > Or just change callers to pass 0?
> > 
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> 
> There is a macro that depends on the constructor taking only 1 parameter
> (DISALLOW_COPY_AND_ASSIGN). I'm seeing whether there is a nice way around the
> issue however as videos usually require no rotation a 0 rotation default seems
> to makes it feel more robust.

I'd say being explicit makes it more clear. Why does DISALLOW_COPY_AND_ASSIGN
require a default constructor? We use that elsewhere with classes that don't
have one. For example LayerTreeHost.

[suderman, 2014-08-05 20:04:30]

So apparently it doesn't, it was throwing errors when other builds errors
existed however once I resolved the rest it worked fine.

I had to change any test involving VideoLayer to include the video rotation
parameter and change layer test common to support the additional parameter.
Unfortunately the other tests aren't useful as they existed to check that
push_properties functioned and therefore isn't relevant for this patch.

[danakj, 2014-08-05 20:18:21]

Thanks! LGTM w/ a couple comments

https://codereview.chromium.org/388643002/diff/340001/cc/layers/video_layer.h
File cc/layers/video_layer.h (right):

https://codereview.chromium.org/388643002/diff/340001/cc/layers/video_layer.h...
cc/layers/video_layer.h:32: inline media::VideoRotation video_rotation() const {
return video_rotation_; }
nit: don't need |inline|

but i don't see any caller of this, remove it?

https://codereview.chromium.org/388643002/diff/340001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/340001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.cc:140: PopulateSharedQuadState(shared_quad_state);
drop this, since you do SetALl below. Can you move teh
CreateAndAppendSharedQuadState() just above the SetAll()?

https://codereview.chromium.org/388643002/diff/340001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl.h (right):

https://codereview.chromium.org/388643002/diff/340001/cc/layers/video_layer_i...
cc/layers/video_layer_impl.h:50: inline media::VideoRotation video_rotation()
const { return video_rotation_; }
nit: no inline

https://codereview.chromium.org/388643002/diff/340001/cc/layers/video_layer_i...
File cc/layers/video_layer_impl_unittest.cc (right):

https://codereview.chromium.org/388643002/diff/340001/cc/layers/video_layer_i...
cc/layers/video_layer_impl_unittest.cc:198: SCOPED_TRACE("180 Degree Rotation");
Since you broke these into separate test cases (thanks) do you need these
SCOPED_TRACEs?

[scherkus (not reviewing), 2014-08-06 23:06:42]

https://codereview.chromium.org/388643002/diff/360001/cc/layers/video_layer.h
File cc/layers/video_layer.h (right):

https://codereview.chromium.org/388643002/diff/360001/cc/layers/video_layer.h...
cc/layers/video_layer.h:33: explicit VideoLayer(VideoFrameProvider* provider,
explicit keyword no longer needed

https://codereview.chromium.org/388643002/diff/360001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/388643002/diff/360001/content/browser/media/m...
content/browser/media/media_browsertest.cc:57:
title_watcher.AlsoWaitForTitle(base::ASCIIToUTF16(expected_title));
we already pass this into the TitleWatcher constructor -- is this needed?

https://codereview.chromium.org/388643002/diff/360001/content/browser/media/m...
content/browser/media/media_browsertest.cc:102: void PlayVideoSize(const char*
media_file, bool http, const char* expected) {
nit: PlayVideoSize isn't really the best name

how about something along the lines of RunVideoSizeTest()

also it's a minor detail, but instead of requiring callers to know the format of
the string, it'd be preferable to have them pass in the sizes as int and have
this helper method construct the string seeing as this method is the one picking
the html page to run

https://codereview.chromium.org/388643002/diff/360001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/360001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:553: gfx_rect.set_size(
nit: for if-statements that are >1 line please use {}

[suderman, 2014-08-07 00:31:31]

https://codereview.chromium.org/388643002/diff/360001/cc/layers/video_layer.h
File cc/layers/video_layer.h (right):

https://codereview.chromium.org/388643002/diff/360001/cc/layers/video_layer.h...
cc/layers/video_layer.h:33: explicit VideoLayer(VideoFrameProvider* provider,
On 2014/08/06 23:06:42, scherkus wrote:
> explicit keyword no longer needed

Done.

https://codereview.chromium.org/388643002/diff/360001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/388643002/diff/360001/content/browser/media/m...
content/browser/media/media_browsertest.cc:57:
title_watcher.AlsoWaitForTitle(base::ASCIIToUTF16(expected_title));
On 2014/08/06 23:06:42, scherkus wrote:
> we already pass this into the TitleWatcher constructor -- is this needed?

The title when the video terminates must match the expected title.
In order to terminate for the dimensions title it needs to add the
title to the title_watcher, however as the title varies on the size
of the video it must explicitly have the expected title added to
the title watcher.

https://codereview.chromium.org/388643002/diff/360001/content/browser/media/m...
content/browser/media/media_browsertest.cc:102: void PlayVideoSize(const char*
media_file, bool http, const char* expected) {
On 2014/08/06 23:06:42, scherkus wrote:
> nit: PlayVideoSize isn't really the best name
> 
> how about something along the lines of RunVideoSizeTest()
> 
> also it's a minor detail, but instead of requiring callers to know the format
of
> the string, it'd be preferable to have them pass in the sizes as int and have
> this helper method construct the string seeing as this method is the one
picking
> the html page to run

Done.

https://codereview.chromium.org/388643002/diff/360001/content/renderer/media/...
File content/renderer/media/webmediaplayer_impl.cc (right):

https://codereview.chromium.org/388643002/diff/360001/content/renderer/media/...
content/renderer/media/webmediaplayer_impl.cc:553: gfx_rect.set_size(
On 2014/08/06 23:06:42, scherkus wrote:
> nit: for if-statements that are >1 line please use {}

Done.

[scherkus (not reviewing), 2014-08-07 00:38:49]

https://codereview.chromium.org/388643002/diff/360001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/388643002/diff/360001/content/browser/media/m...
content/browser/media/media_browsertest.cc:57:
title_watcher.AlsoWaitForTitle(base::ASCIIToUTF16(expected_title));
On 2014/08/07 00:31:31, suderman wrote:
> On 2014/08/06 23:06:42, scherkus wrote:
> > we already pass this into the TitleWatcher constructor -- is this needed?
> 
> The title when the video terminates must match the expected title.
> In order to terminate for the dimensions title it needs to add the
> title to the title_watcher, however as the title varies on the size
> of the video it must explicitly have the expected title added to
> the title watcher.

I get that part, but on line 56 we already add |expected_title| via the
TitleWatcher() constructor, which pushes it into its vector:
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...

I'm confused why we need to add the same string to TitleWatcher's vector twice

https://codereview.chromium.org/388643002/diff/370001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/388643002/diff/370001/content/browser/media/m...
content/browser/media/media_browsertest.cc:159:
IN_PROC_BROWSER_TEST_P(MediaTest, VideoBearRotated0) {
these should be _P -> _F and remove the GetParam() stuff

the _P runs parameterized tests, meaning we'll run these tests over both http://
and file://

we don't need to duplicate the run time of these tests

[suderman, 2014-08-07 00:49:36]

https://codereview.chromium.org/388643002/diff/360001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/388643002/diff/360001/content/browser/media/m...
content/browser/media/media_browsertest.cc:57:
title_watcher.AlsoWaitForTitle(base::ASCIIToUTF16(expected_title));
On 2014/08/07 00:38:48, scherkus wrote:
> On 2014/08/07 00:31:31, suderman wrote:
> > On 2014/08/06 23:06:42, scherkus wrote:
> > > we already pass this into the TitleWatcher constructor -- is this needed?
> > 
> > The title when the video terminates must match the expected title.
> > In order to terminate for the dimensions title it needs to add the
> > title to the title_watcher, however as the title varies on the size
> > of the video it must explicitly have the expected title added to
> > the title watcher.
> 
> I get that part, but on line 56 we already add |expected_title| via the
> TitleWatcher() constructor, which pushes it into its vector:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes...
> 
> I'm confused why we need to add the same string to TitleWatcher's vector twice

Ah, okay. I wasn't aware that the title_watcher automatically added the expected
title. I believe I saw the title timing out and thought it was due to not
watching for the expected title. Removed.

https://codereview.chromium.org/388643002/diff/370001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/388643002/diff/370001/content/browser/media/m...
content/browser/media/media_browsertest.cc:159:
IN_PROC_BROWSER_TEST_P(MediaTest, VideoBearRotated0) {
On 2014/08/07 00:38:48, scherkus wrote:
> these should be _P -> _F and remove the GetParam() stuff
> 
> the _P runs parameterized tests, meaning we'll run these tests over both
http://
> and file://
> 
> we don't need to duplicate the run time of these tests

Done.

[scherkus (not reviewing), 2014-08-07 00:55:47]

https://codereview.chromium.org/388643002/diff/390001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/388643002/diff/390001/content/browser/media/m...
content/browser/media/media_browsertest.cc:102: void RunVideoSizeTest(const
char* media_file, bool http,
remove http param -- we always pass in true

also, we should run these over file:// so we don't have to bother with spawning
the test server

[suderman, 2014-08-07 00:58:41]

https://codereview.chromium.org/388643002/diff/390001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/388643002/diff/390001/content/browser/media/m...
content/browser/media/media_browsertest.cc:102: void RunVideoSizeTest(const
char* media_file, bool http,
On 2014/08/07 00:55:46, scherkus wrote:
> remove http param -- we always pass in true
> 
> also, we should run these over file:// so we don't have to bother with
spawning
> the test server

Done.

[scherkus (not reviewing), 2014-08-07 01:10:45]

https://codereview.chromium.org/388643002/diff/410001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/388643002/diff/410001/content/browser/media/m...
content/browser/media/media_browsertest.cc:102: void RunVideoSizeTest(const
char* media_file, bool http,
Drop the http parameter entirely from this function -- there's no need to give
callers the option.

[suderman, 2014-08-07 01:15:24]

https://codereview.chromium.org/388643002/diff/410001/content/browser/media/m...
File content/browser/media/media_browsertest.cc (right):

https://codereview.chromium.org/388643002/diff/410001/content/browser/media/m...
content/browser/media/media_browsertest.cc:102: void RunVideoSizeTest(const
char* media_file, bool http,
On 2014/08/07 01:10:44, scherkus wrote:
> Drop the http parameter entirely from this function -- there's no need to give
> callers the option.

Done.

[scherkus (not reviewing), 2014-08-07 18:42:59]

lgtm

[suderman, 2014-08-07 19:01:57]

The CQ bit was checked by suderman@chromium.org

[commit-bot: I haz the power, 2014-08-07 19:07:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/suderman@chromium.org/388643002/430001

[suderman, 2014-08-07 20:15:01]

The CQ bit was unchecked by suderman@chromium.org

[suderman, 2014-08-07 20:16:26]

The CQ bit was checked by suderman@chromium.org

[commit-bot: I haz the power, 2014-08-07 20:26:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/suderman@chromium.org/388643002/450001

[commit-bot: I haz the power, 2014-08-08 01:24:52]

Change committed as 288194