Multibuffer reader implementation

Multibuffer reader implementation

Provides a simpler, byte-oriented api for getting data out of a multibuffer.
This CL also has the unit tests for the multibuffer itself. (Since those tests
uses the MultiBufferReader class.)

Depends on:
https://codereview.chromium.org/1165903002

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS1E/edit

BUG=514719, 557589
Committed: https://crrev.com/f8b2532f9f9b4a35f396602bb80dfc34ee102fe4
Cr-Commit-Position: refs/heads/master@{#360244}

Committed: https://crrev.com/a527d792237832e0040b60225fec2dde22df5023
Cr-Commit-Position: refs/heads/master@{#360451}

[hubbe, 2015-11-12 22:21:54]

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

[DaleCurtis, 2015-11-17 06:56:06]

Just some minor stuff. I like the control flow in this much better.

lgtm % nits.

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

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.cc:19: end_(end == -1LL ? (1LL <<
(multibuffer->block_size_shift() + 30)) : end),
Needs an explanation? Don't see anything in comments about -1 being allowed.

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.cc:100: cb_.Reset();
Why here?

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.cc:161: base::Bind(&MultiBufferReader::Call,
weak_factory_.GetWeakPtr(),
I think you can bind directly to the invoker in the callback instead of having
the indirect method.

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
File media/blink/multibuffer_reader.h (right):

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.h:60: int64_t TryRead(unsigned char* data,
int64_t len);
uint8_t*

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.h:66: int Wait(int64_t len, base::Closure cb);
const&

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.h:94: MultiBufferBlockId block(int64_t byte_pos)
const;
No hacker_style() for non-inline methods.

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
File media/blink/multibuffer_unittest.cc (right):

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_unittest.cc:73: blocks_until_deferred_ = rand() %
max_blocks_after_defer_;
rnd_.Rand() ?

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_unittest.cc:219: media::writers[rand() %
media::writers.size()]->Advance();
rnd_.Rand()?

[hubbe, 2015-11-17 23:40:18]

PTAL

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

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.cc:19: end_(end == -1LL ? (1LL <<
(multibuffer->block_size_shift() + 30)) : end),
On 2015/11/17 06:56:06, DaleCurtis wrote:
> Needs an explanation? Don't see anything in comments about -1 being allowed.

Added some comments.

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.cc:100: cb_.Reset();
On 2015/11/17 06:56:06, DaleCurtis wrote:
> Why here?

Where do you suggest I put it?

Or do you wonder why it is here at all?
It's here because of this:

>  // If there is a pending Wait(), it will be cancelled.           
(multibuffer_reader.h, line 59)

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.cc:161: base::Bind(&MultiBufferReader::Call,
weak_factory_.GetWeakPtr(),
On 2015/11/17 06:56:06, DaleCurtis wrote:
> I think you can bind directly to the invoker in the callback instead of having
> the indirect method.

Comment added.

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
File media/blink/multibuffer_reader.h (right):

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.h:60: int64_t TryRead(unsigned char* data,
int64_t len);
On 2015/11/17 06:56:06, DaleCurtis wrote:
> uint8_t*

Done.

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.h:66: int Wait(int64_t len, base::Closure cb);
On 2015/11/17 06:56:06, DaleCurtis wrote:
> const&

Done.

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_reader.h:94: MultiBufferBlockId block(int64_t byte_pos)
const;
On 2015/11/17 06:56:06, DaleCurtis wrote:
> No hacker_style() for non-inline methods.

Now they are inline.... :)

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
File media/blink/multibuffer_unittest.cc (right):

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_unittest.cc:73: blocks_until_deferred_ = rand() %
max_blocks_after_defer_;
On 2015/11/17 06:56:06, DaleCurtis wrote:
> rnd_.Rand() ?

Done.

https://codereview.chromium.org/1420883004/diff/60001/media/blink/multibuffer...
media/blink/multibuffer_unittest.cc:219: media::writers[rand() %
media::writers.size()]->Advance();
On 2015/11/17 06:56:06, DaleCurtis wrote:
> rnd_.Rand()? 

Done.

[DaleCurtis, 2015-11-18 00:46:34]

lgtm, I still think there's an easier way to indirect, but can't figure it out
off the top of my head, so just adding the comment is fine.

[hubbe, 2015-11-18 00:47:55]

On 2015/11/18 00:46:34, DaleCurtis wrote:
> lgtm, I still think there's an easier way to indirect, but can't figure it out
> off the top of my head, so just adding the comment is fine.

I'm happy to clean it up later if you think of it. :)
(Checking in now...)

[hubbe, 2015-11-18 00:48:00]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2015-11-18 00:49:31]

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

[commit-bot: I haz the power, 2015-11-18 01:08:17]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-18 01:08:58]

Patchset 5 (id:??) landed as
https://crrev.com/f8b2532f9f9b4a35f396602bb80dfc34ee102fe4
Cr-Commit-Position: refs/heads/master@{#360244}

[msramek, 2015-11-18 09:27:49]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/1456903002/ by msramek@chromium.org.

The reason for reverting is: This CL probably broke media_blink_unittests:
MultiBufferTest.RandomTest and MultiBufferTest.RandomTest_RangeSupported..

[hubbe, 2015-11-18 19:17:58]

Description was changed from

==========
Multibuffer reader implementation

Provides a simpler, byte-oriented api for getting data out of a multibuffer.
This CL also has the unit tests for the multibuffer itself. (Since those tests
uses the MultiBufferReader class.)

Depends on:
https://codereview.chromium.org/1165903002

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS...

BUG=514719

Committed: https://crrev.com/f8b2532f9f9b4a35f396602bb80dfc34ee102fe4
Cr-Commit-Position: refs/heads/master@{#360244}
==========

to

==========
Multibuffer reader implementation

Provides a simpler, byte-oriented api for getting data out of a multibuffer.
This CL also has the unit tests for the multibuffer itself. (Since those tests
uses the MultiBufferReader class.)

Depends on:
https://codereview.chromium.org/1165903002

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS...

BUG=514719

Committed: https://crrev.com/f8b2532f9f9b4a35f396602bb80dfc34ee102fe4
Cr-Commit-Position: refs/heads/master@{#360244}
==========

[hubbe, 2015-11-18 19:18:48]

Description was changed from

==========
Multibuffer reader implementation

Provides a simpler, byte-oriented api for getting data out of a multibuffer.
This CL also has the unit tests for the multibuffer itself. (Since those tests
uses the MultiBufferReader class.)

Depends on:
https://codereview.chromium.org/1165903002

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS...

BUG=514719

Committed: https://crrev.com/f8b2532f9f9b4a35f396602bb80dfc34ee102fe4
Cr-Commit-Position: refs/heads/master@{#360244}
==========

to

==========
Multibuffer reader implementation

Provides a simpler, byte-oriented api for getting data out of a multibuffer.
This CL also has the unit tests for the multibuffer itself. (Since those tests
uses the MultiBufferReader class.)

Depends on:
https://codereview.chromium.org/1165903002

Media cache design doc:
https://docs.google.com/document/d/15q6LTG0iDUe30QcoMtj4XNmKCa_7W_Q2uUIPFsJhS...

BUG=514719, 557589

Committed: https://crrev.com/f8b2532f9f9b4a35f396602bb80dfc34ee102fe4
Cr-Commit-Position: refs/heads/master@{#360244}
==========

[hubbe, 2015-11-18 19:26:23]

Dale, do you want to take a look at my changes while the trybots are chewing
away?

[DaleCurtis, 2015-11-18 19:29:24]

lgtm

[hubbe, 2015-11-18 21:56:26]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2015-11-18 21:57:35]

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

[commit-bot: I haz the power, 2015-11-19 00:12:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-11-19 00:13:14]

Patchset 6 (id:??) landed as
https://crrev.com/a527d792237832e0040b60225fec2dde22df5023
Cr-Commit-Position: refs/heads/master@{#360451}