Change ChunkDemuxerStream/SourceBufferStream memory limit to size_t type

Change ChunkDemuxerStream/SourceBufferStream memory limit to size_t type

Also moved set_memory_limit methods out of class definition
and renamed to CDS/SBS::SetMemoryLimit (for consistency with
ChunkDemuxer::SetMemoryLimits method).

BUG=498482
Committed: https://crrev.com/bc5335d01161852835a6952236e744fb26edf1aa
Cr-Commit-Position: refs/heads/master@{#339998}

[servolk, 2015-07-15 23:43:01]

servolk@chromium.org changed reviewers:
+ ddorwin@chromium.org, wolenetz@chromium.org

[servolk, 2015-07-16 00:13:22]

PTAL, originally this cleanup (changing memory_limit_ type from signed int to
size_t) was part of https://codereview.chromium.org/1171263004/, which was
rejected, but I think we could still do this to make our code a little cleaner.

[servolk, 2015-07-17 17:22:19]

On 2015/07/16 00:13:22, servolk wrote:
> PTAL, originally this cleanup (changing memory_limit_ type from signed int to
> size_t) was part of https://codereview.chromium.org/1171263004/, which was
> rejected, but I think we could still do this to make our code a little
cleaner.

ping

[wolenetz, 2015-07-17 20:23:53]

My apologies for delay on this and your other reviews. I'm catching up still.
Thanks for reviving this portion of the previous CL. Some comments:

https://codereview.chromium.org/1235403002/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1235403002/diff/1/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:516: void
SourceState::SetMemoryLimits(DemuxerStream::Type type, int memory_limit) {
ISTM this method's decl and defn needs similar change to size_t. wdyt?

https://codereview.chromium.org/1235403002/diff/1/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:1018: void
ChunkDemuxerStream::SetMemoryLimit(size_t memory_limit) {
nit: try to retain ordering of methods in impl as they are in decl. move to just
after text_track_config()?

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
File media/filters/source_buffer_platform.h (right):

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
media/filters/source_buffer_platform.h:8: #include "base/basictypes.h"
nit: Hmm. For style correctness, it looks like both chunk_demuxer.h and
source_buffer_stream.h need this #include line, too.

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:611: ranges_size +=
(*itr)->size_in_bytes();
SourceBufferRange::size_in_bytes() returns int. Conceivably, it could
malfunction and return a negative value and underflow ranges_size, here. Change
it to calculate and return using size_t?

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:640: int
SourceBufferStream::FreeBuffersAfterLastAppended(int total_bytes_to_free) {
ditto: size_t (param, retval, and internals)

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:699: int
SourceBufferStream::FreeBuffers(int total_bytes_to_free,
ditto

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:41: void SetMemoryLimit(int
buffers_of_data) {
nit: param could be negative leading to badness. change to size_t param?

[servolk, 2015-07-17 21:34:07]

https://codereview.chromium.org/1235403002/diff/1/media/filters/chunk_demuxer.cc
File media/filters/chunk_demuxer.cc (right):

https://codereview.chromium.org/1235403002/diff/1/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:516: void
SourceState::SetMemoryLimits(DemuxerStream::Type type, int memory_limit) {
On 2015/07/17 20:23:53, wolenetz_OoO_July_17_PM_PDT wrote:
> ISTM this method's decl and defn needs similar change to size_t. wdyt?

Done.

https://codereview.chromium.org/1235403002/diff/1/media/filters/chunk_demuxer...
media/filters/chunk_demuxer.cc:1018: void
ChunkDemuxerStream::SetMemoryLimit(size_t memory_limit) {
On 2015/07/17 20:23:53, wolenetz_OoO_July_17_PM_PDT wrote:
> nit: try to retain ordering of methods in impl as they are in decl. move to
just
> after text_track_config()?

Done.

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
File media/filters/source_buffer_platform.h (right):

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
media/filters/source_buffer_platform.h:8: #include "base/basictypes.h"
On 2015/07/17 20:23:53, wolenetz_OoO_July_17_PM_PDT wrote:
> nit: Hmm. For style correctness, it looks like both chunk_demuxer.h and
> source_buffer_stream.h need this #include line, too.

Done.

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:611: ranges_size +=
(*itr)->size_in_bytes();
On 2015/07/17 20:23:53, wolenetz_OoO_July_17_PM_PDT wrote:
> SourceBufferRange::size_in_bytes() returns int. Conceivably, it could
> malfunction and return a negative value and underflow ranges_size, here.
Change
> it to calculate and return using size_t?

Done.

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:640: int
SourceBufferStream::FreeBuffersAfterLastAppended(int total_bytes_to_free) {
On 2015/07/17 20:23:53, wolenetz_OoO_July_17_PM_PDT wrote:
> ditto: size_t (param, retval, and internals)

Done.

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream.cc:699: int
SourceBufferStream::FreeBuffers(int total_bytes_to_free,
On 2015/07/17 20:23:53, wolenetz_OoO_July_17_PM_PDT wrote:
> ditto

Done.

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
File media/filters/source_buffer_stream_unittest.cc (right):

https://codereview.chromium.org/1235403002/diff/1/media/filters/source_buffer...
media/filters/source_buffer_stream_unittest.cc:41: void SetMemoryLimit(int
buffers_of_data) {
On 2015/07/17 20:23:53, wolenetz_OoO_July_17_PM_PDT wrote:
> nit: param could be negative leading to badness. change to size_t param?

Done.

[wolenetz, 2015-07-20 19:29:43]

lgtm % a few small things:

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
File media/filters/source_buffer_range.cc (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_range.cc:59: size_in_bytes_ += (*itr)->data_size();
nit: ISTM the DCHECK would be good here, too.

Fixing StreamParserBuffer::data_size() to be a size_t would be a next step,
though I'm fine if that's a separate CL since StreamParserBuffer is involved in
many many places in our pipeline.

Please add a TODO in @ DecoderBuffer::data_size() and side_data_size() to make
them size_t and rework any negative value signalling (if any) to some other
mechanism. IIUC, per the comments in decoder_buffer.h, these values shouldn't
ever be negative, but we'd need to verify that contract has been adhered to, and
also make sure users of DB::*_data_size() work with size_t.

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:692: size_t bytes_to_free =
total_bytes_to_free - bytes_freed;
This new variable is unused. Remove this line?

[ddorwin, 2015-07-20 21:23:57]

ddorwin@chromium.org changed required reviewers:
+ wolenetz@chromium.org

[ddorwin, 2015-07-20 21:23:57]

I just skimmed the CL.

https://codereview.chromium.org/1235403002/diff/40001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/chunk_dem...
media/filters/chunk_demuxer.h:98: void SetMemoryLimit(size_t memory_limit);
Set*Stream*MemoryLimit?

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
File media/filters/source_buffer_range.cc (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_range.cc:379: DCHECK_GE((*itr)->data_size(), 0);
Should BufferQueue also be fixed in a separate CL?

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_range.cc:380: size_t itr_data_size =
static_cast<size_t>((*itr)->data_size());
Does this value always come from internal code or do we need to use checked_cast
for security?

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_stream.h:146: void SetMemoryLimit(size_t
memory_limit);
If all this does is set a member variable, then the previous name was correct:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

[servolk, 2015-07-22 19:02:12]

https://codereview.chromium.org/1235403002/diff/40001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/chunk_dem...
media/filters/chunk_demuxer.h:98: void SetMemoryLimit(size_t memory_limit);
On 2015/07/20 21:23:57, ddorwin wrote:
> Set*Stream*MemoryLimit?

I think it's pretty obvious from the context where this is called, that this is
for a stream. What's the point of repeating that?

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
File media/filters/source_buffer_range.cc (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_range.cc:59: size_in_bytes_ += (*itr)->data_size();
On 2015/07/20 19:29:42, wolenetz wrote:
> nit: ISTM the DCHECK would be good here, too.
> 
> Fixing StreamParserBuffer::data_size() to be a size_t would be a next step,
> though I'm fine if that's a separate CL since StreamParserBuffer is involved
in
> many many places in our pipeline.
> 
> Please add a TODO in @ DecoderBuffer::data_size() and side_data_size() to make
> them size_t and rework any negative value signalling (if any) to some other
> mechanism. IIUC, per the comments in decoder_buffer.h, these values shouldn't
> ever be negative, but we'd need to verify that contract has been adhered to,
and
> also make sure users of DB::*_data_size() work with size_t.

Yes, DecoderBuffer::Initialize already has CHECK_GE(size_, 0), so I think it
should be safe to change DecoderBuffer::size_ and
StreamParserBuffer::data_size() type to size_t as well. But I agree, it's
probably better done as a separate CL, as it would require updating a lot more
places. I've added a TODO.

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_range.cc:379: DCHECK_GE((*itr)->data_size(), 0);
On 2015/07/20 21:23:57, ddorwin wrote:
> Should BufferQueue also be fixed in a separate CL?

BufferQueue is just a typedef for a collection of StreamParserBuffers. As
wolenetz@ noted above what needs to be fixed is /DecoderBuffer. I've added a
TODO for that.

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_range.cc:380: size_t itr_data_size =
static_cast<size_t>((*itr)->data_size());
On 2015/07/20 21:23:57, ddorwin wrote:
> Does this value always come from internal code or do we need to use
checked_cast
> for security?

It comes from StreamParserBuffer::data_size.
I think it should be safe to do just static_cast here, given there's already
CHECK_GE(size_, 0) in DecoderBuffer::Initialize and DecoderBuffer::size_ is only
set in constructors.

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:692: size_t bytes_to_free =
total_bytes_to_free - bytes_freed;
On 2015/07/20 19:29:43, wolenetz wrote:
> This new variable is unused. Remove this line?

No, it's passed into GetRemovalGOP (see the next line).

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
File media/filters/source_buffer_stream.h (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_stream.h:146: void SetMemoryLimit(size_t
memory_limit);
On 2015/07/20 21:23:57, ddorwin wrote:
> If all this does is set a member variable, then the previous name was correct:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

Done.

[wolenetz, 2015-07-22 20:54:40]

still lgtm. Thanks!

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
File media/filters/source_buffer_stream.cc (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/source_bu...
media/filters/source_buffer_stream.cc:692: size_t bytes_to_free =
total_bytes_to_free - bytes_freed;
On 2015/07/22 19:02:11, servolk wrote:
> On 2015/07/20 19:29:43, wolenetz wrote:
> > This new variable is unused. Remove this line?
> 
> No, it's passed into GetRemovalGOP (see the next line).

Acknowledged.

[ddorwin, 2015-07-22 22:48:08]

https://codereview.chromium.org/1235403002/diff/40001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/chunk_dem...
media/filters/chunk_demuxer.h:98: void SetMemoryLimit(size_t memory_limit);
On 2015/07/22 19:02:11, servolk wrote:
> On 2015/07/20 21:23:57, ddorwin wrote:
> > Set*Stream*MemoryLimit?
> 
> I think it's pretty obvious from the context where this is called, that this
is
> for a stream. What's the point of repeating that?

A class/API should be self-documenting and standalone. A reader shouldn't need
to look for existing call sites (or the implementation) to figure out what it
means. Also, comments should generally be unnecessary because functions should
be self-documenting.

https://codereview.chromium.org/1235403002/diff/60001/media/base/decoder_buff...
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/1235403002/diff/60001/media/base/decoder_buff...
media/base/decoder_buffer.h:99: // TODO: data_size should return size_t instead
of int
TODO's require a name:
TODO(servolk):...

[servolk, 2015-07-22 23:04:54]

https://codereview.chromium.org/1235403002/diff/40001/media/filters/chunk_dem...
File media/filters/chunk_demuxer.h (right):

https://codereview.chromium.org/1235403002/diff/40001/media/filters/chunk_dem...
media/filters/chunk_demuxer.h:98: void SetMemoryLimit(size_t memory_limit);
On 2015/07/22 22:48:08, ddorwin wrote:
> On 2015/07/22 19:02:11, servolk wrote:
> > On 2015/07/20 21:23:57, ddorwin wrote:
> > > Set*Stream*MemoryLimit?
> > 
> > I think it's pretty obvious from the context where this is called, that this
> is
> > for a stream. What's the point of repeating that?
> 
> A class/API should be self-documenting and standalone. A reader shouldn't need
> to look for existing call sites (or the implementation) to figure out what it
> means. Also, comments should generally be unnecessary because functions should
> be self-documenting.

Done.

https://codereview.chromium.org/1235403002/diff/60001/media/base/decoder_buff...
File media/base/decoder_buffer.h (right):

https://codereview.chromium.org/1235403002/diff/60001/media/base/decoder_buff...
media/base/decoder_buffer.h:99: // TODO: data_size should return size_t instead
of int
On 2015/07/22 22:48:08, ddorwin wrote:
> TODO's require a name:
> TODO(servolk):...

Done.

[ddorwin, 2015-07-22 23:11:52]

The parts I looked at LG. Thanks.

[servolk, 2015-07-22 23:13:07]

The CQ bit was checked by servolk@chromium.org

[servolk, 2015-07-22 23:13:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
wolenetz@chromium.org
Link to the patchset: https://codereview.chromium.org/1235403002/#ps80001
(title: "A few more minor fixes")

[commit-bot: I haz the power, 2015-07-22 23:13:21]

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

[commit-bot: I haz the power, 2015-07-23 00:17:51]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-07-23 00:18:34]

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