Type conversion fixes, media/ edition.

Type conversion fixes, media/ edition.

This is mostly to fix MSVC warnings about possible value truncation.

BUG=81439
TEST=none

Committed: https://crrev.com/6b3ea7317ee67859ec3b9e0d10e4ecd89440f16e
Cr-Commit-Position: refs/heads/master@{#300571}

[Peter Kasting, 2014-10-16 23:50:42]

pkasting@chromium.org changed reviewers:
+ wolenetz@chromium.org

[Peter Kasting, 2014-10-16 23:50:43]

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc...
media/base/sinc_resampler.cc:182: static_cast<float>(M_PI * (i - kKernelSize / 2
- subsample_offset));
In general in this file, I tried to make the explicit casts exactly match the
locations of the previous implicit casts, since it's not clear to me what
operations are sensitive to possible precision loss.

It may be possible to have fewer casts, better performance, or both by using
floats more in intermediate calculations (e.g. by making all the constants at
the top of this function floats and using a float version of M_PI), but that
sort of a change is beyond my comprehension.  Feel free to consider it.

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc...
media/base/sinc_resampler.cc:193: kernel_storage_[idx] =
static_cast<float>(window *
This is functionally equivalent to the previous code, it just avoids duplicating
the static_cast<> (which otherwise would cause Even More Line Wrapping).

[miu, 2014-10-17 07:42:39]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2014-10-17 07:42:39]

media/cast lgtm

[wolenetz, 2014-10-17 19:23:01]

media/base and media/audio lgtm.

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc...
media/base/sinc_resampler.cc:182: static_cast<float>(M_PI * (i - kKernelSize / 2
- subsample_offset));
On 2014/10/16 23:50:42, Peter Kasting wrote:
> In general in this file, I tried to make the explicit casts exactly match the
> locations of the previous implicit casts, since it's not clear to me what
> operations are sensitive to possible precision loss.
> 
> It may be possible to have fewer casts, better performance, or both by using
> floats more in intermediate calculations (e.g. by making all the constants at
> the top of this function floats and using a float version of M_PI), but that
> sort of a change is beyond my comprehension.  Feel free to consider it.

I've filed https://crbug.com/424695 to track investigation of these potential
improvements. I would prefer keeping that investigation separate from this CL
that has intent to eliminate the implicit typecast MSVC warnings. Thanks for
pointing out this possibility.

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc...
media/base/sinc_resampler.cc:323: return static_cast<float>((1.0 -
kernel_interpolation_factor) * sum1 +
aside: nice wrapping style catch :)

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc...
media/base/sinc_resampler.cc:353: static_cast<float>(1.0 -
kernel_interpolation_factor)));
aside: From what I can tell, I don't think this loses any precision versus the
previous code, though kernel_interpolcation_factor is a double.

[wolenetz, 2014-10-17 19:24:44]

On 2014/10/17 19:23:01, wolenetz wrote:
> media/base and media/audio lgtm.
> 
> https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc
> File media/base/sinc_resampler.cc (right):
> 
>
https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc...
> media/base/sinc_resampler.cc:182: static_cast<float>(M_PI * (i - kKernelSize /
2
> - subsample_offset));
> On 2014/10/16 23:50:42, Peter Kasting wrote:
> > In general in this file, I tried to make the explicit casts exactly match
the
> > locations of the previous implicit casts, since it's not clear to me what
> > operations are sensitive to possible precision loss.
> > 
> > It may be possible to have fewer casts, better performance, or both by using
> > floats more in intermediate calculations (e.g. by making all the constants
at
> > the top of this function floats and using a float version of M_PI), but that
> > sort of a change is beyond my comprehension.  Feel free to consider it.
> 
> I've filed https://crbug.com/424695 to track investigation of these potential
> improvements. I would prefer keeping that investigation separate from this CL
> that has intent to eliminate the implicit typecast MSVC warnings. Thanks for
> pointing out this possibility.
> 
>
https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc...
> media/base/sinc_resampler.cc:323: return static_cast<float>((1.0 -
> kernel_interpolation_factor) * sum1 +
> aside: nice wrapping style catch :)
> 
>
https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc...
> media/base/sinc_resampler.cc:353: static_cast<float>(1.0 -
> kernel_interpolation_factor)));
> aside: From what I can tell, I don't think this loses any precision versus the
> previous code, though kernel_interpolcation_factor is a double.

bots don't like this (cast unittest failing)

[Peter Kasting, 2014-10-21 19:00:24]

On 2014/10/17 19:24:44, wolenetz wrote:
> bots don't like this (cast unittest failing)

Figured this out.  See the delta in cast_defines.h.  The old code worked
seemingly by accident because all portions of the calculation would get
converted to double, and then the fractional part of the second addend would be
so large that the resulting sum would get rounded up (due to double not having
enough precision to represent the entire sum).

