Cleaned out candidateSet member from TMMBRHelp class

Cleaned out candidateSet member from TMMBRHelp class
leaving that class memberless.

BUG=webrtc:5565
Committed: https://crrev.com/2f69ce949855b8e245c712add3c66276dae6f314
Cr-Commit-Position: refs/heads/master@{#13776}

[danilchap, 2016-08-10 11:18:47]

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

[danilchap, 2016-08-10 11:18:48]

One more step clearing TMMBRHelper.

https://codereview.webrtc.org/2234783002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (left):

https://codereview.webrtc.org/2234783002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1259: // Get net bitrate from
bounding set depending on sent packet rate
comment say bounding set is used, but code used candidate set.
New code use bounding set, both because it make more sense and easier.
Result should be the same anyway: 1st element of the bounding set is a minimum
bitrate element from the candidates set (see Step 3 in FindBoundingSet function)

https://codereview.webrtc.org/2234783002/diff/1/webrtc/modules/rtp_rtcp/sourc...
File webrtc/modules/rtp_rtcp/source/tmmbr_help.cc (left):

https://codereview.webrtc.org/2234783002/diff/1/webrtc/modules/rtp_rtcp/sourc...
webrtc/modules/rtp_rtcp/source/tmmbr_help.cc:75:
RTC_DCHECK_EQ(_candidateSet.PacketOH(i), 0u);
this DCHECK removed because it validates input.
Nice code should ensure there are no candidates with 0 bitrate (aka Tmmbr) so
shouldn't run into this check.
Not so nice code (still there) doesn't validate input from the network, so it
might provide zero bitrate and non-zero packet overhead. There is a filter, but
not DCHECK to guard against that kind of data.

[philipel, 2016-08-11 11:57:12]

lgtm with two nits.

https://codereview.webrtc.org/2234783002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2234783002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1263:
kMinVideoBwManagementBitrateBps);
Remove kMinVideoBwManagementBitrateBps and just use
MIN_VIDEO_BW_MANAGEMENT_BITRATE * 1000 here instead.

https://codereview.webrtc.org/2234783002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/tmmbr_help.h (right):

https://codereview.webrtc.org/2234783002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/tmmbr_help.h:50: static uint64_t
CalcMinBitrateBps(
Change uin64_t to int.

[danilchap, 2016-08-11 13:08:35]

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

[danilchap, 2016-08-11 13:08:36]

Stefan, can it be a problem if bandwidth calculation using tmmbr would be
allowed to report bitrate < 30k?

https://codereview.webrtc.org/2234783002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2234783002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1263:
kMinVideoBwManagementBitrateBps);
On 2016/08/11 11:57:12, philipel wrote:
> Remove kMinVideoBwManagementBitrateBps and just use
> MIN_VIDEO_BW_MANAGEMENT_BITRATE * 1000 here instead.

On the 2nd thought I think it is better to remove forcing low-limit completely:
it is up to to BandwidthObserver to decide what to do with oversmall bandwidth
(current implementation actually has different, smaller value for minimum
bitrate)
Stefan, do you think it is better that way or should rtcp receiver still report
30000 bps when it calculate less with Tmmbr?

https://codereview.webrtc.org/2234783002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/tmmbr_help.h (right):

https://codereview.webrtc.org/2234783002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/tmmbr_help.h:50: static uint64_t
CalcMinBitrateBps(
On 2016/08/11 11:57:12, philipel wrote:
> Change uin64_t to int.

Why?
TMMBR/TMMBN packets designed so that they can store 64bit values for bitrate.
Since TmmbItem mirror that network structure, it use 64bit value for bitrate
too.
when calculatng minimum of uint64_t values it feels natural to return uint64_t
value.

[philipel, 2016-08-11 13:33:50]

https://codereview.webrtc.org/2234783002/diff/20001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/tmmbr_help.h (right):

https://codereview.webrtc.org/2234783002/diff/20001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/tmmbr_help.h:50: static uint64_t
CalcMinBitrateBps(
On 2016/08/11 13:08:36, danilchap wrote:
> On 2016/08/11 11:57:12, philipel wrote:
> > Change uin64_t to int.
> 
> Why?
> TMMBR/TMMBN packets designed so that they can store 64bit values for bitrate.
> Since TmmbItem mirror that network structure, it use 64bit value for bitrate
> too.
> when calculatng minimum of uint64_t values it feels natural to return uint64_t
> value.

Right, I only saw the change from 32 bit to 64 bit and assumed this was suppose
to be "just a number", but if that is the case then it makes sense.

[philipel, 2016-08-11 14:03:26]

https://codereview.webrtc.org/2234783002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2234783002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1263: if (bitrate_bps <=
std::numeric_limits<uint32_t>::max())
Is this suppose to be a form of input validation? Is there a reason that the
bandwidth is a 64 bit number if only 32 bit numbers are accepted as valid?

[danilchap, 2016-08-11 14:16:22]

https://codereview.webrtc.org/2234783002/diff/40001/webrtc/modules/rtp_rtcp/s...
File webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc (right):

https://codereview.webrtc.org/2234783002/diff/40001/webrtc/modules/rtp_rtcp/s...
webrtc/modules/rtp_rtcp/source/rtcp_receiver.cc:1263: if (bitrate_bps <=
std::numeric_limits<uint32_t>::max())
On 2016/08/11 14:03:26, philipel wrote:
> Is this suppose to be a form of input validation? Is there a reason that the
> bandwidth is a 64 bit number if only 32 bit numbers are accepted as valid?

Yes, it is a form of input validation.
TMMBR/TMMBN use 6 bits for bitrate exponenta, i.e. designed with support for
64bit bitrates.
TmmbItem::Parse was updated to support 64bit values too, pushing max bitrate
validation away from parsing.
Though current parsing in rtcp_utility validates bitrate is max 32bit.

[danilchap, 2016-08-16 09:06:49]

According to offline discussion it is ok.

> Stefan, can it be a problem if bandwidth calculation using tmmbr would be
> allowed to report bitrate < 30k?

[danilchap, 2016-08-16 10:16:00]

The CQ bit was checked by danilchap@webrtc.org

[danilchap, 2016-08-16 10:16:00]

The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2234783002/#ps60001 (title:
"remove now unused include")

[commit-bot: I haz the power, 2016-08-16 10:16: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, 2016-08-16 10:21:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-16 10:21:46]

Description was changed from

==========
Cleaned out candidateSet member from TMMBRHelp class
leaving that class memberless.

BUG=webrtc:5565
==========

to

==========
Cleaned out candidateSet member from TMMBRHelp class
leaving that class memberless.

BUG=webrtc:5565

Committed: https://crrev.com/2f69ce949855b8e245c712add3c66276dae6f314
Cr-Commit-Position: refs/heads/master@{#13776}
==========

[commit-bot: I haz the power, 2016-08-16 10:21:46]

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