Ignore Camera and Flip bits in CVO when parsing video rotation

Ignore Camera and Flip bits in CVO when parsing video rotation

Currently, if WebRTC receives a CVO byte where the Camera or Flip bit is
set, then rotation is incorrectly parsed as 0. This CL fixes that issue.
The Camera and Flip bit is still unimplemented and will just be ignored
though.

BUG=webrtc:6120
R=danilchap@webrtc.org, pthatcher@webrtc.org, tommi@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/f9e1b922ef3e0cbe70953dfb7a1d4cb2c44a49e3

[magjed_webrtc, 2016-08-25 15:13:17]

Description was changed from

==========
Ignore Camera and Flip bits in CVO when parsing video rotation

Currently, if WebRTC receives a CVO byte where the Camera or Flip bit is
set, then rotation is incorrectly parsed as 0. This CL fixes that issue.
The Camera and Flip bit is still unimplemented and will just be ignored.

BUG=webrtc:6120
==========

to

==========
Ignore Camera and Flip bits in CVO when parsing video rotation

Currently, if WebRTC receives a CVO byte where the Camera or Flip bit is
set, then rotation is incorrectly parsed as 0. This CL fixes that issue.
The Camera and Flip bit is still unimplemented and will just be ignored,
though.

BUG=webrtc:6120
==========

[magjed_webrtc, 2016-08-25 15:13:26]

Description was changed from

==========
Ignore Camera and Flip bits in CVO when parsing video rotation

Currently, if WebRTC receives a CVO byte where the Camera or Flip bit is
set, then rotation is incorrectly parsed as 0. This CL fixes that issue.
The Camera and Flip bit is still unimplemented and will just be ignored,
though.

BUG=webrtc:6120
==========

to

==========
Ignore Camera and Flip bits in CVO when parsing video rotation

Currently, if WebRTC receives a CVO byte where the Camera or Flip bit is
set, then rotation is incorrectly parsed as 0. This CL fixes that issue.
The Camera and Flip bit is still unimplemented and will just be ignored
though.

BUG=webrtc:6120
==========

[magjed_webrtc, 2016-08-25 15:16:54]

Patchset #2 (id:20001) has been deleted

[magjed_webrtc, 2016-08-25 15:17:27]

Patchset #1 (id:1) has been deleted

[magjed_webrtc, 2016-08-25 15:17:40]

magjed@webrtc.org changed reviewers:
+ pthatcher@webrtc.org

[magjed_webrtc, 2016-08-25 15:18:50]

Peter - please take a look.

[danilchap, 2016-08-25 15:54:16]

danilchap@webrtc.org changed reviewers:
+ danilchap@webrtc.org

[danilchap, 2016-08-25 15:54:17]

lgtm,
this is one of the ways to fix the issue.

https://codereview.webrtc.org/2280703002/diff/40001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2280703002/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:788: // This byte actually represents the whole CVO byte:
|0 0 0 0 C F R R|.
Another way to fix this bug 
is to change rtp_utility.cc line 400:
header->extension.videoRotation = ptr[0] & 0x03;
then this byte would be rotation only.
(VideoOrientation::Parse is still unused)

[magjed_webrtc, 2016-08-26 07:56:26]

https://codereview.webrtc.org/2280703002/diff/40001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2280703002/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:788: // This byte actually represents the whole CVO byte:
|0 0 0 0 C F R R|.
On 2016/08/25 15:54:17, danilchap wrote:
> Another way to fix this bug 
> is to change rtp_utility.cc line 400:
> header->extension.videoRotation = ptr[0] & 0x03;
> then this byte would be rotation only.
> (VideoOrientation::Parse is still unused)

Right. I'm not sure if we should parse the rotation and C/F bits directly in
rtp_utility.cc or not. This seemed like the easiest solution, then I don't need
to remove or change the name of ConvertCVOByteToVideoRotation. We mix the names
"videoRotation", "videoOrientation", and CVO interchangeably in the code, so
it's a bit confusing.

[magjed_webrtc, 2016-08-26 07:59:33]

magjed@webrtc.org changed reviewers:
+ tommi@webrtc.org

[magjed_webrtc, 2016-08-26 07:59:58]

tommi - can you take a look as well? I need your approval for
webrtc/common_types.h.

[tommi, 2016-08-29 13:43:17]

The CQ bit was checked by tommi@webrtc.org

[tommi, 2016-08-29 13:43:17]

lgtm

[commit-bot: I haz the power, 2016-08-29 13:43:24]

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

[commit-bot: I haz the power, 2016-08-29 13:56:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-29 13:56:34]

Try jobs failed on following builders:
  android_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/16080)

[pthatcher1, 2016-08-29 18:52:59]

