Replace media::Limits struct with media::limits namespace and update documentation.

Replace media::Limits struct with media::limits namespace and update documentation.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113425

[Ami GONE FROM CHROMIUM, 2011-12-03 02:11:38]

I like what you're doing here.

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h
File media/base/limits.h (right):

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h#newcode17
media/base/limits.h:17: static const int kMaxDimension = (1 << 15) - 1;  //
32767
Drop the statics everywhere in this file.
Right now what you're doing is creating storage for each of these vars in every
compilation unit that includes this .h.

The right way to do this is to declare these consts extern here but define them
in a (new) limits.cc file.  That way storage is only allocated once.

Alternatively, you could use an anonymous enum (assuming none of these ever have
their address taken).  Personally that's my favorite approach.

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h#newcode25
media/base/limits.h:25: // Following limits are used by
AudioParameters::IsValid().
s/Following/The following/

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h#newcode27
media/base/limits.h:27: // A few notes on samples rates of common formats:
s/samples/sample/

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h#newcode28
media/base/limits.h:28: //   - AAC files are limited to 96 Khz.
s/Khz/kHz/ here and below.

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h#newcode39
media/base/limits.h:39: static const int64 kMaxTimeInMicroseconds = kint64max;
This should die.  See my specific comments at call-sites.

http://codereview.chromium.org/8786013/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/8786013/diff/1/media/filters/ffmpeg_demuxer.cc...
media/filters/ffmpeg_demuxer.cc:523: // and set duration to the maximum int64
number to represent infinity.
This is crazy.  If we want \infty we should define a kInfinity, and probably use
an obviously-bogus value like -1.

