Fix progress reporting for multibuffers to report progress for every byte we receive.

Fix progress reporting for multibuffers to report progress for every byte we receive.

BUG=612549
Committed: https://crrev.com/1d3e7432173300d1f65be0ec988178b8aae80726
Cr-Commit-Position: refs/heads/master@{#393609}

[hubbe, 2016-05-12 23:24:15]

hubbe@chromium.org changed reviewers:
+ wolenetz@chromium.org

[wolenetz, 2016-05-12 23:41:17]

Initial comments.
Also, how frequently would this extra per-byte notification potentially occur?
What's the performance cost if this is relatively frequent?

https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer.h
File media/blink/multibuffer.h (right):

https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer.h#n...
media/blink/multibuffer.h:256: int64_t UncommittedBytesAt(const BlockId& block);
nit: document this public method please.

https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer_dat...
File media/blink/multibuffer_data_source_unittest.cc (right):

https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source_unittest.cc:980: ReceiveData(kDataSize / 2);
I'm unfamiliar with this code. Naively, it looks like we've simulated the
fetching of just kDataSize across these two ReceiveData() calls. Why then do we
appear to expect to have buffered more than just kDataSize?

[hubbe, 2016-05-13 00:05:15]

> Also, how frequently would this extra per-byte notification > potentially
occur?

It depends on the network stack, but it's likely to occur 5-20 times per second.

> What's the performance cost if this is relatively frequent?

Cost should be pretty low, the calls are not overly frequent. They do some logn
operations which are not exactly free, but I'd be surprised if they would show
up in any of our measurements.

https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer.h
File media/blink/multibuffer.h (right):

https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer.h#n...
media/blink/multibuffer.h:256: int64_t UncommittedBytesAt(const BlockId& block);
On 2016/05/12 23:41:17, wolenetz wrote:
> nit: document this public method please.

Done.

https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer_dat...
File media/blink/multibuffer_data_source_unittest.cc (right):

https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer_dat...
media/blink/multibuffer_data_source_unittest.cc:980: ReceiveData(kDataSize / 2);
On 2016/05/12 23:41:17, wolenetz wrote:
> I'm unfamiliar with this code. Naively, it looks like we've simulated the
> fetching of just kDataSize across these two ReceiveData() calls. Why then do
we
> appear to expect to have buffered more than just kDataSize?

InitializeWith206Response() also calls receivedata, so we've already received
one block.

[hubbe, 2016-05-13 17:20:32]

hubbe@chromium.org changed reviewers:
+ liberato@chromium.org

[liberato (no reviews please), 2016-05-13 17:45:55]

lgtm

https://codereview.chromium.org/1971373002/diff/20001/media/blink/multibuffer.h
File media/blink/multibuffer.h (right):

https://codereview.chromium.org/1971373002/diff/20001/media/blink/multibuffer...
media/blink/multibuffer.h:118: // return false even if AvailableBytes() returns
true.
s/true./greater than zero, if less than a full block is available./ (?)

https://codereview.chromium.org/1971373002/diff/20001/media/blink/multibuffer...
File media/blink/multibuffer_reader.cc (right):

https://codereview.chromium.org/1971373002/diff/20001/media/blink/multibuffer...
media/blink/multibuffer_reader.cc:191: // We may be destroyed, do not touch
|this|.
i think this comment is out of date, since it posts.

https://codereview.chromium.org/1971373002/diff/20001/media/blink/resource_mu...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1971373002/diff/20001/media/blink/resource_mu...
media/blink/resource_multibuffer_data_provider.cc:134: int64_t bytes = 0;
this would be easy to keep track of as |fifo_| is modified.  probably not worth
it, though.

[hubbe, 2016-05-13 17:51:38]

https://codereview.chromium.org/1971373002/diff/20001/media/blink/multibuffer.h
File media/blink/multibuffer.h (right):

https://codereview.chromium.org/1971373002/diff/20001/media/blink/multibuffer...
media/blink/multibuffer.h:118: // return false even if AvailableBytes() returns
true.
On 2016/05/13 17:45:54, liberato wrote:
> s/true./greater than zero, if less than a full block is available./ (?)

Done.

https://codereview.chromium.org/1971373002/diff/20001/media/blink/multibuffer...
File media/blink/multibuffer_reader.cc (right):

https://codereview.chromium.org/1971373002/diff/20001/media/blink/multibuffer...
media/blink/multibuffer_reader.cc:191: // We may be destroyed, do not touch
|this|.
On 2016/05/13 17:45:55, liberato wrote:
> i think this comment is out of date, since it posts.

Done.

https://codereview.chromium.org/1971373002/diff/20001/media/blink/resource_mu...
File media/blink/resource_multibuffer_data_provider.cc (right):

https://codereview.chromium.org/1971373002/diff/20001/media/blink/resource_mu...
media/blink/resource_multibuffer_data_provider.cc:134: int64_t bytes = 0;
On 2016/05/13 17:45:55, liberato wrote:
> this would be easy to keep track of as |fifo_| is modified.  probably not
worth
> it, though.

Definitely not worth it as there is never more than 2 entries in the fifo at
this point.

[hubbe, 2016-05-13 17:51:45]

The CQ bit was checked by hubbe@chromium.org

[hubbe, 2016-05-13 17:51:47]

The patchset sent to the CQ was uploaded after l-g-t-m from
liberato@chromium.org
Link to the patchset: https://codereview.chromium.org/1971373002/#ps40001
(title: "comments addressed")

[commit-bot: I haz the power, 2016-05-13 17:52:18]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1971373002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1971373002/40001

[commit-bot: I haz the power, 2016-05-13 19:42:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-13 19:44:33]

Description was changed from

==========
Fix progress reporting for multibuffers to report progress for every byte we
receive.
==========

to

==========
Fix progress reporting for multibuffers to report progress for every byte we
receive.

Committed: https://crrev.com/1d3e7432173300d1f65be0ec988178b8aae80726
Cr-Commit-Position: refs/heads/master@{#393609}
==========

[commit-bot: I haz the power, 2016-05-13 19:44:34]

Patchset 3 (id:??) landed as
https://crrev.com/1d3e7432173300d1f65be0ec988178b8aae80726
Cr-Commit-Position: refs/heads/master@{#393609}

[DaleCurtis, 2016-05-16 17:09:35]

dalecurtis@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[DaleCurtis, 2016-05-16 17:09:36]

Hmm, how frequent is this? If it's now become a high frequency thing, we should
rerun the power tests to see if this explains some of the power difference
between BDS.

[hubbe, 2016-05-16 17:17:32]

On 2016/05/16 17:09:36, DaleCurtis wrote:
> Hmm, how frequent is this? If it's now become a high frequency thing, we
should
> rerun the power tests to see if this explains some of the power difference
> between BDS.

It's not frequent enough to be a major power draw in itself, but it might cause
more re-draws of the progress bar and
related JS functions, which could explain the power difference. We should
definitely re-test.

[liberato (no reviews please), 2016-05-16 17:22:51]

On 2016/05/16 17:17:32, hubbe wrote:
> On 2016/05/16 17:09:36, DaleCurtis wrote:
> > Hmm, how frequent is this? If it's now become a high frequency thing, we
> should
> > rerun the power tests to see if this explains some of the power difference
> > between BDS.
> 
> It's not frequent enough to be a major power draw in itself, but it might
cause
> more re-draws of the progress bar and
> related JS functions, which could explain the power difference. We should
> definitely re-test.

i'll set it up tomorrow morning when back at my desk.  should have results by
the afternoon.

is it okay to suppress the progress callback for partial blocks as long as we're
still getting data?  as in, if it's happening 5-20x second, then we'd get a full
block anyway shortly.  it's just the last one that we might need to send
specially.

[hubbe, 2016-05-16 17:55:51]

On 2016/05/16 17:22:51, liberato wrote:
> On 2016/05/16 17:17:32, hubbe wrote:
> > On 2016/05/16 17:09:36, DaleCurtis wrote:
> > > Hmm, how frequent is this? If it's now become a high frequency thing, we
> > should
> > > rerun the power tests to see if this explains some of the power difference
> > > between BDS.
> > 
> > It's not frequent enough to be a major power draw in itself, but it might
> cause
> > more re-draws of the progress bar and
> > related JS functions, which could explain the power difference. We should
> > definitely re-test.
> 
> i'll set it up tomorrow morning when back at my desk.  should have results by
> the afternoon.
> 
> is it okay to suppress the progress callback for partial blocks as long as
we're
> still getting data?  as in, if it's happening 5-20x second, then we'd get a
full
> block anyway shortly.  it's just the last one that we might need to send
> specially.

What you're describing is *probably* ok, but it is not spec compliant.
The progress reporting is already limited by a timer in blink anyways, so there
is an
upper limit on how often the progress will be reported and re-drawn.

[DaleCurtis, 2016-05-16 18:12:01]

Lets figure out if this is hurting power or not and make the call then. If it
hurts power we'll revert this behavior and send some sort of intent to intervene
if we can't find a spec-compliant loophole.

[liberato (no reviews please), 2016-05-16 18:16:42]

On 2016/05/16 18:12:01, DaleCurtis wrote:
> Lets figure out if this is hurting power or not and make the call then. If it
> hurts power we'll revert this behavior and send some sort of intent to
intervene
> if we can't find a spec-compliant loophole.

i wasn't advocating doing it now; i expect that the current CL doesn't impact
power anyway.

-fl

[wolenetz, 2016-05-16 20:05:57]

On 2016/05/13 00:05:15, hubbe wrote:
> > Also, how frequently would this extra per-byte notification > potentially
> occur?
> 
> It depends on the network stack, but it's likely to occur 5-20 times per
second.
> 
> > What's the performance cost if this is relatively frequent?
> 
> Cost should be pretty low, the calls are not overly frequent. They do some
logn
> operations which are not exactly free, but I'd be surprised if they would show
> up in any of our measurements.
> 
> https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer.h
> File media/blink/multibuffer.h (right):
> 
>
https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer.h#n...
> media/blink/multibuffer.h:256: int64_t UncommittedBytesAt(const BlockId&
block);
> On 2016/05/12 23:41:17, wolenetz wrote:
> > nit: document this public method please.
> 
> Done.
> 
>
https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer_dat...
> File media/blink/multibuffer_data_source_unittest.cc (right):
> 
>
https://codereview.chromium.org/1971373002/diff/1/media/blink/multibuffer_dat...
> media/blink/multibuffer_data_source_unittest.cc:980: ReceiveData(kDataSize /
2);
> On 2016/05/12 23:41:17, wolenetz wrote:
> > I'm unfamiliar with this code. Naively, it looks like we've simulated the
> > fetching of just kDataSize across these two ReceiveData() calls. Why then do
> we
> > appear to expect to have buffered more than just kDataSize?
> 
> InitializeWith206Response() also calls receivedata, so we've already received
> one block.

I see. Thanks.

[hubbe, 2016-05-18 23:24:29]

Description was changed from

==========
Fix progress reporting for multibuffers to report progress for every byte we
receive.

Committed: https://crrev.com/1d3e7432173300d1f65be0ec988178b8aae80726
Cr-Commit-Position: refs/heads/master@{#393609}
==========

to

==========
Fix progress reporting for multibuffers to report progress for every byte we
receive.

BUG=612549

Committed: https://crrev.com/1d3e7432173300d1f65be0ec988178b8aae80726
Cr-Commit-Position: refs/heads/master@{#393609}
==========