Apply rotation for texture backed VideoFrames

Apply rotation for texture backed VideoFrames

This CL addresses the bug below where we would see rotated video on
the remote peer of an Android client during a video call. This was due to
rotation information being dropped for texture backed video frames,
which happens in H264 HW decode cases. I added VideoFrame metadata
to carry rotation information, and necessary changes in WebMediaPlayerMS
to paint rotated textures.

BUG=webrtc:6069
TEST=Tested AppRTC connection between N6 and MacOSX using H264
codec. Added unittest.

Committed: https://crrev.com/8f3584a70bc1f4cae6531418d9b3a5ae85ebf464
Cr-Commit-Position: refs/heads/master@{#405847}

[emircan, 2016-07-15 01:16:00]

Description was changed from

==========
fix android rotation issue

BUG=
==========

to

==========
Apply rotation for texture backed VideoFrames

This CL addresses the bug below where we would see rotated video on
the remote peer of an Android client during a video call. This was due to 
rotation information being dropped for texture backed video frames,
which happens in H264 HW decode cases. I added VideoFrame metadata
to carry rotation information, and necessary changes in WebMediaPlayerMS
to handle frames.

BUG=webrtc:6069
TEST=Tested AppRTC connection between N6 and MacOSX using H264 
codec. Added unittest.
==========

[emircan, 2016-07-15 01:16:57]

Description was changed from

==========
Apply rotation for texture backed VideoFrames

This CL addresses the bug below where we would see rotated video on
the remote peer of an Android client during a video call. This was due to 
rotation information being dropped for texture backed video frames,
which happens in H264 HW decode cases. I added VideoFrame metadata
to carry rotation information, and necessary changes in WebMediaPlayerMS
to handle frames.

BUG=webrtc:6069
TEST=Tested AppRTC connection between N6 and MacOSX using H264 
codec. Added unittest.
==========

to

==========
Apply rotation for texture backed VideoFrames

This CL addresses the bug below where we would see rotated video on
the remote peer of an Android client during a video call. This was due to 
rotation information being dropped for texture backed video frames,
which happens in H264 HW decode cases. I added VideoFrame metadata
to carry rotation information, and necessary changes in WebMediaPlayerMS
to paint rotated textures.

BUG=webrtc:6069
TEST=Tested AppRTC connection between N6 and MacOSX using H264 
codec. Added unittest.
==========

[emircan, 2016-07-15 01:21:42]

emircan@chromium.org changed reviewers:
+ mcasas@chromium.org, suderman@chromium.org

[emircan, 2016-07-15 01:21:42]

PTAL.

[emircan, 2016-07-15 01:25:39]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-07-15 01:57:34]

LGTM % comments below (nothing major).

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:272: return
blink::WebSize(current_size.height(), current_size.width());
nit: blink::WebSize has a ctor with gfx::Size as parameter [1]
so you can do
return blink::WebSize(compositor->GetCurrentSize());



[1]
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebSi...

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:507: }
All metadata() methods are marked WARN_UNUSED_RESULT,
but you don't have to use a DVLOG just for that. Instead, 
please use ignore_result() to signify that here, you don't care.

https://cs.chromium.org/chromium/src/base/macros.h?sq=package:chromium&dr=C&r...

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_unittest.cc (right):

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_unittest.cc:722: // applies to player's
natural size.
nit: This is a TEST comment, move it to l.719

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_unittest.cc:726: std::vector<int>
timestamps(tokens, tokens + sizeof(tokens) / sizeof(int));
If |tokens| is constant, make it 
static int kTokens[] = {...}
std::vector<int> timestamps(tokens, tokens +arraysize(tokens));

arraysize() being calculated in compile time.

https://codereview.chromium.org/2150203002/diff/20001/media/base/video_frame_...
File media/base/video_frame_metadata.h (right):

https://codereview.chromium.org/2150203002/diff/20001/media/base/video_frame_...
media/base/video_frame_metadata.h:110: // Indicates that the underlying texture
of this frame is rotated.
nit: not necessarily a Texture.

[emircan, 2016-07-15 17:32:54]

emircan@chromium.org changed reviewers:
+ chcunningham@chromium.org

[emircan, 2016-07-15 17:32:55]

