captureVisibleTab Extension API producing garbled PNG results

captureVisibleTab Extension API producing garbled PNG results
Patch from Sam Larison <qufighter@gmail.com>;

new DCHECK will fail if rowBytes() on SkBitmap is too low

"Windows drivers sometimes require that scanlines (rows in the image) be memory aligned. That is why they are sometimes larger than they would strictly need to be."

Instead of attempting to compute row bytes, we should be using the rowBytes() provided by SkBitmap

BUG=176062
TEST=The test case involves using the "Capture Visible Content" feature of the Google Screen Capture extension on a newly loaded http:// page after installation of the extension.  The bug produces itself if after I size the window my window.innerWidth is 1000 or any other non-multiple of 16 (window.innerWidth % 16 != 0)

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186472

[Sam Larison, 2013-02-27 18:52:21]

For some additional context about this patch and why I think it is necessary:

http://stackoverflow.com/a/2004638

And the evidence of a problem is posted here:
https://code.google.com/p/chromium/issues/detail?id=176062

The bug is apparent in the dev channel, I am running 64 bit windows.  Not sure
how widespread.

No idea if this will fix it, but it makes sense that it might!

[Ivan Korotkov, 2013-02-27 19:02:26]

I'm not familiar with this code, suggesting some reviewers.
If the original bug reproduces for you -- please check if this CL really fixes
it.

[Alexei Svitkine (slow), 2013-02-27 19:06:07]

rowBytes() is a SkBitmap method, so adding reed@ as a reviewer.

Please put the BUG= and TEST= lines as the very last lines of your CL
description.

Is there a website that can be used to test whether this change fixes this? If
so, can you put it into your TEST= description.

[Alexei Svitkine (slow), 2013-03-01 15:45:48]

ping: Mike, can you take a look at this patch?

[Sam Larison, 2013-03-01 20:06:48]

Just in case anyone is trying to reproduce the bug I posted some additional
details.  If your window.innerWidth % 16 == 0 then the bug will not present
itself.

The test case involves using the "Capture Visible Content" feature of the Google
Screen Capture extension on a newly loaded http:// page after installation of
the extension.  The bug produces itself if after I size the window my
window.innerWidth is 1000 or any other non-multiple of 16

Is that too much to write in the test field?

I posted some more evidence in the BUG

Next I am going to confirm the patch fixes the bug... should have the results by
tomorrow.

Since the issue is only apparent in the dev channel, I am wondering if anyone
knows about some driver or directX type changes that happened recently which
could have triggered the page buffer to now be memory aligned.

[Sam Larison, 2013-03-01 21:09:30]

I was able to confirm that the patch fixes the bug (debug build 27.0.1424.0
(184717)) (thanks to the speed of incremental linking).  I tested out 150% dpi
too, since the function modified is also used for favicons.

[tony, 2013-03-04 22:01:12]

qufighter has signed the CLA.

This change looks OK to me, but I would like reed to confirm.

[tony, 2013-03-04 22:01:37]

Also, it should be pretty easy to write a unittest for this.

[Sam Larison, 2013-03-04 22:58:02]

On 2013/03/04 22:01:37, tony wrote:
> Also, it should be pretty easy to write a unittest for this.

I wanted to mention I don't require any attribution for this path, I was just
following instructions.  

I am thinking now the DCHECK line maybe unnecessary.

I was looking at one of the existing tests... the test image is 20x20 pixels
created by constructing an SkBitmap... however it's width * bpp is equal to it's
rowWidth.  To really test we need to construct a bitmap whose row width (or
stride) falls on 16 pixel (DWORD) boundaries while the width does not.  I
haven't looked into the feasibility of trying to fake this yet, although in
theory we can just construct a 32x20 bitmap and then modify it's width to be 20
(might be a private variable, making this difficult), and test the 20x20 area.

The existing tests do not use rowWidth() of the bitmap either, if for some
reason the SkBitmap got into the graphics card and then back into an SkBitmap
perhaps the existing tests would fail on some systems.  In some ways the test
seems silly because if we set it up to fail it will fail, if we set it up to
succeed it will succeed.  If we set it up to only pass some of the time, we will
only have confirmed the bug.

Using width * bitsPerPixel is just not the correct way in the first place,
because the stride (or rowWidth()) in this case accounts for the padding if the
pixels were for some reason memory aligned by driver hardware.

The best test I can imagine fires up the browser, adjusts the window size, draws
to a canvas on the page, then uses chrome.extension.captureVisibleTab and
examines the results, however that also seems very complex, I can write an
extension that does this however I don't know how to automate the test, is this
possible?

[Alexei Svitkine (slow), 2013-03-04 23:01:28]

