Initial implementation of support for onbufferedamountlow

Initial implementation of support for onbufferedamountlow

BUG=https://code.google.com/p/chromium/issues/detail?id=496700

[bemasc2, 2015-06-05 19:35:19]

bemasc@webrtc.org changed reviewers:
+ bemasc@webrtc.org, jiayl@webrtc.org, lally@webrtc.org, pthatcher@webrtc.org

[bemasc2, 2015-06-05 19:35:20]

Add support for buffer level monitoring.

Depends on this Blink change:
https://codereview.chromium.org/1171553002/

Not to be committed until the corresponding Chromium change has been reviewed.

[bemasc2, 2015-06-10 14:25:46]

On 2015/06/05 19:35:20, bemasc2 wrote:
> Add support for buffer level monitoring.
> 
> Depends on this Blink change:
> https://codereview.chromium.org/1171553002/

The Blink change now has approval, so PTAL at this change.

[lally_w, 2015-06-10 18:04:12]

On 2015/06/10 14:25:46, bemasc2 wrote:
> On 2015/06/05 19:35:20, bemasc2 wrote:
> > Add support for buffer level monitoring.
> > 
> > Depends on this Blink change:
> > https://codereview.chromium.org/1171553002/
> 
> The Blink change now has approval, so PTAL at this change.

lgtm assuming you check for observer_ != NULL.

[lally_w, 2015-06-10 18:04:24]

https://webrtc-codereview.appspot.com/54679004/diff/1/app/webrtc/datachannel.cc
File app/webrtc/datachannel.cc (right):

https://webrtc-codereview.appspot.com/54679004/diff/1/app/webrtc/datachannel....
app/webrtc/datachannel.cc:490: if (buffered_amount() < start_buffered_amount) {
if (observer_ && buffered_amount() < start_buffered_amount)

[bemasc2, 2015-06-10 19:09:34]

On 2015/06/10 18:04:24, lally_w wrote:
>
https://webrtc-codereview.appspot.com/54679004/diff/1/app/webrtc/datachannel.cc
> File app/webrtc/datachannel.cc (right):
> 
>
https://webrtc-codereview.appspot.com/54679004/diff/1/app/webrtc/datachannel....
> app/webrtc/datachannel.cc:490: if (buffered_amount() < start_buffered_amount)
{
> if (observer_ && buffered_amount() < start_buffered_amount)

Done.

[pthatcher, 2015-06-10 19:46:15]

https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
File app/webrtc/datachannel.cc (right):

https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
app/webrtc/datachannel.cc:491:
observer_->OnBufferedAmountDecrease(start_buffered_amount);
Why not just make it OnBufferedAmountChanged and let the observer decide which
direction is important (up or down)?

[bemasc2, 2015-06-10 20:06:56]

On 2015/06/10 19:46:15, pthatcher wrote:
>
https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
> File app/webrtc/datachannel.cc (right):
> 
>
https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
> app/webrtc/datachannel.cc:491:
> observer_->OnBufferedAmountDecrease(start_buffered_amount);
> Why not just make it OnBufferedAmountChanged and let the observer decide which
> direction is important (up or down)?

My concern was efficiency.  Doing that would generate more than twice as many
events.  Currently, this change only emits one event every time a block of
messages are removed from the queue.  That design would also emit an event every
time a message was added to the queue.  Since it wouldn't simplify our current
code, or enable any functionality we intend to implement, I figured it would be
best to leave it out.

If you feel differently, I'm happy to add it in and plumb it through.

[pthatcher, 2015-06-10 20:24:16]

https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
File app/webrtc/datachannel.cc (right):

https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
app/webrtc/datachannel.cc:491:
observer_->OnBufferedAmountDecrease(start_buffered_amount);
On 2015/06/10 19:46:15, pthatcher wrote:
> Why not just make it OnBufferedAmountChanged and let the observer decide which
> direction is important (up or down)?

But it's not really an "event", it just a method call, and if the first part of
the method call is:

if (new_buffered_amount < old_buffered_amount) {
  ...
}

Then I don't think performance would matter.


On the other hand, I'm sympathetic to the argument that "we'll never need that"
because it's probably true.    I don't think it matters much either way.

[pthatcher, 2015-06-10 20:24:41]

lgtm

[bemasc2, 2015-06-10 21:11:06]

On 2015/06/10 20:24:16, pthatcher wrote:
>
https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
> File app/webrtc/datachannel.cc (right):
> 
>
https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
> app/webrtc/datachannel.cc:491:
> observer_->OnBufferedAmountDecrease(start_buffered_amount);
> On 2015/06/10 19:46:15, pthatcher wrote:
> > Why not just make it OnBufferedAmountChanged and let the observer decide
which
> > direction is important (up or down)?
> 
> But it's not really an "event", it just a method call, and if the first part
of
> the method call is:
> 
> if (new_buffered_amount < old_buffered_amount) {
>   ...
> }
> 
> Then I don't think performance would matter.

True.  By "event" what I meant was the PostTask call in
RtcDataChannelHandler::Observer::OnBufferedAmountDecrease.  We could move the
"<" check from here to there, so we post the same number of tasks, if you want.

> 
> On the other hand, I'm sympathetic to the argument that "we'll never need
that"
> because it's probably true.    I don't think it matters much either way.

[pthatcher, 2015-06-10 21:27:09]

https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
File app/webrtc/datachannel.cc (right):

https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
app/webrtc/datachannel.cc:491:
observer_->OnBufferedAmountDecrease(start_buffered_amount);
On 2015/06/10 20:24:16, pthatcher wrote:
> On 2015/06/10 19:46:15, pthatcher wrote:
> > Why not just make it OnBufferedAmountChanged and let the observer decide
which
> > direction is important (up or down)?
> 
> But it's not really an "event", it just a method call, and if the first part
of
> the method call is:
> 
> if (new_buffered_amount < old_buffered_amount) {
>   ...
> }
> 
> Then I don't think performance would matter.
> 
> 
> On the other hand, I'm sympathetic to the argument that "we'll never need
that"
> because it's probably true.    I don't think it matters much either way.
> 
> 

I'd prefer that slightly, but don't feel too strongly about it.

[bemasc2, 2015-06-11 21:50:30]

On 2015/06/10 21:27:09, pthatcher wrote:
>
https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
> File app/webrtc/datachannel.cc (right):
> 
>
https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachan...
> app/webrtc/datachannel.cc:491:
> observer_->OnBufferedAmountDecrease(start_buffered_amount);
> On 2015/06/10 20:24:16, pthatcher wrote:
> > On 2015/06/10 19:46:15, pthatcher wrote:
> > > Why not just make it OnBufferedAmountChanged and let the observer decide
> which
> > > direction is important (up or down)?
> > 
> > But it's not really an "event", it just a method call, and if the first part
> of
> > the method call is:
> > 
> > if (new_buffered_amount < old_buffered_amount) {
> >   ...
> > }
> > 
> > Then I don't think performance would matter.
> > 
> > 
> > On the other hand, I'm sympathetic to the argument that "we'll never need
> that"
> > because it's probably true.    I don't think it matters much either way.
> > 
> > 
> 
> I'd prefer that slightly, but don't feel too strongly about it.

OK, I've switched to that formulation.

I also realized that my tests were not actually running, and that I needed to
add Java bindings, so I've done that too.