Type conversion fixes, media/ edition.

media/base/sinc_resampler.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 //
 // Initial input buffer layout, dividing into regions r0_ to r4_ (note: r0_, r3_
 // and r4_ will move after the first load):
 //
 // |----------------|-----------------------------------------|----------------|
 //
 //                                        request_frames_
@@ skipping 160 matching lines @@
 
   // Generates a set of windowed sinc() kernels.
   // We generate a range of sub-sample offsets from 0.0 to 1.0.
   const double sinc_scale_factor = SincScaleFactor(io_sample_rate_ratio_);
   for (int offset_idx = 0; offset_idx <= kKernelOffsetCount; ++offset_idx) {
     const float subsample_offset =
         static_cast<float>(offset_idx) / kKernelOffsetCount;
 
     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)));
     }
   }
 }
 
 void SincResampler::SetRatio(double io_sample_rate_ratio) {
   if (fabs(io_sample_rate_ratio_ - io_sample_rate_ratio) <
       std::numeric_limits<double>::epsilon()) {
     return;
   }
 
   io_sample_rate_ratio_ = io_sample_rate_ratio;
 
   // Optimize reinitialization by reusing values which are independent of
   // |sinc_scale_factor|.  Provides a 3x speedup.
   const double sinc_scale_factor = SincScaleFactor(io_sample_rate_ratio_);
   for (int offset_idx = 0; offset_idx <= kKernelOffsetCount; ++offset_idx) {
     for (int i = 0; i < kKernelSize; ++i) {
       const int idx = i + offset_idx * kKernelSize;
       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)));
     }
   }
 }
 
 void SincResampler::Resample(int frames, float* destination) {
   int remaining_frames = frames;
 
   // Step (1) -- Prime the input buffer at the start of the input stream.
   if (!buffer_primed_ && remaining_frames) {
     read_cb_.Run(request_frames_, r0_);
     buffer_primed_ = true;
   }
 
   // Step (2) -- Resample!  const what we can outside of the loop for speed.  It
   // actually has an impact on ARM performance.  See inner loop comment below.
   const double current_io_ratio = io_sample_rate_ratio_;
   const float* const kernel_ptr = kernel_storage_.get();
   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.
       const double subsample_remainder = virtual_source_idx_ - source_idx;
 
       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_|.
       const float* const k1 = kernel_ptr + offset_idx * kKernelSize;
       const float* const k2 = k1 + kKernelSize;
 
       // Ensure |k1|, |k2| are 16-byte aligned for SIMD usage.  Should always be
       // true so long as kKernelSize is a multiple of 16.
       DCHECK_EQ(0u, reinterpret_cast<uintptr_t>(k1) & 0x0F);
       DCHECK_EQ(0u, reinterpret_cast<uintptr_t>(k2) & 0x0F);
 
       // Initialize input pointer based on quantized |virtual_source_idx_|.
       const float* const input_ptr = r1_ + source_idx;
 
       // Figure out how much to weight each kernel's "convolution".
       const double kernel_interpolation_factor =
           virtual_offset_idx - offset_idx;
       *destination++ = CONVOLVE_FUNC(
           input_ptr, k1, k2, kernel_interpolation_factor);
 
       // 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;
     }
 
     // Wrap back around to the start.
     DCHECK_GE(virtual_source_idx_, block_size_);
     virtual_source_idx_ -= block_size_;
 
     // Step (3) -- Copy r3_, r4_ to r1_, r2_.
     // This wraps the last input frames back to the start of the buffer.
     memcpy(r1_, r3_, sizeof(*input_buffer_.get()) * kKernelSize);
 
     // Step (4) -- Reinitialize regions if necessary.
     if (r0_ == r2_)
       UpdateRegions(true);
 
     // Step (5) -- Refresh the buffer with more input.
     read_cb_.Run(request_frames_, r0_);
   }
 }
 
 int SincResampler::ChunkSize() const {
-  return block_size_ / io_sample_rate_ratio_;
+  return static_cast<int>(block_size_ / io_sample_rate_ratio_);
 }
 
 void SincResampler::Flush() {
   virtual_source_idx_ = 0;
   buffer_primed_ = false;
   memset(input_buffer_.get(), 0,
          sizeof(*input_buffer_.get()) * input_buffer_size_);
   UpdateRegions(false);
 }
 
 float SincResampler::Convolve_C(const float* input_ptr, const float* k1,
                                 const float* k2,
                                 double kernel_interpolation_factor) {
   float sum1 = 0;
   float sum2 = 0;
 
   // Generate a single output sample.  Unrolling this loop hurt performance in
   // local testing.
   int n = kKernelSize;
   while (n--) {
     sum1 += *input_ptr * *k1++;
     sum2 += *input_ptr++ * *k2++;
   }
 
   // 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)
 float SincResampler::Convolve_SSE(const float* input_ptr, const float* k1,
                                   const float* k2,
                                   double kernel_interpolation_factor) {
   __m128 m_input;
   __m128 m_sums1 = _mm_setzero_ps();
   __m128 m_sums2 = _mm_setzero_ps();
 
   // Based on |input_ptr| alignment, we need to use loadu or load.  Unrolling
   // these loops hurt performance in local testing.
   if (reinterpret_cast<uintptr_t>(input_ptr) & 0x0F) {
     for (int i = 0; i < kKernelSize; i += 4) {
       m_input = _mm_loadu_ps(input_ptr + i);
       m_sums1 = _mm_add_ps(m_sums1, _mm_mul_ps(m_input, _mm_load_ps(k1 + i)));
       m_sums2 = _mm_add_ps(m_sums2, _mm_mul_ps(m_input, _mm_load_ps(k2 + i)));
     }
   } else {
     for (int i = 0; i < kKernelSize; i += 4) {
       m_input = _mm_load_ps(input_ptr + i);
       m_sums1 = _mm_add_ps(m_sums1, _mm_mul_ps(m_input, _mm_load_ps(k1 + i)));
       m_sums2 = _mm_add_ps(m_sums2, _mm_mul_ps(m_input, _mm_load_ps(k2 + i)));
     }
   }
 
   // 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.
   float result;
   m_sums2 = _mm_add_ps(_mm_movehl_ps(m_sums1, m_sums1), m_sums1);
   _mm_store_ss(&result, _mm_add_ss(m_sums2, _mm_shuffle_ps(
       m_sums2, m_sums2, 1)));
 
   return result;
 }
@@ skipping 20 matching lines @@
       vmulq_f32(m_sums1, vmovq_n_f32(1.0 - kernel_interpolation_factor)),
       m_sums2, vmovq_n_f32(kernel_interpolation_factor));
 
   // Sum components together.
   float32x2_t m_half = vadd_f32(vget_high_f32(m_sums1), vget_low_f32(m_sums1));
   return vget_lane_f32(vpadd_f32(m_half, m_half), 0);
 }
 #endif
 
 }  // namespace media