Introducing a helper wrapper for YUV-to-RGB convertion and/or scaling.

Introducing helper wrappers for copying a rectangle from a one partial buffer to another. YUV to RGB and RGB to RGB operations are supported. YUV to RGB allows down-scaling of rectangles. The wrappers hide (to some degree) gory details of manipulations with buffer pointers and rectangles. Added unit tests (remoting_unittests.Yuv2Rgb) to verify functionality of the YUV to RGB wrapper.

BUG=109938
TEST=remoting_unittests.Yuv2Rgb

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

[alexeypa (please no reviews), 2012-02-02 00:54:45]

This is the 1st part of the decoder pipeline refactoring.

[Wez, 2012-02-02 01:35:28]

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.cc
File remoting/base/util.cc (right):

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.cc#n...
remoting/base/util.cc:114: DCHECK((dest_rect.x() & 1) == 0 && (dest_rect.y() &
1) == 0);
Since the API specifies that you should always provide even X & Y coords,
DCHECK() on function entry, rather than just on this path.

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.cc#n...
remoting/base/util.cc:173: int bottom = SkScalarCeilToInt(scale_y *
rect.bottom());
Why are you changing from pure integer arithmetic to SkScalar (which might be
float, depending on build settings)?

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.h
File remoting/base/util.h (right):

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.h#ne...
remoting/base/util.h:22: // destination buffers are assumed to contain only
|source_clip| and |dest_clip|
Why have |dest_clip| passed in, rather than letting the caller clip the
|dest_rect| and then checking that?

Why have |source_clip] at all?

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.h#ne...
remoting/base/util.h:79: void CopyRGB32Rect(const uint8* source_buffer,
This doesn't seem to be used anywhere?

[alexeypa (please no reviews), 2012-02-02 21:33:02]

I added quite a few changes. Please take another look.

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.cc
File remoting/base/util.cc (right):

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.cc#n...
remoting/base/util.cc:114: DCHECK((dest_rect.x() & 1) == 0 && (dest_rect.y() &
1) == 0);
On 2012/02/02 01:35:29, Wez wrote:
> Since the API specifies that you should always provide even X & Y coords,
> DCHECK() on function entry, rather than just on this path.

I made the function more tolerant to unaligned rectangles.

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.cc#n...
remoting/base/util.cc:173: int bottom = SkScalarCeilToInt(scale_y *
rect.bottom());
On 2012/02/02 01:35:29, Wez wrote:
> (which might be float, depending on build settings)?

That was needed to support up-scaling at some point and my new code used this.
But then I realized that the scaling routines that we have do not support
upscaling anyway so it is a moot point now. I reverted this function to the
original form.

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.h
File remoting/base/util.h (right):

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.h#ne...
remoting/base/util.h:22: // destination buffers are assumed to contain only
|source_clip| and |dest_clip|
On 2012/02/02 01:35:29, Wez wrote:
> Why have |dest_clip| passed in, rather than letting the caller clip the
> |dest_rect| and then checking that?
> 
> Why have |source_clip] at all?

|source_clip| and |dest_clip| are for the case when buffers are smaller than the
whole image. We can use such buffers as long as the copied |dest_rect| fits
both.

It seems though that these names are confusing. I'll changed them.

https://chromiumcodereview.appspot.com/9320025/diff/1/remoting/base/util.h#ne...
remoting/base/util.h:79: void CopyRGB32Rect(const uint8* source_buffer,
On 2012/02/02 01:35:29, Wez wrote:
> This doesn't seem to be used anywhere?

This will be used by a later change. I decided to pack it to this change because
this function is RGB-to-RGB counterpart of YUV-to-RGB conversion routine above.

[alexeypa (please no reviews), 2012-02-03 20:03:13]

You had lots of CLs to view yesterday. Pinging to make sure mine is still on the
list. :-)

[Wez, 2012-02-03 22:40:35]

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.cc
File remoting/base/util.cc (right):

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.cc#newcode173
remoting/base/util.cc:173: int bottom = SkScalarCeilToInt(scale_y *
rect.bottom());
On 2012/02/02 21:33:03, alexeypa wrote:
> On 2012/02/02 01:35:29, Wez wrote:
> > (which might be float, depending on build settings)?
> 
> That was needed to support up-scaling at some point and my new code used this.
> But then I realized that the scaling routines that we have do not support
> upscaling anyway so it is a moot point now. I reverted this function to the
> original form.

