new image from backend desc

new image from backend desc

BUG=485243
Committed: https://skia.googlesource.com/skia/+/8b26b99c97473f020df4b9d4ba789e074e06cedd

[reed1, 2015-05-01 19:33:32]

reed@google.com changed reviewers:
+ bsalomon@google.com, robertphillips@google.com

[reed1, 2015-05-01 19:33:32]

how can we finish the impl?
chrome could uses this in gl_renderer.cc so it doesn't have to create a
GrPixelRef ...

[reed1, 2015-05-05 21:18:51]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-05 21:19:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1121813002/80001

[commit-bot: I haz the power, 2015-05-05 21:25:17]

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

[commit-bot: I haz the power, 2015-05-05 21:25:18]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-05-07 16:15:43]

ptal

[reed1, 2015-05-07 16:15:52]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-07 16:16:14]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1121813002/100001

[commit-bot: I haz the power, 2015-05-07 16:17:27]

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

[commit-bot: I haz the power, 2015-05-07 16:17:28]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[reed1, 2015-05-07 16:22:54]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-07 16:23:10]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1121813002/120001

[commit-bot: I haz the power, 2015-05-07 16:29:12]

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

[commit-bot: I haz the power, 2015-05-07 16:29:13]

Dry run: This issue passed the CQ dry run.

[robertphillips, 2015-05-07 16:50:04]

https://codereview.chromium.org/1121813002/diff/120001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/1121813002/diff/120001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1754: 
Move the #include up ?
Do we even need this ?

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:37: SkShader*
SkImage_Gpu::onNewShader(SkShader::TileMode tileX,
line these guys up?

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:89: 
So, where is the copy happening?

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu.h
File src/image/SkImage_Gpu.h (right):

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.h:21: 
Maybe a comment as to why 'w' & 'h' can differ from tex->width() & tex->height ?

[bsalomon, 2015-05-07 17:05:57]

great to see the bitmapiness of SkImage_Gpu is going away.

https://codereview.chromium.org/1121813002/diff/120001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/1121813002/diff/120001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1749: void GrWrapTextureInBitmap(GrTexture* src, int w,
int h, bool isOpaque, SkBitmap* dst) {
This code should maybe move to SkGr.h/cpp where there are other utilities for
converting between sk and gr things.

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:24: GrContext* ctx = tex->getContext();
This actually can fail if the GrContext was told to abandon (e.g. the GL context
went away).

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:93: }
Yeah, I think you need a GrContext::copySurface call here with
kFlushWrites_PixelOp to ensure the copy is issued to GL before returning.

[reed1, 2015-05-07 17:49:12]

https://codereview.chromium.org/1121813002/diff/120001/src/gpu/SkGpuDevice.cpp
File src/gpu/SkGpuDevice.cpp (right):

