Improving the decoder pipeline.

remoting/base/util.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.
 
 #include "remoting/base/util.h"
 
 #include <math.h>
 
 #include "base/logging.h"
 #include "base/stringprintf.h"
@@ skipping 201 matching lines @@
   int x = RoundToTwosMultiple(rect.left());
   int y = RoundToTwosMultiple(rect.top());
   int right = RoundToTwosMultiple(rect.right() + 1);
   int bottom = RoundToTwosMultiple(rect.bottom() + 1);
   return SkIRect::MakeLTRB(x, y, right, bottom);
 }
 
 SkIRect ScaleRect(const SkIRect& rect,
                   const SkISize& in_size,
                   const SkISize& out_size) {
-  int left = (rect.left() * out_size.width()) / in_size.width();
-  int top = (rect.top() * out_size.height()) / in_size.height();
-  int right = (rect.right() * out_size.width() + out_size.width() - 1) /
-      in_size.width();
-  int bottom = (rect.bottom() * out_size.height() + out_size.height() - 1) /
-      in_size.height();
+  // Use floating point to up/down scale more accurately.

[Wez, 2012/02/17 23:42:17]

Given that the results are still integer, how is this more accurate than the
existing code?

[alexeypa (please no reviews), 2012/02/21 23:00:44]

Up-scaling didn't work properly in all cases with the previous version of the
code. We need up-scaling for validating the clipping area when scaling the view.

[Wez, 2012/02/23 00:11:09]

Do remember in what way it was broken? I'd prefer that we fix the integer
arithmetic rather than start reintroducing floating-point code.

[alexeypa (please no reviews), 2012/02/23 17:10:33]

Sorry, I don't remember. I do remember that I hit in some extreme case and the
error made sense at the time. :-)

I think floating point is not too bad, given that it is used in the decoder and,
in the future, by the RGB-to-YUV converter/scaler anyway. 

[Wez, 2012/02/23 22:59:26]

OK; there's no intrinsic reason why doing the integer calculation should be any
less accurate than the float one.
Neither the decoder not converter/scaler should be using floating point either,
as far as I'm aware...

[alexeypa (please no reviews), 2012/02/23 23:59:14]

OK, here is the bug I was talking about. Say in_size.width() == 10,
out_size.width() == 1000 and rect.right() == 10. The original formula gives you
1099. The floating point one - 1000.

The bug is caused by the fact that we are adding (out_size.width() - 1), not
(in_size.width() - 1), which would be correct. Now there are two ways to fix it:
convert to floating point or fix the integer arithmetic. The former one is
straightforward and easy to read. The latter one is more tricky to understand
(which is proven by the bug) and less readable. I chose the former one for its
readability and because this is not a performance critical path in the code.
We want use SSE which is the same thing as floating point from CPU point of
view. CPU will have to swap FPU state anyway if either was used.

[Wez, 2012/02/24 00:12:30]

Correct. :)
We may have a large number of rectangles in an update, so on more limited
platforms (e.g. ARM) the floating-point hit can be a problem.  Since we're going
from integer inputs to integer outputs, fixing my broken arithmetic seems
preferably to jumping into and out of floating-point.
Our SSE code is fixed-point; or are you just referring to the actual registers
that the instructions use?

[alexeypa (please no reviews), 2012/02/24 16:46:56]

I don't think anything can be called a performance critical path without first
measuring it. :-)

I did a couple micro-benchmarks last night. Floating point on x86 was noticeably
worse than integer that surprised me - floating point version has twice less
divisions. It appears that the compiler generates too many helper calls. So we
were right on this one. x64 performance was the same which is not surprising at
all. x64 is irrelevant to Chrome at the moment.

I suspect floating point on ARM will actually be better because ARM uses
software division for both integer and floating point (except that NEON has
hardware approximation for getting the reciprocal to emulate floating point
division). So two vs four divisions will probably show up. It was too late for
installing Adroid NDK yesterday. :-)

Given all that I'm reverting to the integer version, which seems to be a better
choice so far.
I meant the actual register state. Once SSE or FP is used CPU has load/store
both FP and SSE state.    (Yes, I'm aware of lazy FPU state loading :-) ).

[Wez, 2012/02/24 18:02:04]

Sorry, I should have pointed out that my comments on performance impact are
based on past experience implementing this code for ARM & x86.
Except on Linux. ;)
If I remember correctly, integer division on ARM is considerably cheaper than
floating-point but still considerably more expensive than other integer
operations.  You can work around that by approximating (a / b) as ((a * (2^k /
b)) >> k), and choosing k to minimize rounding errors while fitting in your
registers.
OK.  Thanks for the thorough investigation!
That's a valid concern, although I'd expect floating point execution time to
outweigh load/store overhead on any hot code-paths.  Again, something we can
look into if we see performance issues. :)

+  float scale_x = static_cast<float>(out_size.width()) / in_size.width();
+  float scale_y = static_cast<float>(out_size.height()) / in_size.height();
+
+  int left = static_cast<int>(floor(scale_x * rect.left()));
+  int top = static_cast<int>(floor(scale_y * rect.top()));
+  int right = static_cast<int>(ceil(scale_x * rect.right()));
+  int bottom = static_cast<int>(ceil(scale_y * rect.bottom()));
   return SkIRect::MakeLTRB(left, top, right, bottom);
 }
 
 void CopyRect(const uint8* src_plane,
               int src_plane_stride,
               uint8* dest_plane,
               int dest_plane_stride,
               int bytes_per_pixel,
               const SkIRect& rect) {
   // Get the address of the starting point.
@@ skipping 34 matching lines @@
   // Copy bits.
   CopyRect(source_buffer + source_offset,
            source_stride,
            dest_buffer + dest_offset,
            dest_stride,
            GetBytesPerPixel(media::VideoFrame::RGB32),
            SkIRect::MakeWH(dest_rect.width(), dest_rect.height()));
 }
 
 }  // namespace remoting