I think the full-frame scaling routines support up-scaling, so we could probably
fall back to use those.  We didn't bother getting up-scaling working for the
sub-rect scaler because the client doesn't never scales up the host (unless you
use full page zoom...)

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.h
File remoting/base/util.h (right):

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.h#newcode22
remoting/base/util.h:22: // destination buffers are assumed to contain only
|source_clip| and |dest_clip|
On 2012/02/02 21:33:03, alexeypa wrote:
> On 2012/02/02 01:35:29, Wez wrote:
> > Why have |dest_clip| passed in, rather than letting the caller clip the
> > |dest_rect| and then checking that?
> > 
> > Why have |source_clip] at all?
> 
> |source_clip| and |dest_clip| are for the case when buffers are smaller than
the
> whole image. We can use such buffers as long as the copied |dest_rect| fits
> both.
> 
> It seems though that these names are confusing. I'll changed them.

So |source_clip| and |dest_clip| indicate the actual source and destination
buffer dimensions, and |source_size| and |dest_size| the dimensions of the
logical input and output areas.  "clip" seems a reasonable term.

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.h#newcode29
remoting/base/util.h:29: // rectangle fits the corresponding clipping areas.
nit: Usually you'd phrase these as pre-conditions of the API, e.g. "The source
and destination clip rectangles must fall entirely within the source or
destination dimensions, respectively" - the fact that you'll DCHECK if they
don't is not part of the API contract, just a sanity-check.

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.h#newcode79
remoting/base/util.h:79: void CopyRGB32Rect(const uint8* source_buffer,
On 2012/02/02 21:33:03, alexeypa wrote:
> On 2012/02/02 01:35:29, Wez wrote:
> > This doesn't seem to be used anywhere?
> 
> This will be used by a later change. I decided to pack it to this change
because
> this function is RGB-to-RGB counterpart of YUV-to-RGB conversion routine
above.

OK, but please mention in the CL description that you've added it.

http://codereview.chromium.org/9320025/diff/10005/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/9320025/diff/10005/media/base/yuv_convert.cc#n...
media/base/yuv_convert.cc:340: source_width + 1);
Not sure why this needed to change?

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc
File remoting/base/util.cc (right):

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:108: DCHECK(source_buffer_rect ==
AlignRect(source_buffer_rect));
Don't CalculateYOffset() and CalculateUVOffset() already verify the alignment?

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:125: // ConvertYUVToRGB32().
Why are we calculating a smaller aligned rect than the destination?

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:151: // Now see if some pixels weren't copied due to
alignment.
Why not just align the top and left of the rectangle to even coordinates and
avoid all this complexity?

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:327: SkIRect::MakeWH(dest_rect.width(),
dest_rect.height()));
Is there no SkRect::size() method?

[alexeypa (please no reviews), 2012-02-03 23:29:35]

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.cc
File remoting/base/util.cc (right):

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.cc#newcode173
remoting/base/util.cc:173: int bottom = SkScalarCeilToInt(scale_y *
rect.bottom());
I guess we could do this if we decide to support up-scaling in the future. Right
now there is no code that would use this functionality.

On 2012/02/03 22:40:35, Wez wrote:
> On 2012/02/02 21:33:03, alexeypa wrote:
> > On 2012/02/02 01:35:29, Wez wrote:
> > > (which might be float, depending on build settings)?
> > 
> > That was needed to support up-scaling at some point and my new code used
this.
> > But then I realized that the scaling routines that we have do not support
> > upscaling anyway so it is a moot point now. I reverted this function to the
> > original form.
> 
> I think the full-frame scaling routines support up-scaling, so we could
probably
> fall back to use those.  We didn't bother getting up-scaling working for the
> sub-rect scaler because the client doesn't never scales up the host (unless
you
> use full page zoom...)

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.h
File remoting/base/util.h (right):

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.h#newcode29
remoting/base/util.h:29: // rectangle fits the corresponding clipping areas.
On 2012/02/03 22:40:35, Wez wrote:
> nit: Usually you'd phrase these as pre-conditions of the API, e.g. "The source
> and destination clip rectangles must fall entirely within the source or
> destination dimensions, respectively" - the fact that you'll DCHECK if they
> don't is not part of the API contract, just a sanity-check.

Done.

