Type conversion fixes, media/ edition.

media/base/sinc_resampler.cc

Index: media/base/sinc_resampler.cc
diff --git a/media/base/sinc_resampler.cc b/media/base/sinc_resampler.cc
index 900648e67601ea4d0bdb07ecb658d57c1b527d78..280cd68eb19cad6643f6eccb85e13168024e0b02 100644
--- a/media/base/sinc_resampler.cc
+++ b/media/base/sinc_resampler.cc
@@ -178,23 +178,22 @@ void SincResampler::InitializeKernel() {
 
     for (int i = 0; i < kKernelSize; ++i) {
       const int idx = i + offset_idx * kKernelSize;
-      const float pre_sinc = M_PI * (i - kKernelSize / 2 - subsample_offset);
+      const float pre_sinc =
+          static_cast<float>(M_PI * (i - kKernelSize / 2 - subsample_offset));

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

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.

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

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.

       kernel_pre_sinc_storage_[idx] = pre_sinc;
 
       // Compute Blackman window, matching the offset of the sinc().
       const float x = (i - subsample_offset) / kKernelSize;
-      const float window =
-          kA0 - kA1 * cos(2.0 * M_PI * x) + kA2 * cos(4.0 * M_PI * x);
+      const float window = static_cast<float>(kA0 - kA1 * cos(2.0 * M_PI * x) +
+          kA2 * cos(4.0 * M_PI * x));
       kernel_window_storage_[idx] = window;
 
       // Compute the sinc with offset, then window the sinc() function and store
       // at the correct offset.
-      if (pre_sinc == 0) {
-        kernel_storage_[idx] = sinc_scale_factor * window;
-      } else {
-        kernel_storage_[idx] =
-            window * sin(sinc_scale_factor * pre_sinc) / pre_sinc;
-      }
+      kernel_storage_[idx] = static_cast<float>(window *

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

This is functionally equivalent to the previous code, it just avoids duplicating
the static_cast<> (which otherwise would cause Even More Line Wrapping).

+          ((pre_sinc == 0) ?
+              sinc_scale_factor :
+              (sin(sinc_scale_factor * pre_sinc) / pre_sinc)));
     }
   }
 }
@@ -216,12 +215,10 @@ void SincResampler::SetRatio(double io_sample_rate_ratio) {
       const float window = kernel_window_storage_[idx];
       const float pre_sinc = kernel_pre_sinc_storage_[idx];
 
-      if (pre_sinc == 0) {
-        kernel_storage_[idx] = sinc_scale_factor * window;
-      } else {
-        kernel_storage_[idx] =
-            window * sin(sinc_scale_factor * pre_sinc) / pre_sinc;
-      }
+      kernel_storage_[idx] = static_cast<float>(window *
+          ((pre_sinc == 0) ?
+              sinc_scale_factor :
+              (sin(sinc_scale_factor * pre_sinc) / pre_sinc)));
     }
   }
 }
@@ -242,7 +239,7 @@ void SincResampler::Resample(int frames, float* destination) {
   while (remaining_frames) {
     // Note: The loop construct here can severely impact performance on ARM
     // or when built with clang.  See https://codereview.chromium.org/18566009/
-    int source_idx = virtual_source_idx_;
+    int source_idx = static_cast<int>(virtual_source_idx_);
     while (source_idx < block_size_) {
       // |virtual_source_idx_| lies in between two kernel offsets so figure out
       // what they are.
@@ -250,7 +247,7 @@ void SincResampler::Resample(int frames, float* destination) {
 
       const double virtual_offset_idx =
           subsample_remainder * kKernelOffsetCount;
-      const int offset_idx = virtual_offset_idx;
+      const int offset_idx = static_cast<int>(virtual_offset_idx);
 
       // We'll compute "convolutions" for the two kernels which straddle
       // |virtual_source_idx_|.
@@ -273,7 +270,7 @@ void SincResampler::Resample(int frames, float* destination) {
 
       // Advance the virtual index.
       virtual_source_idx_ += current_io_ratio;
-      source_idx = virtual_source_idx_;
+      source_idx = static_cast<int>(virtual_source_idx_);
 
       if (!--remaining_frames)
         return;
@@ -297,7 +294,7 @@ void SincResampler::Resample(int frames, float* destination) {
 }
 
 int SincResampler::ChunkSize() const {
-  return block_size_ / io_sample_rate_ratio_;
+  return static_cast<int>(block_size_ / io_sample_rate_ratio_);
 }
 
 void SincResampler::Flush() {
@@ -323,8 +320,8 @@ float SincResampler::Convolve_C(const float* input_ptr, const float* k1,
   }
 
   // Linearly interpolate the two "convolutions".
-  return (1.0 - kernel_interpolation_factor) * sum1
-      + kernel_interpolation_factor * sum2;
+  return static_cast<float>((1.0 - kernel_interpolation_factor) * sum1 +

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

aside: nice wrapping style catch :)

+      kernel_interpolation_factor * sum2);
 }
 
 #if defined(ARCH_CPU_X86_FAMILY)
@@ -352,8 +349,10 @@ float SincResampler::Convolve_SSE(const float* input_ptr, const float* k1,
   }
 
   // Linearly interpolate the two "convolutions".
-  m_sums1 = _mm_mul_ps(m_sums1, _mm_set_ps1(1.0 - kernel_interpolation_factor));
-  m_sums2 = _mm_mul_ps(m_sums2, _mm_set_ps1(kernel_interpolation_factor));
+  m_sums1 = _mm_mul_ps(m_sums1, _mm_set_ps1(
+      static_cast<float>(1.0 - kernel_interpolation_factor)));

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

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.

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

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:15]

Acknowledged.

+  m_sums2 = _mm_mul_ps(m_sums2, _mm_set_ps1(
+      static_cast<float>(kernel_interpolation_factor)));
   m_sums1 = _mm_add_ps(m_sums1, m_sums2);
 
   // Sum components together.