add new readPixels with direct memory parameters

add new readPixels with direct memory parameters

BUG=skia:

Committed: http://code.google.com/p/skia/source/detail?r=13840

[reed1, 2014-03-14 18:51:23]

WIP

[reed1, 2014-03-17 17:55:59]

ptal -- will land w/ guard for Chrome, until I can fix its call-sites

[bsalomon, 2014-03-17 18:19:47]

https://codereview.chromium.org/199413013/diff/120001/bench/PremulAndUnpremul...
File bench/PremulAndUnpremulAlphaOpsBench.cpp (left):

https://codereview.chromium.org/199413013/diff/120001/bench/PremulAndUnpremul...
bench/PremulAndUnpremulAlphaOpsBench.cpp:14: class
PremulAndUnpremulAlphaOpsBench : public SkBenchmark {
Removing this benchmark scares me a little bit. The GrContext read/write pixel
code that makes this fast is really tricky and easy to regress.

[reed1, 2014-03-17 18:47:09]

https://codereview.chromium.org/199413013/diff/120001/bench/PremulAndUnpremul...
File bench/PremulAndUnpremulAlphaOpsBench.cpp (left):

https://codereview.chromium.org/199413013/diff/120001/bench/PremulAndUnpremul...
bench/PremulAndUnpremulAlphaOpsBench.cpp:14: class
PremulAndUnpremulAlphaOpsBench : public SkBenchmark {
On 2014/03/17 18:19:47, bsalomon wrote:
> Removing this benchmark scares me a little bit. The GrContext read/write pixel
> code that makes this fast is really tricky and easy to regress.

Restored and updated

[reed1, 2014-03-17 18:47:19]

ptal

[bsalomon, 2014-03-17 19:00:06]

lgtm

[reed1, 2014-03-17 19:36:45]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-03-17 19:36:54]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/reed@google.com/199413013/140001

[commit-bot: I haz the power, 2014-03-17 19:37:14]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-17 19:37:19]

Failed to apply patch for src/core/SkDevice.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file src/core/SkDevice.cpp
  Hunk #3 FAILED at 176.
  1 out of 4 hunks FAILED -- saving rejects to file src/core/SkDevice.cpp.rej

Patch:       src/core/SkDevice.cpp
Index: src/core/SkDevice.cpp
diff --git a/src/core/SkDevice.cpp b/src/core/SkDevice.cpp
index
3cfe89cfeca9143b7b12acafdc53a16d241261fc..42a7cf9f7902a54fb68be6b22c0175055d36d025
100644
--- a/src/core/SkDevice.cpp
+++ b/src/core/SkDevice.cpp
@@ -112,6 +112,7 @@ const SkBitmap& SkBaseDevice::accessBitmap(bool
changePixels) {
     return bitmap;
 }
 
