Replace size_t with int in a few media classes.

Replace size_t with int in a few media classes.

This addressed TODOs in DataSource::Read() and AudioRendererAlgorithmBase, which led to converting Buffer and SeekableBuffer as well.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=129140

[scherkus (not reviewing), 2012-03-26 22:35:24]

fischman: based on size_t vs int discussions in STO
vrk: FYI as I fixed your TODO

[Ami GONE FROM CHROMIUM, 2012-03-26 23:01:20]

LGTM % q.

http://codereview.chromium.org/9854031/diff/11002/media/base/data_source.h
File media/base/data_source.h (right):

http://codereview.chromium.org/9854031/diff/11002/media/base/data_source.h#ne...
media/base/data_source.h:43: virtual void Read(int64 position, int size, uint8*
data,
Shouldn't |position| be int, too?

Generally my only worry about this CL is overflowing 32-bit signed int.  The
only way I can think of that *might* do that is playing a large local file and
getting it all to try to squeeze into a Buffer.  Have you verified that this
doesn't break local >2G files?

[scherkus (not reviewing), 2012-03-26 23:19:39]

https://chromiumcodereview.appspot.com/9854031/diff/11002/media/base/data_sou...
File media/base/data_source.h (right):

https://chromiumcodereview.appspot.com/9854031/diff/11002/media/base/data_sou...
media/base/data_source.h:43: virtual void Read(int64 position, int size, uint8*
data,
On 2012/03/26 23:01:20, Ami Fischman wrote:
> Shouldn't |position| be int, too?

We do <32-bit reads on >32-bit resources hence the int64 position, but int size.

> Generally my only worry about this CL is overflowing 32-bit signed int.  The
> only way I can think of that *might* do that is playing a large local file and
> getting it all to try to squeeze into a Buffer.  Have you verified that this
> doesn't break local >2G files?

Testing variety of >2G files and all is good. In general our reads are on the
order of ~32kb (FFmpeg's default bitstream parser read size).

[xhwang, 2012-03-26 23:21:08]

Does this mean we prefer to using int than size_t for general sizes and even
byte counts in media code?

On 2012/03/26 23:19:39, scherkus wrote:
>
https://chromiumcodereview.appspot.com/9854031/diff/11002/media/base/data_sou...
> File media/base/data_source.h (right):
> 
>
https://chromiumcodereview.appspot.com/9854031/diff/11002/media/base/data_sou...
> media/base/data_source.h:43: virtual void Read(int64 position, int size,
uint8*
> data,
> On 2012/03/26 23:01:20, Ami Fischman wrote:
> > Shouldn't |position| be int, too?
> 
> We do <32-bit reads on >32-bit resources hence the int64 position, but int
size.
> 
> > Generally my only worry about this CL is overflowing 32-bit signed int.  The
> > only way I can think of that *might* do that is playing a large local file
and
> > getting it all to try to squeeze into a Buffer.  Have you verified that this
> > doesn't break local >2G files?
> 
> Testing variety of >2G files and all is good. In general our reads are on the
> order of ~32kb (FFmpeg's default bitstream parser read size).

[scherkus (not reviewing), 2012-03-26 23:27:39]

On 2012/03/26 23:21:08, xhwang wrote:
> Does this mean we prefer to using int than size_t for general sizes and even
> byte counts in media code?

Yes.

The only time we should be using unsigned integers is when dealing with APIs
that return unsigned integers (see: STL and some parts of FFmpeg) but the code
we write should prefer int over size_t.

> On 2012/03/26 23:19:39, scherkus wrote:
> >
>
https://chromiumcodereview.appspot.com/9854031/diff/11002/media/base/data_sou...
> > File media/base/data_source.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/9854031/diff/11002/media/base/data_sou...
> > media/base/data_source.h:43: virtual void Read(int64 position, int size,
> uint8*
> > data,
> > On 2012/03/26 23:01:20, Ami Fischman wrote:
> > > Shouldn't |position| be int, too?
> > 
> > We do <32-bit reads on >32-bit resources hence the int64 position, but int
> size.
> > 
> > > Generally my only worry about this CL is overflowing 32-bit signed int. 
The
> > > only way I can think of that *might* do that is playing a large local file
> and
> > > getting it all to try to squeeze into a Buffer.  Have you verified that
this
> > > doesn't break local >2G files?
> > 
> > Testing variety of >2G files and all is good. In general our reads are on
the
> > order of ~32kb (FFmpeg's default bitstream parser read size).

[Ami GONE FROM CHROMIUM, 2012-03-26 23:37:00]

>
> The only time we should be using unsigned integers is when dealing with
> APIs
> that return unsigned integers
>

...and when implementing some forms of bit fields in an integral type.

[commit-bot: I haz the power, 2012-03-27 00:12:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/9854031/6061

[commit-bot: I haz the power, 2012-03-27 00:44:55]

Try job failure for 9854031-6061 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-03-27 01:03:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scherkus@chromium.org/9854031/9049

[commit-bot: I haz the power, 2012-03-27 03:46:23]

Try job failure for 9854031-9049 (retry) on win_rel for step "ui_tests".
It's a second try, previously, steps "ui_tests, browser_tests, unit_tests"
failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[Peter Kasting, 2012-03-27 19:02:28]

I asked Andrew over IM to revert this patch if possible.

The rest of Chrome not only allows but explicitly encourages size_t where
applicable; see the "types" section of
https://sites.google.com/a/chromium.org/dev/developers/coding-style?pli=1 .  I
think it unwise to make media differ, especially if it used to not differ, and
also if the motivation for changing was based on a different interpretation of
the style rules than the rest of Chrome has.

(For reference, we have talked with style reviewers in the past and clarified
that the intent of the Google style guide was never to discourage size_t
either.)

[Ami GONE FROM CHROMIUM, 2012-03-27 20:26:08]

> The rest of Chrome not only allows but explicitly encourages size_t where
> applicable

The "applicable" here refers to the style guide's mention of "counts of objects
or bytes in memory".  Presumably the rationale is that such quantities are
naturally bound by the arch's limits, so size_t is guaranteed to be sufficient
(avoids overflow).

The cost of this guaranteed-not-overflowable approach is significantly increased
subtlety around subtraction (unsigned subtraction can result in wrap-around).
Buffer management in particular is prone to this sort of subtraction, so it
seems worthwhile to avoid the subtlety when possible.  In the code in question
we know the sizes we deal with are bound pretty small, and certainly fit in 31
bits, so the trade-off is worthwhile.

A different supporting point: the guide encourages use of size_t when that fits
with the surrounding eco-system (STL, syscalls, etc); this patch demonstrates
that s/size_t/int/g increases compatibility with the surrounding eco-system
involved here (static_cast<>'s are removed, not added).

I'd like to see this change stick.