cast: cleanup rtp header and parsing

cast: clean up lint errors on commit
cast: cleanup rtp header and parsing

- Remove redundant rtp_header_parser which was mostly duplicated
  code that was modified for tests. Add all the necessary functionality
  to the existing rtp_parser and update tests to use rtp_parser

- remove rtp_receiver_defines.h and integrate it with rtp_defines.h where
  it makes more sense since it is not specific to receiver

- These changes help bring in adaptive playout data parsing into the tests
  which will help add unittests for adaptive playout for future CLs

BUG=

Committed: https://crrev.com/64c95062775d2779d6a588942f3b9ac24f5ccb9b
Cr-Commit-Position: refs/heads/master@{#352853}

[Irfan, 2015-09-30 23:03:10]

isheriff@chromium.org changed reviewers:
+ miu@chromium.org, xjz@chromium.org

[Irfan, 2015-09-30 23:04:05]

This clean up CL mostly started as a result of me needing to add unit tests for
the bugs in adaptive playout handling in the sender code.

I will have a follow up CL which actually leverages the clean up done here

[xjz, 2015-10-01 00:11:53]

lgtm

[miu, 2015-10-01 00:36:23]

lgtm!

Minor things:

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/BUILD.gn
File media/cast/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/BUILD.gn#...
media/cast/BUILD.gn:309: "net/rtp/rtp_defines.h",
Since this target depends on the :net target (via :receiver or :sender), do we
need to include rtp_defines.h in the sources list here?  (I could be mistaken,
but the build should work if the sources are listed in upstream targets.)

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/BUILD.gn#...
media/cast/BUILD.gn:312: "net/rtp/rtp_parser.cc"
ditto: Looks like rtp_parser.* is included in :net.

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/cast.gyp
File media/cast/cast.gyp (right):

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/cast.gyp#...
media/cast/cast.gyp:110: 'net/rtp/rtp_defines.h',
Should rtp_defines.* be in the cast_net target instead?  Also, whatever the
choice, this should be consistent with the GN build rules.

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/cast_test...
File media/cast/cast_testing.gypi (right):

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/cast_test...
media/cast/cast_testing.gypi:105: 'net/rtp/rtp_defines.h',
ditto: Is rtp_defines.* needed?

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/cast_test...
media/cast/cast_testing.gypi:109: 'net/rtp/rtp_parser.cc',
ditto: Is rtp_parser.* needed?

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/net/rtp/r...
File media/cast/net/rtp/rtp_header_parser.h (left):

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/net/rtp/r...
media/cast/net/rtp/rtp_header_parser.h:15: // TODO(miu): Kill this and use
RtpCastHeader instead.
Yep.  This got lost in my attempts to create crbugs for all the clean-up issues.
 ;-)

[Irfan, 2015-10-01 19:26:52]

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/BUILD.gn
File media/cast/BUILD.gn (right):

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/BUILD.gn#...
media/cast/BUILD.gn:309: "net/rtp/rtp_defines.h",
On 2015/10/01 00:36:22, miu wrote:
> Since this target depends on the :net target (via :receiver or :sender), do we
> need to include rtp_defines.h in the sources list here?  (I could be mistaken,
> but the build should work if the sources are listed in upstream targets.)

Done.

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/BUILD.gn#...
media/cast/BUILD.gn:312: "net/rtp/rtp_parser.cc"
On 2015/10/01 00:36:22, miu wrote:
> ditto: Looks like rtp_parser.* is included in :net.

Done.

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/cast.gyp
File media/cast/cast.gyp (right):

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/cast.gyp#...
media/cast/cast.gyp:110: 'net/rtp/rtp_defines.h',
On 2015/10/01 00:36:22, miu wrote:
> Should rtp_defines.* be in the cast_net target instead?  Also, whatever the
> choice, this should be consistent with the GN build rules.

Done.

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/cast_test...
File media/cast/cast_testing.gypi (right):

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/cast_test...
media/cast/cast_testing.gypi:105: 'net/rtp/rtp_defines.h',
On 2015/10/01 00:36:22, miu wrote:
> ditto: Is rtp_defines.* needed?

Done.

https://chromiumcodereview.appspot.com/1377273003/diff/1/media/cast/cast_test...
media/cast/cast_testing.gypi:109: 'net/rtp/rtp_parser.cc',
On 2015/10/01 00:36:22, miu wrote:
> ditto: Is rtp_parser.* needed?

Yes

[Irfan, 2015-10-01 19:32:39]

PTAL, I cleaned up some duplicated constants in rtp_defines.h in patchset #2 as
well

[miu, 2015-10-02 19:13:45]

https://codereview.chromium.org/1377273003/diff/40001/media/cast/net/rtp/rtp_...
File media/cast/net/rtp/rtp_defines.h (right):

https://codereview.chromium.org/1377273003/diff/40001/media/cast/net/rtp/rtp_...
media/cast/net/rtp/rtp_defines.h:15: static const uint16 kCastHeaderLength = 7;
If any of these constants are only used in one .cc file, it would generally be
better to remove them from this header file.  This would help prevent
duplication of data across compilation outputs.

[Irfan, 2015-10-06 21:33:03]

https://codereview.chromium.org/1377273003/diff/40001/media/cast/net/rtp/rtp_...
File media/cast/net/rtp/rtp_defines.h (right):

https://codereview.chromium.org/1377273003/diff/40001/media/cast/net/rtp/rtp_...
media/cast/net/rtp/rtp_defines.h:15: static const uint16 kCastHeaderLength = 7;
On 2015/10/02 19:13:45, miu wrote:
> If any of these constants are only used in one .cc file, it would generally be
> better to remove them from this header file.  This would help prevent
> duplication of data across compilation outputs.

These constants do get used on multiple .cc files

[miu, 2015-10-06 22:01:05]

still lgtm

[Irfan, 2015-10-07 16:02:31]

The CQ bit was checked by isheriff@chromium.org

[Irfan, 2015-10-07 16:02:31]

The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org
Link to the patchset: https://codereview.chromium.org/1377273003/#ps60001
(title: "rebased")

[commit-bot: I haz the power, 2015-10-07 16:09:12]

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

[commit-bot: I haz the power, 2015-10-07 16:16:57]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-10-07 16:17:41]

Patchset 4 (id:??) landed as
https://crrev.com/64c95062775d2779d6a588942f3b9ac24f5ccb9b
Cr-Commit-Position: refs/heads/master@{#352853}