Add support for VP9 packetization/depacketization.

Add support for VP9 packetization/depacketization.

RTP payload format for VP9:
https://www.ietf.org/id/draft-uberti-payload-vp9-01.txt

BUG=webrtc:4148, webrtc:4168, chromium:500602
TBR=mflodman

Committed: https://crrev.com/f38ea3caa39887c63e7d4862dcf420d4a35c1073
Cr-Commit-Position: refs/heads/master@{#9649}

[åsapersson, 2015-07-10 12:35:40]

asapersson@webrtc.org changed reviewers:
+ stefan@webrtc.org

[åsapersson, 2015-07-10 12:39:59]

Patchset #1 (id:1) has been deleted

[åsapersson, 2015-07-10 12:43:18]

Made a separate cl for the packetization.

[åsapersson, 2015-07-10 14:41:55]

Patchset #2 (id:40001) has been deleted

[åsapersson, 2015-07-10 14:42:12]

Patchset #2 (id:60001) has been deleted

[åsapersson, 2015-07-10 14:43:03]

Patchset #1 (id:20001) has been deleted

[åsapersson, 2015-07-13 08:18:04]

Patchset #3 (id:120001) has been deleted

[åsapersson, 2015-07-13 08:18:15]

Patchset #2 (id:100001) has been deleted

[åsapersson, 2015-07-13 08:18:54]

Patchset #1 (id:80001) has been deleted

[stefan-webrtc, 2015-07-14 09:40:10]

We should probably try to fuzz this thing as well. Maybe pbos can help.

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/interface...
File webrtc/modules/interface/module_common_types.h (right):

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/interface...
webrtc/modules/interface/module_common_types.h:110: bool interPicPredicted;  //
This layer frame is dependent on previously coded
If you don't mind, I would prefer none of these members were camelCase. I know
other members in this file are, but maybe a good thing to start correcting
now...

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right):

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:83: if (hdr.flexibleMode)
if {} else {}

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:111: length += hdr.pidDiff[i] >
0x3F ? 2 : 1;
It is not entirely clear to me where 0x3F comes from. Do you know?

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:146: length +=
hdr.gof.numFramesInGof;          // N_G
I'm not sure I follow this. Is this supposed to represent the length of:
      +-+-+-+-+-+-+-+-+
 N_G: |  T  |U| R |-|-|
      +-+-+-+-+-+-+-+-+

that is, 1 byte for each frame in the GOF? I guess that makes sense.

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:291: }
I think we should DCHECK on these instead.

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h (right):

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h:19: */
Make this a regular comment using //

[pbos-webrtc, 2015-07-14 09:45:44]

On 2015/07/14 09:40:10, stefan-webrtc (holmer) wrote:
> We should probably try to fuzz this thing as well. Maybe pbos can help.

I can, do you need this fuzzed before submitting?

[åsapersson, 2015-07-14 10:53:55]

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/interface...
File webrtc/modules/interface/module_common_types.h (right):

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/interface...
webrtc/modules/interface/module_common_types.h:110: bool interPicPredicted;  //
This layer frame is dependent on previously coded
On 2015/07/14 09:40:10, stefan-webrtc (holmer) wrote:
> If you don't mind, I would prefer none of these members were camelCase. I know
> other members in this file are, but maybe a good thing to start correcting
> now...

Done.

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right):

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:83: if (hdr.flexibleMode)
On 2015/07/14 09:40:10, stefan-webrtc (holmer) wrote:
> if {} else {}

Done.

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:111: length += hdr.pidDiff[i] >
0x3F ? 2 : 1;
On 2015/07/14 09:40:10, stefan-webrtc (holmer) wrote:
> It is not entirely clear to me where 0x3F comes from. Do you know?

Added comment.

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:146: length +=
hdr.gof.numFramesInGof;          // N_G
On 2015/07/14 09:40:10, stefan-webrtc (holmer) wrote:
> I'm not sure I follow this. Is this supposed to represent the length of:
>       +-+-+-+-+-+-+-+-+
>  N_G: |  T  |U| R |-|-|
>       +-+-+-+-+-+-+-+-+
> 
> that is, 1 byte for each frame in the GOF? I guess that makes sense.

Updated comment.

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:291: }
On 2015/07/14 09:40:10, stefan-webrtc (holmer) wrote:
> I think we should DCHECK on these instead.

Done.

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h (right):

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.h:19: */
On 2015/07/14 09:40:10, stefan-webrtc (holmer) wrote:
> Make this a regular comment using //

Done.

[åsapersson, 2015-07-17 12:38:27]

asapersson@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[åsapersson, 2015-07-17 12:38:57]

Adding Magnus for webrtc/modules/interface/module_common_types.h.

[stefan-webrtc, 2015-07-28 06:58:14]

A few comments, but overall I think this is starting to get ready to go.

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right):