https://codereview.webrtc.org/2280703002/diff/40001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2280703002/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:788: // This byte actually represents the whole CVO byte:
|0 0 0 0 C F R R|.
On 2016/08/26 07:56:26, magjed_webrtc wrote:
> On 2016/08/25 15:54:17, danilchap wrote:
> > Another way to fix this bug 
> > is to change rtp_utility.cc line 400:
> > header->extension.videoRotation = ptr[0] & 0x03;
> > then this byte would be rotation only.
> > (VideoOrientation::Parse is still unused)
> 
> Right. I'm not sure if we should parse the rotation and C/F bits directly in
> rtp_utility.cc or not. This seemed like the easiest solution, then I don't
need
> to remove or change the name of ConvertCVOByteToVideoRotation. We mix the
names
> "videoRotation", "videoOrientation", and CVO interchangeably in the code, so
> it's a bit confusing.

I think it makes most sense for videoRotation here to be an enum, like we have
videoRotation in other places.  If it's not an enum, then it should be named
something that makes it clear it is a CVObyte, perhaps calling it |cvo|.  But
giving it a name that sounds like an enum and value that's a cvo-specific byte
is confusing.

[magjed_webrtc, 2016-08-31 11:06:59]

https://codereview.webrtc.org/2280703002/diff/40001/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2280703002/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:788: // This byte actually represents the whole CVO byte:
|0 0 0 0 C F R R|.
On 2016/08/29 18:52:59, pthatcher1 wrote:
> On 2016/08/26 07:56:26, magjed_webrtc wrote:
> > On 2016/08/25 15:54:17, danilchap wrote:
> > > Another way to fix this bug 
> > > is to change rtp_utility.cc line 400:
> > > header->extension.videoRotation = ptr[0] & 0x03;
> > > then this byte would be rotation only.
> > > (VideoOrientation::Parse is still unused)
> > 
> > Right. I'm not sure if we should parse the rotation and C/F bits directly in
> > rtp_utility.cc or not. This seemed like the easiest solution, then I don't
> need
> > to remove or change the name of ConvertCVOByteToVideoRotation. We mix the
> names
> > "videoRotation", "videoOrientation", and CVO interchangeably in the code, so
> > it's a bit confusing.
> 
> I think it makes most sense for videoRotation here to be an enum, like we have
> videoRotation in other places.  If it's not an enum, then it should be named
> something that makes it clear it is a CVObyte, perhaps calling it |cvo|.  But
> giving it a name that sounds like an enum and value that's a cvo-specific byte
> is confusing.  

To be clear, this mixup is not something I added in this CL. It has been like
that since CVO was added here: https://webrtc-codereview.appspot.com/42439004.
Actually, reading the review discussion in that CL, it was you that pushed for
changing the name of this byte from coordinationOfVideoOrientation to
videoRotation.

In the latest patch set, I discard the camera and flip bits of the CVO byte in
order to make videoRotation an enum. It will be future work for someone to parse
and store the camera and flip bits in rtp_utility.cc and
rtp_header_extensions.cc.

[pthatcher1, 2016-08-31 17:54:03]

lgtm 

On 2016/08/31 11:06:59, magjed_webrtc wrote:
> https://codereview.webrtc.org/2280703002/diff/40001/webrtc/common_types.h
> File webrtc/common_types.h (right):
> 
>
https://codereview.webrtc.org/2280703002/diff/40001/webrtc/common_types.h#new...
> webrtc/common_types.h:788: // This byte actually represents the whole CVO
byte:
> |0 0 0 0 C F R R|.
> On 2016/08/29 18:52:59, pthatcher1 wrote:
> > On 2016/08/26 07:56:26, magjed_webrtc wrote:
> > > On 2016/08/25 15:54:17, danilchap wrote:
> > > > Another way to fix this bug 
> > > > is to change rtp_utility.cc line 400:
> > > > header->extension.videoRotation = ptr[0] & 0x03;
> > > > then this byte would be rotation only.
> > > > (VideoOrientation::Parse is still unused)
> > > 
> > > Right. I'm not sure if we should parse the rotation and C/F bits directly
in
> > > rtp_utility.cc or not. This seemed like the easiest solution, then I don't
> > need
> > > to remove or change the name of ConvertCVOByteToVideoRotation. We mix the
> > names
> > > "videoRotation", "videoOrientation", and CVO interchangeably in the code,
so
> > > it's a bit confusing.
> > 
> > I think it makes most sense for videoRotation here to be an enum, like we
have
> > videoRotation in other places.  If it's not an enum, then it should be named
> > something that makes it clear it is a CVObyte, perhaps calling it |cvo|. 
But
> > giving it a name that sounds like an enum and value that's a cvo-specific
byte
> > is confusing.  
> 
> To be clear, this mixup is not something I added in this CL. It has been like
> that since CVO was added here: https://webrtc-codereview.appspot.com/42439004.