http://codereview.chromium.org/9320025/diff/1/remoting/base/util.h#newcode79
remoting/base/util.h:79: void CopyRGB32Rect(const uint8* source_buffer,
On 2012/02/03 22:40:35, Wez wrote:
> On 2012/02/02 21:33:03, alexeypa wrote:
> > On 2012/02/02 01:35:29, Wez wrote:
> > > This doesn't seem to be used anywhere?
> > 
> > This will be used by a later change. I decided to pack it to this change
> because
> > this function is RGB-to-RGB counterpart of YUV-to-RGB conversion routine
> above.
> 
> OK, but please mention in the CL description that you've added it.

Done.

http://codereview.chromium.org/9320025/diff/10005/media/base/yuv_convert.cc
File media/base/yuv_convert.cc (right):

http://codereview.chromium.org/9320025/diff/10005/media/base/yuv_convert.cc#n...
media/base/yuv_convert.cc:340: source_width + 1);
On 2012/02/03 22:40:35, Wez wrote:
> Not sure why this needed to change?

Because I hit this DCHECK on a legitimate call (Yuv2Rgb unit test hits it).
Because of +2 above the function can overrun the Y buffer. You can see that the
same guard for UV buffers.

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc
File remoting/base/util.cc (right):

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:108: DCHECK(source_buffer_rect ==
AlignRect(source_buffer_rect));
On 2012/02/03 22:40:35, Wez wrote:
> Don't CalculateYOffset() and CalculateUVOffset() already verify the alignment?

Done.

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:125: // ConvertYUVToRGB32().
We are not allowed to touch pixels outside of the region. That would be really
confusing for the caller. So the only way to copy an aligned rect by the
optimized routine is to copy a smaller one.

On 2012/02/03 22:40:35, Wez wrote:
> Why are we calculating a smaller aligned rect than the destination?

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:151: // Now see if some pixels weren't copied due to
alignment.
There are a few reasons I settled on this approach.

Alignment requirement is only a consequence of using ConvertYUVToRGB32() which
itself is used as a performance optimization only. Enforcing alignment when
scaling is performed would be very confusing for the caller because it does not
make any sense - the accesses to YUV buffer would be not aligned anyway.

On the other hand removing the alignment requirement reduces  caller's head ace
a lot. Now there is not need to track down 
 all the paths were rectangles are being changed. Performance-sensitive callers
still can align buffers to avoid any additional cost.

And at last doing it this way preserves the optimization because the bulk of
pixels is still copied by an optimized routine. (Which makes me think that the
condition choosing 1:1 or scaling should also see how large is the region that
we are coping and use scaling function to deal with small unaligned rectangles.


On 2012/02/03 22:40:35, Wez wrote:
> Why not just align the top and left of the rectangle to even coordinates and
> avoid all this complexity?

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:327: SkIRect::MakeWH(dest_rect.width(),
dest_rect.height()));
Nope. SkIRect.offset does not work as well because it would modify the object in
place.

On 2012/02/03 22:40:35, Wez wrote:
> Is there no SkRect::size() method?

[Wez, 2012-02-04 01:03:43]

OK, LGTM but see comments below.

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc
File remoting/base/util.cc (right):

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:125: // ConvertYUVToRGB32().
On 2012/02/03 23:29:35, alexeypa wrote:
> We are not allowed to touch pixels outside of the region. That would be really
> confusing for the caller. So the only way to copy an aligned rect by the
> optimized routine is to copy a smaller one.
> 
> On 2012/02/03 22:40:35, Wez wrote:
> > Why are we calculating a smaller aligned rect than the destination?
> 