I think it should be possible to write a unit test that constructs a SkBitmap
that currently doesn't work correctly with this code, but does with the fix and
use the PNGCodec to encode it and check the results.

Such a test should go to png_codec_unittest.cc.

[Sam Larison, 2013-03-05 03:07:24]

I have added a unit test!

With the patch applied it passes, otherwise fails.

out\Debug\ui_unittests.exe --gtest_filter=PNGCodec.*

Note: Google Test filter = PNGCodec.EncodeBGRASkBitmapStridePadded
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from PNGCodec
[ RUN      ] PNGCodec.EncodeBGRASkBitmapStridePadded
[       OK ] PNGCodec.EncodeBGRASkBitmapStridePadded (1 ms)
[----------] 1 test from PNGCodec (2 ms total)

[----------] Global test environment tear-down
[==========] 1 test from 1 test case ran. (4 ms total)
[  PASSED  ] 1 test.

[tony, 2013-03-05 18:20:41]

LGTM, but I am not an OWNER of this directory.

[Alexei Svitkine (slow), 2013-03-05 18:24:28]

(I'm an owner. Looking now.)

[Alexei Svitkine (slow), 2013-03-05 18:29:26]

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
File ui/gfx/codec/png_codec_unittest.cc (right):

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
ui/gfx/codec/png_codec_unittest.cc:974: const int w = 20, h = 20, padded_w = 32;
Separate lines please.

I suggest using constant naming convention for these, i.e. kWidth, kPaddedWidth,
etc.

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
ui/gfx/codec/png_codec_unittest.cc:984: for (int i = 0; i < padded_w * h; i++) {
Extract padded_w * h into a variable, e.g.

const int kBufferSize = kHeight * kPaddedWidth;

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
ui/gfx/codec/png_codec_unittest.cc:1002: uint32_t original_pixel =
original_bitmap.getAddr32(0, y)[x];
Can you do *original_bitmap.getAddr32(x, y) here?

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
ui/gfx/codec/png_codec_unittest.cc:1003: uint32_t decoded_pixel =
decoded_bitmap.getAddr32(0, y)[x];
ditto

[Sam Larison, 2013-03-05 20:30:41]

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
File ui/gfx/codec/png_codec_unittest.cc (right):

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
ui/gfx/codec/png_codec_unittest.cc:974: const int w = 20, h = 20, padded_w = 32;
On 2013/03/05 18:29:26, Alexei Svitkine wrote:
> Separate lines please.
> 
> I suggest using constant naming convention for these, i.e. kWidth,
kPaddedWidth,
> etc.

I have changed the naming conventions... except I only separated my new variable
kPaddedWidth onto it's own line, shall kWidth and kHeight each be on their own
lines too?

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
ui/gfx/codec/png_codec_unittest.cc:1002: uint32_t original_pixel =
original_bitmap.getAddr32(0, y)[x];
On 2013/03/05 18:29:26, Alexei Svitkine wrote:
> Can you do *original_bitmap.getAddr32(x, y) here?

I attempted doing so however this produced unsatisfactory results and the test
fails (making this change to other tests causes those to fail too), not exactly
sure why.  All the other tests do it this way.  Do you think that's okay?

I see unfortunately I missed at least one of the other changes... I will submit
another patch-set after I hear back about this.

[Alexei Svitkine (slow), 2013-03-05 21:35:23]

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
File ui/gfx/codec/png_codec_unittest.cc (right):

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
ui/gfx/codec/png_codec_unittest.cc:974: const int w = 20, h = 20, padded_w = 32;
On 2013/03/05 20:30:41, qufighter wrote:
> On 2013/03/05 18:29:26, Alexei Svitkine wrote:
> > Separate lines please.
> > 
> > I suggest using constant naming convention for these, i.e. kWidth,
> kPaddedWidth,
> > etc.
> 
> I have changed the naming conventions... except I only separated my new
variable
> kPaddedWidth onto it's own line, shall kWidth and kHeight each be on their own
> lines too?

Each on a separate line, please.

https://codereview.chromium.org/12313139/diff/13001/ui/gfx/codec/png_codec_un...
ui/gfx/codec/png_codec_unittest.cc:1002: uint32_t original_pixel =
original_bitmap.getAddr32(0, y)[x];
On 2013/03/05 20:30:41, qufighter wrote:
> On 2013/03/05 18:29:26, Alexei Svitkine wrote:
> > Can you do *original_bitmap.getAddr32(x, y) here?
> 
> I attempted doing so however this produced unsatisfactory results and the test
> fails (making this change to other tests causes those to fail too), not
exactly
> sure why.  All the other tests do it this way.  Do you think that's okay?
> 
> I see unfortunately I missed at least one of the other changes... I will
submit
> another patch-set after I hear back about this.

Okay, that's fine then.

[Sam Larison, 2013-03-05 22:24:00]

Updates to the unit test have been submitted.  I verified the unit test still
confirms the bug and proposed solution.

[Alexei Svitkine (slow), 2013-03-05 22:28:55]

Almost there.

https://codereview.chromium.org/12313139/diff/26001/ui/gfx/codec/png_codec_un...
File ui/gfx/codec/png_codec_unittest.cc (right):

https://codereview.chromium.org/12313139/diff/26001/ui/gfx/codec/png_codec_un...
ui/gfx/codec/png_codec_unittest.cc:978: int paddedPixels = kPaddedWidth *
kHeight;
Please use either the kConstant or unix_hacker naming convention here. Make the
var const as well.

I suggest appending "size" to the name, since "pixels" sounds like the actual
buffer.

https://codereview.chromium.org/12313139/diff/26001/ui/gfx/codec/png_codec_un...
ui/gfx/codec/png_codec_unittest.cc:979: int rowBytes = kPaddedWidth *
kBytesPerPixel;
Please use either the kConstant or unix_hacker naming convention here. Make the
var const as well.

[Sam Larison, 2013-03-05 22:49:48]

Should I send a reply to notify when posting updated patch sets?

I have made changes according to the suggestions.

For some reason when I first had multiplication result going into a const int
the test exited without any explanation... perhaps it was something else,
because it works now.

I have verified the test against the patch case again.

Perhaps kPaddedSize => kPaddedPixelCount 
although the pad area is not "pixels".

[Alexei Svitkine (slow), 2013-03-05 22:52:46]

LGTM

On 2013/03/05 22:49:48, qufighter wrote:
> Should I send a reply to notify when posting updated patch sets?

Yes, reviewers don't get notified otherwise.

[commit-bot: I haz the power, 2013-03-05 22:56:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qufighter@gmail.com/12313139/31001

[commit-bot: I haz the power, 2013-03-05 22:56:11]

Failed to apply patch for AUTHORS:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file AUTHORS
  Hunk #1 FAILED at 227.
  1 out of 1 hunk FAILED -- saving rejects to file AUTHORS.rej

Patch:       AUTHORS
Index: AUTHORS
diff --git a/AUTHORS b/AUTHORS
index
1e404bd6b097a6ce10574502d731f4cbac4cfb48..057a5f0d1ade5dacbc5611f98bbd4020e8f7855c
100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -227,3 +227,4 @@ Matheus Bratfisch <matheusbrat@gmail.com>
 Horia Olaru <olaru@adobe.com>
 Horia Olaru <horia.olaru@gmail.com>
 Opera Software ASA <*@opera.com>
+Sam Larison <qufighter@gmail.com>

[Alexei Svitkine (slow), 2013-03-05 22:57:44]

Looks like some of the files were updated since you sent out your CL.

Please gclient sync and post a new patchset.

[Sam Larison, 2013-03-06 00:08:13]

On 2013/03/05 22:57:44, Alexei Svitkine wrote:
> Looks like some of the files were updated since you sent out your CL.
> 
> Please gclient sync and post a new patchset.

Had to learn about git rebase... and had some trouble with fork in cygwin when
trying to fetch upstream... I think it will work now!

[commit-bot: I haz the power, 2013-03-06 15:10:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qufighter@gmail.com/12313139/35001

[commit-bot: I haz the power, 2013-03-06 15:24:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_rel_device.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[Sam Larison, 2013-03-06 15:56:50]

DCHECK line that I added was only compiling successfully on win_x64, removed the
line, not sure if casting should have been attempted instead.  I also think I
should not have checked the commit box.

[Sam Larison, 2013-03-06 16:20:49]

Added casting of input.rowBytes() 
Now mirrors JPEG side... as originally intended.

[commit-bot: I haz the power, 2013-03-06 16:25:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qufighter@gmail.com/12313139/52001

[Alexei Svitkine (slow), 2013-03-06 16:27:27]

On 2013/03/06 15:56:50, qufighter wrote:
> DCHECK line that I added was only compiling successfully on win_x64, removed
the
> line, not sure if casting should have been attempted instead.  I also think I
> should not have checked the commit box.

Please re-add the dcheck and cast things appropriately to make it compile
elsewhere.

[Sam Larison, 2013-03-06 16:39:58]

The dcheck is back in set 10, set 9 seems to be compiling properly elsewhere.

[commit-bot: I haz the power, 2013-03-06 16:40:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qufighter@gmail.com/12313139/58004

[commit-bot: I haz the power, 2013-03-06 19:06:58]

Change committed as 186472