https://codereview.chromium.org/1121813002/diff/120001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1749: void GrWrapTextureInBitmap(GrTexture* src, int w,
int h, bool isOpaque, SkBitmap* dst) {
On 2015/05/07 17:05:56, bsalomon wrote:
> This code should maybe move to SkGr.h/cpp where there are other utilities for
> converting between sk and gr things.

Done.

https://codereview.chromium.org/1121813002/diff/120001/src/gpu/SkGpuDevice.cp...
src/gpu/SkGpuDevice.cpp:1754: 
On 2015/05/07 16:50:03, robertphillips wrote:
> Move the #include up ?
> Do we even need this ?

Done.

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:24: GrContext* ctx = tex->getContext();
On 2015/05/07 17:05:56, bsalomon wrote:
> This actually can fail if the GrContext was told to abandon (e.g. the GL
context
> went away).

Done.

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:37: SkShader*
SkImage_Gpu::onNewShader(SkShader::TileMode tileX,
On 2015/05/07 16:50:04, robertphillips wrote:
> line these guys up?

Done.

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:89: 
On 2015/05/07 16:50:04, robertphillips wrote:
> So, where is the copy happening?

Done.

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:93: }
On 2015/05/07 17:05:56, bsalomon wrote:
> Yeah, I think you need a GrContext::copySurface call here with
> kFlushWrites_PixelOp to ensure the copy is issued to GL before returning.

Done.

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu.h
File src/image/SkImage_Gpu.h (right):

https://codereview.chromium.org/1121813002/diff/120001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.h:21: 
On 2015/05/07 16:50:04, robertphillips wrote:
> Maybe a comment as to why 'w' & 'h' can differ from tex->width() & tex->height
?

Done.

[bsalomon, 2015-05-07 17:57:29]

https://codereview.chromium.org/1121813002/diff/140001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/1121813002/diff/140001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:95: if (!ctx->copySurface(dst, src)) {
I think you want to pass kFlushWrites_PixelOp to ensure that the copy has been
issued to GL after before copySurface has returned.

[reed1, 2015-05-07 19:39:30]

ptal

https://codereview.chromium.org/1121813002/diff/140001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/1121813002/diff/140001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:95: if (!ctx->copySurface(dst, src)) {
On 2015/05/07 17:57:29, bsalomon wrote:
> I think you want to pass kFlushWrites_PixelOp to ensure that the copy has been
> issued to GL after before copySurface has returned.

Done.

[reed1, 2015-05-07 19:39:40]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-07 19:39:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1121813002/200001

[commit-bot: I haz the power, 2015-05-07 19:41:34]

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

[commit-bot: I haz the power, 2015-05-07 19:41:36]

Dry run: Try jobs failed on following builders:
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Debug-Trybot on client.skia (JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)
  Test-Ubuntu-GCC-GCE-CPU-AVX2-x86_64-Release-Shared-Trybot on client.skia
(JOB_FAILED,
http://build.chromium.org/p/client.skia/builders/Test-Ubuntu-GCC-GCE-CPU-AVX2...)

[bsalomon, 2015-05-07 19:52:23]

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cpp
File tests/GrSurfaceTest.cpp (right):

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cp...
tests/GrSurfaceTest.cpp:74: static SkImage* make_gpu_image(GrContext* ctx, int
w, int h, SkColor color) {
This file is really about testing GrSurface, not SkSurface. Maybe this belongs
in SurfaceTest.cpp?

There is already a similar cheat for wrapping a texture there in
test_wrapped_texture_surface

[robertphillips, 2015-05-07 20:01:30]

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:59: 
one line?

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:88: info.profileType());
init to 0 ?

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:89: uint32_t flags = GrContext::kFlushWrites_PixelOp;
Do we test this premul/unpremul case ?

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:90: if (kUnpremul_SkAlphaType == info.alphaType() &&
kPremul_SkAlphaType == fAlphaType) {
// Have the readPixels perform the conversion from premul to unpremul
?

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:97: // do we have to manually fix-up the alpha
channel?
Ganesh should also be able to do the unpremul to premul conversion in the read. 
Maybe add a TODO for Ganesh ?

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:137: // need to be a rendertarget for readpixels to
work
rm the dead '//' comment/code ?

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:144: 
overlength

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:151: }
Should we be passing the fSampleCnt parameter from srcDesc as SkImage_Gpu's
sampleCountForNewSurfaces parameter ?

[robertphillips, 2015-05-07 20:09:08]

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cpp
File tests/GrSurfaceTest.cpp (right):

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cp...
tests/GrSurfaceTest.cpp:106: 
// Test wrapping a GrBackendObject in a surface, rendering to it, and then
reading back

?

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cp...
tests/GrSurfaceTest.cpp:116: const SkColor srcColor = SK_ColorRED;
constify 'expected' too ?

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cp...
tests/GrSurfaceTest.cpp:128: 
// Test pulling a GrBackendObject from a GrTexture, wrapping it in a surface,
rendering to it and then reading back

?

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cp...
tests/GrSurfaceTest.cpp:159: test_tex_image(reporter, ctx, srcTex, w, h, true,
expected);
Re-scribble on 'tex' and readback again ?

[reed1, 2015-05-07 20:28:00]

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu.cpp
File src/image/SkImage_Gpu.cpp (right):

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:59: 
On 2015/05/07 20:01:29, robertphillips wrote:
> one line?

hmmm, is that a pseudo standard for our code (other than { return NULL; } ?

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:88: info.profileType());
On 2015/05/07 20:01:29, robertphillips wrote:
> init to 0 ?

Done.

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:89: uint32_t flags = GrContext::kFlushWrites_PixelOp;
On 2015/05/07 20:01:29, robertphillips wrote:
> Do we test this premul/unpremul case ?

Dunno for sure, but there is a gob of preexisting image tests.

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:90: if (kUnpremul_SkAlphaType == info.alphaType() &&
kPremul_SkAlphaType == fAlphaType) {
On 2015/05/07 20:01:29, robertphillips wrote:
> // Have the readPixels perform the conversion from premul to unpremul
> ?

Done.

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:97: // do we have to manually fix-up the alpha
channel?
On 2015/05/07 20:01:29, robertphillips wrote:
> Ganesh should also be able to do the unpremul to premul conversion in the
read. 
> Maybe add a TODO for Ganesh ?

Done.

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:137: // need to be a rendertarget for readpixels to
work
On 2015/05/07 20:01:29, robertphillips wrote:
> rm the dead '//' comment/code ?

Done.

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:144: 
On 2015/05/07 20:01:29, robertphillips wrote:
> overlength

Done.

https://codereview.chromium.org/1121813002/diff/200001/src/image/SkImage_Gpu....
src/image/SkImage_Gpu.cpp:151: }
On 2015/05/07 20:01:29, robertphillips wrote:
> Should we be passing the fSampleCnt parameter from srcDesc as SkImage_Gpu's
> sampleCountForNewSurfaces parameter ?

Done.

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cpp
File tests/GrSurfaceTest.cpp (right):

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cp...
tests/GrSurfaceTest.cpp:74: static SkImage* make_gpu_image(GrContext* ctx, int
w, int h, SkColor color) {
On 2015/05/07 19:52:22, bsalomon wrote:
> This file is really about testing GrSurface, not SkSurface. Maybe this belongs
> in SurfaceTest.cpp?
> 
> There is already a similar cheat for wrapping a texture there in
> test_wrapped_texture_surface

Done.

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cp...
tests/GrSurfaceTest.cpp:106: 
On 2015/05/07 20:09:08, robertphillips wrote:
> // Test wrapping a GrBackendObject in a surface, rendering to it, and then
> reading back
> 
> ?

Acknowledged.

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cp...
tests/GrSurfaceTest.cpp:116: const SkColor srcColor = SK_ColorRED;
On 2015/05/07 20:09:07, robertphillips wrote:
> constify 'expected' too ?

Done.

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cp...
tests/GrSurfaceTest.cpp:128: 
On 2015/05/07 20:09:08, robertphillips wrote:
> // Test pulling a GrBackendObject from a GrTexture, wrapping it in a surface,
> rendering to it and then reading back
> 
> ?

Acknowledged.

https://codereview.chromium.org/1121813002/diff/220001/tests/GrSurfaceTest.cp...
tests/GrSurfaceTest.cpp:159: test_tex_image(reporter, ctx, srcTex, w, h, true,
expected);
On 2015/05/07 20:09:08, robertphillips wrote:
> Re-scribble on 'tex' and readback again ?

Done.

[reed1, 2015-05-07 20:28:10]

The CQ bit was checked by reed@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-05-07 20:28:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1121813002/260001

[commit-bot: I haz the power, 2015-05-07 20:34:40]

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

[commit-bot: I haz the power, 2015-05-07 20:34:41]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-05-07 20:42:19]

ptal

[bsalomon, 2015-05-07 21:24:17]

lgtm

[reed1, 2015-05-07 21:38:54]

The CQ bit was checked by reed@google.com to run a CQ dry run

[reed1, 2015-05-07 21:38:54]

The patchset sent to the CQ was uploaded after l-g-t-m from bsalomon@google.com
Link to the patchset: https://codereview.chromium.org/1121813002/#ps280001
(title: "set samplecount to 0 (for now)")

[commit-bot: I haz the power, 2015-05-07 21:39:21]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1121813002/280001

[commit-bot: I haz the power, 2015-05-07 21:45:28]

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

[commit-bot: I haz the power, 2015-05-07 21:45:29]

Dry run: This issue passed the CQ dry run.

[reed1, 2015-05-07 22:35:37]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2015-05-07 22:36:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1121813002/280001

[commit-bot: I haz the power, 2015-05-07 22:36:29]

Committed patchset #15 (id:280001) as
https://skia.googlesource.com/skia/+/8b26b99c97473f020df4b9d4ba789e074e06cedd