I think it's reasonable to require the caller to arrange alignment correctly. 
In the non-scaling case it involves having a buffer that is at most 1 pixel
wider and higher than the clip area, and it greatly simplifies this code.

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:151: // Now see if some pixels weren't copied due to
alignment.
On 2012/02/03 23:29:35, alexeypa wrote:
> There are a few reasons I settled on this approach.
> 
> Alignment requirement is only a consequence of using ConvertYUVToRGB32() which
> itself is used as a performance optimization only. Enforcing alignment when
> scaling is performed would be very confusing for the caller because it does
not
> make any sense - the accesses to YUV buffer would be not aligned anyway.
> 
> On the other hand removing the alignment requirement reduces  caller's head
ace
> a lot. Now there is not need to track down 
>  all the paths were rectangles are being changed. Performance-sensitive
callers
> still can align buffers to avoid any additional cost.
> 
> And at last doing it this way preserves the optimization because the bulk of
> pixels is still copied by an optimized routine. (Which makes me think that the
> condition choosing 1:1 or scaling should also see how large is the region that
> we are coping and use scaling function to deal with small unaligned
rectangles.
> 
> 
> On 2012/02/03 22:40:35, Wez wrote:
> > Why not just align the top and left of the rectangle to even coordinates and
> > avoid all this complexity?
> 

Your point is that in the 1:1 case the caller needs to worry about alignment,
but not otherwise?  Fair point.

If you want to stick with this approach then you could simplify this code by
something like:

  SkRegion edges(original_rect);
  edges.op(aligned_inner_rect, kSubtract);
  for (SkIRect in edges)
    ScaleYUVToRGB32WithRect(...)

[alexeypa (please no reviews), 2012-02-06 05:45:16]

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc
File remoting/base/util.cc (right):

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:125: // ConvertYUVToRGB32().
I'll stick to this approach for now. However I'll re-evaluate the alignment
requirements when we will be switching to libyuv.

On 2012/02/04 01:03:43, Wez wrote:
> On 2012/02/03 23:29:35, alexeypa wrote:
> > We are not allowed to touch pixels outside of the region. That would be
really
> > confusing for the caller. So the only way to copy an aligned rect by the
> > optimized routine is to copy a smaller one.
> > 
> > On 2012/02/03 22:40:35, Wez wrote:
> > > Why are we calculating a smaller aligned rect than the destination?
> > 
> 
> I think it's reasonable to require the caller to arrange alignment correctly. 
> In the non-scaling case it involves having a buffer that is at most 1 pixel
> wider and higher than the clip area, and it greatly simplifies this code.

http://codereview.chromium.org/9320025/diff/10005/remoting/base/util.cc#newco...
remoting/base/util.cc:151: // Now see if some pixels weren't copied due to
alignment.
On 2012/02/04 01:03:43, Wez wrote:
> On 2012/02/03 23:29:35, alexeypa wrote:
> > There are a few reasons I settled on this approach.
> > 
> > Alignment requirement is only a consequence of using ConvertYUVToRGB32()
which
> > itself is used as a performance optimization only. Enforcing alignment when
> > scaling is performed would be very confusing for the caller because it does
> not
> > make any sense - the accesses to YUV buffer would be not aligned anyway.
> > 
> > On the other hand removing the alignment requirement reduces  caller's head
> ace
> > a lot. Now there is not need to track down 
> >  all the paths were rectangles are being changed. Performance-sensitive
> callers
> > still can align buffers to avoid any additional cost.
> > 
> > And at last doing it this way preserves the optimization because the bulk of
> > pixels is still copied by an optimized routine. (Which makes me think that
the
> > condition choosing 1:1 or scaling should also see how large is the region
that
> > we are coping and use scaling function to deal with small unaligned
> rectangles.
> > 
> > 
> > On 2012/02/03 22:40:35, Wez wrote:
> > > Why not just align the top and left of the rectangle to even coordinates
and
> > > avoid all this complexity?
> > 
> 
> Your point is that in the 1:1 case the caller needs to worry about alignment,
> but not otherwise?  Fair point.
> 
> If you want to stick with this approach then you could simplify this code by
> something like:
> 
>   SkRegion edges(original_rect);
>   edges.op(aligned_inner_rect, kSubtract);
>   for (SkIRect in edges)
>     ScaleYUVToRGB32WithRect(...)

Done.

[commit-bot: I haz the power, 2012-02-06 05:45:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9320025/11011

[commit-bot: I haz the power, 2012-02-06 07:29:43]

Try job failure for 9320025-11011 (retry) on mac_rel for step
"test_shell_tests".
It's a second try, previously, steps "test_shell_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-06 07:33:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9320025/11011

[commit-bot: I haz the power, 2012-02-06 07:40:56]

Try job failure for 9320025-11011 on linux_rel for step "update".
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-02-06 15:18:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9320025/11011

[commit-bot: I haz the power, 2012-02-06 17:14:11]

Try job failure for 9320025-11011 (retry) on mac_rel for steps
"test_shell_tests, ui_tests".
It's a second try, previously, steps "test_shell_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-02-06 18:02:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9320025/11011

[commit-bot: I haz the power, 2012-02-06 21:56:27]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is win_rel, revision is 120614, job name
was 9320025-11011 (retry) (previous was lost) (retry) (previous was lost).

[commit-bot: I haz the power, 2012-02-06 22:21:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9320025/11011

[commit-bot: I haz the power, 2012-02-07 03:23:47]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is mac_rel, revision is 120690, job name
was 9320025-11011 (previous was lost) (retry) (previous was lost) (retry).

[commit-bot: I haz the power, 2012-02-07 06:31:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/9320025/11011

[commit-bot: I haz the power, 2012-02-07 07:47:23]

Change committed as 120739