+#ifdef SK_SUPPORT_LEGACY_READPIXELSCONFIG
 bool SkBaseDevice::readPixels(SkBitmap* bitmap, int x, int y,
                               SkCanvas::Config8888 config8888) {
     if (SkBitmap::kARGB_8888_Config != bitmap->config() ||
@@ -154,6 +155,10 @@ bool SkBaseDevice::readPixels(SkBitmap* bitmap, int x, int
y,
     }
     return result;
 }
+bool SkBaseDevice::onReadPixels(const SkBitmap&, int x, int y,
SkCanvas::Config8888) {
+    return false;
+}
+#endif
 
 SkSurface* SkBaseDevice::newSurface(const SkImageInfo&) { return NULL; }
 
@@ -171,6 +176,20 @@ void SkBaseDevice::drawDRRect(const SkDraw& draw, const
SkRRect& outer,
     this->drawPath(draw, path, paint, preMatrix, pathIsMutable);
 }
 
+bool SkBaseDevice::readPixels(const SkImageInfo& info, void* dstP, size_t
rowBytes, int x, int y) {
+#ifdef SK_DEBUG
+    SkASSERT(info.width() > 0 && info.height() > 0);
+    SkASSERT(dstP);
+    SkASSERT(rowBytes >= info.minRowBytes());
+    SkASSERT(x >= 0 && y >= 0);
+    
+    const SkImageInfo& srcInfo = this->imageInfo();
+    SkASSERT(x + info.width() <= srcInfo.width());
+    SkASSERT(y + info.height() <= srcInfo.height());
+#endif
+    return this->onReadPixels(info, dstP, rowBytes, x, y);
+}
+
 bool SkBaseDevice::writePixelsDirect(const SkImageInfo& info, const void*
pixels, size_t rowBytes,
                                      int x, int y) {
 #ifdef SK_DEBUG
@@ -190,7 +209,7 @@ bool SkBaseDevice::onWritePixels(const SkImageInfo&, const
void*, size_t, int, i
     return false;
 }
 
-bool SkBaseDevice::onReadPixels(const SkBitmap&, int x, int y,
SkCanvas::Config8888) {
+bool SkBaseDevice::onReadPixels(const SkImageInfo&, void*, size_t, int x, int
y) {
     return false;
 }

[reed1, 2014-03-17 19:47:01]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-03-17 19:47:06]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/reed@google.com/199413013/20002

[commit-bot: I haz the power, 2014-03-17 19:56:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-17 19:56:49]

Retried try job too often on Build-Mac10.8-Clang-x86-Release-Trybot for step(s)
BuildBench, BuildEverything, BuildTools
http://108.170.219.164:10117/buildstatus?builder=Build-Mac10.8-Clang-x86-Rele...

[reed1, 2014-03-17 21:10:17]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-03-17 21:10:23]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/reed@google.com/199413013/140002

[commit-bot: I haz the power, 2014-03-17 21:31:39]

Change committed as 13840

[scroggo, 2014-03-24 15:31:36]

lgtm

https://codereview.chromium.org/199413013/diff/140002/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/199413013/diff/140002/include/core/SkDevice.h...
include/core/SkDevice.h:378: * Implements readPixels API. The caller will ensure
that:
Is this comment intended to apply to the new onReadPixels?

If so, it needs to be updated. Otherwise it should go inside the #ifdef

https://codereview.chromium.org/199413013/diff/140002/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/199413013/diff/140002/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:764: // here x,y are either 0 or negative
What's the use case for calling this function with x and y less than zero? It
appears we're assuming their pixels have been initialized (since we don't touch
some pixels)?

[reed1, 2014-03-24 15:53:05]

https://codereview.chromium.org/199413013/diff/140002/include/core/SkDevice.h
File include/core/SkDevice.h (right):

https://codereview.chromium.org/199413013/diff/140002/include/core/SkDevice.h...
include/core/SkDevice.h:378: * Implements readPixels API. The caller will ensure
that:
On 2014/03/24 15:31:36, scroggo wrote:
> Is this comment intended to apply to the new onReadPixels?
> 
> If so, it needs to be updated. Otherwise it should go inside the #ifdef

good point -- I'll move the comment into the ifdef

https://codereview.chromium.org/199413013/diff/140002/src/core/SkCanvas.cpp
File src/core/SkCanvas.cpp (right):

https://codereview.chromium.org/199413013/diff/140002/src/core/SkCanvas.cpp#n...
src/core/SkCanvas.cpp:764: // here x,y are either 0 or negative
On 2014/03/24 15:31:36, scroggo wrote:
> What's the use case for calling this function with x and y less than zero? It
> appears we're assuming their pixels have been initialized (since we don't
touch
> some pixels)?

We definitely support (in the public canvas version) requesting a rectangle that
is not strictly contained by the canvas' bounds. Thus we do assume that if the
caller ever does that, that they have initialized their memory ahead of time
(i.e. not our problem).

[scroggo, 2014-07-11 15:21:25]

https://codereview.chromium.org/199413013/diff/140002/bench/PremulAndUnpremul...
File bench/PremulAndUnpremulAlphaOpsBench.cpp (right):

https://codereview.chromium.org/199413013/diff/140002/bench/PremulAndUnpremul...
bench/PremulAndUnpremulAlphaOpsBench.cpp:39: *fBmp1.getAddr32(w, h) =
SkColorSetARGB(h & 0xFF, w & 0xFF, w & 0xFF, w & 0xFF);
Can't this result in an invalid premultiplied color? e.g. for the upper right
hand corner (w=W-1, h=0), the color will be:

a=0, r=255, g=255, h=255

I'm guessing this is the cause of
http://108.170.220.120:10117/builders/Test-Ubuntu12-ShuttleA-GTX660-x86-Debug...

[bsalomon, 2014-07-11 15:30:40]