chcunningham@ for OWNERS review on /media/*

[chcunningham, 2016-07-15 18:25:52]

media lgtm

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:272: return
blink::WebSize(current_size.height(), current_size.width());
I think he means to swap width and height here. IIUC your suggestion wouldn't do
the rotation.

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:507: }
On 2016/07/15 01:57:34, mcasas wrote:
> All metadata() methods are marked WARN_UNUSED_RESULT,
> but you don't have to use a DVLOG just for that. Instead, 
> please use ignore_result() to signify that here, you don't care.
> 
>
https://cs.chromium.org/chromium/src/base/macros.h?sq=package:chromium&dr=C&r...

He does care though right? Line 510 where he uses video_rotation_

[emircan, 2016-07-15 18:43:13]

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms.cc (right):

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:272: return
blink::WebSize(current_size.height(), current_size.width());
On 2016/07/15 01:57:34, mcasas wrote:
> nit: blink::WebSize has a ctor with gfx::Size as parameter [1]
> so you can do
> return blink::WebSize(compositor->GetCurrentSize());
> 
> 
> 
> [1]
>
https://cs.chromium.org/chromium/src/third_party/WebKit/public/platform/WebSi...

I am swapping height and width here as ccunningham@ pointed out.

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:507: }
On 2016/07/15 01:57:34, mcasas wrote:
> All metadata() methods are marked WARN_UNUSED_RESULT,
> but you don't have to use a DVLOG just for that. Instead, 
> please use ignore_result() to signify that here, you don't care.
> 
>
https://cs.chromium.org/chromium/src/base/macros.h?sq=package:chromium&dr=C&r...

Done.

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms.cc:507: }
On 2016/07/15 18:25:52, chcunningham wrote:
> On 2016/07/15 01:57:34, mcasas wrote:
> > All metadata() methods are marked WARN_UNUSED_RESULT,
> > but you don't have to use a DVLOG just for that. Instead, 
> > please use ignore_result() to signify that here, you don't care.
> > 
> >
>
https://cs.chromium.org/chromium/src/base/macros.h?sq=package:chromium&dr=C&r...
> 
> He does care though right? Line 510 where he uses video_rotation_

What I ignore here is the return bool of GetRotation, video_rotation_ is used in
all cases.

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
File content/renderer/media/webmediaplayer_ms_unittest.cc (right):

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_unittest.cc:722: // applies to player's
natural size.
On 2016/07/15 01:57:34, mcasas wrote:
> nit: This is a TEST comment, move it to l.719

Done.

https://codereview.chromium.org/2150203002/diff/20001/content/renderer/media/...
content/renderer/media/webmediaplayer_ms_unittest.cc:726: std::vector<int>
timestamps(tokens, tokens + sizeof(tokens) / sizeof(int));
On 2016/07/15 01:57:34, mcasas wrote:
> If |tokens| is constant, make it 
> static int kTokens[] = {...}
> std::vector<int> timestamps(tokens, tokens +arraysize(tokens));
> 
> arraysize() being calculated in compile time.

Done.

https://codereview.chromium.org/2150203002/diff/20001/media/base/video_frame_...
File media/base/video_frame_metadata.h (right):

https://codereview.chromium.org/2150203002/diff/20001/media/base/video_frame_...
media/base/video_frame_metadata.h:110: // Indicates that the underlying texture
of this frame is rotated.
On 2016/07/15 01:57:34, mcasas wrote:
> nit: not necessarily a Texture.

Done.

[emircan, 2016-07-15 20:02:40]

The CQ bit was checked by emircan@chromium.org

[emircan, 2016-07-15 20:02:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
chcunningham@chromium.org, mcasas@chromium.org
Link to the patchset: https://codereview.chromium.org/2150203002/#ps40001
(title: "Comments.")

[commit-bot: I haz the power, 2016-07-15 20:03:36]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 20:33:00]

Description was changed from

==========
Apply rotation for texture backed VideoFrames

This CL addresses the bug below where we would see rotated video on
the remote peer of an Android client during a video call. This was due to 
rotation information being dropped for texture backed video frames,
which happens in H264 HW decode cases. I added VideoFrame metadata
to carry rotation information, and necessary changes in WebMediaPlayerMS
to paint rotated textures.

BUG=webrtc:6069
TEST=Tested AppRTC connection between N6 and MacOSX using H264 
codec. Added unittest.
==========

to

==========
Apply rotation for texture backed VideoFrames

This CL addresses the bug below where we would see rotated video on
the remote peer of an Android client during a video call. This was due to 
rotation information being dropped for texture backed video frames,
which happens in H264 HW decode cases. I added VideoFrame metadata
to carry rotation information, and necessary changes in WebMediaPlayerMS
to paint rotated textures.

BUG=webrtc:6069
TEST=Tested AppRTC connection between N6 and MacOSX using H264 
codec. Added unittest.
==========

[commit-bot: I haz the power, 2016-07-15 20:33:02]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-07-15 20:33:18]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 20:35:28]

Description was changed from

==========
Apply rotation for texture backed VideoFrames

This CL addresses the bug below where we would see rotated video on
the remote peer of an Android client during a video call. This was due to 
rotation information being dropped for texture backed video frames,
which happens in H264 HW decode cases. I added VideoFrame metadata
to carry rotation information, and necessary changes in WebMediaPlayerMS
to paint rotated textures.

BUG=webrtc:6069
TEST=Tested AppRTC connection between N6 and MacOSX using H264 
codec. Added unittest.
==========

to

==========
Apply rotation for texture backed VideoFrames

This CL addresses the bug below where we would see rotated video on
the remote peer of an Android client during a video call. This was due to
rotation information being dropped for texture backed video frames,
which happens in H264 HW decode cases. I added VideoFrame metadata
to carry rotation information, and necessary changes in WebMediaPlayerMS
to paint rotated textures.

BUG=webrtc:6069
TEST=Tested AppRTC connection between N6 and MacOSX using H264
codec. Added unittest.

Committed: https://crrev.com/8f3584a70bc1f4cae6531418d9b3a5ae85ebf464
Cr-Commit-Position: refs/heads/master@{#405847}
==========

[commit-bot: I haz the power, 2016-07-15 20:35:29]

Patchset 2 (id:??) landed as
https://crrev.com/8f3584a70bc1f4cae6531418d9b3a5ae85ebf464
Cr-Commit-Position: refs/heads/master@{#405847}