BufferedDataSource: don't reallocate buffer unnecessarily

BufferedDataSource: don't reallocate buffer unnecessarily

When buffer size is different than kInitialReadBufferSize then
intermediate buffer would be reallocated on each Read() operation
which is unnecessary.

Committed: https://crrev.com/360e2c0067dec6b1b2c25c654d3f9e4a7f257a34
Cr-Commit-Position: refs/heads/master@{#296469}

[mmal, 2014-09-23 20:10:28]

mmaliszkiewicz@opera.com changed reviewers:
+ vrk@chromium.org, xhwang@chromium.org

[mmal, 2014-09-23 20:10:44]

PTAL

[xhwang, 2014-09-23 20:56:22]

https://codereview.chromium.org/594733004/diff/1/media/blink/buffered_data_so...
File media/blink/buffered_data_source.cc (right):

https://codereview.chromium.org/594733004/diff/1/media/blink/buffered_data_so...
media/blink/buffered_data_source.cc:319: intermediate_read_buffer_size_ = size;
Thanks for fixing this! The fix looks good. But I wonder can we just use a
vector<uint8> for intermediate_read_buffer_ so that we can drop
intermediate_read_buffer_size_ altogether...

[mmal, 2014-09-24 13:59:55]

Patchset #2 (id:20001) has been deleted

[mmal, 2014-09-24 14:06:19]

On 2014/09/23 20:56:22, xhwang wrote:
>
https://codereview.chromium.org/594733004/diff/1/media/blink/buffered_data_so...
> File media/blink/buffered_data_source.cc (right):
> 
>
https://codereview.chromium.org/594733004/diff/1/media/blink/buffered_data_so...
> media/blink/buffered_data_source.cc:319: intermediate_read_buffer_size_ =
size;
> Thanks for fixing this! The fix looks good. But I wonder can we just use a
> vector<uint8> for intermediate_read_buffer_ so that we can drop
> intermediate_read_buffer_size_ altogether...

Done.

I wonder whether this change will have any impact in real-world buffer usage.

[xhwang, 2014-09-24 16:27:16]

lgtm % comments

Again, thanks for fixing this!

https://codereview.chromium.org/594733004/diff/40001/media/blink/buffered_dat...
File media/blink/buffered_data_source.cc (right):

https://codereview.chromium.org/594733004/diff/40001/media/blink/buffered_dat...
media/blink/buffered_data_source.cc:105:
intermediate_read_buffer_.resize(kInitialReadBufferSize);
You can do this in the initialization list by using vector's ctor:

explicit vector (size_type n, const value_type& val = value_type(),
                 const allocator_type& alloc = allocator_type());

https://codereview.chromium.org/594733004/diff/40001/media/blink/buffered_dat...
media/blink/buffered_data_source.cc:304: int size = 0;
You can change the type to size_t, then you don't need to static_cast on l.316.

https://codereview.chromium.org/594733004/diff/40001/media/blink/buffered_dat...
media/blink/buffered_data_source.cc:317: intermediate_read_buffer_.resize(size);
For the record: This has another benefit. If the new size is less than the
capacity of the vector, then we don't actually allocate a new buffer.

https://codereview.chromium.org/594733004/diff/40001/media/blink/buffered_dat...
media/blink/buffered_data_source.cc:317: intermediate_read_buffer_.resize(size);
So the buffer size can only grow and never shrinks... I am not super familiar
with how this is used. Not sure what's the typical size of this buffer, and
whether we could be holding too much memory...

https://codereview.chromium.org/594733004/diff/40001/media/blink/buffered_dat...
media/blink/buffered_data_source.cc:322: &intermediate_read_buffer_[0],
This is safe since the intermediate_read_buffer_ is never empty. But add a
DCHECK(!intermediate_read_buffer_.empty()) at l.319 to make this explicit?

[xhwang, 2014-09-24 16:39:28]

https://codereview.chromium.org/594733004/diff/40001/media/blink/buffered_dat...
File media/blink/buffered_data_source.cc (right):

https://codereview.chromium.org/594733004/diff/40001/media/blink/buffered_dat...
media/blink/buffered_data_source.cc:317: intermediate_read_buffer_.resize(size);
On 2014/09/24 16:27:16, xhwang wrote:
> So the buffer size can only grow and never shrinks... I am not super familiar
> with how this is used. Not sure what's the typical size of this buffer, and
> whether we could be holding too much memory... 

I did a quick test on Youtube and a test clip and it seems the read size is
relatively stable. So this shouldn't be a concern.

[mmal, 2014-09-24 17:08:31]

On 2014/09/24 16:39:28, xhwang wrote:
>
https://codereview.chromium.org/594733004/diff/40001/media/blink/buffered_dat...
> File media/blink/buffered_data_source.cc (right):
> 
>
https://codereview.chromium.org/594733004/diff/40001/media/blink/buffered_dat...
> media/blink/buffered_data_source.cc:317:
intermediate_read_buffer_.resize(size);
> On 2014/09/24 16:27:16, xhwang wrote:
> > So the buffer size can only grow and never shrinks... I am not super
familiar
> > with how this is used. Not sure what's the typical size of this buffer, and
> > whether we could be holding too much memory... 
> 
> I did a quick test on Youtube and a test clip and it seems the read size is
> relatively stable. So this shouldn't be a concern.

Looks like this class is used only for non-MSE media. Might be difficult to spot
on YT.
As I saw with different files buffer size seems to be stable. Microsoft Media
Foundation uses 64k buffer so "typical ffmpeg size" wasn't very convenient for
us (Opera) ;)

[mmal, 2014-09-24 17:21:40]

The CQ bit was checked by mmaliszkiewicz@opera.com

[commit-bot: I haz the power, 2014-09-24 17:22:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/594733004/60001

[commit-bot: I haz the power, 2014-09-24 18:23:12]

Committed patchset #3 (id:60001) as 61a54d95a8497de5bffae564cd4bc361305bc821

[commit-bot: I haz the power, 2014-09-24 18:24:57]

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