Add SkCanvas::writePixels that takes info+pixels directly

Add SkCanvas::writePixels that takes info+pixels directly

add corresponding methods to device (w/ diff name to avoid colliding with exising virtuals)

BUG=skia:

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

[reed1, 2014-03-05 20:50:51]

still todo:
- add tests
- prepare path to deprecate the old api that uses Config8888

[bsalomon, 2014-03-05 21:08:31]

lgtm.. maybe the existing read/write tests can be extended to test the new APIs
(and then eventually have testing of the old APIs removed).

https://codereview.chromium.org/180113010/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/180113010/diff/1/include/core/SkCanvas.h#newc...
include/core/SkCanvas.h:306: bool writePixels(const SkImageInfo& info, const
void* pixels, size_t rowBytes) {
is this one useful?

[robertphillips, 2014-03-05 23:19:19]

https://codereview.chromium.org/180113010/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/180113010/diff/1/include/core/SkCanvas.h#newc...
include/core/SkCanvas.h:297: *  pixels, performing any colortype/alphatype
transformations needed (in the case where the
missing ')'

[reed1, 2014-03-06 16:49:48]

Still looking unittest issue in DeferredCanvas, but PTAL

btw - my read of chrome is that there is exactly 1 caller that passing in a
config8888, and that is in ImageBuffer in service of <canvas>. All other
callsites take the default arg (i.e. just native pmcolor).

https://codereview.chromium.org/180113010/diff/1/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/180113010/diff/1/include/core/SkCanvas.h#newc...
include/core/SkCanvas.h:297: *  pixels, performing any colortype/alphatype
transformations needed (in the case where the
On 2014/03/05 23:19:19, robertphillips wrote:
> missing ')'

Done.

https://codereview.chromium.org/180113010/diff/1/include/core/SkCanvas.h#newc...
include/core/SkCanvas.h:306: bool writePixels(const SkImageInfo& info, const
void* pixels, size_t rowBytes) {
On 2014/03/05 21:08:32, bsalomon wrote:
> is this one useful?

No. Removed.

[bsalomon, 2014-03-06 17:48:11]

https://codereview.chromium.org/180113010/diff/60001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/180113010/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:282: *  SkPicture, writePixels() is ignored!
DEPRECATED?

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

https://codereview.chromium.org/180113010/diff/60001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:743: // should we call this->flush() for the device?
I don't think so. GrContext knows to flush on a write if it thinks it has to.
Currently that's always, but it could be relaxed in the future.

[Justin Novosad, 2014-03-06 17:57:24]

https://codereview.chromium.org/180113010/diff/60001/src/utils/SkDeferredCanv...
File src/utils/SkDeferredCanvas.cpp (right):

https://codereview.chromium.org/180113010/diff/60001/src/utils/SkDeferredCanv...
src/utils/SkDeferredCanvas.cpp:519: if (info.width() == deviceInfo.width() &&
info.height() == deviceInfo.height()) {
This logic has a built-in assumption that the source image is pre-clipped. Is
that a correct assumption?  If not we need something more like what is in the
legacy writePixels. If this is correct, then perhaps a comment would be helpful.
 Something like: info is pre-clipped. Therefore, a match in size is a sufficient
condition to detect the complete occlusion of the canvas.

[reed1, 2014-03-06 20:21:29]

https://codereview.chromium.org/180113010/diff/60001/include/core/SkCanvas.h
File include/core/SkCanvas.h (right):

https://codereview.chromium.org/180113010/diff/60001/include/core/SkCanvas.h#...
include/core/SkCanvas.h:282: *  SkPicture, writePixels() is ignored!
On 2014/03/06 17:48:12, bsalomon wrote:
> DEPRECATED?

Done.

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

https://codereview.chromium.org/180113010/diff/60001/src/core/SkCanvas.cpp#ne...
src/core/SkCanvas.cpp:743: // should we call this->flush() for the device?
On 2014/03/06 17:48:12, bsalomon wrote:
> I don't think so. GrContext knows to flush on a write if it thinks it has to.
> Currently that's always, but it could be relaxed in the future.

Done.

https://codereview.chromium.org/180113010/diff/60001/src/utils/SkDeferredCanv...
File src/utils/SkDeferredCanvas.cpp (right):

https://codereview.chromium.org/180113010/diff/60001/src/utils/SkDeferredCanv...
src/utils/SkDeferredCanvas.cpp:519: if (info.width() == deviceInfo.width() &&
info.height() == deviceInfo.height()) {
On 2014/03/06 17:57:24, junov wrote:
> This logic has a built-in assumption that the source image is pre-clipped. Is
> that a correct assumption?  If not we need something more like what is in the
> legacy writePixels. If this is correct, then perhaps a comment would be
helpful.
>  Something like: info is pre-clipped. Therefore, a match in size is a
sufficient
> condition to detect the complete occlusion of the canvas.

I will update the above asserts to make that assumption clearer, and I will add
dox to SkBaseDevice.h that state this.

[reed1, 2014-03-06 20:39:17]

Note:

I disabled 4 tests in DeferredCanvasTest. These (afaik) were looking for
possible short-cuts in the impl, where a small bitmap that was also immutable
would be deferred (as a drawSprite) rather than passing through to writePixels.

The new model for writePixels is that it is only ever given raw pointers, so
such a check would always fail anyway (there is no pixelref to be marked
immutable). The old code assumed that there was no clipping, as drawSprite
respects the clip but writePixels does not.

Anyway, I wanted to call out this difference. If deferred is being used for
<canvas>, this may be no loss, since the call-site in ImageBuffer.cpp never has
an immutable pixelref, so this code-path could not have been taken.

[Justin Novosad, 2014-03-06 21:32:27]

lgtm for the deferred canvas stuff

[bsalomon, 2014-03-06 21:59:52]

On 2014/03/06 21:32:27, junov wrote:
> lgtm for the deferred canvas stuff

lgtm for the other stuff

[reed1, 2014-03-06 22:14:33]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-03-06 22:14:44]

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

[commit-bot: I haz the power, 2014-03-07 03:25:24]

Change committed as 13697