https://codereview.chromium.org/199413013/diff/140002/bench/PremulAndUnpremul...
File bench/PremulAndUnpremulAlphaOpsBench.cpp (right):

https://codereview.chromium.org/199413013/diff/140002/bench/PremulAndUnpremul...
bench/PremulAndUnpremulAlphaOpsBench.cpp:39: *fBmp1.getAddr32(w, h) =
SkColorSetARGB(h & 0xFF, w & 0xFF, w & 0xFF, w & 0xFF);
On 2014/07/11 15:21:25, scroggo wrote:
> Can't this result in an invalid premultiplied color? e.g. for the upper right
> hand corner (w=W-1, h=0), the color will be:
> 
> a=0, r=255, g=255, h=255
> 
> I'm guessing this is the cause of
>
http://108.170.220.120:10117/builders/Test-Ubuntu12-ShuttleA-GTX660-x86-Debug...

Yeah, I think the intention was for bmp1 to be unpremul and bmp2 to be premul.
That would make the comments in the onDraw() loop make sense.

[scroggo, 2014-07-11 15:36:26]

On 2014/07/11 15:30:40, bsalomon wrote:
>
https://codereview.chromium.org/199413013/diff/140002/bench/PremulAndUnpremul...
> File bench/PremulAndUnpremulAlphaOpsBench.cpp (right):
> 
>
https://codereview.chromium.org/199413013/diff/140002/bench/PremulAndUnpremul...
> bench/PremulAndUnpremulAlphaOpsBench.cpp:39: *fBmp1.getAddr32(w, h) =
> SkColorSetARGB(h & 0xFF, w & 0xFF, w & 0xFF, w & 0xFF);
> On 2014/07/11 15:21:25, scroggo wrote:
> > Can't this result in an invalid premultiplied color? e.g. for the upper
right
> > hand corner (w=W-1, h=0), the color will be:
> > 
> > a=0, r=255, g=255, h=255
> > 
> > I'm guessing this is the cause of
> >
>
http://108.170.220.120:10117/builders/Test-Ubuntu12-ShuttleA-GTX660-x86-Debug...
> 
> Yeah, I think the intention was for bmp1 to be unpremul and bmp2 to be premul.
> That would make the comments in the onDraw() loop make sense.

In that case, shouldn't we be setting the SkColorType of bmp1 to kUnpremul? (It
also seems like the way we're
using SkColorSetARGB is a hack, since we don't actually want an SkColor.) I'm
wondering if Mike's change in
https://codereview.chromium.org/377303002/ had a side effect of attempting to do
a draw, exposing a bug in
this test.

[scroggo, 2014-07-11 17:16:25]

On 2014/07/11 15:36:26, scroggo wrote:
> On 2014/07/11 15:30:40, bsalomon wrote:
> >
>
https://codereview.chromium.org/199413013/diff/140002/bench/PremulAndUnpremul...
> > File bench/PremulAndUnpremulAlphaOpsBench.cpp (right):
> > 
> >
>
https://codereview.chromium.org/199413013/diff/140002/bench/PremulAndUnpremul...
> > bench/PremulAndUnpremulAlphaOpsBench.cpp:39: *fBmp1.getAddr32(w, h) =
> > SkColorSetARGB(h & 0xFF, w & 0xFF, w & 0xFF, w & 0xFF);
> > On 2014/07/11 15:21:25, scroggo wrote:
> > > Can't this result in an invalid premultiplied color? e.g. for the upper
> right
> > > hand corner (w=W-1, h=0), the color will be:
> > > 
> > > a=0, r=255, g=255, h=255
> > > 
> > > I'm guessing this is the cause of
> > >
> >
>
http://108.170.220.120:10117/builders/Test-Ubuntu12-ShuttleA-GTX660-x86-Debug...
> > 
> > Yeah, I think the intention was for bmp1 to be unpremul and bmp2 to be
premul.
> > That would make the comments in the onDraw() loop make sense.
> 
> In that case, shouldn't we be setting the SkColorType of bmp1 to kUnpremul?
(It
> also seems like the way we're
> using SkColorSetARGB is a hack, since we don't actually want an SkColor.) I'm
> wondering if Mike's change in
> https://codereview.chromium.org/377303002/ had a side effect of attempting to
do
> a draw, exposing a bug in
> this test.

Uploaded a fix in https://codereview.chromium.org/382243005/