WebRTC Code Reviews
Help | Chromium Project | Sign in
(71)

Issue 54679004: Initial implementation of support for onbufferedamountlow

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 8 months ago by bemasc1
Modified:
3 years, 8 months ago
CC:
webrtc-reviews_webrtc.org
Base URL:
https://chromium.googlesource.com/external/webrtc/trunk/talk.git@master
Target Ref:
refs/heads/master
Project:
webrtc
Visibility:
Public.

Description

Initial implementation of support for onbufferedamountlow BUG=https://code.google.com/p/chromium/issues/detail?id=496700

Patch Set 1 #

Total comments: 1

Patch Set 2 : Check for |observer_| being null during a call to DataChannel::Send #

Total comments: 3

Patch Set 3 : Switch from Decrease to Change, fix tests, and add Java bindings #

Unified diffs Side-by-side diffs Delta from patch set Stats (+59 lines, -2 lines) Patch
M app/webrtc/datachannel.cc View 1 2 3 chunks +12 lines, -1 line 0 comments Download
M app/webrtc/datachannel_unittest.cc View 1 2 4 chunks +26 lines, -1 line 0 comments Download
M app/webrtc/datachannelinterface.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M app/webrtc/java/jni/peerconnection_jni.cc View 1 2 3 chunks +10 lines, -0 lines 0 comments Download
M app/webrtc/java/src/org/webrtc/DataChannel.java View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M app/webrtc/java/testcommon/src/org/webrtc/PeerConnectionTest.java View 1 2 1 chunk +5 lines, -0 lines 0 comments Download
M app/webrtc/test/mockpeerconnectionobservers.h View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
Project "webrtc" does not have a commit queue.

Messages

Total messages: 13 (1 generated)
bemasc2
Add support for buffer level monitoring. Depends on this Blink change: https://codereview.chromium.org/1171553002/ Not to be ...
3 years, 8 months ago (2015-06-05 19:35:20 UTC) #2
bemasc2
On 2015/06/05 19:35:20, bemasc2 wrote: > Add support for buffer level monitoring. > > Depends ...
3 years, 8 months ago (2015-06-10 14:25:46 UTC) #3
lally_w
On 2015/06/10 14:25:46, bemasc2 wrote: > On 2015/06/05 19:35:20, bemasc2 wrote: > > Add support ...
3 years, 8 months ago (2015-06-10 18:04:12 UTC) #4
lally_w
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.cc#newcode490 app/webrtc/datachannel.cc:490: if (buffered_amount() < start_buffered_amount) { if (observer_ && buffered_amount() ...
3 years, 8 months ago (2015-06-10 18:04:24 UTC) #5
bemasc2
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.cc#newcode490 > ...
3 years, 8 months ago (2015-06-10 19:09:34 UTC) #6
pthatcher
https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachannel.cc File app/webrtc/datachannel.cc (right): https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachannel.cc#newcode491 app/webrtc/datachannel.cc:491: observer_->OnBufferedAmountDecrease(start_buffered_amount); Why not just make it OnBufferedAmountChanged and let ...
3 years, 8 months ago (2015-06-10 19:46:15 UTC) #7
bemasc2
On 2015/06/10 19:46:15, pthatcher wrote: > https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachannel.cc > File app/webrtc/datachannel.cc (right): > > https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachannel.cc#newcode491 > ...
3 years, 8 months ago (2015-06-10 20:06:56 UTC) #8
pthatcher
https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachannel.cc File app/webrtc/datachannel.cc (right): https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachannel.cc#newcode491 app/webrtc/datachannel.cc:491: observer_->OnBufferedAmountDecrease(start_buffered_amount); On 2015/06/10 19:46:15, pthatcher wrote: > Why not ...
3 years, 8 months ago (2015-06-10 20:24:16 UTC) #9
pthatcher
lgtm
3 years, 8 months ago (2015-06-10 20:24:41 UTC) #10
bemasc2
On 2015/06/10 20:24:16, pthatcher wrote: > https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachannel.cc > File app/webrtc/datachannel.cc (right): > > https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachannel.cc#newcode491 > ...
3 years, 8 months ago (2015-06-10 21:11:06 UTC) #11
pthatcher
https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachannel.cc File app/webrtc/datachannel.cc (right): https://webrtc-codereview.appspot.com/54679004/diff/20001/app/webrtc/datachannel.cc#newcode491 app/webrtc/datachannel.cc:491: observer_->OnBufferedAmountDecrease(start_buffered_amount); On 2015/06/10 20:24:16, pthatcher wrote: > On 2015/06/10 ...
3 years, 8 months ago (2015-06-10 21:27:09 UTC) #12
bemasc2
3 years, 8 months ago (2015-06-11 21:50:30 UTC) #13
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 245c2c2-tainted