Add support for buffer level monitoring. Depends on this Blink change: https://codereview.chromium.org/1171553002/ Not to be ...
5 years, 7 months ago
(2015-06-05 19:35:20 UTC)
#2
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.
On 2015/06/05 19:35:20, bemasc2 wrote: > Add support for buffer level monitoring. > > Depends ...
5 years, 7 months ago
(2015-06-10 14:25:46 UTC)
#3
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.
On 2015/06/10 14:25:46, bemasc2 wrote: > On 2015/06/05 19:35:20, bemasc2 wrote: > > Add support ...
5 years, 7 months ago
(2015-06-10 18:04:12 UTC)
#4
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.
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 ...
5 years, 7 months ago
(2015-06-10 19:46:15 UTC)
#7
5 years, 7 months ago
(2015-06-10 20:06:56 UTC)
#8
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.
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 ...
5 years, 7 months ago
(2015-06-10 20:24:16 UTC)
#9
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.
5 years, 7 months ago
(2015-06-10 21:11:06 UTC)
#11
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.
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 ...
5 years, 7 months ago
(2015-06-10 21:27:09 UTC)
#12
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.
5 years, 7 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.
Issue 54679004: Initial implementation of support for onbufferedamountlow
Created 5 years, 7 months ago by bemasc1
Modified 5 years, 7 months ago
Reviewers: pthatcher, lally_w, jiayang, bemasc2
Base URL: https://chromium.googlesource.com/external/webrtc/trunk/talk.git@master
Comments: 4