Remove unnecessary RTPHeaderParser, following https://codereview.webrtc.org/2659563002/

Remove unnecessary RTPHeaderParser, following https://codereview.webrtc.org/2659563002/

BUG=webrtc:6847
Review-Url: https://codereview.webrtc.org/2663063008
Cr-Commit-Position: refs/heads/master@{#16457}
Committed: https://chromium.googlesource.com/external/webrtc/+/6b34124a6d4733cf2d95986b4920bccec8b39397

[the sun, 2017-02-01 13:21:56]

solenberg@webrtc.org changed reviewers:
+ nisse@webrtc.org

[nisse-webrtc, 2017-02-01 14:50:18]

https://codereview.webrtc.org/2663063008/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.cc (left):

https://codereview.webrtc.org/2663063008/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:332: 
I would have expected Call to pass on its parsed headers down the pipeline. Are
the headers not needed at all, or are they parsed again further down?

https://codereview.webrtc.org/2663063008/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2663063008/diff/1/webrtc/call/call.cc#newcode210
webrtc/call/call.cc:210: bool transport_cc = false;
Does style guide suggest always providing default values?

This field ought to always be initialized by the constructor. Use of the default
constructor is an error (but unfortunately it is required because std::map
assignment seems to use the default constructor + assignment operator. If you
know any way to delete the default constructor, that would be nice).

I intentionally left it uninitialized here, since that lets tools like valgrind
and msan detect any improper use of the uninitialized value. Adding a default
value here defeats those tools.

[the sun, 2017-02-01 15:12:07]

https://codereview.webrtc.org/2663063008/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.cc (left):

https://codereview.webrtc.org/2663063008/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:332: 
On 2017/02/01 14:50:17, nisse-webrtc wrote:
> I would have expected Call to pass on its parsed headers down the pipeline.
Are
> the headers not needed at all, or are they parsed again further down?

They are parsed again inside voe::Channel::ReceivedRTPPacket()

https://codereview.webrtc.org/2663063008/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2663063008/diff/1/webrtc/call/call.cc#newcode210
webrtc/call/call.cc:210: bool transport_cc = false;
On 2017/02/01 14:50:17, nisse-webrtc wrote:
> Does style guide suggest always providing default values?

I don't know. It's a habit that has saved me many times though.

> This field ought to always be initialized by the constructor. Use of the
default
> constructor is an error (but unfortunately it is required because std::map
> assignment seems to use the default constructor + assignment operator. If you
> know any way to delete the default constructor, that would be nice).
> 
> I intentionally left it uninitialized here, since that lets tools like
valgrind
> and msan detect any improper use of the uninitialized value. Adding a default
> value here defeats those tools. 

That's a good argument, but relies on test coverage and infrastructure to hold.
By always initializing members we will get consistent behavior in the case where
the data type is used in the wrong way, thus making bugs easier to find.

IMO, the only place where it would be a good idea to not initialize members
would be in inner loops.

[nisse-webrtc, 2017-02-02 08:10:11]

LGTM.

https://codereview.webrtc.org/2663063008/diff/1/webrtc/audio/audio_receive_st...
File webrtc/audio/audio_receive_stream.cc (left):

https://codereview.webrtc.org/2663063008/diff/1/webrtc/audio/audio_receive_st...
webrtc/audio/audio_receive_stream.cc:332: 
On 2017/02/01 15:12:06, the sun wrote:
> On 2017/02/01 14:50:17, nisse-webrtc wrote:
> > I would have expected Call to pass on its parsed headers down the pipeline.
> Are
> > the headers not needed at all, or are they parsed again further down?
> 
> They are parsed again inside voe::Channel::ReceivedRTPPacket()

I see. An opportunity for later improvement...

https://codereview.webrtc.org/2663063008/diff/1/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2663063008/diff/1/webrtc/call/call.cc#newcode210
webrtc/call/call.cc:210: bool transport_cc = false;
On 2017/02/01 15:12:06, the sun wrote:
> On 2017/02/01 14:50:17, nisse-webrtc wrote:
> > Does style guide suggest always providing default values?
> 
> I don't know. It's a habit that has saved me many times though.
> 
> > This field ought to always be initialized by the constructor. Use of the
> default
> > constructor is an error (but unfortunately it is required because std::map
> > assignment seems to use the default constructor + assignment operator. If
you
> > know any way to delete the default constructor, that would be nice).
> > 
> > I intentionally left it uninitialized here, since that lets tools like
> valgrind
> > and msan detect any improper use of the uninitialized value. Adding a
default
> > value here defeats those tools. 
> 
> That's a good argument, but relies on test coverage and infrastructure to
hold.
> By always initializing members we will get consistent behavior in the case
where
> the data type is used in the wrong way, thus making bugs easier to find.
> 
> IMO, the only place where it would be a good idea to not initialize members
> would be in inner loops.

Ok.

[the sun, 2017-02-06 20:50:50]

The CQ bit was checked by solenberg@webrtc.org

[the sun, 2017-02-06 20:50:50]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2663063008/#ps40001 (title:
"remove unrelated change")

[commit-bot: I haz the power, 2017-02-06 20:51:01]

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, 2017-02-06 21:39:39]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486414250336740,
"parent_rev": "f748ca47b0e275b73da046734be522cbcbc6864b", "commit_rev":
"6b34124a6d4733cf2d95986b4920bccec8b39397"}

[commit-bot: I haz the power, 2017-02-06 21:39:42]

Description was changed from

==========
Remove unnecessary RTPHeaderParser, following
https://codereview.webrtc.org/2659563002/

BUG=webrtc:6847
==========

to

==========
Remove unnecessary RTPHeaderParser, following
https://codereview.webrtc.org/2659563002/

BUG=webrtc:6847

Review-Url: https://codereview.webrtc.org/2663063008
Cr-Commit-Position: refs/heads/master@{#16457}
Committed:
https://chromium.googlesource.com/external/webrtc/+/6b34124a6d4733cf2d95986b4...
==========

[commit-bot: I haz the power, 2017-02-06 21:39:43]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/6b34124a6d4733cf2d95986b4...