Yes, you found the mess.  Not your fault.

> Actually, reading the review discussion in that CL, it was you that pushed for
> changing the name of this byte from coordinationOfVideoOrientation to
> videoRotation.

Ah, my fault :).  I wish I had noticed back during that review that this was a
byte with more bits.  On the other hand, "coordinationOfVideoOrientation" is no
more clear that it's a byte (which includes more than orientation) any more than
"videoOrientation" is.  So I was just suggesting a shorter version of wrong.

> 
> In the latest patch set, I discard the camera and flip bits of the CVO byte in
> order to make videoRotation an enum. It will be future work for someone to
parse
> and store the camera and flip bits in rtp_utility.cc and
> rtp_header_extensions.cc.

Thanks for making the meaning match the name.  I like it.

[magjed_webrtc, 2016-09-01 10:11:18]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-09-01 10:11:18]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2280703002/#ps60001 (title:
"Only store rotation, not CVO byte")

[commit-bot: I haz the power, 2016-09-01 10:11:22]

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

[commit-bot: I haz the power, 2016-09-01 10:31:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 10:31:22]

Try jobs failed on following builders:
  linux_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_rel/builds/17837)
  linux_ubsan_vptr on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)

[magjed_webrtc, 2016-09-01 11:13:12]

The CQ bit was checked by magjed@webrtc.org

[magjed_webrtc, 2016-09-01 11:13:12]

The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org,
danilchap@webrtc.org, pthatcher@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2280703002/#ps80001 (title:
"Update unittests")

[commit-bot: I haz the power, 2016-09-01 11:13:22]

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

[commit-bot: I haz the power, 2016-09-01 13:13:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-01 13:13:58]

Try jobs failed on following builders:
  android_arm64_rel on master.tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[commit-bot: I haz the power, 2016-09-01 17:58:31]

Description was changed from

==========
Ignore Camera and Flip bits in CVO when parsing video rotation

Currently, if WebRTC receives a CVO byte where the Camera or Flip bit is
set, then rotation is incorrectly parsed as 0. This CL fixes that issue.
The Camera and Flip bit is still unimplemented and will just be ignored
though.

BUG=webrtc:6120
==========

to

==========
Ignore Camera and Flip bits in CVO when parsing video rotation

Currently, if WebRTC receives a CVO byte where the Camera or Flip bit is
set, then rotation is incorrectly parsed as 0. This CL fixes that issue.
The Camera and Flip bit is still unimplemented and will just be ignored
though.

BUG=webrtc:6120
R=danilchap@webrtc.org, pthatcher@webrtc.org, tommi@webrtc.org

Committed: https://crrev.com/f9e1b922ef3e0cbe70953dfb7a1d4cb2c44a49e3
Cr-Commit-Position: refs/heads/master@{#14027}
==========

[commit-bot: I haz the power, 2016-09-01 17:58:32]

Patchset 3 (id:??) landed as
https://crrev.com/f9e1b922ef3e0cbe70953dfb7a1d4cb2c44a49e3
Cr-Commit-Position: refs/heads/master@{#14027}

[magjed_webrtc, 2016-09-01 17:58:33]

Description was changed from

==========
Ignore Camera and Flip bits in CVO when parsing video rotation

Currently, if WebRTC receives a CVO byte where the Camera or Flip bit is
set, then rotation is incorrectly parsed as 0. This CL fixes that issue.
The Camera and Flip bit is still unimplemented and will just be ignored
though.

BUG=webrtc:6120
R=danilchap@webrtc.org, pthatcher@webrtc.org, tommi@webrtc.org

Committed: https://crrev.com/f9e1b922ef3e0cbe70953dfb7a1d4cb2c44a49e3
Cr-Commit-Position: refs/heads/master@{#14027}
==========

to

==========
Ignore Camera and Flip bits in CVO when parsing video rotation

Currently, if WebRTC receives a CVO byte where the Camera or Flip bit is
set, then rotation is incorrectly parsed as 0. This CL fixes that issue.
The Camera and Flip bit is still unimplemented and will just be ignored
though.

BUG=webrtc:6120
R=danilchap@webrtc.org, pthatcher@webrtc.org, tommi@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/f9e1b922ef3e0cbe70953dfb7...
==========

[magjed_webrtc, 2016-09-01 17:58:34]

Committed patchset #3 (id:80001) manually as
f9e1b922ef3e0cbe70953dfb7a1d4cb2c44a49e3 (presubmit successful).

[magjed_webrtc, 2016-09-02 08:03:03]

A revert of this CL (patchset #3 id:80001) has been created in
https://codereview.webrtc.org/2300323002/ by magjed@webrtc.org.

The reason for reverting is: Breaks downstream build..