https://codereview.webrtc.org/1232023006/diff/180001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:111: length += hdr.pidDiff[i] >
0x3F ? 2 : 1;
On 2015/07/14 10:53:54, asapersson wrote:
> On 2015/07/14 09:40:10, stefan-webrtc (holmer) wrote:
> > It is not entirely clear to me where 0x3F comes from. Do you know?
> 
> Added comment.

Maybe we should DCHECK that P_DIFF doesn't become too big for the extended
version as well?

https://codereview.webrtc.org/1232023006/diff/220001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right):

https://codereview.webrtc.org/1232023006/diff/220001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:255: uint32_t n_bit = (i ==
vp9.num_ref_pics - 1) ? 0 : 1;
Could these be bool instead?

https://codereview.webrtc.org/1232023006/diff/220001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:425: while (p_diff >
scaled_pid) {
We should make sure we have a test which verifies this. I.e., a test where the
picture id wraps between a frame and its reference.

https://codereview.webrtc.org/1232023006/diff/220001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:649: uint32_t v_bit =
(hdr_.ss_data_available && b_bit) ? 1 : 0;
Preferably make these bool as well. Or does it make the code less readable?

https://codereview.webrtc.org/1232023006/diff/220001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:714: //
parsed_payload->type.Video.codec = kRtpVideoVp9;
Should be easily done in this CL as well, or?

[stefan-webrtc, 2015-07-28 06:59:20]

https://codereview.webrtc.org/1232023006/diff/240001/webrtc/modules/interface...
File webrtc/modules/interface/module_common_types.h (right):

https://codereview.webrtc.org/1232023006/diff/240001/webrtc/modules/interface...
webrtc/modules/interface/module_common_types.h:133: num_ref_pics;  // Number of
reference pictures used by this layer frame.
Maybe move this comment to the line above instead? I think this line wraps in a
strange way.

[åsapersson, 2015-07-28 09:06:34]

Patchset #7 (id:260001) has been deleted

[åsapersson, 2015-07-28 09:07:58]

https://codereview.webrtc.org/1232023006/diff/220001/webrtc/modules/rtp_rtcp/...
File webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc (right):

https://codereview.webrtc.org/1232023006/diff/220001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:255: uint32_t n_bit = (i ==
vp9.num_ref_pics - 1) ? 0 : 1;
On 2015/07/28 06:58:14, stefan-webrtc (holmer) wrote:
> Could these be bool instead?

Done.

https://codereview.webrtc.org/1232023006/diff/220001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:425: while (p_diff >
scaled_pid) {
On 2015/07/28 06:58:14, stefan-webrtc (holmer) wrote:
> We should make sure we have a test which verifies this. I.e., a test where the
> picture id wraps between a frame and its reference.

Done in TestRefIdx and ParseRefIdx. Updated code to actually compare expected
and actual in VerifyHeader.

https://codereview.webrtc.org/1232023006/diff/220001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:649: uint32_t v_bit =
(hdr_.ss_data_available && b_bit) ? 1 : 0;
On 2015/07/28 06:58:14, stefan-webrtc (holmer) wrote:
> Preferably make these bool as well. Or does it make the code less readable?

Done.

https://codereview.webrtc.org/1232023006/diff/220001/webrtc/modules/rtp_rtcp/...
webrtc/modules/rtp_rtcp/source/rtp_format_vp9.cc:714: //
parsed_payload->type.Video.codec = kRtpVideoVp9;
On 2015/07/28 06:58:14, stefan-webrtc (holmer) wrote:
> Should be easily done in this CL as well, or?

Done.

https://codereview.webrtc.org/1232023006/diff/240001/webrtc/modules/interface...
File webrtc/modules/interface/module_common_types.h (right):

https://codereview.webrtc.org/1232023006/diff/240001/webrtc/modules/interface...
webrtc/modules/interface/module_common_types.h:133: num_ref_pics;  // Number of
reference pictures used by this layer frame.
On 2015/07/28 06:59:20, stefan-webrtc (holmer) wrote:
> Maybe move this comment to the line above instead? I think this line wraps in
a
> strange way.

Done.

[stefan-webrtc, 2015-07-28 09:29:46]

lgtm

[åsapersson, 2015-07-28 11:02:01]

The CQ bit was checked by asapersson@webrtc.org

[åsapersson, 2015-07-28 11:02:02]

The patchset sent to the CQ was uploaded after l-g-t-m from stefan@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1232023006/#ps300001 (title:
"rebase")

[commit-bot: I haz the power, 2015-07-28 11:02:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1232023006/300001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1232023006/300001

[commit-bot: I haz the power, 2015-07-28 11:03:03]

Committed patchset #8 (id:300001)

[commit-bot: I haz the power, 2015-07-28 11:03:07]

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

[pbos-webrtc, 2015-07-29 08:12:36]

On 2015/07/28 11:03:07, commit-bot: I haz the power wrote:
> Patchset 8 (id:??) landed as
> https://crrev.com/f38ea3caa39887c63e7d4862dcf420d4a35c1073
> Cr-Commit-Position: refs/heads/master@{#9649}

FYI: Fuzzer's been covering this for ~20 hours without catching anything. :)