Allow parsing empty RTCP TargetBitrate messages, but stop sending them.

Allow parsing empty RTCP TargetBitrate messages, but stop sending them.

Also, add ToString() convenience method to the target bitrate struct. Super useful when doing printf debugging :)

BUG=webrtc:7858
Review-Url: https://codereview.webrtc.org/2947633003
Cr-Commit-Position: refs/heads/master@{#18717}
Committed: https://chromium.googlesource.com/external/webrtc/+/d0fc37a8841ed70b5338ce80ac9c3e8ac139a310

[sprang_webrtc, 2017-06-20 09:11:54]

Description was changed from

==========
Allow parsing empty RTCP TargetBitrate messages, but stop sending them.

BUG=webrtc:7858
==========

to

==========
Allow parsing empty RTCP TargetBitrate messages, but stop sending them.

Also, add ToString() convenience method to the target bitrate struct. Super
useful when doing printf debugging :)

BUG=webrtc:7858
==========

[sprang_webrtc, 2017-06-20 09:15:25]

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

[danilchap, 2017-06-20 10:29:27]

lgtm

https://codereview.webrtc.org/2947633003/diff/1/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2947633003/diff/1/webrtc/common_types.h#newcode634
webrtc/common_types.h:634: std::string ToString() const;
may be also (or instead) add operator<< or PrintTo function to make this struct
readable in gtests

https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

https://codereview.webrtc.org/2947633003/diff/1/webrtc/video/payload_router_u...
File webrtc/video/payload_router_unittest.cc (right):

https://codereview.webrtc.org/2947633003/diff/1/webrtc/video/payload_router_u...
webrtc/video/payload_router_unittest.cc:194: BitrateAllocation layer0_bitrate;
is it same as bitrate? why do you need two variables?

https://codereview.webrtc.org/2947633003/diff/1/webrtc/video/payload_router_u...
webrtc/video/payload_router_unittest.cc:199: EXPECT_CALL(rtp_2,
SetVideoBitrateAllocation(_)).Times(0);
can you explain (with test name or comments, better both) why expectation are
like this?

[danilchap, 2017-06-20 10:32:07]

https://codereview.webrtc.org/2947633003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right):

https://codereview.webrtc.org/2947633003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:14: #include
"webrtc/base/logging.h"
may be remove this include

[sprang_webrtc, 2017-06-20 15:23:08]

The CQ bit was checked by sprang@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-20 15:23:12]

Dry run: CQ is trying da patch.

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

[sprang_webrtc, 2017-06-20 15:24:33]

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

[sprang_webrtc, 2017-06-20 15:24:34]

+stefan as owner for webrtc/common_types

https://codereview.webrtc.org/2947633003/diff/1/webrtc/common_types.h
File webrtc/common_types.h (right):

https://codereview.webrtc.org/2947633003/diff/1/webrtc/common_types.h#newcode634
webrtc/common_types.h:634: std::string ToString() const;
On 2017/06/20 10:29:26, danilchap wrote:
> may be also (or instead) add operator<< or PrintTo function to make this
struct
> readable in gtests
> 
>
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuid...

Done.

https://codereview.webrtc.org/2947633003/diff/1/webrtc/video/payload_router_u...
File webrtc/video/payload_router_unittest.cc (right):

https://codereview.webrtc.org/2947633003/diff/1/webrtc/video/payload_router_u...
webrtc/video/payload_router_unittest.cc:194: BitrateAllocation layer0_bitrate;
On 2017/06/20 10:29:27, danilchap wrote:
> is it same as bitrate? why do you need two variables?

I don't, I was just lazy (copy/paste from other test).
Fixed.

https://codereview.webrtc.org/2947633003/diff/1/webrtc/video/payload_router_u...
webrtc/video/payload_router_unittest.cc:199: EXPECT_CALL(rtp_2,
SetVideoBitrateAllocation(_)).Times(0);
On 2017/06/20 10:29:26, danilchap wrote:
> can you explain (with test name or comments, better both) why expectation are
> like this?

Done.

https://codereview.webrtc.org/2947633003/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc (right):

https://codereview.webrtc.org/2947633003/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_packet/target_bitrate.cc:14: #include
"webrtc/base/logging.h"
On 2017/06/20 10:32:07, danilchap wrote:
> may be remove this include

Done.

[commit-bot: I haz the power, 2017-06-20 16:08:50]

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

[commit-bot: I haz the power, 2017-06-20 16:08:51]

Dry run: This issue passed the CQ dry run.

[stefan-webrtc, 2017-06-22 11:10:38]

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

https://codereview.webrtc.org/2947633003/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:641: std::ostream& operator<<(std::ostream& os, const
BitrateAllocation& bitrate);
Where is this used? To me it seems like it should be in a test helper if we need
it only for testing?

[sprang_webrtc, 2017-06-22 11:28:29]

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

https://codereview.webrtc.org/2947633003/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:641: std::ostream& operator<<(std::ostream& os, const
BitrateAllocation& bitrate);
On 2017/06/22 11:10:38, stefan-webrtc wrote:
> Where is this used? To me it seems like it should be in a test helper if we
need
> it only for testing? 

It is not used, at the moment.
It is however something I use frequently when debugging, and I have grown tired
of constantly having to re-implement or patch this locally when that happens.
The operator<< is useful if you want to log it in a test, or just for getting an
immediate text representation inside a graphical debugger.
Where I exactly I've used it has varied. Sometime in the pacing module,
sometimes in the video coding module, sometimes in the rtp/rtcp module... so
having it in common types makes sense to me.

[stefan-webrtc, 2017-06-22 11:38:12]

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