The new code makes the ceil() action explicit.

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc...
media/base/sinc_resampler.cc:353: static_cast<float>(1.0 -
kernel_interpolation_factor)));
On 2014/10/17 19:23:01, wolenetz wrote:
> aside: From what I can tell, I don't think this loses any precision versus the
> previous code, though kernel_interpolcation_factor is a double.

Right, it shouldn't, since _mm_set_ps1() takes a float.  I got burned in some
other repo by trying to do this instead:

  _mm_set_ps1(1.0f - static_cast<float>(kernel_interpolation_factor));

...so I avoided that.

[wolenetz, 2014-10-21 19:15:16]

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc
File media/base/sinc_resampler.cc (right):

https://codereview.chromium.org/638123004/diff/1/media/base/sinc_resampler.cc...
media/base/sinc_resampler.cc:353: static_cast<float>(1.0 -
kernel_interpolation_factor)));
On 2014/10/21 19:00:24, Peter Kasting wrote:
> On 2014/10/17 19:23:01, wolenetz wrote:
> > aside: From what I can tell, I don't think this loses any precision versus
the
> > previous code, though kernel_interpolcation_factor is a double.
> 
> Right, it shouldn't, since _mm_set_ps1() takes a float.  I got burned in some
> other repo by trying to do this instead:
> 
>   _mm_set_ps1(1.0f - static_cast<float>(kernel_interpolation_factor));
> 
> ...so I avoided that.

Acknowledged.

https://codereview.chromium.org/638123004/diff/20001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/638123004/diff/20001/media/cast/cast_defines....
media/cast/cast_defines.h:181: // We need to ceil() here because the calculation
of |fractions| in
nit: fix compilation by #including std::ceil

https://codereview.chromium.org/638123004/diff/20001/media/cast/cast_defines....
media/cast/cast_defines.h:184: static_cast<int64>(std::ceil(ntp_fractions /
kMagicFractionalUnit));
Though this works for the unit tests' hardcoded times, does this introduce lack
of round-tripping for other times? Would rounding solve this better (e.g.
std::floor(0.5 + ntp_fractions / kMagic..))?

[wolenetz, 2014-10-21 19:22:00]

https://codereview.chromium.org/638123004/diff/20001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/638123004/diff/20001/media/cast/cast_defines....
media/cast/cast_defines.h:184: static_cast<int64>(std::ceil(ntp_fractions /
kMagicFractionalUnit));
On 2014/10/21 19:15:15, wolenetz wrote:
> Though this works for the unit tests' hardcoded times, does this introduce
lack
> of round-tripping for other times? Would rounding solve this better (e.g.
> std::floor(0.5 + ntp_fractions / kMagic..))?

From offline chat w/pkasting@, the behavior-invariant patch would use ceil()
here. There could be other issues with rounding vs ceil/floor/truncation
pre-existing in this code. miu@, please take a look.

[Peter Kasting, 2014-10-21 19:34:47]

https://codereview.chromium.org/638123004/diff/20001/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/638123004/diff/20001/media/cast/cast_defines....
media/cast/cast_defines.h:181: // We need to ceil() here because the calculation
of |fractions| in
On 2014/10/21 19:15:15, wolenetz wrote:
> nit: fix compilation by #including std::ceil

Done.

https://codereview.chromium.org/638123004/diff/20001/media/cast/cast_defines....
media/cast/cast_defines.h:184: static_cast<int64>(std::ceil(ntp_fractions /
kMagicFractionalUnit));
On 2014/10/21 19:22:00, wolenetz wrote:
> On 2014/10/21 19:15:15, wolenetz wrote:
> > Though this works for the unit tests' hardcoded times, does this introduce
> lack
> > of round-tripping for other times? Would rounding solve this better (e.g.
> > std::floor(0.5 + ntp_fractions / kMagic..))?
> 
> From offline chat w/pkasting@, the behavior-invariant patch would use ceil()
> here. There could be other issues with rounding vs ceil/floor/truncation
> pre-existing in this code. miu@, please take a look.

More detail:

ceil() here is correct if we're trying to invert ConvertTimeToFractions().  The
question, really, is whether ConvertTimeToFractions() is correct to truncate the
calculation of |fractions|, or if instead it should round.  If it _should_
round, then we'd want to round here as well to match.

I'm happy to do either.

[miu, 2014-10-21 20:58:08]

As discussed offline, ceil() is good.  media/cast still lgtm.

[Peter Kasting, 2014-10-21 20:58:22]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-21 21:00:03]

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

[commit-bot: I haz the power, 2014-10-21 23:53:27]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-21 23:55:21]

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