http://codereview.chromium.org/8786013/diff/1/media/filters/ffmpeg_demuxer.cc...
media/filters/ffmpeg_demuxer.cc:565: max_duration_.InMicroseconds() <
limits::kMaxTimeInMicroseconds) {
This would be a lot clearer as != kInfinity IMO.

http://codereview.chromium.org/8786013/diff/1/media/filters/ffmpeg_demuxer.cc...
media/filters/ffmpeg_demuxer.cc:568: return 8000 * filesize_in_bytes /
max_duration_.InMilliseconds();
This can SIGFPE b/c the >0 check is done in microseconds but here we divide by
milliseconds.

http://codereview.chromium.org/8786013/diff/1/webkit/media/webmediaplayer_imp...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8786013/diff/1/webkit/media/webmediaplayer_imp...
webkit/media/webmediaplayer_impl.cc:501: if (duration.InMicroseconds() ==
media::limits::kMaxTimeInMicroseconds)
I *think* this is the only place that cares about \infty and would want to use
the constant I proposed earlier.
But I find myself wondering whether webkit really wants to be getting back
infinity for this float (if it doesn't, then probably the constant and the logic
around it don't need to exist).

[scherkus (not reviewing), 2011-12-06 02:05:57]

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h
File media/base/limits.h (right):

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h#newcode17
media/base/limits.h:17: static const int kMaxDimension = (1 << 15) - 1;  //
32767
On 2011/12/03 02:11:38, Ami Fischman wrote:
> Drop the statics everywhere in this file.
> Right now what you're doing is creating storage for each of these vars in
every
> compilation unit that includes this .h.
> 
> The right way to do this is to declare these consts extern here but define
them
> in a (new) limits.cc file.  That way storage is only allocated once.
> 
> Alternatively, you could use an anonymous enum (assuming none of these ever
have
> their address taken).  Personally that's my favorite approach.

Done.

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h#newcode25
media/base/limits.h:25: // Following limits are used by
AudioParameters::IsValid().
On 2011/12/03 02:11:38, Ami Fischman wrote:
> s/Following/The following/

Done.

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h#newcode28
media/base/limits.h:28: //   - AAC files are limited to 96 Khz.
On 2011/12/03 02:11:38, Ami Fischman wrote:
> s/Khz/kHz/ here and below.

Done.

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h#newcode39
media/base/limits.h:39: static const int64 kMaxTimeInMicroseconds = kint64max;
On 2011/12/03 02:11:38, Ami Fischman wrote:
> This should die.  See my specific comments at call-sites.

Done.

http://codereview.chromium.org/8786013/diff/1/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/8786013/diff/1/media/filters/ffmpeg_demuxer.cc...
media/filters/ffmpeg_demuxer.cc:523: // and set duration to the maximum int64
number to represent infinity.
On 2011/12/03 02:11:38, Ami Fischman wrote:
> This is crazy.  If we want \infty we should define a kInfinity, and probably
use
> an obviously-bogus value like -1.

That makes sense for durations, but if we're going to pick sentinel values why
not use a value that's as close to infinity as we're going to get? it has the
nice property of being able to use std::max()/min()

http://codereview.chromium.org/8786013/diff/1/media/filters/ffmpeg_demuxer.cc...
media/filters/ffmpeg_demuxer.cc:565: max_duration_.InMicroseconds() <
limits::kMaxTimeInMicroseconds) {
On 2011/12/03 02:11:38, Ami Fischman wrote:
> This would be a lot clearer as != kInfinity IMO.

Done.

http://codereview.chromium.org/8786013/diff/1/media/filters/ffmpeg_demuxer.cc...
media/filters/ffmpeg_demuxer.cc:568: return 8000 * filesize_in_bytes /
max_duration_.InMilliseconds();
On 2011/12/03 02:11:38, Ami Fischman wrote:
> This can SIGFPE b/c the >0 check is done in microseconds but here we divide by
> milliseconds.

Done.

http://codereview.chromium.org/8786013/diff/1/webkit/media/webmediaplayer_imp...
File webkit/media/webmediaplayer_impl.cc (right):

http://codereview.chromium.org/8786013/diff/1/webkit/media/webmediaplayer_imp...
webkit/media/webmediaplayer_impl.cc:501: if (duration.InMicroseconds() ==
media::limits::kMaxTimeInMicroseconds)
On 2011/12/03 02:11:38, Ami Fischman wrote:
> I *think* this is the only place that cares about \infty and would want to use
> the constant I proposed earlier.
> But I find myself wondering whether webkit really wants to be getting back
> infinity for this float (if it doesn't, then probably the constant and the
logic
> around it don't need to exist).

we use infinity to represent an unbounded stream/duration unknown stream:
http://www.whatwg.org/specs/web-apps/current-work/multipage/video.html#dom-me...

[scherkus (not reviewing), 2011-12-06 02:44:53]

crap looks like windows enums aren't as l33t:

..\..\media/base/limits.h(41) : error C2220: warning treated as error - no
'object' file generated
..\..\media/base/limits.h(41) : warning C4341: 'kInfiniteDuration' : signed
value is out of range for enum constant
..\..\media/base/limits.h(41) : warning C4309: 'initializing' : truncation of
constant value
webmediaplayer_impl.cc
..\..\media/base/limits.h(41) : error C2220: warning treated as error - no
'object' file generated
..\..\media/base/limits.h(41) : warning C4341: 'kInfiniteDuration' : signed
value is out of range for enum constant
..\..\media/base/limits.h(41) : warning C4309: 'initializing' : truncation of
constant value



one way to solve this is use -1
the other is to do limits.cc

[Ami GONE FROM CHROMIUM, 2011-12-07 01:04:48]

Another alternative to the infinite-duration sentinel problem is to add a
HasDuration() to FilterHost.

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h
File media/base/limits.h (right):

http://codereview.chromium.org/8786013/diff/1/media/base/limits.h#newcode25
media/base/limits.h:25: // Following limits are used by
AudioParameters::IsValid().
On 2011/12/06 02:05:57, scherkus wrote:
> On 2011/12/03 02:11:38, Ami Fischman wrote:
> > s/Following/The following/
> 
> Done.

Nope.

http://codereview.chromium.org/8786013/diff/4001/media/base/limits.h
File media/base/limits.h (right):

http://codereview.chromium.org/8786013/diff/4001/media/base/limits.h#newcode28
media/base/limits.h:28: // A few notes on samples rates of common formats:
depluralize "samples"

http://codereview.chromium.org/8786013/diff/4001/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/8786013/diff/4001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:566: return 0;
Need braces.

http://codereview.chromium.org/8786013/diff/4001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:572: return bytes * 8000000.0 / duration;
Personally I prefer 8e6 to counting zeros, but I don't think that's a
chromium-dev majority opinion.

[scherkus (not reviewing), 2011-12-07 05:56:56]

great scott!!

I don't think kInfiniteDuration belongs in media::limits anyway!

moved it to buffers.h along the other TimeDelta constant, kNoTimestamp

it also gets around both the windows thing as well as having to use
.InMicroseconds()

The TimeDelta::FromXXX() methods are all static inlined and I _think_ as a
result don't result in creating a constructor call during initialization, but
TBH not sure how to verify that

http://codereview.chromium.org/8786013/diff/4001/media/base/limits.h
File media/base/limits.h (right):

http://codereview.chromium.org/8786013/diff/4001/media/base/limits.h#newcode28
media/base/limits.h:28: // A few notes on samples rates of common formats:
On 2011/12/07 01:04:48, Ami Fischman wrote:
> depluralize "samples"

whoops I must of lost those changes when I ditched limits.cc and went with the
anon enum approach!

http://codereview.chromium.org/8786013/diff/4001/media/filters/ffmpeg_demuxer.cc
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/8786013/diff/4001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:566: return 0;
On 2011/12/07 01:04:48, Ami Fischman wrote:
> Need braces.

Done.

http://codereview.chromium.org/8786013/diff/4001/media/filters/ffmpeg_demuxer...
media/filters/ffmpeg_demuxer.cc:572: return bytes * 8000000.0 / duration;
On 2011/12/07 01:04:48, Ami Fischman wrote:
> Personally I prefer 8e6 to counting zeros, but I don't think that's a
> chromium-dev majority opinion.

I like my zeros!

also my efforts to prevent overflow are somewhat futile considering we truncate
to int :~(

[Ami GONE FROM CHROMIUM, 2011-12-07 17:10:51]

LGTM modulo question.

http://codereview.chromium.org/8786013/diff/14001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/8786013/diff/14001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:564: max_duration_ != kInfiniteDuration ||
test reversed?

[scherkus (not reviewing), 2011-12-07 18:01:05]

http://codereview.chromium.org/8786013/diff/14001/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

http://codereview.chromium.org/8786013/diff/14001/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:564: max_duration_ != kInfiniteDuration ||
On 2011/12/07 17:10:51, Ami Fischman wrote:
> test reversed?

nice catch!

FFmpegDemuxerTest.GetBitrate_UnsetInContainer_KnownSize caught it as well (go
acolwell!)