https://codereview.webrtc.org/2947633003/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:641: std::ostream& operator<<(std::ostream& os, const
BitrateAllocation& bitrate);
On 2017/06/22 11:28:29, sprang_webrtc wrote:
> On 2017/06/22 11:10:38, stefan-webrtc wrote:
> > Where is this used? To me it seems like it should be in a test helper if we
> need
> > it only for testing? 
> 
> It is not used, at the moment.
> It is however something I use frequently when debugging, and I have grown
tired
> of constantly having to re-implement or patch this locally when that happens.
> The operator<< is useful if you want to log it in a test, or just for getting
an
> immediate text representation inside a graphical debugger.
> Where I exactly I've used it has varied. Sometime in the pacing module,
> sometimes in the video coding module, sometimes in the rtp/rtcp module... so
> having it in common types makes sense to me.

Ok, I'm mostly worried about adding usage of stringstream, which I remember that
tommi has been trying to get rid of from webrtc since it's very heavy. That's
why I'd prefer to only build this into tests...

[sprang_webrtc, 2017-06-22 12:06:57]

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

https://codereview.webrtc.org/2947633003/diff/40001/webrtc/common_types.h#new...
webrtc/common_types.h:641: std::ostream& operator<<(std::ostream& os, const
BitrateAllocation& bitrate);
On 2017/06/22 11:38:12, stefan-webrtc wrote:
> On 2017/06/22 11:28:29, sprang_webrtc wrote:
> > On 2017/06/22 11:10:38, stefan-webrtc wrote:
> > > Where is this used? To me it seems like it should be in a test helper if
we
> > need
> > > it only for testing? 
> > 
> > It is not used, at the moment.
> > It is however something I use frequently when debugging, and I have grown
> tired
> > of constantly having to re-implement or patch this locally when that
happens.
> > The operator<< is useful if you want to log it in a test, or just for
getting
> an
> > immediate text representation inside a graphical debugger.
> > Where I exactly I've used it has varied. Sometime in the pacing module,
> > sometimes in the video coding module, sometimes in the rtp/rtcp module... so
> > having it in common types makes sense to me.
> 
> Ok, I'm mostly worried about adding usage of stringstream, which I remember
that
> tommi has been trying to get rid of from webrtc since it's very heavy. That's
> why I'd prefer to only build this into tests...

Right.. I'll add a comment about about using this only in tests for now, perhaps
I can switch to something simpler after discussing with him?

[stefan-webrtc, 2017-06-22 12:09:42]

On 2017/06/22 12:06:57, sprang_webrtc wrote:
> https://codereview.webrtc.org/2947633003/diff/40001/webrtc/common_types.h
> File webrtc/common_types.h (right):
> 
>
https://codereview.webrtc.org/2947633003/diff/40001/webrtc/common_types.h#new...
> webrtc/common_types.h:641: std::ostream& operator<<(std::ostream& os, const
> BitrateAllocation& bitrate);
> On 2017/06/22 11:38:12, stefan-webrtc wrote:
> > On 2017/06/22 11:28:29, sprang_webrtc wrote:
> > > On 2017/06/22 11:10:38, stefan-webrtc wrote:
> > > > Where is this used? To me it seems like it should be in a test helper if
> we
> > > need
> > > > it only for testing? 
> > > 
> > > It is not used, at the moment.
> > > It is however something I use frequently when debugging, and I have grown
> > tired
> > > of constantly having to re-implement or patch this locally when that
> happens.
> > > The operator<< is useful if you want to log it in a test, or just for
> getting
> > an
> > > immediate text representation inside a graphical debugger.
> > > Where I exactly I've used it has varied. Sometime in the pacing module,
> > > sometimes in the video coding module, sometimes in the rtp/rtcp module...
so
> > > having it in common types makes sense to me.
> > 
> > Ok, I'm mostly worried about adding usage of stringstream, which I remember
> that
> > tommi has been trying to get rid of from webrtc since it's very heavy.
That's
> > why I'd prefer to only build this into tests...
> 
> Right.. I'll add a comment about about using this only in tests for now,
perhaps
> I can switch to something simpler after discussing with him?

Sounds good.

lgtm

[sprang_webrtc, 2017-06-22 12:11:56]

The CQ bit was checked by sprang@webrtc.org

[sprang_webrtc, 2017-06-22 12:11:57]

The patchset sent to the CQ was uploaded after l-g-t-m from danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2947633003/#ps60001 (title:
"Add comment about using ToString only in tests")

[commit-bot: I haz the power, 2017-06-22 12:12:04]

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-06-22 12:40:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498133516810290,
"parent_rev": "7583390d1a3a7c4e9a77da0d77250abac0c34d1d", "commit_rev":
"d0fc37a8841ed70b5338ce80ac9c3e8ac139a310"}

[commit-bot: I haz the power, 2017-06-22 12:40:29]

Description was changed from

==========
Allow parsing empty RTCP TargetBitrate messages, but stop sending them.

Also, add ToString() convenience method to the target bitrate struct. Super
useful when doing printf debugging :)

BUG=webrtc:7858
==========

to

==========
Allow parsing empty RTCP TargetBitrate messages, but stop sending them.

Also, add ToString() convenience method to the target bitrate struct. Super
useful when doing printf debugging :)

BUG=webrtc:7858

Review-Url: https://codereview.webrtc.org/2947633003
Cr-Commit-Position: refs/heads/master@{#18717}
Committed:
https://chromium.googlesource.com/external/webrtc/+/d0fc37a8841ed70b5338ce80a...
==========

[commit-bot: I haz the power, 2017-06-22 12:40:31]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/d0fc37a8841ed70b5338ce80a...