cc: Adding support for RGBA_4444 tile textures

Adding support for RGBA_4444 tile textures

BUG=245774, 272539
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=223830

[Sami, 2013-08-08 10:11:01]

Hi Kaan, thanks for looking at this. Some suggestions below.

https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:158: canvas.drawBitmap(bitmap_16, 0, 0);
|canvas| is still a 32-bit bitmap so this is really just going from 32 => 16 =>
32 bits, i.e., throwing away precision :) I think we need to add a way to make
PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which isn't supported by
Skia AFAIK).

https://codereview.chromium.org/21159007/diff/11001/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/11001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1323: if (use_texture_storage_ext_ &&
IsTextureFormatSupportedForStorage(format)) {
glTexStorage2D should also work with RGBA4_OES so try adding that to the list in
IsTextureFormatSupportedForStorage().

https://codereview.chromium.org/21159007/diff/11001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1340: texture_data_type,
This parameter only sets the format of the data that is passed into
glTexImage2D. To set the format of the texture itself, we need to set |format|
to RGBA4_OES. Probably the best way to do that is to initialize
best_texture_format_ to RGBA4_OES.

There are also a bunch of assumptions in ResourceProvider that sizeof(pixel) ==
4 which we'd need to fix.

https://codereview.chromium.org/21159007/diff/11001/gpu/command_buffer/servic...
File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right):

https://codereview.chromium.org/21159007/diff/11001/gpu/command_buffer/servic...
gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:55:
!base::android::SysUtils::IsLowEndDevice()) {
Just curious: was it the EGLImage creation or texture specification using the
EGLImage that failed?

https://codereview.chromium.org/21159007/diff/11001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/21159007/diff/11001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:186: DCHECK_EQ(5, red_size);
The implementation might not support RGB565 so checking exactly for that is
probably too strict. We might get a RGBA4444 or even RGBA8888 surface.

[kaanb, 2013-08-08 17:25:05]

https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:158: canvas.drawBitmap(bitmap_16, 0, 0);
On 2013/08/08 10:11:01, Sami wrote:
> |canvas| is still a 32-bit bitmap so this is really just going from 32 => 16
=>
> 32 bits, i.e., throwing away precision :) I think we need to add a way to make
> PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which isn't supported
by
> Skia AFAIK).

Let me check with Skia folks if they'd be willing to make a shortcut for us
here.

https://codereview.chromium.org/21159007/diff/11001/cc/resources/resource_pro...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/11001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1323: if (use_texture_storage_ext_ &&
IsTextureFormatSupportedForStorage(format)) {
On 2013/08/08 10:11:01, Sami wrote:
> glTexStorage2D should also work with RGBA4_OES so try adding that to the list
in
> IsTextureFormatSupportedForStorage().

I'm getting black tiles when I change the internalformat parameter of
glTexImage2D to RGBA4_OES and dtrainor@ noticed the same when he changed it for
the browser compositor. Could it be possible that we're not supporting the
extension that adds RGBA4_OES?

https://codereview.chromium.org/21159007/diff/11001/cc/resources/resource_pro...
cc/resources/resource_provider.cc:1340: texture_data_type,
On 2013/08/08 10:11:01, Sami wrote:
> This parameter only sets the format of the data that is passed into
> glTexImage2D. To set the format of the texture itself, we need to set |format|
> to RGBA4_OES. Probably the best way to do that is to initialize
> best_texture_format_ to RGBA4_OES.
> 
> There are also a bunch of assumptions in ResourceProvider that sizeof(pixel)
==
> 4 which we'd need to fix.

See the comment on RGBA4_OES above.

[bsalomon, 2013-08-08 18:40:15]

https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:158: canvas.drawBitmap(bitmap_16, 0, 0);
On 2013/08/08 17:25:05, kaanb wrote:
> On 2013/08/08 10:11:01, Sami wrote:
> > |canvas| is still a 32-bit bitmap so this is really just going from 32 => 16
> =>
> > 32 bits, i.e., throwing away precision :) I think we need to add a way to
make
> > PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which isn't supported
> by
> > Skia AFAIK).
> 
> Let me check with Skia folks if they'd be willing to make a shortcut for us
> here.

Hi, What's the intent here? Are you trying to go from a 16bit sw bitmap to a
16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a texture backed bitmap?

Also, I'm not sure how much it matters but it looks like accessBitmap() could
take false for the "changePixels" param since you are just reading from it.

[kaanb, 2013-08-08 18:46:37]

On 2013/08/08 18:40:15, bsalomon wrote:
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> File cc/resources/raster_worker_pool.cc (right):
> 
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> cc/resources/raster_worker_pool.cc:158: canvas.drawBitmap(bitmap_16, 0, 0);
> On 2013/08/08 17:25:05, kaanb wrote:
> > On 2013/08/08 10:11:01, Sami wrote:
> > > |canvas| is still a 32-bit bitmap so this is really just going from 32 =>
16
> > =>
> > > 32 bits, i.e., throwing away precision :) I think we need to add a way to
> make
> > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which isn't
supported
> > by
> > > Skia AFAIK).
> > 
> > Let me check with Skia folks if they'd be willing to make a shortcut for us
> > here.
> 
> Hi, What's the intent here? Are you trying to go from a 16bit sw bitmap to a
> 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a texture backed bitmap?
> 
> Also, I'm not sure how much it matters but it looks like accessBitmap() could
> take false for the "changePixels" param since you are just reading from it.

Yes, I eventually want to upload the 16bit sw bitmap to a 16bit texture.
bitmap_32 is created here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...

The buffer_ field that's backing the bitmap is allocated here:
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...

[bsalomon, 2013-08-08 19:01:22]

On 2013/08/08 18:46:37, kaanb wrote:
> On 2013/08/08 18:40:15, bsalomon wrote:
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > File cc/resources/raster_worker_pool.cc (right):
> > 
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > cc/resources/raster_worker_pool.cc:158: canvas.drawBitmap(bitmap_16, 0, 0);
> > On 2013/08/08 17:25:05, kaanb wrote:
> > > On 2013/08/08 10:11:01, Sami wrote:
> > > > |canvas| is still a 32-bit bitmap so this is really just going from 32
=>
> 16
> > > =>
> > > > 32 bits, i.e., throwing away precision :) I think we need to add a way
to
> > make
> > > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which isn't
> supported
> > > by
> > > > Skia AFAIK).
> > > 
> > > Let me check with Skia folks if they'd be willing to make a shortcut for
us
> > > here.
> > 
> > Hi, What's the intent here? Are you trying to go from a 16bit sw bitmap to a
> > 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a texture backed
bitmap?
> > 
> > Also, I'm not sure how much it matters but it looks like accessBitmap()
could
> > take false for the "changePixels" param since you are just reading from it.
> 
> Yes, I eventually want to upload the 16bit sw bitmap to a 16bit texture.
> bitmap_32 is created here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
> 
> The buffer_ field that's backing the bitmap is allocated here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...

So would it be correct to say that you're not looking to use Skia to upload the
16bit sw bitmap to a texture but rather the code that does so will extract the
bitmap backing the canvas (or really the canvas's device)? And so the
drawBitmap(bitmap_16) call is attempting to replace the canvas/device's 32 bit
bitmap with a 16 bit one?

[kaanb, 2013-08-08 19:27:35]

On 2013/08/08 19:01:22, bsalomon wrote:
> On 2013/08/08 18:46:37, kaanb wrote:
> > On 2013/08/08 18:40:15, bsalomon wrote:
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > File cc/resources/raster_worker_pool.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > cc/resources/raster_worker_pool.cc:158: canvas.drawBitmap(bitmap_16, 0,
0);
> > > On 2013/08/08 17:25:05, kaanb wrote:
> > > > On 2013/08/08 10:11:01, Sami wrote:
> > > > > |canvas| is still a 32-bit bitmap so this is really just going from 32
> =>
> > 16
> > > > =>
> > > > > 32 bits, i.e., throwing away precision :) I think we need to add a way
> to
> > > make
> > > > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which isn't
> > supported
> > > > by
> > > > > Skia AFAIK).
> > > > 
> > > > Let me check with Skia folks if they'd be willing to make a shortcut for
> us
> > > > here.
> > > 
> > > Hi, What's the intent here? Are you trying to go from a 16bit sw bitmap to
a
> > > 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a texture backed
> bitmap?
> > > 
> > > Also, I'm not sure how much it matters but it looks like accessBitmap()
> could
> > > take false for the "changePixels" param since you are just reading from
it.
> > 
> > Yes, I eventually want to upload the 16bit sw bitmap to a 16bit texture.
> > bitmap_32 is created here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
> > 
> > The buffer_ field that's backing the bitmap is allocated here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...
> 
> So would it be correct to say that you're not looking to use Skia to upload
the
> 16bit sw bitmap to a texture but rather the code that does so will extract the
> bitmap backing the canvas (or really the canvas's device)? And so the
> drawBitmap(bitmap_16) call is attempting to replace the canvas/device's 32 bit
> bitmap with a 16 bit one?

Yes, your statement is correct.

[bsalomon, 2013-08-08 19:52:51]

On 2013/08/08 19:27:35, kaanb wrote:
> On 2013/08/08 19:01:22, bsalomon wrote:
> > On 2013/08/08 18:46:37, kaanb wrote:
> > > On 2013/08/08 18:40:15, bsalomon wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > File cc/resources/raster_worker_pool.cc (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > cc/resources/raster_worker_pool.cc:158: canvas.drawBitmap(bitmap_16, 0,
> 0);
> > > > On 2013/08/08 17:25:05, kaanb wrote:
> > > > > On 2013/08/08 10:11:01, Sami wrote:
> > > > > > |canvas| is still a 32-bit bitmap so this is really just going from
32
> > =>
> > > 16
> > > > > =>
> > > > > > 32 bits, i.e., throwing away precision :) I think we need to add a
way
> > to
> > > > make
> > > > > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which isn't
> > > supported
> > > > > by
> > > > > > Skia AFAIK).
> > > > > 
> > > > > Let me check with Skia folks if they'd be willing to make a shortcut
for
> > us
> > > > > here.
> > > > 
> > > > Hi, What's the intent here? Are you trying to go from a 16bit sw bitmap
to
> a
> > > > 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a texture backed
> > bitmap?
> > > > 
> > > > Also, I'm not sure how much it matters but it looks like accessBitmap()
> > could
> > > > take false for the "changePixels" param since you are just reading from
> it.
> > > 
> > > Yes, I eventually want to upload the 16bit sw bitmap to a 16bit texture.
> > > bitmap_32 is created here:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
> > > 
> > > The buffer_ field that's backing the bitmap is allocated here:
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...
> > 
> > So would it be correct to say that you're not looking to use Skia to upload
> the
> > 16bit sw bitmap to a texture but rather the code that does so will extract
the
> > bitmap backing the canvas (or really the canvas's device)? And so the
> > drawBitmap(bitmap_16) call is attempting to replace the canvas/device's 32
bit
> > bitmap with a 16 bit one?
> 
> Yes, your statement is correct.

Ok, so you won't be able to attach a 4444 bitmap behind a canvas anymore (now
that they are no longer renderable in Skia). Is it possible to make the consumer
of the sw bitmap (the code that does the upload) take a SkBitmap rather than an
SkCanvas or SkDevice? This seems more correct, anyway, as SkCanvas is a
abstraction for a rendering destination (and may be actually backed by a
SkPicture/SkPipe/PDF document/etc) while SkBitmap is a pixel rectangle.

[kaanb, 2013-08-08 22:09:53]

On 2013/08/08 19:52:51, bsalomon wrote:
> On 2013/08/08 19:27:35, kaanb wrote:
> > On 2013/08/08 19:01:22, bsalomon wrote:
> > > On 2013/08/08 18:46:37, kaanb wrote:
> > > > On 2013/08/08 18:40:15, bsalomon wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > File cc/resources/raster_worker_pool.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > cc/resources/raster_worker_pool.cc:158: canvas.drawBitmap(bitmap_16,
0,
> > 0);
> > > > > On 2013/08/08 17:25:05, kaanb wrote:
> > > > > > On 2013/08/08 10:11:01, Sami wrote:
> > > > > > > |canvas| is still a 32-bit bitmap so this is really just going
from
> 32
> > > =>
> > > > 16
> > > > > > =>
> > > > > > > 32 bits, i.e., throwing away precision :) I think we need to add a
> way
> > > to
> > > > > make
> > > > > > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which isn't
> > > > supported
> > > > > > by
> > > > > > > Skia AFAIK).
> > > > > > 
> > > > > > Let me check with Skia folks if they'd be willing to make a shortcut
> for
> > > us
> > > > > > here.
> > > > > 
> > > > > Hi, What's the intent here? Are you trying to go from a 16bit sw
bitmap
> to
> > a
> > > > > 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a texture backed
> > > bitmap?
> > > > > 
> > > > > Also, I'm not sure how much it matters but it looks like
accessBitmap()
> > > could
> > > > > take false for the "changePixels" param since you are just reading
from
> > it.
> > > > 
> > > > Yes, I eventually want to upload the 16bit sw bitmap to a 16bit texture.
> > > > bitmap_32 is created here:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
> > > > 
> > > > The buffer_ field that's backing the bitmap is allocated here:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...
> > > 
> > > So would it be correct to say that you're not looking to use Skia to
upload
> > the
> > > 16bit sw bitmap to a texture but rather the code that does so will extract
> the
> > > bitmap backing the canvas (or really the canvas's device)? And so the
> > > drawBitmap(bitmap_16) call is attempting to replace the canvas/device's 32
> bit
> > > bitmap with a 16 bit one?
> > 
> > Yes, your statement is correct.
> 
> Ok, so you won't be able to attach a 4444 bitmap behind a canvas anymore (now
> that they are no longer renderable in Skia). Is it possible to make the
consumer
> of the sw bitmap (the code that does the upload) take a SkBitmap rather than
an
> SkCanvas or SkDevice? This seems more correct, anyway, as SkCanvas is a
> abstraction for a rendering destination (and may be actually backed by a
> SkPicture/SkPipe/PDF document/etc) while SkBitmap is a pixel rectangle.

I've copied reveman@ who owns the texture upload codepath and can answer your
question.

[kaanb, 2013-08-08 23:30:44]

https://codereview.chromium.org/21159007/diff/11001/gpu/command_buffer/servic...
File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right):

https://codereview.chromium.org/21159007/diff/11001/gpu/command_buffer/servic...
gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:55:
!base::android::SysUtils::IsLowEndDevice()) {
On 2013/08/08 10:11:01, Sami wrote:
> Just curious: was it the EGLImage creation or texture specification using the
> EGLImage that failed?

EGLImage creation failed:
W/Adreno200-ES20( 2380): <oglLinkEGLImage2GLTexture:1921>: GL_INVALID_VALUE
F/chromium( 2380): [FATAL:async_pixel_transfer_manager_egl.cc(206)] Check
failed: ((EGLImageKHR)0) != egl_image_ (0x00000000 vs. 0x00000000)

[kaanb, 2013-08-09 02:10:46]

On 2013/08/08 19:52:51, bsalomon wrote:
> On 2013/08/08 19:27:35, kaanb wrote:
> > On 2013/08/08 19:01:22, bsalomon wrote:
> > > On 2013/08/08 18:46:37, kaanb wrote:
> > > > On 2013/08/08 18:40:15, bsalomon wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > File cc/resources/raster_worker_pool.cc (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > cc/resources/raster_worker_pool.cc:158: canvas.drawBitmap(bitmap_16,
0,
> > 0);
> > > > > On 2013/08/08 17:25:05, kaanb wrote:
> > > > > > On 2013/08/08 10:11:01, Sami wrote:
> > > > > > > |canvas| is still a 32-bit bitmap so this is really just going
from
> 32
> > > =>
> > > > 16
> > > > > > =>
> > > > > > > 32 bits, i.e., throwing away precision :) I think we need to add a
> way
> > > to
> > > > > make
> > > > > > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which isn't
> > > > supported
> > > > > > by
> > > > > > > Skia AFAIK).
> > > > > > 
> > > > > > Let me check with Skia folks if they'd be willing to make a shortcut
> for
> > > us
> > > > > > here.
> > > > > 
> > > > > Hi, What's the intent here? Are you trying to go from a 16bit sw
bitmap
> to
> > a
> > > > > 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a texture backed
> > > bitmap?
> > > > > 
> > > > > Also, I'm not sure how much it matters but it looks like
accessBitmap()
> > > could
> > > > > take false for the "changePixels" param since you are just reading
from
> > it.
> > > > 
> > > > Yes, I eventually want to upload the 16bit sw bitmap to a 16bit texture.
> > > > bitmap_32 is created here:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
> > > > 
> > > > The buffer_ field that's backing the bitmap is allocated here:
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...
> > > 
> > > So would it be correct to say that you're not looking to use Skia to
upload
> > the
> > > 16bit sw bitmap to a texture but rather the code that does so will extract
> the
> > > bitmap backing the canvas (or really the canvas's device)? And so the
> > > drawBitmap(bitmap_16) call is attempting to replace the canvas/device's 32
> bit
> > > bitmap with a 16 bit one?
> > 
> > Yes, your statement is correct.
> 
> Ok, so you won't be able to attach a 4444 bitmap behind a canvas anymore (now
> that they are no longer renderable in Skia). Is it possible to make the
consumer
> of the sw bitmap (the code that does the upload) take a SkBitmap rather than
an
> SkCanvas or SkDevice? This seems more correct, anyway, as SkCanvas is a
> abstraction for a rendering destination (and may be actually backed by a
> SkPicture/SkPipe/PDF document/etc) while SkBitmap is a pixel rectangle.

Actually the code that does the upload doesn't deal with Skia (it's in
ResourceProvider::BeginSetPixels()). We construct a bitmap backed by a
PixelBuffer (created in ResourceProvider::AcquirePixelBuffer() and mapped in
ResourceProvider::MapPixelBuffer()) and rasterize into it. 

I talked to reveman and he suggested I try something like I uploaded where we
create a 16-bit device from the pixelBuffer, within the raster task create a
temporary 32-bit device, rasterize into it and copy over to 16-bit device at the
end. However I'm hitting an assert with the following stack (I had commented out
the assert in SkDevice constructor that checks for 4444)

  0192642d  SkBlitter::Choose(SkBitmap const&, SkMatrix const&, SkPaint const&,
void*, unsigned int) 
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkBlitter.cpp:980
  v------>  SkAutoBlitterChoose                                                 
                    
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:64
  010ebb8f  SkDraw::drawRect(SkRect const&, SkPaint const&) const               
                    
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:887
  01930bf3  SkDraw::drawBitmap(SkBitmap const&, SkMatrix const&, SkPaint const&)
const               
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:1327
  010e5767  SkCanvas::internalDrawBitmap(SkBitmap const&, SkMatrix const&,
SkPaint const*)           
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:975
  010e585d  SkCanvas::drawBitmap(SkBitmap const&, float, float, SkPaint const*) 
                    
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:1746
  0178555b  RunRasterOnThread                                                   
                    
/usr/local/google/code/clankium-master/src/out/Debug/../../cc/resources/raster_worker_pool.cc:165
  01781285  RunOnWorkerThread           

Is it possible to make this codepath support 4444?

[Sami, 2013-08-09 10:44:57]

On 2013/08/08 23:30:44, kaanb wrote:
> I'm getting black tiles when I change the internalformat parameter of
> glTexImage2D to RGBA4_OES and dtrainor@ noticed the same when he changed it
for
> the browser compositor. Could it be possible that we're not supporting the
> extension that adds RGBA4_OES?

Looks like we don't support OES_required_internalformat on the command buffer
side, which explains why this doesn't work. Also, I checked that for instance
Nexus 4 doesn't support that extension but Nexus 10 does, so it's not something
we can rely on. Probably it's then better to do what you did originally and
upload with GL_UNSIGNED_SHORT_4_4_4_4 and assume the driver does the right
thing. We should be able to verify with memory maps that the tile textures are
actually 16 bit.

> > Just curious: was it the EGLImage creation or texture specification using
the
> > EGLImage that failed?
> 
> EGLImage creation failed:
> W/Adreno200-ES20( 2380): <oglLinkEGLImage2GLTexture:1921>: GL_INVALID_VALUE
> F/chromium( 2380): [FATAL:async_pixel_transfer_manager_egl.cc(206)] Check
> failed: ((EGLImageKHR)0) != egl_image_ (0x00000000 vs. 0x00000000)

Ah, too bad. The list of formats EGLImage supports is very driver specific, and
probably 8888 is the only universally supported one.

[tomhudson, 2013-08-09 10:58:01]

On 2013/08/09 02:10:46, kaanb wrote:
> Is it possible to make this codepath support 4444?

Skia removed all 4444 codepaths a couple of months ago. Working around one
assert is just going to land you in other asserts.

They've been convinced for years that 4444 didn't provide adequate quality for
modern expectations under any circumstances; if you've got solid
counter-evidence that'd be useful to share. All of the Skia image comparisons
I'm aware of have looked far, far worse than the comparison images posted on the
bug.

Are you looking for https://codereview.chromium.org/22350003
(http://code.google.com/p/skia/source/detail?r=10621), which Leon added just
Wednesday?
SkBitmap::copyTo() should support your use case better.

I don't think you really explained why you aren't doing things the way Brian
suggests, or what the route through SkDevices was intended to achieve.

[bsalomon, 2013-08-09 13:05:12]

On 2013/08/09 02:10:46, kaanb wrote:
> On 2013/08/08 19:52:51, bsalomon wrote:
> > On 2013/08/08 19:27:35, kaanb wrote:
> > > On 2013/08/08 19:01:22, bsalomon wrote:
> > > > On 2013/08/08 18:46:37, kaanb wrote:
> > > > > On 2013/08/08 18:40:15, bsalomon wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > > File cc/resources/raster_worker_pool.cc (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > > cc/resources/raster_worker_pool.cc:158: canvas.drawBitmap(bitmap_16,
> 0,
> > > 0);
> > > > > > On 2013/08/08 17:25:05, kaanb wrote:
> > > > > > > On 2013/08/08 10:11:01, Sami wrote:
> > > > > > > > |canvas| is still a 32-bit bitmap so this is really just going
> from
> > 32
> > > > =>
> > > > > 16
> > > > > > > =>
> > > > > > > > 32 bits, i.e., throwing away precision :) I think we need to add
a
> > way
> > > > to
> > > > > > make
> > > > > > > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which
isn't
> > > > > supported
> > > > > > > by
> > > > > > > > Skia AFAIK).
> > > > > > > 
> > > > > > > Let me check with Skia folks if they'd be willing to make a
shortcut
> > for
> > > > us
> > > > > > > here.
> > > > > > 
> > > > > > Hi, What's the intent here? Are you trying to go from a 16bit sw
> bitmap
> > to
> > > a
> > > > > > 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a texture
backed
> > > > bitmap?
> > > > > > 
> > > > > > Also, I'm not sure how much it matters but it looks like
> accessBitmap()
> > > > could
> > > > > > take false for the "changePixels" param since you are just reading
> from
> > > it.
> > > > > 
> > > > > Yes, I eventually want to upload the 16bit sw bitmap to a 16bit
texture.
> > > > > bitmap_32 is created here:
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
> > > > > 
> > > > > The buffer_ field that's backing the bitmap is allocated here:
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...
> > > > 
> > > > So would it be correct to say that you're not looking to use Skia to
> upload
> > > the
> > > > 16bit sw bitmap to a texture but rather the code that does so will
extract
> > the
> > > > bitmap backing the canvas (or really the canvas's device)? And so the
> > > > drawBitmap(bitmap_16) call is attempting to replace the canvas/device's
32
> > bit
> > > > bitmap with a 16 bit one?
> > > 
> > > Yes, your statement is correct.
> > 
> > Ok, so you won't be able to attach a 4444 bitmap behind a canvas anymore
(now
> > that they are no longer renderable in Skia). Is it possible to make the
> consumer
> > of the sw bitmap (the code that does the upload) take a SkBitmap rather than
> an
> > SkCanvas or SkDevice? This seems more correct, anyway, as SkCanvas is a
> > abstraction for a rendering destination (and may be actually backed by a
> > SkPicture/SkPipe/PDF document/etc) while SkBitmap is a pixel rectangle.
> 
> Actually the code that does the upload doesn't deal with Skia (it's in
> ResourceProvider::BeginSetPixels()). We construct a bitmap backed by a
> PixelBuffer (created in ResourceProvider::AcquirePixelBuffer() and mapped in
> ResourceProvider::MapPixelBuffer()) and rasterize into it. 
> 
> I talked to reveman and he suggested I try something like I uploaded where we
> create a 16-bit device from the pixelBuffer, within the raster task create a
> temporary 32-bit device, rasterize into it and copy over to 16-bit device at
the
> end. However I'm hitting an assert with the following stack (I had commented
out
> the assert in SkDevice constructor that checks for 4444)
> 
>   0192642d  SkBlitter::Choose(SkBitmap const&, SkMatrix const&, SkPaint
const&,
> void*, unsigned int) 
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkBlitter.cpp:980
>   v------>  SkAutoBlitterChoose                                               
 
>                     
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:64
>   010ebb8f  SkDraw::drawRect(SkRect const&, SkPaint const&) const             
 
>                     
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:887
>   01930bf3  SkDraw::drawBitmap(SkBitmap const&, SkMatrix const&, SkPaint
const&)
> const               
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:1327
>   010e5767  SkCanvas::internalDrawBitmap(SkBitmap const&, SkMatrix const&,
> SkPaint const*)           
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:975
>   010e585d  SkCanvas::drawBitmap(SkBitmap const&, float, float, SkPaint
const*) 
>                     
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:1746
>   0178555b  RunRasterOnThread                                                 
 
>                     
>
/usr/local/google/code/clankium-master/src/out/Debug/../../cc/resources/raster_worker_pool.cc:165
>   01781285  RunOnWorkerThread           
> 
> Is it possible to make this codepath support 4444?

I think you can effectively do what you want, you just need pass around the
resulting 16bit buffer as an SkBitmap rather than as an SkDevice in an SkCanvas.
AFAICT you aren't ever rendering into the 16bit buffer once it is created as a
copy of the 32bit buffer so you shouldn't need SkDevice or SkCanvas.

[kaanb, 2013-08-12 18:36:35]

On 2013/08/09 10:58:01, tomhudson wrote:
> On 2013/08/09 02:10:46, kaanb wrote:
> > Is it possible to make this codepath support 4444?
> 
> Skia removed all 4444 codepaths a couple of months ago. Working around one
> assert is just going to land you in other asserts.
> 
> They've been convinced for years that 4444 didn't provide adequate quality for
> modern expectations under any circumstances; if you've got solid
> counter-evidence that'd be useful to share. All of the Skia image comparisons
> I'm aware of have looked far, far worse than the comparison images posted on
the
> bug.
> 
> Are you looking for https://codereview.chromium.org/22350003
> (http://code.google.com/p/skia/source/detail?r=10621), which Leon added just
> Wednesday?
> SkBitmap::copyTo() should support your use case better.
> 
> I don't think you really explained why you aren't doing things the way Brian
> suggests, or what the route through SkDevices was intended to achieve.

Well, the RunRasterOnThread API takes in an SkDevice so I had to draw the bitmap
back to it.

[kaanb, 2013-08-12 18:38:21]

On 2013/08/09 13:05:12, bsalomon wrote:
> On 2013/08/09 02:10:46, kaanb wrote:
> > On 2013/08/08 19:52:51, bsalomon wrote:
> > > On 2013/08/08 19:27:35, kaanb wrote:
> > > > On 2013/08/08 19:01:22, bsalomon wrote:
> > > > > On 2013/08/08 18:46:37, kaanb wrote:
> > > > > > On 2013/08/08 18:40:15, bsalomon wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > > > File cc/resources/raster_worker_pool.cc (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > > > cc/resources/raster_worker_pool.cc:158:
canvas.drawBitmap(bitmap_16,
> > 0,
> > > > 0);
> > > > > > > On 2013/08/08 17:25:05, kaanb wrote:
> > > > > > > > On 2013/08/08 10:11:01, Sami wrote:
> > > > > > > > > |canvas| is still a 32-bit bitmap so this is really just going
> > from
> > > 32
> > > > > =>
> > > > > > 16
> > > > > > > > =>
> > > > > > > > > 32 bits, i.e., throwing away precision :) I think we need to
add
> a
> > > way
> > > > > to
> > > > > > > make
> > > > > > > > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which
> isn't
> > > > > > supported
> > > > > > > > by
> > > > > > > > > Skia AFAIK).
> > > > > > > > 
> > > > > > > > Let me check with Skia folks if they'd be willing to make a
> shortcut
> > > for
> > > > > us
> > > > > > > > here.
> > > > > > > 
> > > > > > > Hi, What's the intent here? Are you trying to go from a 16bit sw
> > bitmap
> > > to
> > > > a
> > > > > > > 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a texture
> backed
> > > > > bitmap?
> > > > > > > 
> > > > > > > Also, I'm not sure how much it matters but it looks like
> > accessBitmap()
> > > > > could
> > > > > > > take false for the "changePixels" param since you are just reading
> > from
> > > > it.
> > > > > > 
> > > > > > Yes, I eventually want to upload the 16bit sw bitmap to a 16bit
> texture.
> > > > > > bitmap_32 is created here:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
> > > > > > 
> > > > > > The buffer_ field that's backing the bitmap is allocated here:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...
> > > > > 
> > > > > So would it be correct to say that you're not looking to use Skia to
> > upload
> > > > the
> > > > > 16bit sw bitmap to a texture but rather the code that does so will
> extract
> > > the
> > > > > bitmap backing the canvas (or really the canvas's device)? And so the
> > > > > drawBitmap(bitmap_16) call is attempting to replace the
canvas/device's
> 32
> > > bit
> > > > > bitmap with a 16 bit one?
> > > > 
> > > > Yes, your statement is correct.
> > > 
> > > Ok, so you won't be able to attach a 4444 bitmap behind a canvas anymore
> (now
> > > that they are no longer renderable in Skia). Is it possible to make the
> > consumer
> > > of the sw bitmap (the code that does the upload) take a SkBitmap rather
than
> > an
> > > SkCanvas or SkDevice? This seems more correct, anyway, as SkCanvas is a
> > > abstraction for a rendering destination (and may be actually backed by a
> > > SkPicture/SkPipe/PDF document/etc) while SkBitmap is a pixel rectangle.
> > 
> > Actually the code that does the upload doesn't deal with Skia (it's in
> > ResourceProvider::BeginSetPixels()). We construct a bitmap backed by a
> > PixelBuffer (created in ResourceProvider::AcquirePixelBuffer() and mapped in
> > ResourceProvider::MapPixelBuffer()) and rasterize into it. 
> > 
> > I talked to reveman and he suggested I try something like I uploaded where
we
> > create a 16-bit device from the pixelBuffer, within the raster task create a
> > temporary 32-bit device, rasterize into it and copy over to 16-bit device at
> the
> > end. However I'm hitting an assert with the following stack (I had commented
> out
> > the assert in SkDevice constructor that checks for 4444)
> > 
> >   0192642d  SkBlitter::Choose(SkBitmap const&, SkMatrix const&, SkPaint
> const&,
> > void*, unsigned int) 
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkBlitter.cpp:980
> >   v------>  SkAutoBlitterChoose                                             
 
>  
> >                     
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:64
> >   010ebb8f  SkDraw::drawRect(SkRect const&, SkPaint const&) const           
 
>  
> >                     
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:887
> >   01930bf3  SkDraw::drawBitmap(SkBitmap const&, SkMatrix const&, SkPaint
> const&)
> > const               
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:1327
> >   010e5767  SkCanvas::internalDrawBitmap(SkBitmap const&, SkMatrix const&,
> > SkPaint const*)           
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:975
> >   010e585d  SkCanvas::drawBitmap(SkBitmap const&, float, float, SkPaint
> const*) 
> >                     
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:1746
> >   0178555b  RunRasterOnThread                                               
 
>  
> >                     
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../cc/resources/raster_worker_pool.cc:165
> >   01781285  RunOnWorkerThread           
> > 
> > Is it possible to make this codepath support 4444?
> 
> I think you can effectively do what you want, you just need pass around the
> resulting 16bit buffer as an SkBitmap rather than as an SkDevice in an
SkCanvas.
> AFAICT you aren't ever rendering into the 16bit buffer once it is created as a
> copy of the 32bit buffer so you shouldn't need SkDevice or SkCanvas.

I guess the only reasonable way I can achieve that is by passing in an SkBitmap
to the RunRasterOnThread method. Also the 16-bit bitmap needs to be backed by
the pixel-buffer allocated via the ResourceProvider::AcquirePixelBuffer, should
I pass the buffer pointer as well?

[bsalomon, 2013-08-12 18:48:31]

On 2013/08/12 18:38:21, kaanb wrote:
> On 2013/08/09 13:05:12, bsalomon wrote:
> > On 2013/08/09 02:10:46, kaanb wrote:
> > > On 2013/08/08 19:52:51, bsalomon wrote:
> > > > On 2013/08/08 19:27:35, kaanb wrote:
> > > > > On 2013/08/08 19:01:22, bsalomon wrote:
> > > > > > On 2013/08/08 18:46:37, kaanb wrote:
> > > > > > > On 2013/08/08 18:40:15, bsalomon wrote:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > > > > File cc/resources/raster_worker_pool.cc (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > > > > cc/resources/raster_worker_pool.cc:158:
> canvas.drawBitmap(bitmap_16,
> > > 0,
> > > > > 0);
> > > > > > > > On 2013/08/08 17:25:05, kaanb wrote:
> > > > > > > > > On 2013/08/08 10:11:01, Sami wrote:
> > > > > > > > > > |canvas| is still a 32-bit bitmap so this is really just
going
> > > from
> > > > 32
> > > > > > =>
> > > > > > > 16
> > > > > > > > > =>
> > > > > > > > > > 32 bits, i.e., throwing away precision :) I think we need to
> add
> > a
> > > > way
> > > > > > to
> > > > > > > > make
> > > > > > > > > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device (which
> > isn't
> > > > > > > supported
> > > > > > > > > by
> > > > > > > > > > Skia AFAIK).
> > > > > > > > > 
> > > > > > > > > Let me check with Skia folks if they'd be willing to make a
> > shortcut
> > > > for
> > > > > > us
> > > > > > > > > here.
> > > > > > > > 
> > > > > > > > Hi, What's the intent here? Are you trying to go from a 16bit sw
> > > bitmap
> > > > to
> > > > > a
> > > > > > > > 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a texture
> > backed
> > > > > > bitmap?
> > > > > > > > 
> > > > > > > > Also, I'm not sure how much it matters but it looks like
> > > accessBitmap()
> > > > > > could
> > > > > > > > take false for the "changePixels" param since you are just
reading
> > > from
> > > > > it.
> > > > > > > 
> > > > > > > Yes, I eventually want to upload the 16bit sw bitmap to a 16bit
> > texture.
> > > > > > > bitmap_32 is created here:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
> > > > > > > 
> > > > > > > The buffer_ field that's backing the bitmap is allocated here:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...
> > > > > > 
> > > > > > So would it be correct to say that you're not looking to use Skia to
> > > upload
> > > > > the
> > > > > > 16bit sw bitmap to a texture but rather the code that does so will
> > extract
> > > > the
> > > > > > bitmap backing the canvas (or really the canvas's device)? And so
the
> > > > > > drawBitmap(bitmap_16) call is attempting to replace the
> canvas/device's
> > 32
> > > > bit
> > > > > > bitmap with a 16 bit one?
> > > > > 
> > > > > Yes, your statement is correct.
> > > > 
> > > > Ok, so you won't be able to attach a 4444 bitmap behind a canvas anymore
> > (now
> > > > that they are no longer renderable in Skia). Is it possible to make the
> > > consumer
> > > > of the sw bitmap (the code that does the upload) take a SkBitmap rather
> than
> > > an
> > > > SkCanvas or SkDevice? This seems more correct, anyway, as SkCanvas is a
> > > > abstraction for a rendering destination (and may be actually backed by a
> > > > SkPicture/SkPipe/PDF document/etc) while SkBitmap is a pixel rectangle.
> > > 
> > > Actually the code that does the upload doesn't deal with Skia (it's in
> > > ResourceProvider::BeginSetPixels()). We construct a bitmap backed by a
> > > PixelBuffer (created in ResourceProvider::AcquirePixelBuffer() and mapped
in
> > > ResourceProvider::MapPixelBuffer()) and rasterize into it. 
> > > 
> > > I talked to reveman and he suggested I try something like I uploaded where
> we
> > > create a 16-bit device from the pixelBuffer, within the raster task create
a
> > > temporary 32-bit device, rasterize into it and copy over to 16-bit device
at
> > the
> > > end. However I'm hitting an assert with the following stack (I had
commented
> > out
> > > the assert in SkDevice constructor that checks for 4444)
> > > 
> > >   0192642d  SkBlitter::Choose(SkBitmap const&, SkMatrix const&, SkPaint
> > const&,
> > > void*, unsigned int) 
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkBlitter.cpp:980
> > >   v------>  SkAutoBlitterChoose                                           
 
>  
> >  
> > >                     
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:64
> > >   010ebb8f  SkDraw::drawRect(SkRect const&, SkPaint const&) const         
 
>  
> >  
> > >                     
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:887
> > >   01930bf3  SkDraw::drawBitmap(SkBitmap const&, SkMatrix const&, SkPaint
> > const&)
> > > const               
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:1327
> > >   010e5767  SkCanvas::internalDrawBitmap(SkBitmap const&, SkMatrix const&,
> > > SkPaint const*)           
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:975
> > >   010e585d  SkCanvas::drawBitmap(SkBitmap const&, float, float, SkPaint
> > const*) 
> > >                     
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:1746
> > >   0178555b  RunRasterOnThread                                             
 
>  
> >  
> > >                     
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../cc/resources/raster_worker_pool.cc:165
> > >   01781285  RunOnWorkerThread           
> > > 
> > > Is it possible to make this codepath support 4444?
> > 
> > I think you can effectively do what you want, you just need pass around the
> > resulting 16bit buffer as an SkBitmap rather than as an SkDevice in an
> SkCanvas.
> > AFAICT you aren't ever rendering into the 16bit buffer once it is created as
a
> > copy of the 32bit buffer so you shouldn't need SkDevice or SkCanvas.
> 
> I guess the only reasonable way I can achieve that is by passing in an
SkBitmap
> to the RunRasterOnThread method. Also the 16-bit bitmap needs to be backed by
> the pixel-buffer allocated via the ResourceProvider::AcquirePixelBuffer,
should
> I pass the buffer pointer as well?

That sounds like a good approach to me, but I'm not familiar with how
ResourceProvider works so I'm not sure if you need to pass more than the bitmap.
Hopefully someone more familiar with the Chromium side of this can answer that.
SkBitmap sits on top of a subclassable ref-counted object called SkPixelRef
which can be extended to handle custom allocation/freeing of pixel data. I'm not
sure if that is necessary or advisable in this context.

[reveman, 2013-08-12 19:47:12]

On 2013/08/12 18:48:31, bsalomon wrote:
> On 2013/08/12 18:38:21, kaanb wrote:
> > On 2013/08/09 13:05:12, bsalomon wrote:
> > > On 2013/08/09 02:10:46, kaanb wrote:
> > > > On 2013/08/08 19:52:51, bsalomon wrote:
> > > > > On 2013/08/08 19:27:35, kaanb wrote:
> > > > > > On 2013/08/08 19:01:22, bsalomon wrote:
> > > > > > > On 2013/08/08 18:46:37, kaanb wrote:
> > > > > > > > On 2013/08/08 18:40:15, bsalomon wrote:
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > > > > > File cc/resources/raster_worker_pool.cc (right):
> > > > > > > > > 
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > > > > > cc/resources/raster_worker_pool.cc:158:
> > canvas.drawBitmap(bitmap_16,
> > > > 0,
> > > > > > 0);
> > > > > > > > > On 2013/08/08 17:25:05, kaanb wrote:
> > > > > > > > > > On 2013/08/08 10:11:01, Sami wrote:
> > > > > > > > > > > |canvas| is still a 32-bit bitmap so this is really just
> going
> > > > from
> > > > > 32
> > > > > > > =>
> > > > > > > > 16
> > > > > > > > > > =>
> > > > > > > > > > > 32 bits, i.e., throwing away precision :) I think we need
to
> > add
> > > a
> > > > > way
> > > > > > > to
> > > > > > > > > make
> > > > > > > > > > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device
(which
> > > isn't
> > > > > > > > supported
> > > > > > > > > > by
> > > > > > > > > > > Skia AFAIK).
> > > > > > > > > > 
> > > > > > > > > > Let me check with Skia folks if they'd be willing to make a
> > > shortcut
> > > > > for
> > > > > > > us
> > > > > > > > > > here.
> > > > > > > > > 
> > > > > > > > > Hi, What's the intent here? Are you trying to go from a 16bit
sw
> > > > bitmap
> > > > > to
> > > > > > a
> > > > > > > > > 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a
texture
> > > backed
> > > > > > > bitmap?
> > > > > > > > > 
> > > > > > > > > Also, I'm not sure how much it matters but it looks like
> > > > accessBitmap()
> > > > > > > could
> > > > > > > > > take false for the "changePixels" param since you are just
> reading
> > > > from
> > > > > > it.
> > > > > > > > 
> > > > > > > > Yes, I eventually want to upload the 16bit sw bitmap to a 16bit
> > > texture.
> > > > > > > > bitmap_32 is created here:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
> > > > > > > > 
> > > > > > > > The buffer_ field that's backing the bitmap is allocated here:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...
> > > > > > > 
> > > > > > > So would it be correct to say that you're not looking to use Skia
to
> > > > upload
> > > > > > the
> > > > > > > 16bit sw bitmap to a texture but rather the code that does so will
> > > extract
> > > > > the
> > > > > > > bitmap backing the canvas (or really the canvas's device)? And so
> the
> > > > > > > drawBitmap(bitmap_16) call is attempting to replace the
> > canvas/device's
> > > 32
> > > > > bit
> > > > > > > bitmap with a 16 bit one?
> > > > > > 
> > > > > > Yes, your statement is correct.
> > > > > 
> > > > > Ok, so you won't be able to attach a 4444 bitmap behind a canvas
anymore
> > > (now
> > > > > that they are no longer renderable in Skia). Is it possible to make
the
> > > > consumer
> > > > > of the sw bitmap (the code that does the upload) take a SkBitmap
rather
> > than
> > > > an
> > > > > SkCanvas or SkDevice? This seems more correct, anyway, as SkCanvas is
a
> > > > > abstraction for a rendering destination (and may be actually backed by
a
> > > > > SkPicture/SkPipe/PDF document/etc) while SkBitmap is a pixel
rectangle.
> > > > 
> > > > Actually the code that does the upload doesn't deal with Skia (it's in
> > > > ResourceProvider::BeginSetPixels()). We construct a bitmap backed by a
> > > > PixelBuffer (created in ResourceProvider::AcquirePixelBuffer() and
mapped
> in
> > > > ResourceProvider::MapPixelBuffer()) and rasterize into it. 
> > > > 
> > > > I talked to reveman and he suggested I try something like I uploaded
where
> > we
> > > > create a 16-bit device from the pixelBuffer, within the raster task
create
> a
> > > > temporary 32-bit device, rasterize into it and copy over to 16-bit
device
> at
> > > the
> > > > end. However I'm hitting an assert with the following stack (I had
> commented
> > > out
> > > > the assert in SkDevice constructor that checks for 4444)
> > > > 
> > > >   0192642d  SkBlitter::Choose(SkBitmap const&, SkMatrix const&, SkPaint
> > > const&,
> > > > void*, unsigned int) 
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkBlitter.cpp:980
> > > >   v------>  SkAutoBlitterChoose                                         
 
>  
> >  
> > >  
> > > >                     
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:64
> > > >   010ebb8f  SkDraw::drawRect(SkRect const&, SkPaint const&) const       
 
>  
> >  
> > >  
> > > >                     
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:887
> > > >   01930bf3  SkDraw::drawBitmap(SkBitmap const&, SkMatrix const&, SkPaint
> > > const&)
> > > > const               
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:1327
> > > >   010e5767  SkCanvas::internalDrawBitmap(SkBitmap const&, SkMatrix
const&,
> > > > SkPaint const*)           
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:975
> > > >   010e585d  SkCanvas::drawBitmap(SkBitmap const&, float, float, SkPaint
> > > const*) 
> > > >                     
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:1746
> > > >   0178555b  RunRasterOnThread                                           
 
>  
> >  
> > >  
> > > >                     
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../cc/resources/raster_worker_pool.cc:165
> > > >   01781285  RunOnWorkerThread           
> > > > 
> > > > Is it possible to make this codepath support 4444?
> > > 
> > > I think you can effectively do what you want, you just need pass around
the
> > > resulting 16bit buffer as an SkBitmap rather than as an SkDevice in an
> > SkCanvas.
> > > AFAICT you aren't ever rendering into the 16bit buffer once it is created
as
> a
> > > copy of the 32bit buffer so you shouldn't need SkDevice or SkCanvas.
> > 
> > I guess the only reasonable way I can achieve that is by passing in an
> SkBitmap
> > to the RunRasterOnThread method. Also the 16-bit bitmap needs to be backed
by
> > the pixel-buffer allocated via the ResourceProvider::AcquirePixelBuffer,
> should
> > I pass the buffer pointer as well?

You can just pass a "SkBitmap*" to RunRasterOnThread instead if that helps.
AcquirePixelBuffer ignores the texture format and assumes 4 bytes per pixel at
the moment. That might be fine for testing purposes as it's more than you need
in this case but to get the full benefit of using 16bpp you probably want to fix
that.

> 
> That sounds like a good approach to me, but I'm not familiar with how
> ResourceProvider works so I'm not sure if you need to pass more than the
bitmap.
> Hopefully someone more familiar with the Chromium side of this can answer
that.
> SkBitmap sits on top of a subclassable ref-counted object called SkPixelRef
> which can be extended to handle custom allocation/freeing of pixel data. I'm
not
> sure if that is necessary or advisable in this context.

[kaanb, 2013-08-12 23:01:42]

On 2013/08/12 19:47:12, David Reveman wrote:
> On 2013/08/12 18:48:31, bsalomon wrote:
> > On 2013/08/12 18:38:21, kaanb wrote:
> > > On 2013/08/09 13:05:12, bsalomon wrote:
> > > > On 2013/08/09 02:10:46, kaanb wrote:
> > > > > On 2013/08/08 19:52:51, bsalomon wrote:
> > > > > > On 2013/08/08 19:27:35, kaanb wrote:
> > > > > > > On 2013/08/08 19:01:22, bsalomon wrote:
> > > > > > > > On 2013/08/08 18:46:37, kaanb wrote:
> > > > > > > > > On 2013/08/08 18:40:15, bsalomon wrote:
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > > > > > > File cc/resources/raster_worker_pool.cc (right):
> > > > > > > > > > 
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/21159007/diff/11001/cc/resources/raster_worke...
> > > > > > > > > > cc/resources/raster_worker_pool.cc:158:
> > > canvas.drawBitmap(bitmap_16,
> > > > > 0,
> > > > > > > 0);
> > > > > > > > > > On 2013/08/08 17:25:05, kaanb wrote:
> > > > > > > > > > > On 2013/08/08 10:11:01, Sami wrote:
> > > > > > > > > > > > |canvas| is still a 32-bit bitmap so this is really just
> > going
> > > > > from
> > > > > > 32
> > > > > > > > =>
> > > > > > > > > 16
> > > > > > > > > > > =>
> > > > > > > > > > > > 32 bits, i.e., throwing away precision :) I think we
need
> to
> > > add
> > > > a
> > > > > > way
> > > > > > > > to
> > > > > > > > > > make
> > > > > > > > > > > > PixelBufferWorkerPoolTaskImpl give us a 16-bit device
> (which
> > > > isn't
> > > > > > > > > supported
> > > > > > > > > > > by
> > > > > > > > > > > > Skia AFAIK).
> > > > > > > > > > > 
> > > > > > > > > > > Let me check with Skia folks if they'd be willing to make
a
> > > > shortcut
> > > > > > for
> > > > > > > > us
> > > > > > > > > > > here.
> > > > > > > > > > 
> > > > > > > > > > Hi, What's the intent here? Are you trying to go from a
16bit
> sw
> > > > > bitmap
> > > > > > to
> > > > > > > a
> > > > > > > > > > 16bit texture? bitmap_16 is a sw bitmap, is bitmap_32 a
> texture
> > > > backed
> > > > > > > > bitmap?
> > > > > > > > > > 
> > > > > > > > > > Also, I'm not sure how much it matters but it looks like
> > > > > accessBitmap()
> > > > > > > > could
> > > > > > > > > > take false for the "changePixels" param since you are just
> > reading
> > > > > from
> > > > > > > it.
> > > > > > > > > 
> > > > > > > > > Yes, I eventually want to upload the 16bit sw bitmap to a
16bit
> > > > texture.
> > > > > > > > > bitmap_32 is created here:
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
> > > > > > > > > 
> > > > > > > > > The buffer_ field that's backing the bitmap is allocated here:
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou...
> > > > > > > > 
> > > > > > > > So would it be correct to say that you're not looking to use
Skia
> to
> > > > > upload
> > > > > > > the
> > > > > > > > 16bit sw bitmap to a texture but rather the code that does so
will
> > > > extract
> > > > > > the
> > > > > > > > bitmap backing the canvas (or really the canvas's device)? And
so
> > the
> > > > > > > > drawBitmap(bitmap_16) call is attempting to replace the
> > > canvas/device's
> > > > 32
> > > > > > bit
> > > > > > > > bitmap with a 16 bit one?
> > > > > > > 
> > > > > > > Yes, your statement is correct.
> > > > > > 
> > > > > > Ok, so you won't be able to attach a 4444 bitmap behind a canvas
> anymore
> > > > (now
> > > > > > that they are no longer renderable in Skia). Is it possible to make
> the
> > > > > consumer
> > > > > > of the sw bitmap (the code that does the upload) take a SkBitmap
> rather
> > > than
> > > > > an
> > > > > > SkCanvas or SkDevice? This seems more correct, anyway, as SkCanvas
is
> a
> > > > > > abstraction for a rendering destination (and may be actually backed
by
> a
> > > > > > SkPicture/SkPipe/PDF document/etc) while SkBitmap is a pixel
> rectangle.
> > > > > 
> > > > > Actually the code that does the upload doesn't deal with Skia (it's in
> > > > > ResourceProvider::BeginSetPixels()). We construct a bitmap backed by a
> > > > > PixelBuffer (created in ResourceProvider::AcquirePixelBuffer() and
> mapped
> > in
> > > > > ResourceProvider::MapPixelBuffer()) and rasterize into it. 
> > > > > 
> > > > > I talked to reveman and he suggested I try something like I uploaded
> where
> > > we
> > > > > create a 16-bit device from the pixelBuffer, within the raster task
> create
> > a
> > > > > temporary 32-bit device, rasterize into it and copy over to 16-bit
> device
> > at
> > > > the
> > > > > end. However I'm hitting an assert with the following stack (I had
> > commented
> > > > out
> > > > > the assert in SkDevice constructor that checks for 4444)
> > > > > 
> > > > >   0192642d  SkBlitter::Choose(SkBitmap const&, SkMatrix const&,
SkPaint
> > > > const&,
> > > > > void*, unsigned int) 
> > > > >
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkBlitter.cpp:980
> > > > >   v------>  SkAutoBlitterChoose                                       
 
>  
> >  
> > >  
> > > >  
> > > > >                     
> > > > >
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:64
> > > > >   010ebb8f  SkDraw::drawRect(SkRect const&, SkPaint const&) const     
 
>  
> >  
> > >  
> > > >  
> > > > >                     
> > > > >
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:887
> > > > >   01930bf3  SkDraw::drawBitmap(SkBitmap const&, SkMatrix const&,
SkPaint
> > > > const&)
> > > > > const               
> > > > >
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkDraw.cpp:1327
> > > > >   010e5767  SkCanvas::internalDrawBitmap(SkBitmap const&, SkMatrix
> const&,
> > > > > SkPaint const*)           
> > > > >
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:975
> > > > >   010e585d  SkCanvas::drawBitmap(SkBitmap const&, float, float,
SkPaint
> > > > const*) 
> > > > >                     
> > > > >
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../third_party/skia/src/core/SkCanvas.cpp:1746
> > > > >   0178555b  RunRasterOnThread                                         
 
>  
> >  
> > >  
> > > >  
> > > > >                     
> > > > >
> > > >
> > >
> >
>
/usr/local/google/code/clankium-master/src/out/Debug/../../cc/resources/raster_worker_pool.cc:165
> > > > >   01781285  RunOnWorkerThread           
> > > > > 
> > > > > Is it possible to make this codepath support 4444?
> > > > 
> > > > I think you can effectively do what you want, you just need pass around
> the
> > > > resulting 16bit buffer as an SkBitmap rather than as an SkDevice in an
> > > SkCanvas.
> > > > AFAICT you aren't ever rendering into the 16bit buffer once it is
created
> as
> > a
> > > > copy of the 32bit buffer so you shouldn't need SkDevice or SkCanvas.
> > > 
> > > I guess the only reasonable way I can achieve that is by passing in an
> > SkBitmap
> > > to the RunRasterOnThread method. Also the 16-bit bitmap needs to be backed
> by
> > > the pixel-buffer allocated via the ResourceProvider::AcquirePixelBuffer,
> > should
> > > I pass the buffer pointer as well?
> 
> You can just pass a "SkBitmap*" to RunRasterOnThread instead if that helps.
> AcquirePixelBuffer ignores the texture format and assumes 4 bytes per pixel at
> the moment. That might be fine for testing purposes as it's more than you need
> in this case but to get the full benefit of using 16bpp you probably want to
fix
> that.
> 
> > 
> > That sounds like a good approach to me, but I'm not familiar with how
> > ResourceProvider works so I'm not sure if you need to pass more than the
> bitmap.
> > Hopefully someone more familiar with the Chromium side of this can answer
> that.
> > SkBitmap sits on top of a subclassable ref-counted object called SkPixelRef
> > which can be extended to handle custom allocation/freeing of pixel data. I'm
> not
> > sure if that is necessary or advisable in this context.

OK, I changed the RunRasterOnThread API to take in the mapped buffer instead of
the SkDevice*. However content is still garbage, is there any other piece I'm
missing?

[no sievers, 2013-08-13 01:13:58]

Not sure the change to make the GLSurface belongs in this patch. That's
orthogonal right?

https://codereview.chromium.org/21159007/diff/71001/gpu/command_buffer/servic...
File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right):

https://codereview.chromium.org/21159007/diff/71001/gpu/command_buffer/servic...
gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:55:
!base::android::SysUtils::IsLowEndDevice()) {
It'd be nice if this could be another property. One that indicates we are using
16bit textures which don't work with EGL images.

https://codereview.chromium.org/21159007/diff/71001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/21159007/diff/71001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:131: use_16bit_surface =
base::android::SysUtils::IsLowEndDevice();
can you make this static so it's clear that we are not supposed to start using
different configs?

https://codereview.chromium.org/21159007/diff/71001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:150: EGL_SURFACE_TYPE, EGL_WINDOW_BIT | EGL_PBUFFER_BIT,
We need to make sure that this config works across devices with pbuffers and
windows for virtual contexts (offscreen contexts might be made current with both
onscreen and offscreen surfaces).

https://codereview.chromium.org/21159007/diff/71001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:171: use_16bit_surface ? kConfigAttribs16bit :
kConfigAttribs,
It seems like num_configs could be >0 (since it's more vague above). If you are
DCHECKing() for the precise config you want below, why don't you just add it to
kConfigAttribs16bit in the first place then?

https://codereview.chromium.org/21159007/diff/71001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:177: return false;
Also, if we don't get a 16bit config, should we fall back to 32bit?

[no sievers, 2013-08-13 01:21:11]

Regarding the cc changes you might want to start a thread on graphics-dev to see
what the right way is to plumb through the pixel format.

I think the problem right now is that
- cc has hardcoded the canvas/bitmap allocation to 32bits in many places
- it uses resources/platform_color.h to figure out the component order

It's likely that your change only hardcodes things in a few places but is not
compatible in other places (what about readback, what about different texture
upload paths?).

I don't know, but maybe platform color could be extended to handle this.

[no sievers, 2013-08-13 01:26:30]

https://codereview.chromium.org/21159007/diff/71001/gpu/command_buffer/servic...
File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right):

https://codereview.chromium.org/21159007/diff/71001/gpu/command_buffer/servic...
gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:55:
!base::android::SysUtils::IsLowEndDevice()) {
On 2013/08/13 01:13:58, sievers wrote:
> It'd be nice if this could be another property. One that indicates we are
using
> 16bit textures which don't work with EGL images.

Actually, what happens currently if you use glAsyncTexImage2D() with a format
that is not compatible with EGL image?

I think it should be more dynamic to fall back to a different uploader.

[kaanb, 2013-08-13 04:20:39]

https://codereview.chromium.org/21159007/diff/71001/gpu/command_buffer/servic...
File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right):

https://codereview.chromium.org/21159007/diff/71001/gpu/command_buffer/servic...
gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:55:
!base::android::SysUtils::IsLowEndDevice()) {
On 2013/08/13 01:26:30, sievers wrote:
> On 2013/08/13 01:13:58, sievers wrote:
> > It'd be nice if this could be another property. One that indicates we are
> using
> > 16bit textures which don't work with EGL images.
> 
> Actually, what happens currently if you use glAsyncTexImage2D() with a format
> that is not compatible with EGL image?
> 
> I think it should be more dynamic to fall back to a different uploader.
> 

Opened crbug.com/271929 and added a comment that explains why this check is here

https://codereview.chromium.org/21159007/diff/71001/ui/gl/gl_surface_egl.cc
File ui/gl/gl_surface_egl.cc (right):

https://codereview.chromium.org/21159007/diff/71001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:131: use_16bit_surface =
base::android::SysUtils::IsLowEndDevice();
On 2013/08/13 01:13:58, sievers wrote:
> can you make this static so it's clear that we are not supposed to start using
> different configs?

Done.

https://codereview.chromium.org/21159007/diff/71001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:171: use_16bit_surface ? kConfigAttribs16bit :
kConfigAttribs,
On 2013/08/13 01:13:58, sievers wrote:
> It seems like num_configs could be >0 (since it's more vague above). If you
are
> DCHECKing() for the precise config you want below, why don't you just add it
to
> kConfigAttribs16bit in the first place then?

The trouble is if you explicitly ask for 5-6-5 you get a 8-8-8 or 8-8-8-8 as
higher resolutions have higher priority. Just asking for 16bits is the only
reliable way of getting 16bits. It is explained here:

http://malideveloper.arm.com/develop-for-mali/sample-code/selecting-the-corre...

https://codereview.chromium.org/21159007/diff/71001/ui/gl/gl_surface_egl.cc#n...
ui/gl/gl_surface_egl.cc:177: return false;
On 2013/08/13 01:13:58, sievers wrote:
> Also, if we don't get a 16bit config, should we fall back to 32bit?

I think the eglChooseConfig will automatically select the 32bit configuration if
it's the only available one. So we don't need a fallback logic.

[kaanb, 2013-09-04 01:08:20]

I've removed the gl_surface_egl.cc changes from this patch, so it only changes
the tile textures to 16bit.

Could everyone take a look again please? The known issues with this patch are
black flickering at the end of scrolls and a crash in SkBlurMaskFilter.

[kaanb, 2013-09-05 01:37:28]

On 2013/09/04 01:08:20, kaanb wrote:
> I've removed the gl_surface_egl.cc changes from this patch, so it only changes
> the tile textures to 16bit.
> 
> Could everyone take a look again please? The known issues with this patch are
> black flickering at the end of scrolls and a crash in SkBlurMaskFilter.

ping!

I fixed the black flicker during raster-on-demand and the crash in
SkBlurMaskFilter didn't turn out to be related to my patch.
I'd really appreciate if the cc OWNER could take a look and give some feedback.

[epennerAtGoogle, 2013-09-05 01:54:14]

Couple comments of the same theme. It would be nice if most of this code just
took an extra GL type parameter rather than a boolean.

I believe ResourcePool only looks at format/size right now. We should make it
use both format/type/size so 16-bit/32-bit textures can both exist in the pool.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource.cc
File cc/resources/resource.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource.cc...
cc/resources/resource.cc:17: size_t Resource::BytesPerPixel(GLenum format, bool
is_16_bit_resource) {
Could we just pass in the GL type here rather than the extra boolean? And then
DCHECK that only the supported combos of format/type are passed in?

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_po...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_po...
cc/resources/resource_pool.h:25: bool use_16_bit_resource);
Can we avoid this boolean in favor of a real format/type? There are many
formats/types that will result in 16 bits.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:688: best_texture_format_(0),
Could we have best_texture_type in addition to best_texture_format, and avoid
any #ifdefs/booleans?

[enne (OOO), 2013-09-05 02:01:19]

https://codereview.chromium.org/21159007/diff/102001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1710: SkBitmap bitmap_16;
This should probably be a member along with on_demand_tile_raster_bitmap_.  I
suspect the goal of making it a member is to not have to reallocate the buffer
if it ends up being the same size.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/prioritized...
File cc/resources/prioritized_resource.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/prioritized...
cc/resources/prioritized_resource.cc:31: // TODO(kaanb): check this codepath
Hopefully this codepath is not hit on Android once you got rid of the ImageLayer
for the glow?

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:180: bitmap_32.copyTo(&bitmap_16,
SkBitmap::kARGB_4444_Config, &allocator);
Just to be clear, this is copying from the 8888 bitmap stored in buffer onto
buffer itself as a 4444 bitmap? Is that safe?

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:693: use_16_bit_textures_ = true;
Please make this a LayerTreeSetting and keep the #ifdef out of cc so that this
setting can be tested in a device-independent manner.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1175: GLenum texture_data_type =
use_16_bit_textures_
Rather than having to pass around booleans and settings that force every texture
into one mode or the other, can resources just have a texture_data_type value? 
Maybe there could just be a LayerTreeSetting for preferred texture_data_type and
ResourceProvider could try to respect that when generating resources.

That way you could (potentially) have two different formats at once and cc gets
a feature rather than a mode.

https://codereview.chromium.org/21159007/diff/102001/content/browser/android/...
File content/browser/android/overscroll_glow.cc (right):

https://codereview.chromium.org/21159007/diff/102001/content/browser/android/...
content/browser/android/overscroll_glow.cc:49:
scoped_refptr<cc::PictureImageLayer> layer = cc::PictureImageLayer::Create();
This seems like the right change to make so that Android isn't using old-style
TiledLayer, but did you figure out what else was wrong with the glow?

[Sami, 2013-09-05 14:13:25]

Thanks Kaan, I think this has improved a lot. Could you update the changelog to
say which bits of the pipeline are getting 16-bit support with this patch?

https://codereview.chromium.org/21159007/diff/102001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1708: uint8_t* bitmap_pixels = NULL;
Nit: const uint8_t*

https://codereview.chromium.org/21159007/diff/102001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1710: SkBitmap bitmap_16;
On 2013/09/05 02:01:19, enne wrote:
> This should probably be a member along with on_demand_tile_raster_bitmap_.  I
> suspect the goal of making it a member is to not have to reallocate the buffer
> if it ends up being the same size.

Furthermore, since bitmap_16 is on the stack, bitmap_pixels will point to free'd
memory as soon as we leave this block. Maybe that explains the corruption you
were seeing.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/image_raste...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:37:
task_->RunOnWorkerThread(thread_index, buffer_, task_->resource()->size());
Any reason not to pass the bitmap into RunOnWorkerThread instead of the raw
buffer? I find passing void* pointers around like this a little scary.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:131: SkBitmap bitmap_32;
If we gave this function an SkBitmapDevice instead of SkBaseDevice we wouldn't
need to change much of the existing bitmap allocation logic. To convert to 16
bit you could then do this:

SkBitmap bitmap_32;
bitmap_32.setPixelRef(device->getPixelRef());
bitmap_32.copyTo(&bitmap_16, SkBitmap::kARGB_4444_Config, &allocator);

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:180: bitmap_32.copyTo(&bitmap_16,
SkBitmap::kARGB_4444_Config, &allocator);
On 2013/09/05 02:01:19, enne wrote:
> Just to be clear, this is copying from the 8888 bitmap stored in buffer onto
> buffer itself as a 4444 bitmap? Is that safe?

If I read this right we're copying from bitmap_32 (that was allocated storage on
line 135) to bitmap_16 which points to buffer. Tom and I think it should be
safe. Furthermore copyTo() internally uses a temporary allocation so you can
also copy from a bitmap to itself.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource.cc
File cc/resources/resource.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource.cc...
cc/resources/resource.cc:17: size_t Resource::BytesPerPixel(GLenum format, bool
is_16_bit_resource) {
On 2013/09/05 01:54:15, epennerAtGoogle wrote:
> Could we just pass in the GL type here rather than the extra boolean? And then
> DCHECK that only the supported combos of format/type are passed in?

+1, I think that would make this a lot cleaner.

https://codereview.chromium.org/21159007/diff/102001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:214: // TODO(kaanb): anything to do to
GL_UNPACK_ALIGNMENT for 16_bit?
Yes, it needs to be set to 2 bytes. Otherwise odd-width bitmaps will look, well,
odd :) The alternative is to allocate our 16 bit source bitmaps with 4-byte
aligned strides. I'm not sure if Skia supports that.

https://codereview.chromium.org/21159007/diff/102001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.cc (right):

https://codereview.chromium.org/21159007/diff/102001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.cc:95: if (base::android::SysUtils::IsLowEndDevice())
{
Is this mainly for UI resources? You could consider landing this change
separately since it's not really connected to the compositor changes.

https://codereview.chromium.org/21159007/diff/102001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.cc:111: return skia::ImageOperations::Resize(bitmap,
Should we do 16 bit here too?

[reveman, 2013-09-05 16:24:17]

Is this supposed to change the internalformat of textures or just the format of
the data passed to the driver. The patch seems to be doing the later, which I'm
not sure is a good idea.

If you want to change the actual internalformat of textures, then adjusting the
data type used for pixel data is not enough.

Is support for GL_RGBA4 as internalformat what we want? Resource class already
has a format member that represents the internalformat and data format/type can
derived from that without adding a new 16bit bool or new format enum that needs
to be passed around.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/image_raste...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:37:
task_->RunOnWorkerThread(thread_index, buffer_, task_->resource()->size());
you're no longer respecting the value of |stride_|.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:135: bitmap_32.allocPixels();
we shouldn't be allocating a temporary bitmap unless necessary. not clear this
code works at all with 32bpp resources. how is |buffer| modified in that case?

https://codereview.chromium.org/21159007/diff/102001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.h (right):

https://codereview.chromium.org/21159007/diff/102001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.h:26: class CC_EXPORT TextureUploader {
Do we need support for this in the old uploader? Can we not just limit it to
impl-side painting?

[Sami, 2013-09-05 16:30:54]

On 2013/09/05 16:24:17, David Reveman wrote:
> Is this supposed to change the internalformat of textures or just the format
of
> the data passed to the driver. The patch seems to be doing the later, which
I'm
> not sure is a good idea.
> 
> If you want to change the actual internalformat of textures, then adjusting
the
> data type used for pixel data is not enough.
> 
> Is support for GL_RGBA4 as internalformat what we want? Resource class already
> has a format member that represents the internalformat and data format/type
can
> derived from that without adding a new 16bit bool or new format enum that
needs
> to be passed around.

This was brought up around comment #11:

"""Looks like we don't support OES_required_internalformat on the command buffer
side, which explains why this doesn't work. Also, I checked that for instance
Nexus 4 doesn't support that extension but Nexus 10 does, so it's not something
we can rely on. Probably it's then better to do what you did originally and
upload with GL_UNSIGNED_SHORT_4_4_4_4 and assume the driver does the right
thing. We should be able to verify with memory maps that the tile textures are
actually 16 bit."""

Since that Kaan has also checked that GL_UNSIGNED_SHORT_4_4_4_4 does the right
thing -- at least on the device he was using (Galaxy Nexus?).

[reveman, 2013-09-05 17:35:55]

On 2013/09/05 16:30:54, Sami wrote:
> On 2013/09/05 16:24:17, David Reveman wrote:
> > Is this supposed to change the internalformat of textures or just the format
> of
> > the data passed to the driver. The patch seems to be doing the later, which
> I'm
> > not sure is a good idea.
> > 
> > If you want to change the actual internalformat of textures, then adjusting
> the
> > data type used for pixel data is not enough.
> > 
> > Is support for GL_RGBA4 as internalformat what we want? Resource class
already
> > has a format member that represents the internalformat and data format/type
> can
> > derived from that without adding a new 16bit bool or new format enum that
> needs
> > to be passed around.
> 
> This was brought up around comment #11:
> 
> """Looks like we don't support OES_required_internalformat on the command
buffer
> side, which explains why this doesn't work. Also, I checked that for instance
> Nexus 4 doesn't support that extension but Nexus 10 does, so it's not
something
> we can rely on. Probably it's then better to do what you did originally and
> upload with GL_UNSIGNED_SHORT_4_4_4_4 and assume the driver does the right
> thing. We should be able to verify with memory maps that the tile textures are
> actually 16 bit."""
> 
> Since that Kaan has also checked that GL_UNSIGNED_SHORT_4_4_4_4 does the right
> thing -- at least on the device he was using (Galaxy Nexus?).

Would it makes sense to still handle this as a GL_RGBA4 internalformat within
the compositor code? I think that would be much cleaner and better represent
what we like to do. ResourceProvider (or GLES service) can handle the case where
GL_RGBA4 is not available and GL_RGBA needs to be used.

I suspect that using GL_RGBA with drivers that support GL_RGBA4 might result in
32bpp textures and just a bunch of work converting the pixel data. I think we
need a system that allows us to avoid that.

Also, how does the driver know that it should use 16bpp if the texture is
initially defined without any pixel data? I guess we rely on that not being the
case?

[Sami, 2013-09-05 17:58:32]

On 2013/09/05 17:35:55, David Reveman wrote:
> Would it makes sense to still handle this as a GL_RGBA4 internalformat within
> the compositor code? I think that would be much cleaner and better represent
> what we like to do. ResourceProvider (or GLES service) can handle the case
where
> GL_RGBA4 is not available and GL_RGBA needs to be used.

Yeah, I like the idea of doing this at the service level. Without something like
OES_required_internalformat there's no guarantee about what the driver will do,
and if we can limit that magic to the service side a lot of the other code
becomes simpler.

> I suspect that using GL_RGBA with drivers that support GL_RGBA4 might result
in
> 32bpp textures and just a bunch of work converting the pixel data. I think we
> need a system that allows us to avoid that.
> 
> Also, how does the driver know that it should use 16bpp if the texture is
> initially defined without any pixel data? I guess we rely on that not being
the
> case?

Again we can't really tell what the driver will do, but I think if we ensure the
initial texture definition also uses GL_UNSIGNED_SHORT_4_4_4_4 as the format
with a NULL data pointer we should get the expected result.

[epennerAtGoogle, 2013-09-05 18:35:10]

> Again we can't really tell what the driver will do, but I think if we ensure
the
> initial texture definition also uses GL_UNSIGNED_SHORT_4_4_4_4 as the format
> with a NULL data pointer we should get the expected result.

GLES hasn't  even guaranteed 8 bits per pixel all of this time, but we have
always assumed we get that. So I think the actual GL involved is okay, and I
would say there is no need for format extensions unless there is actually a
tangible benefit.

However, passing around both format and type does seem counter intuitive and
could be error prone. Would it make sense to instead pass around a CC-specific
enum through most of the compositor, that doesn't even depend on GL?  And then
we choose the correct GL parameters based on that? That will limit the ugly
internal-format-assumptions to one place, and make it easy to change the GL
commands in response to extensions, without changing the compositor's code a
second time.

[kaanb, 2013-09-06 02:05:53]

Addressed all comments but 2. Uploading this so London folks can have a look
before the weekend. Also TextureUploader needs some cleanup as well.

https://codereview.chromium.org/21159007/diff/102001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1708: uint8_t* bitmap_pixels = NULL;
On 2013/09/05 14:13:26, Sami wrote:
> Nit: const uint8_t*

Done.

https://codereview.chromium.org/21159007/diff/102001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1710: SkBitmap bitmap_16;
On 2013/09/05 02:01:19, enne wrote:
> This should probably be a member along with on_demand_tile_raster_bitmap_.  I
> suspect the goal of making it a member is to not have to reallocate the buffer
> if it ends up being the same size.

Done.

https://codereview.chromium.org/21159007/diff/102001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1710: SkBitmap bitmap_16;
On 2013/09/05 14:13:26, Sami wrote:
> On 2013/09/05 02:01:19, enne wrote:
> > This should probably be a member along with on_demand_tile_raster_bitmap_. 
I
> > suspect the goal of making it a member is to not have to reallocate the
buffer
> > if it ends up being the same size.
> 
> Furthermore, since bitmap_16 is on the stack, bitmap_pixels will point to
free'd
> memory as soon as we leave this block. Maybe that explains the corruption you
> were seeing.

Great point. The overscroll glow is fixed now.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/image_raste...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:37:
task_->RunOnWorkerThread(thread_index, buffer_, task_->resource()->size());
On 2013/09/05 16:24:17, David Reveman wrote:
> you're no longer respecting the value of |stride_|.

Done.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/image_raste...
cc/resources/image_raster_worker_pool.cc:37:
task_->RunOnWorkerThread(thread_index, buffer_, task_->resource()->size());
On 2013/09/05 14:13:26, Sami wrote:
> Any reason not to pass the bitmap into RunOnWorkerThread instead of the raw
> buffer? I find passing void* pointers around like this a little scary.

Basically it is to ensure the result of copyTo ends up in transfer buffer
memory.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/prioritized...
File cc/resources/prioritized_resource.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/prioritized...
cc/resources/prioritized_resource.cc:31: // TODO(kaanb): check this codepath
On 2013/09/05 02:01:19, enne wrote:
> Hopefully this codepath is not hit on Android once you got rid of the
ImageLayer
> for the glow?

We'll probably disable the glow, however with the latest version of the patch it
started working again.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:131: SkBitmap bitmap_32;
On 2013/09/05 14:13:26, Sami wrote:
> If we gave this function an SkBitmapDevice instead of SkBaseDevice we wouldn't
> need to change much of the existing bitmap allocation logic. To convert to 16
> bit you could then do this:
> 
> SkBitmap bitmap_32;
> bitmap_32.setPixelRef(device->getPixelRef());
> bitmap_32.copyTo(&bitmap_16, SkBitmap::kARGB_4444_Config, &allocator);

Apparently getPixelRef is protected not public, so cannot be called from here.
Also I need the PixelBuffer pointer here to pass to the IdentityAllocator
otherwise copyTo would copy over to some random place in heap and not into the
transfer buffer memory.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:135: bitmap_32.allocPixels();
On 2013/09/05 16:24:17, David Reveman wrote:
> we shouldn't be allocating a temporary bitmap unless necessary. not clear this
> code works at all with 32bpp resources. how is |buffer| modified in that case?

Good point. Fixed.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:180: bitmap_32.copyTo(&bitmap_16,
SkBitmap::kARGB_4444_Config, &allocator);
On 2013/09/05 14:13:26, Sami wrote:
> On 2013/09/05 02:01:19, enne wrote:
> > Just to be clear, this is copying from the 8888 bitmap stored in buffer onto
> > buffer itself as a 4444 bitmap? Is that safe?
> 
> If I read this right we're copying from bitmap_32 (that was allocated storage
on
> line 135) to bitmap_16 which points to buffer. Tom and I think it should be
> safe. Furthermore copyTo() internally uses a temporary allocation so you can
> also copy from a bitmap to itself.

Also, note that we want the destination of the buffer to be the PixelBuffer
allocated in ResourceProvider::AcquirePixelBuffer so we do the IdentityAllocator
trick.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:180: bitmap_32.copyTo(&bitmap_16,
SkBitmap::kARGB_4444_Config, &allocator);
On 2013/09/05 02:01:19, enne wrote:
> Just to be clear, this is copying from the 8888 bitmap stored in buffer onto
> buffer itself as a 4444 bitmap? Is that safe?

Actually 8888 bitmap uses temporary heap memory in this flow.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource.cc
File cc/resources/resource.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource.cc...
cc/resources/resource.cc:17: size_t Resource::BytesPerPixel(GLenum format, bool
is_16_bit_resource) {
On 2013/09/05 14:13:26, Sami wrote:
> On 2013/09/05 01:54:15, epennerAtGoogle wrote:
> > Could we just pass in the GL type here rather than the extra boolean? And
then
> > DCHECK that only the supported combos of format/type are passed in?
> 
> +1, I think that would make this a lot cleaner.

On my TODO list for tomorrow.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_po...
File cc/resources/resource_pool.h (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_po...
cc/resources/resource_pool.h:25: bool use_16_bit_resource);
On 2013/09/05 01:54:15, epennerAtGoogle wrote:
> Can we avoid this boolean in favor of a real format/type? There are many
> formats/types that will result in 16 bits.

Done.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:688: best_texture_format_(0),
On 2013/09/05 01:54:15, epennerAtGoogle wrote:
> Could we have best_texture_type in addition to best_texture_format, and avoid
> any #ifdefs/booleans?

Done.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:693: use_16_bit_textures_ = true;
On 2013/09/05 02:01:19, enne wrote:
> Please make this a LayerTreeSetting and keep the #ifdef out of cc so that this
> setting can be tested in a device-independent manner.

Done.

https://codereview.chromium.org/21159007/diff/102001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1175: GLenum texture_data_type =
use_16_bit_textures_
On 2013/09/05 02:01:19, enne wrote:
> Rather than having to pass around booleans and settings that force every
texture
> into one mode or the other, can resources just have a texture_data_type value?

> Maybe there could just be a LayerTreeSetting for preferred texture_data_type
and
> ResourceProvider could try to respect that when generating resources.
> 
> That way you could (potentially) have two different formats at once and cc
gets
> a feature rather than a mode.

Great point, I didn't have time for it today. I'll look into it tomorrow.

https://codereview.chromium.org/21159007/diff/102001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/102001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:214: // TODO(kaanb): anything to do to
GL_UNPACK_ALIGNMENT for 16_bit?
On 2013/09/05 14:13:26, Sami wrote:
> Yes, it needs to be set to 2 bytes. Otherwise odd-width bitmaps will look,
well,
> odd :) The alternative is to allocate our 16 bit source bitmaps with 4-byte
> aligned strides. I'm not sure if Skia supports that.

Silly question, how and where should it be set?

https://codereview.chromium.org/21159007/diff/102001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.h (right):

https://codereview.chromium.org/21159007/diff/102001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.h:26: class CC_EXPORT TextureUploader {
On 2013/09/05 16:24:17, David Reveman wrote:
> Do we need support for this in the old uploader? Can we not just limit it to
> impl-side painting?

Raster-on-demand still uses this class. Otherwise I get weird GL errors that
cause flickering.

https://codereview.chromium.org/21159007/diff/102001/content/browser/android/...
File content/browser/android/overscroll_glow.cc (right):

https://codereview.chromium.org/21159007/diff/102001/content/browser/android/...
content/browser/android/overscroll_glow.cc:49:
scoped_refptr<cc::PictureImageLayer> layer = cc::PictureImageLayer::Create();
On 2013/09/05 02:01:19, enne wrote:
> This seems like the right change to make so that Android isn't using old-style
> TiledLayer, but did you figure out what else was wrong with the glow?

Probably bitmap_16 being on stack that Sami pointed out. jdduke@ tells me this
class will be refactored soon.

Yet, we may disable the glow on low-end devices due to other reasons.

https://codereview.chromium.org/21159007/diff/102001/ui/gfx/android/java_bitm...
File ui/gfx/android/java_bitmap.cc (right):

https://codereview.chromium.org/21159007/diff/102001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.cc:95: if (base::android::SysUtils::IsLowEndDevice())
{
On 2013/09/05 14:13:26, Sami wrote:
> Is this mainly for UI resources? You could consider landing this change
> separately since it's not really connected to the compositor changes.

Done.

https://codereview.chromium.org/21159007/diff/102001/ui/gfx/android/java_bitm...
ui/gfx/android/java_bitmap.cc:111: return skia::ImageOperations::Resize(bitmap,
On 2013/09/05 14:13:26, Sami wrote:
> Should we do 16 bit here too?

Removed this file from the patch since the overscroll glow is working now.

[epennerAtGoogle, 2013-09-06 03:24:44]

Just a couple comments, only regarding the addition of the Enum. I didn't look
at the rest yet.

I think the enum is only useful if it can replace _both_ the format and type
parameters. The problem with 'GLEnum format' is that it doesn't mean anything
and doesn't distinguish 8888 vs 4444 to GLES. So the compositor needs to pass
the CC-Enum everywhere, and then determine the format/type when invoking GL.

Desktop GL can do fancy type conversions during upload, but we don't need those
in the compositor, so one Enum should be  enough to specify both the source
pixel data format and the format, for GLES.

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource_po...
File cc/resources/resource_pool.cc (right):

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource_po...
cc/resources/resource_pool.cc:13: GLenum format,
Again, I we should be able to have one Enum that fully specifies both format and
type (for our purposes at least). So we shouldn't need the GLenum?

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (left):

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource_pr...
cc/resources/resource_provider.h:67: GLenum best_texture_format() const { return
best_texture_format_; }
I was hoping we could get rid of the GLenum completely here. That would involve
changing any code that calls this to respect the Enum. The problem with
returning the GLenum is that it is RGBA for both 4444 and 8888.

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource_pr...
cc/resources/resource_provider.h:55: enum TextureType {
I'm interested in what others think here, but I was thinking this could replace
both the format and and type parameters throughout resource provider. Then, only
when we invoke GL, we can choose the GL format and GL type based on this Enum.

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource_pr...
cc/resources/resource_provider.h:57: RGBA_4444,
Do we need BGRA_8888 also? The idea to use this enum is that it would
encapsulate _both_ the format and type parameters, and I believe we have
BGRA_EXT used as a format elsewhere.

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource_pr...
cc/resources/resource_provider.h:62: bool use_rgba_4444_textures);
How about TextureType best_texture_type, and no boolean?

[reveman, 2013-09-06 03:47:14]

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource_pr...
cc/resources/resource_provider.h:55: enum TextureType {
On 2013/09/06 03:24:44, epennerAtGoogle wrote:
> I'm interested in what others think here, but I was thinking this could
replace
> both the format and and type parameters throughout resource provider. Then,
only
> when we invoke GL, we can choose the GL format and GL type based on this Enum.

Maybe OK to just use the internal formats defined by standard GL. ie. GL_RGBA8,
GL_RGBA4, GL_BGRA and just translate those to supported
internalformat,pixel-data-format,pixel-data-type. If we think that might be
confusing then new enum values such as these are fine with me.

Whatever we do, it should replace format and and type parameters throughout RP
as you already mentioned.

[epennerAtGoogle, 2013-09-06 04:22:44]

> Maybe OK to just use the internal formats defined by standard GL. ie.
GL_RGBA8,
> GL_RGBA4, GL_BGRA and just translate those to supported
> internalformat,pixel-data-format,pixel-data-type. If we think that might be
> confusing then new enum values such as these are fine with me.
> 
> Whatever we do, it should replace format and and type parameters throughout RP
> as you already mentioned.

That works with me too. That also sounds easier as a first pass (we can always
follow up to make our own enum).

The take away is, we want one parameter for this, and no booleans except maybe
as a setting that is used to determine the best format initially.

[Sami, 2013-09-06 12:55:41]

>
https://codereview.chromium.org/21159007/diff/102001/cc/resources/image_raste...
> cc/resources/image_raster_worker_pool.cc:37:
> task_->RunOnWorkerThread(thread_index, buffer_, task_->resource()->size());
> On 2013/09/05 14:13:26, Sami wrote:
> > Any reason not to pass the bitmap into RunOnWorkerThread instead of the raw
> > buffer? I find passing void* pointers around like this a little scary.
> 
> Basically it is to ensure the result of copyTo ends up in transfer buffer
> memory.

Right. I was wondering whether we could just create the bitmap here (with the
identity allocator) so that the rest of the code wouldn't need to deal with
opaque pointers. I guess it is a little tricky to avoid reallocating the bitmap
by mistake. Maybe passing the pixel ref to the worker would work?
 
>
https://codereview.chromium.org/21159007/diff/102001/cc/resources/prioritized...
> File cc/resources/prioritized_resource.cc (right):
> 
>
https://codereview.chromium.org/21159007/diff/102001/cc/resources/prioritized...
> cc/resources/prioritized_resource.cc:31: // TODO(kaanb): check this codepath
> On 2013/09/05 02:01:19, enne wrote:
> > Hopefully this codepath is not hit on Android once you got rid of the
> ImageLayer
> > for the glow?
> 
> We'll probably disable the glow, however with the latest version of the patch
it
> started working again.
> 
>
https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
> File cc/resources/raster_worker_pool.cc (right):
> 
>
https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
> cc/resources/raster_worker_pool.cc:131: SkBitmap bitmap_32;
> Apparently getPixelRef is protected not public, so cannot be called from here.
> Also I need the PixelBuffer pointer here to pass to the IdentityAllocator
> otherwise copyTo would copy over to some random place in heap and not into the
> transfer buffer memory.

Ah, too bad, I missed that it was protected.

>
https://codereview.chromium.org/21159007/diff/102001/cc/scheduler/texture_upl...
> cc/scheduler/texture_uploader.cc:214: // TODO(kaanb): anything to do to
> GL_UNPACK_ALIGNMENT for 16_bit?
> On 2013/09/05 14:13:26, Sami wrote:
> > Yes, it needs to be set to 2 bytes. Otherwise odd-width bitmaps will look,
> well,
> > odd :) The alternative is to allocate our 16 bit source bitmaps with 4-byte
> > aligned strides. I'm not sure if Skia supports that.
> 
> Silly question, how and where should it be set?

Looks like Skia doesn't really let you override the bitmap stride, so we'll need
to call glPixelStorei(GL_UNPACK_ALIGNMENT, 2) before uploading 16 bit data and
restore the default with glPixelStorei(GL_UNPACK_ALIGNMENT, 4) afterwards. I'm
not sure if the async upload code respects this setting.

[kaanb, 2013-09-07 01:36:20]

On 2013/09/06 12:55:41, Sami wrote:
> >
>
https://codereview.chromium.org/21159007/diff/102001/cc/resources/image_raste...
> > cc/resources/image_raster_worker_pool.cc:37:
> > task_->RunOnWorkerThread(thread_index, buffer_, task_->resource()->size());
> > On 2013/09/05 14:13:26, Sami wrote:
> > > Any reason not to pass the bitmap into RunOnWorkerThread instead of the
raw
> > > buffer? I find passing void* pointers around like this a little scary.
> > 
> > Basically it is to ensure the result of copyTo ends up in transfer buffer
> > memory.
> 
> Right. I was wondering whether we could just create the bitmap here (with the
> identity allocator) so that the rest of the code wouldn't need to deal with
> opaque pointers. I guess it is a little tricky to avoid reallocating the
bitmap
> by mistake. Maybe passing the pixel ref to the worker would work?

I'll give it a shot and let you know how it goes.
  
> >
>
https://codereview.chromium.org/21159007/diff/102001/cc/resources/prioritized...
> > File cc/resources/prioritized_resource.cc (right):
> > 
> >
>
https://codereview.chromium.org/21159007/diff/102001/cc/resources/prioritized...
> > cc/resources/prioritized_resource.cc:31: // TODO(kaanb): check this codepath
> > On 2013/09/05 02:01:19, enne wrote:
> > > Hopefully this codepath is not hit on Android once you got rid of the
> > ImageLayer
> > > for the glow?
> > 
> > We'll probably disable the glow, however with the latest version of the
patch
> it
> > started working again.
> > 
> >
>
https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
> > File cc/resources/raster_worker_pool.cc (right):
> > 
> >
>
https://codereview.chromium.org/21159007/diff/102001/cc/resources/raster_work...
> > cc/resources/raster_worker_pool.cc:131: SkBitmap bitmap_32;
> > Apparently getPixelRef is protected not public, so cannot be called from
here.
> > Also I need the PixelBuffer pointer here to pass to the IdentityAllocator
> > otherwise copyTo would copy over to some random place in heap and not into
the
> > transfer buffer memory.
> 
> Ah, too bad, I missed that it was protected.
> 
> >
>
https://codereview.chromium.org/21159007/diff/102001/cc/scheduler/texture_upl...
> > cc/scheduler/texture_uploader.cc:214: // TODO(kaanb): anything to do to
> > GL_UNPACK_ALIGNMENT for 16_bit?
> > On 2013/09/05 14:13:26, Sami wrote:
> > > Yes, it needs to be set to 2 bytes. Otherwise odd-width bitmaps will look,
> > well,
> > > odd :) The alternative is to allocate our 16 bit source bitmaps with
4-byte
> > > aligned strides. I'm not sure if Skia supports that.
> > 
> > Silly question, how and where should it be set?
> 
> Looks like Skia doesn't really let you override the bitmap stride, so we'll
need
> to call glPixelStorei(GL_UNPACK_ALIGNMENT, 2) before uploading 16 bit data and
> restore the default with glPixelStorei(GL_UNPACK_ALIGNMENT, 4) afterwards. I'm
> not sure if the async upload code respects this setting.

I had a chat regarding GL_UNPACK_ALIGNMENT with epenner@ and he thinks we should
be fine since all our allocations are multiples of 4. I'm no expert on this area
so I'll leave it to you to figure out what needs to be done.

[epennerAtGoogle, 2013-09-07 02:00:33]

> I had a chat regarding GL_UNPACK_ALIGNMENT with epenner@ and he thinks we
should
> be fine since all our allocations are multiples of 4. I'm no expert on this
area
> so I'll leave it to you to figure out what needs to be done.

I guess in resource provider we could have an 13x13 texture or something. If we
don't
add any code, we should at least add DCHECKs that the size is a multiple of 2?

[Sami, 2013-09-09 13:28:40]

On 2013/09/07 02:00:33, epennerAtGoogle wrote:
> > I had a chat regarding GL_UNPACK_ALIGNMENT with epenner@ and he thinks we
> should
> > be fine since all our allocations are multiples of 4. I'm no expert on this
> area
> > so I'll leave it to you to figure out what needs to be done.
> 
> I guess in resource provider we could have an 13x13 texture or something. If
we
> don't
> add any code, we should at least add DCHECKs that the size is a multiple of 2?

At minimum let's add a CHECK/DCHECK for this.

[vangelis, 2013-09-10 01:52:18]

https://codereview.chromium.org/21159007/diff/125001/cc/resources/prioritized...
File cc/resources/prioritized_resource.cc (right):

https://codereview.chromium.org/21159007/diff/125001/cc/resources/prioritized...
cc/resources/prioritized_resource.cc:31: // TODO(kaanb): check this codepath
Is this TODO still valid?

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource.cc
File cc/resources/resource.cc (right):

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource.cc...
cc/resources/resource.cc:14: return MemorySizeBytes(size_, format_,
is_16_bit_resource_);
It would be cleaner if this parameter was a bytes_per_component rather than a
boolean is_16_bit_resource (it's not self-explanatory what the
!is_16_bit_resource_ really means).

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource.cc...
cc/resources/resource.cc:22: components_per_pixel = (is_16_bit_resource) ? 2 :
4;
I think what you want to change here is the bytes_per_component, not the
components_per_pixel.

[kaanb, 2013-09-11 04:01:51]

Rebased and uploaded a new version. The only TODO remaining is to unify the
format and TextureType parameters. It would be great if everyone could take
another look before I do that.

https://codereview.chromium.org/21159007/diff/125001/cc/resources/prioritized...
File cc/resources/prioritized_resource.cc (right):

https://codereview.chromium.org/21159007/diff/125001/cc/resources/prioritized...
cc/resources/prioritized_resource.cc:31: // TODO(kaanb): check this codepath
On 2013/09/10 01:52:19, vangelis wrote:
> Is this TODO still valid?

Removed.

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource.cc
File cc/resources/resource.cc (right):

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource.cc...
cc/resources/resource.cc:14: return MemorySizeBytes(size_, format_,
is_16_bit_resource_);
On 2013/09/10 01:52:19, vangelis wrote:
> It would be cleaner if this parameter was a bytes_per_component rather than a
> boolean is_16_bit_resource (it's not self-explanatory what the
> !is_16_bit_resource_ really means). 

I changed it to be the texture type.

https://codereview.chromium.org/21159007/diff/125001/cc/resources/resource.cc...
cc/resources/resource.cc:22: components_per_pixel = (is_16_bit_resource) ? 2 :
4;
On 2013/09/10 01:52:19, vangelis wrote:
> I think what you want to change here is the bytes_per_component, not the
> components_per_pixel.

Yes, moved this method to ResourceProvider and I renamed bytes_per_component to
bits_per_component.

[vangelis, 2013-09-11 06:11:36]

https://codereview.chromium.org/21159007/diff/139001/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.cc:7: #include <vector>
Is this new include needed?

https://codereview.chromium.org/21159007/diff/139001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:665: GL_RGBA,
Is the switch from RGB to RGBA done on purpose here?

https://codereview.chromium.org/21159007/diff/139001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:139: bitmap_32.allocPixels();
Is there some way we could avoid re-allocating this temporary buffer?  Maybe
cache and re-use it between invocations?

[epennerAtGoogle, 2013-09-11 19:21:03]

Few comments of the same theme again. Getting close IMO.

https://codereview.chromium.org/21159007/diff/139001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.cc:98: GL_RGBA,
Isn't this redundant, with RGBA_8888 being specified below?

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource.h#...
cc/resources/resource.h:24: format_(format),
In what case can format vary independently from Type?

Can't we just remove it from the constructor and set the format based on type?

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
cc/resources/resource_provider.h:90: TextureType texture_type);
This is nice, IMO :)

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
cc/resources/resource_provider.h:343: static GLenum GetTextureDataType(GLenum
texture_type);
Could we call this GetGLTextureType, and also add a function GetGLTextureFormat,
and get rid of 'GLenum format' in the remaining few places? It looks like we are
almost there. Is there a reason to keep 'GLenum format' as a parameter anywhere?

[reveman, 2013-09-11 20:19:12]

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
cc/resources/resource_provider.h:56: INVALID_TYPE,
Do we need INVALID_TYPE? It's easier to maintain and add new enum values if all
values are valid.

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
cc/resources/resource_provider.h:90: TextureType texture_type);
Could the enum type be TextureFormat instead and replace "GLenum format"?

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
cc/resources/resource_provider.h:343: static GLenum GetTextureDataType(GLenum
texture_type);
On 2013/09/11 19:21:04, epennerAtGoogle wrote:
> Could we call this GetGLTextureType, and also add a function
GetGLTextureFormat,
> and get rid of 'GLenum format' in the remaining few places? It looks like we
are
> almost there. Is there a reason to keep 'GLenum format' as a parameter
anywhere?

Yes, something like the following set of helper functions would be nice:
static size_t GetBytesPerPixel(TextureFormat format);
static GLenum GetGLInternalFormat(TextureFormat format);
static GLenum GetGLDataFormat(TextureFormat format);
static GLenum GetGLDataType(TextureFormat format);

[kaanb, 2013-09-12 07:53:42]

I got rid of all GLenum formats throughtout cc/ All of cc/ now uses
ResourceProvider::TextureFormat to specify texture formats.

Please take a look again.

https://codereview.chromium.org/21159007/diff/139001/cc/layers/heads_up_displ...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/layers/heads_up_displ...
cc/layers/heads_up_display_layer_impl.cc:98: GL_RGBA,
On 2013/09/11 19:21:04, epennerAtGoogle wrote:
> Isn't this redundant, with RGBA_8888 being specified below?

Done.

https://codereview.chromium.org/21159007/diff/139001/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.cc:7: #include <vector>
On 2013/09/11 06:11:37, vangelis wrote:
> Is this new include needed? 

include-what-you-use presubmit script asked me to add it.

https://codereview.chromium.org/21159007/diff/139001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:665: GL_RGBA,
On 2013/09/11 06:11:37, vangelis wrote:
> Is the switch from RGB to RGBA done on purpose here? 

Yes, epenner@ thought it was a bug we were using GL_RGB here.

https://codereview.chromium.org/21159007/diff/139001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:139: bitmap_32.allocPixels();
On 2013/09/11 06:11:37, vangelis wrote:
> Is there some way we could avoid re-allocating this temporary buffer?  Maybe
> cache and re-use it between invocations? 

It may be difficult as this method could get called with different sizes,
reveman@ any ideas?

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource.h
File cc/resources/resource.h (right):

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource.h#...
cc/resources/resource.h:24: format_(format),
On 2013/09/11 19:21:04, epennerAtGoogle wrote:
> In what case can format vary independently from Type?
> 
> Can't we just remove it from the constructor and set the format based on type?

Done.

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
cc/resources/resource_provider.h:56: INVALID_TYPE,
On 2013/09/11 20:19:13, David Reveman wrote:
> Do we need INVALID_TYPE? It's easier to maintain and add new enum values if
all
> values are valid.

There were cases where 0 was being used as the GL format, so I replaced them
with INVALID_FORMAT

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
cc/resources/resource_provider.h:90: TextureType texture_type);
On 2013/09/11 20:19:13, David Reveman wrote:
> Could the enum type be TextureFormat instead and replace "GLenum format"?

Done.

https://codereview.chromium.org/21159007/diff/139001/cc/resources/resource_pr...
cc/resources/resource_provider.h:343: static GLenum GetTextureDataType(GLenum
texture_type);
On 2013/09/11 20:19:13, David Reveman wrote:
> On 2013/09/11 19:21:04, epennerAtGoogle wrote:
> > Could we call this GetGLTextureType, and also add a function
> GetGLTextureFormat,
> > and get rid of 'GLenum format' in the remaining few places? It looks like we
> are
> > almost there. Is there a reason to keep 'GLenum format' as a parameter
> anywhere?
> 
> Yes, something like the following set of helper functions would be nice:
> static size_t GetBytesPerPixel(TextureFormat format);
> static GLenum GetGLInternalFormat(TextureFormat format);
> static GLenum GetGLDataFormat(TextureFormat format);
> static GLenum GetGLDataType(TextureFormat format);

Done.

[reveman, 2013-09-12 15:57:38]

https://codereview.chromium.org/21159007/diff/139001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:138: if (texture_type_ ==
ResourceProvider::RGBA_4444) {
please make this a switch statement. that way the compiler will tell anyone
adding a new format that they need to update this code.

https://codereview.chromium.org/21159007/diff/139001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:139: bitmap_32.allocPixels();
On 2013/09/12 07:53:44, kaanb wrote:
> On 2013/09/11 06:11:37, vangelis wrote:
> > Is there some way we could avoid re-allocating this temporary buffer?  Maybe
> > cache and re-use it between invocations? 
> 
> It may be difficult as this method could get called with different sizes,
> reveman@ any ideas?

you could keep an array of cached bitmaps based on thread_index but I would
prefer if that was done as a follow up and not part of this patch.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/118001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:233: ResourceProvider::TextureFormat
texture_format_;
Don't think you need this. You can get the format using Resource::format().

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:45: default:
Remove "default:" after removing INVALID_FORMAT enum value.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:56: format ==
cc::ResourceProvider::BGRA_8888);
please change this to a switch statement now that format is an enum.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:944: static_cast<TextureFormat>(it->format));
I think we should avoid this static_cast. Can you move the conversion to a
switch statement above that guarantees that the result is always a valid
TextureFormat enum value?

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:999: resource->format =
GetGLDataFormat(source->texture_format);
I think this should be "resource->format = source->texture_format;" as that is
what the conversion code above assumes.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1214:
GetGLDataFormat(resource->texture_format),
I think we should use a separate GetGLInternalFormat() function here even if
that might return the same thing as GetGLDataFormat right now. It makes it more
clear that these are not necessarily the same thing.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1358: GetGLDataFormat(format),
GetGLInternalFormat

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1479: default:
remove "default:" case after removing INVALID_FORMAT.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1492: default:
remove "default:" case after removing INVALID_FORMAT.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1510: default:
remove "default:" case after removing INVALID_FORMAT.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1530: default:
remove "default:" case after removing INVALID_FORMAT.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.h:55: enum TextureFormat {
This will need to be set properly for software resources too. Should it just be
"enum Format" instead?

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.h:56: INVALID_FORMAT,
what's needed to remove INVALID_FORMAT? Is this just the default value? Can we
just use RGBA_8888 as default value instead? I think not having that enum value
will improve the code significantly.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/transferabl...
File cc/resources/transferable_resource.h (right):

https://codereview.chromium.org/21159007/diff/118001/cc/resources/transferabl...
cc/resources/transferable_resource.h:36: uint32 texture_type;
don't think you need this anymore

[vangelis, 2013-09-12 16:41:55]

https://codereview.chromium.org/21159007/diff/139001/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.cc:7: #include <vector>
On 2013/09/12 07:53:44, kaanb wrote:
> On 2013/09/11 06:11:37, vangelis wrote:
> > Is this new include needed? 
> 
> include-what-you-use presubmit script asked me to add it.

Hmm... Maybe from some previous incarnation of the code?  I'm not seeing any new
<vector> stuff in your diff.

[kaanb, 2013-09-13 00:11:07]

https://codereview.chromium.org/21159007/diff/139001/cc/layers/texture_layer_...
File cc/layers/texture_layer_impl.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/layers/texture_layer_...
cc/layers/texture_layer_impl.cc:7: #include <vector>
On 2013/09/12 16:41:56, vangelis wrote:
> On 2013/09/12 07:53:44, kaanb wrote:
> > On 2013/09/11 06:11:37, vangelis wrote:
> > > Is this new include needed? 
> > 
> > include-what-you-use presubmit script asked me to add it.
> 
> Hmm... Maybe from some previous incarnation of the code?  I'm not seeing any
new
> <vector> stuff in your diff.

vector is being used in line 116. I think the presubmit started enforcing the
include-what-you-use after that code was added.

https://codereview.chromium.org/21159007/diff/139001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/139001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:138: if (texture_type_ ==
ResourceProvider::RGBA_4444) {
On 2013/09/12 15:57:39, David Reveman wrote:
> please make this a switch statement. that way the compiler will tell anyone
> adding a new format that they need to update this code.

Done.

https://codereview.chromium.org/21159007/diff/139001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:139: bitmap_32.allocPixels();
On 2013/09/12 15:57:39, David Reveman wrote:
> On 2013/09/12 07:53:44, kaanb wrote:
> > On 2013/09/11 06:11:37, vangelis wrote:
> > > Is there some way we could avoid re-allocating this temporary buffer? 
Maybe
> > > cache and re-use it between invocations? 
> > 
> > It may be difficult as this method could get called with different sizes,
> > reveman@ any ideas?
> 
> you could keep an array of cached bitmaps based on thread_index but I would
> prefer if that was done as a follow up and not part of this patch.

Okay.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/118001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:233: ResourceProvider::TextureFormat
texture_format_;
On 2013/09/12 15:57:40, David Reveman wrote:
> Don't think you need this. You can get the format using Resource::format().

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:45: default:
On 2013/09/12 15:57:40, David Reveman wrote:
> Remove "default:" after removing INVALID_FORMAT enum value.

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:56: format ==
cc::ResourceProvider::BGRA_8888);
On 2013/09/12 15:57:40, David Reveman wrote:
> please change this to a switch statement now that format is an enum.

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:944: static_cast<TextureFormat>(it->format));
On 2013/09/12 15:57:40, David Reveman wrote:
> I think we should avoid this static_cast. Can you move the conversion to a
> switch statement above that guarantees that the result is always a valid
> TextureFormat enum value?

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:999: resource->format =
GetGLDataFormat(source->texture_format);
On 2013/09/12 15:57:40, David Reveman wrote:
> I think this should be "resource->format = source->texture_format;" as that is
> what the conversion code above assumes.

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1214:
GetGLDataFormat(resource->texture_format),
On 2013/09/12 15:57:40, David Reveman wrote:
> I think we should use a separate GetGLInternalFormat() function here even if
> that might return the same thing as GetGLDataFormat right now. It makes it
more
> clear that these are not necessarily the same thing.

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1358: GetGLDataFormat(format),
On 2013/09/12 15:57:40, David Reveman wrote:
> GetGLInternalFormat

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1479: default:
On 2013/09/12 15:57:40, David Reveman wrote:
> remove "default:" case after removing INVALID_FORMAT.

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1492: default:
On 2013/09/12 15:57:40, David Reveman wrote:
> remove "default:" case after removing INVALID_FORMAT.

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1510: default:
On 2013/09/12 15:57:40, David Reveman wrote:
> remove "default:" case after removing INVALID_FORMAT.

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1530: default:
On 2013/09/12 15:57:40, David Reveman wrote:
> remove "default:" case after removing INVALID_FORMAT.

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.h:55: enum TextureFormat {
On 2013/09/12 15:57:40, David Reveman wrote:
> This will need to be set properly for software resources too. Should it just
be
> "enum Format" instead?

Done.

https://codereview.chromium.org/21159007/diff/118001/cc/resources/resource_pr...
cc/resources/resource_provider.h:56: INVALID_FORMAT,
On 2013/09/12 15:57:40, David Reveman wrote:
> what's needed to remove INVALID_FORMAT? Is this just the default value? Can we
> just use RGBA_8888 as default value instead? I think not having that enum
value
> will improve the code significantly.

Done.

[kaanb, 2013-09-13 00:15:52]

Ready for a final look,

adding piman@ for OWNERS approval for gpu/
adding kbr@ for OWNERS approval for content/renderer/gpu

[enne (OOO), 2013-09-13 01:39:43]

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:235: DCHECK(texture_format == RGBA_8888);
DCHECK_EQ, here and elsewhere.

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:944: case 0:
If somebody switched the enum order, this would break.  Can you make this case
RGBA_8888: format = RGBA_8888, etc?

Alternatively, why not just static cast and DCHECK?

https://codereview.chromium.org/21159007/diff/157001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:107: cc::ResourceProvider::Format GetTextureFormat(
Should ResourceProvider::BestTextureFormat just take into account the
use_rgba_4444_tiles setting?

https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:284: unsigned int bytes_per_pixel =
ResourceProvider::BytesPerPixel(format);
unsigned int => size_t, here and elsewhere.

[piman, 2013-09-13 01:41:59]

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:956: }
Why not make TransferableResource use ResourceProvider::Format?

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.h:343: static unsigned GetStride(Format format);
What does this represent, and how is it related to BytesPerPixel? "stride"
usually means the number of bytes per row, but that depends on the width...

https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/servi...
File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right):

https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/servi...
gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:55:
!base::android::SysUtils::IsLowEndDevice()) {
Why is this needed here?

[Ken Russell (switch to Gerrit), 2013-09-13 03:08:55]

content/renderer/gpu LGTM modulo one issue.

https://codereview.chromium.org/21159007/diff/157001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/21159007/diff/157001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:287:
base::android::SysUtils::IsLowEndDevice() &&
It looks like this line requires conditional compilation.

[kaanb, 2013-09-13 03:43:55]

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:235: DCHECK(texture_format == RGBA_8888);
On 2013/09/13 01:39:44, enne wrote:
> DCHECK_EQ, here and elsewhere.

Done.

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:944: case 0:
On 2013/09/13 01:39:44, enne wrote:
> If somebody switched the enum order, this would break.  Can you make this case
> RGBA_8888: format = RGBA_8888, etc?
> 
> Alternatively, why not just static cast and DCHECK?

reveman@ didn't like the static_cast

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:956: }
On 2013/09/13 01:42:01, piman wrote:
> Why not make TransferableResource use ResourceProvider::Format?

It looks like TransferableResource is part of an IPC message and compilation
fails when I make TransferableResource use ResourceProvider::Format

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.h:343: static unsigned GetStride(Format format);
On 2013/09/13 01:42:01, piman wrote:
> What does this represent, and how is it related to BytesPerPixel? "stride"
> usually means the number of bytes per row, but that depends on the width...

It is used by the TextureUploader to decide what value to use for rounding.

https://codereview.chromium.org/21159007/diff/157001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:107: cc::ResourceProvider::Format GetTextureFormat(
On 2013/09/13 01:39:44, enne wrote:
> Should ResourceProvider::BestTextureFormat just take into account the
> use_rgba_4444_tiles setting?

I want to ensure we use 4444 only for tile textures for now. There are frontend
resources that need 32-bit textures even for low end devices.

https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:284: unsigned int bytes_per_pixel =
ResourceProvider::BytesPerPixel(format);
On 2013/09/13 01:39:44, enne wrote:
> unsigned int => size_t, here and elsewhere.

Done.

https://codereview.chromium.org/21159007/diff/157001/content/renderer/gpu/ren...
File content/renderer/gpu/render_widget_compositor.cc (right):

https://codereview.chromium.org/21159007/diff/157001/content/renderer/gpu/ren...
content/renderer/gpu/render_widget_compositor.cc:287:
base::android::SysUtils::IsLowEndDevice() &&
On 2013/09/13 03:08:56, Ken Russell wrote:
> It looks like this line requires conditional compilation.

It is already in an #if defined(OS_ANDROID) block

https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/servi...
File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right):

https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/servi...
gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:55:
!base::android::SysUtils::IsLowEndDevice()) {
On 2013/09/13 01:42:01, piman wrote:
> Why is this needed here?

See crbug.com/271929, I also added a TODO here and referenced the bug.

[enne (OOO), 2013-09-13 03:53:24]

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:944: case 0:
On 2013/09/13 03:43:56, kaanb wrote:
> On 2013/09/13 01:39:44, enne wrote:
> > If somebody switched the enum order, this would break.  Can you make this
case
> > RGBA_8888: format = RGBA_8888, etc?
> > 
> > Alternatively, why not just static cast and DCHECK?
> 
> reveman@ didn't like the static_cast

I think static_cast + CHECK would be the cleanest, but I think even just using
enum values in each case statement would be an improvement over literals.

[piman, 2013-09-13 04:56:50]

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:956: }
On 2013/09/13 03:43:56, kaanb wrote:
> On 2013/09/13 01:42:01, piman wrote:
> > Why not make TransferableResource use ResourceProvider::Format?
> 
> It looks like TransferableResource is part of an IPC message and compilation
> fails when I make TransferableResource use ResourceProvider::Format

Sure, so, we should fix that.
Enums are fine to pass over IPC, you just need to declare them using
IPC_ENUM_TRAITS (see other instances in cc_messages.h).
It still needs to be checked on the receiving side (for security), but at least
you don't need a switch.

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.h:343: static unsigned GetStride(Format format);
On 2013/09/13 03:43:56, kaanb wrote:
> On 2013/09/13 01:42:01, piman wrote:
> > What does this represent, and how is it related to BytesPerPixel? "stride"
> > usually means the number of bytes per row, but that depends on the width...
> 
> It is used by the TextureUploader to decide what value to use for rounding.

GetStrideAlignment?
Please add a comment about what it means, since it's not obvious. But, see also
my comment there, I think this is not needed.

https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:217: ResourceProvider::GetStride(format));
I don't understand the change here, to accomodate 4444.
1- You check that source_rect.width() is multiple of 2. Since bytes_per_pixel is
2, then bytes_per_pixel*source_rect.width() is a multiple of 4 still.
2- if it wasn't, then you'd have a problem anyway. You're not changing the
GL_UNPACK_ALIGNMENT, so if rows were not aligned on 4 bytes, you'd corrupt the
upload and access invalid data.

So leaving it at 4 is actually more correct, and doesn't need the GetStride.

https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/servi...
File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right):

https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/servi...
gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:55:
!base::android::SysUtils::IsLowEndDevice()) {
On 2013/09/13 03:43:56, kaanb wrote:
> On 2013/09/13 01:42:01, piman wrote:
> > Why is this needed here?
> 
> See crbug.com/271929, I also added a TODO here and referenced the bug.

But then it means a compromised or buggy renderer can crash the gpu process (the
browser in the android case) on a !IsLowEndDevice(). Additional code is needed
to prevent the renderer from sending 4444 data through the AsyncPixelTransfer
mechanism, to make it fail instead of crashing.

[reveman, 2013-09-13 14:32:48]

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:956: }
On 2013/09/13 04:56:51, piman wrote:
> On 2013/09/13 03:43:56, kaanb wrote:
> > On 2013/09/13 01:42:01, piman wrote:
> > > Why not make TransferableResource use ResourceProvider::Format?
> > 
> > It looks like TransferableResource is part of an IPC message and compilation
> > fails when I make TransferableResource use ResourceProvider::Format
> 
> Sure, so, we should fix that.
> Enums are fine to pass over IPC, you just need to declare them using
> IPC_ENUM_TRAITS (see other instances in cc_messages.h).
> It still needs to be checked on the receiving side (for security), but at
least
> you don't need a switch.

What's the proper way to check that enum values are valid? Ideally we'd have an
enum_cast<Format>() impl that checks against min/max enum values but I think
that would require something like this to be done cleanly:
https://codereview.chromium.org/22911037/

Instead of the above, I was thinking something like this:

Format format = RGBA_8888;
switch (it->format) {
  case RGBA_8888:
  case RGBA_4444:
  case BGRA_8888:
  case LUMINANCE_8:
    format = static_cast<Format>(it->format);
    break;
  default:
    NOTREACHED();
}

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1522: texture_data_type =
GL_UNSIGNED_SHORT_4_4_4_4;
nit: I'd remove the temporary variable and just "return
GL_UNSIGNED_SHORT_4_4_4_4;" here and in functions below.

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.h:388: Format texture_format;
just "Format format;" as it doesn't just apply to texture backed GL resources.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:132: SkBitmap bitmap_32;
can we drop the _32 suffix here?

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:136: stride);
stride is not going to be correct here unless format is RGBA_8888 or BGRA_8888.
you should probably move this into the switch statement and not set stride in
the case of RGBA_4444.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:143: case ResourceProvider::LUMINANCE_8:
Using SkBitmap::kARGB_8888_Config for LUMINANCE_8 doesn't look right. I'd rather
we make it explicit that this format is not supported.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:148: SkBitmapDevice device_32(bitmap_32);
drop _32?

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:190:
"RasterWorkerPoolTaskImpl::RunRasterOnThread::Downsample");
If this is worth a trace event then I prefer if you moved it to a helper
function.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:194: }
This is currently hardcoded to only support RGBA_4444. I think it needs to be
generalized a bit. Maybe add a SkBitmapConfigFromFormat helper function that
determines the config to use with copyTo and you can also use that to determine
if the copy is needed. ie. bitmap.getConfig() !=
SkBitmapConfigFromFormat(format).

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:239: ResourceProvider::Format
texture_format_;
I don't think you need this. You can get the format from resource()->format().

https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:111: provider->best_texture_format();
should we instead just adjust best_texture_format() when use_rgba_4444_tiles is
used?

https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:736: 
nit: no need to add this blank line.

[reveman, 2013-09-13 14:59:47]

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:944: case 0:
On 2013/09/13 03:53:25, enne wrote:
> On 2013/09/13 03:43:56, kaanb wrote:
> > On 2013/09/13 01:39:44, enne wrote:
> > > If somebody switched the enum order, this would break.  Can you make this
> case
> > > RGBA_8888: format = RGBA_8888, etc?
> > > 
> > > Alternatively, why not just static cast and DCHECK?
> > 
> > reveman@ didn't like the static_cast
> 
> I think static_cast + CHECK would be the cleanest, but I think even just using
> enum values in each case statement would be an improvement over literals.

yes, it's the CHECK I want and I think a switch statement is the cleanest way to
do that at the moment.

[piman, 2013-09-13 17:06:29]

On Fri, Sep 13, 2013 at 7:32 AM, <reveman@chromium.org> wrote:

>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.cc<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc>
> File cc/resources/resource_**provider.cc (right):
>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.**cc#newcode956<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode956>
> cc/resources/resource_**provider.cc:956: }
> On 2013/09/13 04:56:51, piman wrote:
>
>> On 2013/09/13 03:43:56, kaanb wrote:
>> > On 2013/09/13 01:42:01, piman wrote:
>> > > Why not make TransferableResource use ResourceProvider::Format?
>> >
>> > It looks like TransferableResource is part of an IPC message and
>>
> compilation
>
>> > fails when I make TransferableResource use ResourceProvider::Format
>>
>
>  Sure, so, we should fix that.
>> Enums are fine to pass over IPC, you just need to declare them using
>> IPC_ENUM_TRAITS (see other instances in cc_messages.h).
>> It still needs to be checked on the receiving side (for security), but
>>
> at least
>
>> you don't need a switch.
>>
>
> What's the proper way to check that enum values are valid?


The best way is to check at the IPC level in the ParamTraits, by refusing
the message if the value is not in the enum.
Note that in C++, invalid enum values are undefined behavior, so you need
to check before casting to the enum.


> Ideally we'd
> have an enum_cast<Format>() impl that checks against min/max enum values
> but I think that would require something like this to be done cleanly:
>
https://codereview.chromium.**org/22911037/<https://codereview.chromium.org/2...
>
> Instead of the above, I was thinking something like this:
>
> Format format = RGBA_8888;
> switch (it->format) {
>   case RGBA_8888:
>   case RGBA_4444:
>   case BGRA_8888:
>   case LUMINANCE_8:
>     format = static_cast<Format>(it->**format);
>     break;
>   default:
>     NOTREACHED();
> }
>

Note that you can't NOTREACHED(). The browser can't trust the renderer. It
needs to deal with an invalid value.


> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.**cc#newcode1522<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode1522>
> cc/resources/resource_**provider.cc:1522: texture_data_type =
> GL_UNSIGNED_SHORT_4_4_4_4;
> nit: I'd remove the temporary variable and just "return
> GL_UNSIGNED_SHORT_4_4_4_4;" here and in functions below.
>
>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.h<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h>
> File cc/resources/resource_**provider.h (right):
>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.h#**newcode388<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h#newcode388>
> cc/resources/resource_**provider.h:388: Format texture_format;
> just "Format format;" as it doesn't just apply to texture backed GL
> resources.
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc>
> File cc/resources/raster_worker_**pool.cc (right):
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode132<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode132>
> cc/resources/raster_worker_**pool.cc:132: SkBitmap bitmap_32;
> can we drop the _32 suffix here?
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode136<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode136>
> cc/resources/raster_worker_**pool.cc:136: stride);
> stride is not going to be correct here unless format is RGBA_8888 or
> BGRA_8888. you should probably move this into the switch statement and
> not set stride in the case of RGBA_4444.
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode143<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode143>
> cc/resources/raster_worker_**pool.cc:143: case
> ResourceProvider::LUMINANCE_8:
> Using SkBitmap::kARGB_8888_Config for LUMINANCE_8 doesn't look right.
> I'd rather we make it explicit that this format is not supported.
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode148<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode148>
> cc/resources/raster_worker_**pool.cc:148: SkBitmapDevice
> device_32(bitmap_32);
> drop _32?
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode190<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode190>
> cc/resources/raster_worker_**pool.cc:190:
> "RasterWorkerPoolTaskImpl::**RunRasterOnThread::Downsample"**);
> If this is worth a trace event then I prefer if you moved it to a helper
> function.
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode194<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode194>
> cc/resources/raster_worker_**pool.cc:194: }
> This is currently hardcoded to only support RGBA_4444. I think it needs
> to be generalized a bit. Maybe add a SkBitmapConfigFromFormat helper
> function that determines the config to use with copyTo and you can also
> use that to determine if the copy is needed. ie. bitmap.getConfig() !=
> SkBitmapConfigFromFormat(**format).
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode239<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode239>
> cc/resources/raster_worker_**pool.cc:239: ResourceProvider::Format
> texture_format_;
> I don't think you need this. You can get the format from
> resource()->format().
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/tile_manager.cc<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc>
> File cc/resources/tile_manager.cc (right):
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/tile_manager.cc#**newcode111<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc#newcode111>
> cc/resources/tile_manager.cc:**111: provider->best_texture_format(**);
> should we instead just adjust best_texture_format() when
> use_rgba_4444_tiles is used?
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/tile_manager.cc#**newcode736<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc#newcode736>
> cc/resources/tile_manager.cc:**736:
> nit: no need to add this blank line.
>
>
https://codereview.chromium.**org/21159007/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kaanb, 2013-09-13 19:57:37]

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:944: case 0:
On 2013/09/13 14:59:48, David Reveman wrote:
> On 2013/09/13 03:53:25, enne wrote:
> > On 2013/09/13 03:43:56, kaanb wrote:
> > > On 2013/09/13 01:39:44, enne wrote:
> > > > If somebody switched the enum order, this would break.  Can you make
this
> > case
> > > > RGBA_8888: format = RGBA_8888, etc?
> > > > 
> > > > Alternatively, why not just static cast and DCHECK?
> > > 
> > > reveman@ didn't like the static_cast
> > 
> > I think static_cast + CHECK would be the cleanest, but I think even just
using
> > enum values in each case statement would be an improvement over literals.
> 
> yes, it's the CHECK I want and I think a switch statement is the cleanest way
to
> do that at the moment.

Done.

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:956: }
On 2013/09/13 14:32:49, David Reveman wrote:
> On 2013/09/13 04:56:51, piman wrote:
> > On 2013/09/13 03:43:56, kaanb wrote:
> > > On 2013/09/13 01:42:01, piman wrote:
> > > > Why not make TransferableResource use ResourceProvider::Format?
> > > 
> > > It looks like TransferableResource is part of an IPC message and
compilation
> > > fails when I make TransferableResource use ResourceProvider::Format
> > 
> > Sure, so, we should fix that.
> > Enums are fine to pass over IPC, you just need to declare them using
> > IPC_ENUM_TRAITS (see other instances in cc_messages.h).
> > It still needs to be checked on the receiving side (for security), but at
> least
> > you don't need a switch.
> 
> What's the proper way to check that enum values are valid? Ideally we'd have
an
> enum_cast<Format>() impl that checks against min/max enum values but I think
> that would require something like this to be done cleanly:
> https://codereview.chromium.org/22911037/
> 
> Instead of the above, I was thinking something like this:
> 
> Format format = RGBA_8888;
> switch (it->format) {
>   case RGBA_8888:
>   case RGBA_4444:
>   case BGRA_8888:
>   case LUMINANCE_8:
>     format = static_cast<Format>(it->format);
>     break;
>   default:
>     NOTREACHED();
> }

I implemented reveman's suggestion for now. When change the type of the format
field to ResourceProvider::Format I get the following error:
../../cc/resources/transferable_resource.h:33:3: error: 'ResourceProvider' does
not name a type

possibly because of the circular dependency? (i.e. resource_provider.h includes
transferable_resource.h and to add the enum field I need to include
resource_provider.h from transferable_resource.h)

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1522: texture_data_type =
GL_UNSIGNED_SHORT_4_4_4_4;
On 2013/09/13 14:32:49, David Reveman wrote:
> nit: I'd remove the temporary variable and just "return
> GL_UNSIGNED_SHORT_4_4_4_4;" here and in functions below.

Done.

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.h:343: static unsigned GetStride(Format format);
On 2013/09/13 04:56:51, piman wrote:
> On 2013/09/13 03:43:56, kaanb wrote:
> > On 2013/09/13 01:42:01, piman wrote:
> > > What does this represent, and how is it related to BytesPerPixel? "stride"
> > > usually means the number of bytes per row, but that depends on the
width...
> > 
> > It is used by the TextureUploader to decide what value to use for rounding.
> 
> GetStrideAlignment?
> Please add a comment about what it means, since it's not obvious. But, see
also
> my comment there, I think this is not needed.

Removed.

https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_pr...
cc/resources/resource_provider.h:388: Format texture_format;
On 2013/09/13 14:32:49, David Reveman wrote:
> just "Format format;" as it doesn't just apply to texture backed GL resources.

Done.

https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:217: ResourceProvider::GetStride(format));
On 2013/09/13 04:56:51, piman wrote:
> I don't understand the change here, to accomodate 4444.
> 1- You check that source_rect.width() is multiple of 2. Since bytes_per_pixel
is
> 2, then bytes_per_pixel*source_rect.width() is a multiple of 4 still.
> 2- if it wasn't, then you'd have a problem anyway. You're not changing the
> GL_UNPACK_ALIGNMENT, so if rows were not aligned on 4 bytes, you'd corrupt the
> upload and access invalid data.
> 
> So leaving it at 4 is actually more correct, and doesn't need the GetStride.

Done.

https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/servi...
File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right):

https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/servi...
gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:55:
!base::android::SysUtils::IsLowEndDevice()) {
On 2013/09/13 04:56:51, piman wrote:
> On 2013/09/13 03:43:56, kaanb wrote:
> > On 2013/09/13 01:42:01, piman wrote:
> > > Why is this needed here?
> > 
> > See crbug.com/271929, I also added a TODO here and referenced the bug.
> 
> But then it means a compromised or buggy renderer can crash the gpu process
(the
> browser in the android case) on a !IsLowEndDevice(). Additional code is needed
> to prevent the renderer from sending 4444 data through the AsyncPixelTransfer
> mechanism, to make it fail instead of crashing.

Can you clarify what you mean by "Additional code is needed"? I checked with
epenner@ and he tells me just a DCHECK would fire when we try to send 4444 data
and DCHECKs are NOOPs in production code.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:132: SkBitmap bitmap_32;
On 2013/09/13 14:32:49, David Reveman wrote:
> can we drop the _32 suffix here?

Done.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:136: stride);
On 2013/09/13 14:32:49, David Reveman wrote:
> stride is not going to be correct here unless format is RGBA_8888 or
BGRA_8888.
> you should probably move this into the switch statement and not set stride in
> the case of RGBA_4444.

Done.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:143: case ResourceProvider::LUMINANCE_8:
On 2013/09/13 14:32:49, David Reveman wrote:
> Using SkBitmap::kARGB_8888_Config for LUMINANCE_8 doesn't look right. I'd
rather
> we make it explicit that this format is not supported.

Done.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:148: SkBitmapDevice device_32(bitmap_32);
On 2013/09/13 14:32:49, David Reveman wrote:
> drop _32?

Done.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:190:
"RasterWorkerPoolTaskImpl::RunRasterOnThread::Downsample");
On 2013/09/13 14:32:49, David Reveman wrote:
> If this is worth a trace event then I prefer if you moved it to a helper
> function.

Done.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:194: }
On 2013/09/13 14:32:49, David Reveman wrote:
> This is currently hardcoded to only support RGBA_4444. I think it needs to be
> generalized a bit. Maybe add a SkBitmapConfigFromFormat helper function that
> determines the config to use with copyTo and you can also use that to
determine
> if the copy is needed. ie. bitmap.getConfig() !=
> SkBitmapConfigFromFormat(format).

Done.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:239: ResourceProvider::Format
texture_format_;
On 2013/09/13 14:32:49, David Reveman wrote:
> I don't think you need this. You can get the format from resource()->format().

Done.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:111: provider->best_texture_format();
On 2013/09/13 14:32:49, David Reveman wrote:
> should we instead just adjust best_texture_format() when use_rgba_4444_tiles
is
> used?

I want to make sure we only use 4444 for tile textures. I think enforcing it
here is cleaner.

https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:736: 
On 2013/09/13 14:32:49, David Reveman wrote:
> nit: no need to add this blank line.

Done.

[reveman, 2013-09-13 20:24:30]

https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:111: provider->best_texture_format();
On 2013/09/13 19:57:39, kaanb wrote:
> On 2013/09/13 14:32:49, David Reveman wrote:
> > should we instead just adjust best_texture_format() when use_rgba_4444_tiles
> is
> > used?
> 
> I want to make sure we only use 4444 for tile textures. I think enforcing it
> here is cleaner.

I'm not sure this is the right place though. Maybe it's better to add
ResourceProvider::best_tile_texture_format()?

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:191: highp_threshold_min));
nit: looks like this fits on one line

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:950: NOTREACHED();
As piman already pointed out, we can't use NOTREACHED here if this is the place
where we check the validity of the format value. Just removing this default case
and using RGBA_8888 for invalid values might be fine.

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1226: DCHECK(resource->format != RGBA_4444 ||
Can we avoid this format specific check? Shouldn't "(resource->size.width() % 2)
== 0" be true for all formats?

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1522: return GL_UNSIGNED_BYTE;
I think you can add a NOTREACHED() above this line.

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1535: return GL_RGBA;
and NOTREACHED() here too.

[piman, 2013-09-13 20:39:44]

On Fri, Sep 13, 2013 at 12:57 PM, <kaanb@chromium.org> wrote:

>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.cc<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc>
> File cc/resources/resource_**provider.cc (right):
>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.**cc#newcode944<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode944>
> cc/resources/resource_**provider.cc:944: case 0:
> On 2013/09/13 14:59:48, David Reveman wrote:
>
>> On 2013/09/13 03:53:25, enne wrote:
>> > On 2013/09/13 03:43:56, kaanb wrote:
>> > > On 2013/09/13 01:39:44, enne wrote:
>> > > > If somebody switched the enum order, this would break.  Can you
>>
> make this
>
>> > case
>> > > > RGBA_8888: format = RGBA_8888, etc?
>> > > >
>> > > > Alternatively, why not just static cast and DCHECK?
>> > >
>> > > reveman@ didn't like the static_cast
>> >
>> > I think static_cast + CHECK would be the cleanest, but I think even
>>
> just using
>
>> > enum values in each case statement would be an improvement over
>>
> literals.
>
>  yes, it's the CHECK I want and I think a switch statement is the
>>
> cleanest way to
>
>> do that at the moment.
>>
>
> Done.
>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.**cc#newcode956<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode956>
> cc/resources/resource_**provider.cc:956: }
>
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> On 2013/09/13 04:56:51, piman wrote:
>> > On 2013/09/13 03:43:56, kaanb wrote:
>> > > On 2013/09/13 01:42:01, piman wrote:
>> > > > Why not make TransferableResource use ResourceProvider::Format?
>> > >
>> > > It looks like TransferableResource is part of an IPC message and
>>
> compilation
>
>> > > fails when I make TransferableResource use
>>
> ResourceProvider::Format
>
>> >
>> > Sure, so, we should fix that.
>> > Enums are fine to pass over IPC, you just need to declare them using
>> > IPC_ENUM_TRAITS (see other instances in cc_messages.h).
>> > It still needs to be checked on the receiving side (for security),
>>
> but at
>
>> least
>> > you don't need a switch.
>>
>
>  What's the proper way to check that enum values are valid? Ideally
>>
> we'd have an
>
>> enum_cast<Format>() impl that checks against min/max enum values but I
>>
> think
>
>> that would require something like this to be done cleanly:
>>
https://codereview.chromium.**org/22911037/<https://codereview.chromium.org/2...
>>
>
>  Instead of the above, I was thinking something like this:
>>
>
>  Format format = RGBA_8888;
>> switch (it->format) {
>>    case RGBA_8888:
>>    case RGBA_4444:
>>    case BGRA_8888:
>>    case LUMINANCE_8:
>>      format = static_cast<Format>(it->**format);
>>      break;
>>    default:
>>      NOTREACHED();
>> }
>>
>
> I implemented reveman's suggestion for now. When change the type of the
> format field to ResourceProvider::Format I get the following error:
> ../../cc/resources/**transferable_resource.h:33:3: error:
> 'ResourceProvider' does not name a type
>
> possibly because of the circular dependency? (i.e. resource_provider.h
> includes transferable_resource.h and to add the enum field I need to
> include resource_provider.h from transferable_resource.h)


Move the enum definition out of the class?

I'm very uncomfortable at having an enum hidden in an int at the IPC level,
because that hides the security properties. The security team does not
audit the code in resource_provider, and so a change in the security
properties would be completely missed, possibly causing vulnerabilities.

Having the IPC refuse the message if it's incorrect is much safer rather
than accepting it and faking bad values.


>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.**cc#newcode1522<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode1522>
> cc/resources/resource_**provider.cc:1522: texture_data_type =
> GL_UNSIGNED_SHORT_4_4_4_4;
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> nit: I'd remove the temporary variable and just "return
>> GL_UNSIGNED_SHORT_4_4_4_4;" here and in functions below.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.h<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h>
> File cc/resources/resource_**provider.h (right):
>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.h#**newcode343<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h#newcode343>
> cc/resources/resource_**provider.h:343: static unsigned GetStride(Format
> format);
> On 2013/09/13 04:56:51, piman wrote:
>
>> On 2013/09/13 03:43:56, kaanb wrote:
>> > On 2013/09/13 01:42:01, piman wrote:
>> > > What does this represent, and how is it related to BytesPerPixel?
>>
> "stride"
>
>> > > usually means the number of bytes per row, but that depends on the
>>
> width...
>
>> >
>> > It is used by the TextureUploader to decide what value to use for
>>
> rounding.
>
>  GetStrideAlignment?
>> Please add a comment about what it means, since it's not obvious. But,
>>
> see also
>
>> my comment there, I think this is not needed.
>>
>
> Removed.
>
>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
resources/resource_provider.h#**newcode388<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h#newcode388>
> cc/resources/resource_**provider.h:388: Format texture_format;
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> just "Format format;" as it doesn't just apply to texture backed GL
>>
> resources.
>
> Done.
>
>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
scheduler/texture_uploader.cc<https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_uploader.cc>
> File cc/scheduler/texture_uploader.**cc (right):
>
> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>
scheduler/texture_uploader.cc#**newcode217<https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_uploader.cc#newcode217>
> cc/scheduler/texture_uploader.**cc:217:
> ResourceProvider::GetStride(**format));
> On 2013/09/13 04:56:51, piman wrote:
>
>> I don't understand the change here, to accomodate 4444.
>> 1- You check that source_rect.width() is multiple of 2. Since
>>
> bytes_per_pixel is
>
>> 2, then bytes_per_pixel*source_rect.**width() is a multiple of 4 still.
>> 2- if it wasn't, then you'd have a problem anyway. You're not changing
>>
> the
>
>> GL_UNPACK_ALIGNMENT, so if rows were not aligned on 4 bytes, you'd
>>
> corrupt the
>
>> upload and access invalid data.
>>
>
>  So leaving it at 4 is actually more correct, and doesn't need the
>>
> GetStride.
>
> Done.
>
>
> https://codereview.chromium.**org/21159007/diff/157001/gpu/**
>
command_buffer/service/async_**pixel_transfer_manager_**android.cc<https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/service/async_pixel_transfer_manager_android.cc>
> File gpu/command_buffer/service/**async_pixel_transfer_manager_**
> android.cc
> (right):
>
> https://codereview.chromium.**org/21159007/diff/157001/gpu/**
> command_buffer/service/async_**pixel_transfer_manager_**
>
android.cc#newcode55<https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/service/async_pixel_transfer_manager_android.cc#newcode55>
> gpu/command_buffer/service/**async_pixel_transfer_manager_**android.cc:55:
> !base::android::SysUtils::**IsLowEndDevice()) {
> On 2013/09/13 04:56:51, piman wrote:
>
>> On 2013/09/13 03:43:56, kaanb wrote:
>> > On 2013/09/13 01:42:01, piman wrote:
>> > > Why is this needed here?
>> >
>> > See crbug.com/271929, I also added a TODO here and referenced the
>>
> bug.
>
>  But then it means a compromised or buggy renderer can crash the gpu
>>
> process (the
>
>> browser in the android case) on a !IsLowEndDevice(). Additional code
>>
> is needed
>
>> to prevent the renderer from sending 4444 data through the
>>
> AsyncPixelTransfer
>
>> mechanism, to make it fail instead of crashing.
>>
>
> Can you clarify what you mean by "Additional code is needed"? I checked
> with epenner@ and he tells me just a DCHECK would fire when we try to
> send 4444 data and DCHECKs are NOOPs in production code.


The DCHECKs become incorrect. If eglCreateImageKHR can now fail, the code
has to correctly support it. The DCHECKs give a false indication.
Now it's very possible that the reset of the code behaves correctly
if egl_image_ is NULL, in which case the only thing you need to do is
replace the DCHECKs by DLOG_IF(ERROR, ...)


>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc>
> File cc/resources/raster_worker_**pool.cc (right):
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode132<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode132>
> cc/resources/raster_worker_**pool.cc:132: SkBitmap bitmap_32;
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> can we drop the _32 suffix here?
>>
>
> Done.
>
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode136<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode136>
> cc/resources/raster_worker_**pool.cc:136: stride);
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> stride is not going to be correct here unless format is RGBA_8888 or
>>
> BGRA_8888.
>
>> you should probably move this into the switch statement and not set
>>
> stride in
>
>> the case of RGBA_4444.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode143<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode143>
> cc/resources/raster_worker_**pool.cc:143: case
> ResourceProvider::LUMINANCE_8:
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> Using SkBitmap::kARGB_8888_Config for LUMINANCE_8 doesn't look right.
>>
> I'd rather
>
>> we make it explicit that this format is not supported.
>>
>
> Done.
>
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode148<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode148>
> cc/resources/raster_worker_**pool.cc:148: SkBitmapDevice
> device_32(bitmap_32);
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> drop _32?
>>
>
> Done.
>
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode190<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode190>
> cc/resources/raster_worker_**pool.cc:190:
> "RasterWorkerPoolTaskImpl::**RunRasterOnThread::Downsample"**);
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> If this is worth a trace event then I prefer if you moved it to a
>>
> helper
>
>> function.
>>
>
> Done.
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode194<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode194>
> cc/resources/raster_worker_**pool.cc:194: }
>
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> This is currently hardcoded to only support RGBA_4444. I think it
>>
> needs to be
>
>> generalized a bit. Maybe add a SkBitmapConfigFromFormat helper
>>
> function that
>
>> determines the config to use with copyTo and you can also use that to
>>
> determine
>
>> if the copy is needed. ie. bitmap.getConfig() !=
>> SkBitmapConfigFromFormat(**format).
>>
>
> Done.
>
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/raster_worker_pool.**cc#newcode239<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode239>
> cc/resources/raster_worker_**pool.cc:239: ResourceProvider::Format
> texture_format_;
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> I don't think you need this. You can get the format from
>>
> resource()->format().
>
> Done.
>
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/tile_manager.cc<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc>
> File cc/resources/tile_manager.cc (right):
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/tile_manager.cc#**newcode111<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc#newcode111>
> cc/resources/tile_manager.cc:**111: provider->best_texture_format(**);
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> should we instead just adjust best_texture_format() when
>>
> use_rgba_4444_tiles is
>
>> used?
>>
>
> I want to make sure we only use 4444 for tile textures. I think
> enforcing it here is cleaner.
>
> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>
resources/tile_manager.cc#**newcode736<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc#newcode736>
> cc/resources/tile_manager.cc:**736:
>
> On 2013/09/13 14:32:49, David Reveman wrote:
>
>> nit: no need to add this blank line.
>>
>
> Done.
>
>
https://codereview.chromium.**org/21159007/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kaanb1, 2013-09-13 21:22:03]

On Fri, Sep 13, 2013 at 1:39 PM, Antoine Labour <piman@chromium.org> wrote:

>
>
>
> On Fri, Sep 13, 2013 at 12:57 PM, <kaanb@chromium.org> wrote:
>
>>
>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>
resources/resource_provider.cc<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc>
>> File cc/resources/resource_**provider.cc (right):
>>
>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>
resources/resource_provider.**cc#newcode944<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode944>
>> cc/resources/resource_**provider.cc:944: case 0:
>> On 2013/09/13 14:59:48, David Reveman wrote:
>>
>>> On 2013/09/13 03:53:25, enne wrote:
>>> > On 2013/09/13 03:43:56, kaanb wrote:
>>> > > On 2013/09/13 01:39:44, enne wrote:
>>> > > > If somebody switched the enum order, this would break.  Can you
>>>
>> make this
>>
>>> > case
>>> > > > RGBA_8888: format = RGBA_8888, etc?
>>> > > >
>>> > > > Alternatively, why not just static cast and DCHECK?
>>> > >
>>> > > reveman@ didn't like the static_cast
>>> >
>>> > I think static_cast + CHECK would be the cleanest, but I think even
>>>
>> just using
>>
>>> > enum values in each case statement would be an improvement over
>>>
>> literals.
>>
>>  yes, it's the CHECK I want and I think a switch statement is the
>>>
>> cleanest way to
>>
>>> do that at the moment.
>>>
>>
>> Done.
>>
>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>
resources/resource_provider.**cc#newcode956<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode956>
>> cc/resources/resource_**provider.cc:956: }
>>
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> On 2013/09/13 04:56:51, piman wrote:
>>> > On 2013/09/13 03:43:56, kaanb wrote:
>>> > > On 2013/09/13 01:42:01, piman wrote:
>>> > > > Why not make TransferableResource use ResourceProvider::Format?
>>> > >
>>> > > It looks like TransferableResource is part of an IPC message and
>>>
>> compilation
>>
>>> > > fails when I make TransferableResource use
>>>
>> ResourceProvider::Format
>>
>>> >
>>> > Sure, so, we should fix that.
>>> > Enums are fine to pass over IPC, you just need to declare them using
>>> > IPC_ENUM_TRAITS (see other instances in cc_messages.h).
>>> > It still needs to be checked on the receiving side (for security),
>>>
>> but at
>>
>>> least
>>> > you don't need a switch.
>>>
>>
>>  What's the proper way to check that enum values are valid? Ideally
>>>
>> we'd have an
>>
>>> enum_cast<Format>() impl that checks against min/max enum values but I
>>>
>> think
>>
>>> that would require something like this to be done cleanly:
>>>
https://codereview.chromium.**org/22911037/<https://codereview.chromium.org/2...
>>>
>>
>>  Instead of the above, I was thinking something like this:
>>>
>>
>>  Format format = RGBA_8888;
>>> switch (it->format) {
>>>    case RGBA_8888:
>>>    case RGBA_4444:
>>>    case BGRA_8888:
>>>    case LUMINANCE_8:
>>>      format = static_cast<Format>(it->**format);
>>>      break;
>>>    default:
>>>      NOTREACHED();
>>> }
>>>
>>
>> I implemented reveman's suggestion for now. When change the type of the
>> format field to ResourceProvider::Format I get the following error:
>> ../../cc/resources/**transferable_resource.h:33:3: error:
>> 'ResourceProvider' does not name a type
>>
>> possibly because of the circular dependency? (i.e. resource_provider.h
>> includes transferable_resource.h and to add the enum field I need to
>> include resource_provider.h from transferable_resource.h)
>
>
> Move the enum definition out of the class?
>
> I'm very uncomfortable at having an enum hidden in an int at the IPC
> level, because that hides the security properties. The security team does
> not audit the code in resource_provider, and so a change in the security
> properties would be completely missed, possibly causing vulnerabilities.
>
> Having the IPC refuse the message if it's incorrect is much safer rather
> than accepting it and faking bad values.
>
>
>>
>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>
resources/resource_provider.**cc#newcode1522<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode1522>
>> cc/resources/resource_**provider.cc:1522: texture_data_type =
>> GL_UNSIGNED_SHORT_4_4_4_4;
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> nit: I'd remove the temporary variable and just "return
>>> GL_UNSIGNED_SHORT_4_4_4_4;" here and in functions below.
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>
resources/resource_provider.h<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h>
>> File cc/resources/resource_**provider.h (right):
>>
>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>
resources/resource_provider.h#**newcode343<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h#newcode343>
>> cc/resources/resource_**provider.h:343: static unsigned GetStride(Format
>> format);
>> On 2013/09/13 04:56:51, piman wrote:
>>
>>> On 2013/09/13 03:43:56, kaanb wrote:
>>> > On 2013/09/13 01:42:01, piman wrote:
>>> > > What does this represent, and how is it related to BytesPerPixel?
>>>
>> "stride"
>>
>>> > > usually means the number of bytes per row, but that depends on the
>>>
>> width...
>>
>>> >
>>> > It is used by the TextureUploader to decide what value to use for
>>>
>> rounding.
>>
>>  GetStrideAlignment?
>>> Please add a comment about what it means, since it's not obvious. But,
>>>
>> see also
>>
>>> my comment there, I think this is not needed.
>>>
>>
>> Removed.
>>
>>
>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>
resources/resource_provider.h#**newcode388<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h#newcode388>
>> cc/resources/resource_**provider.h:388: Format texture_format;
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> just "Format format;" as it doesn't just apply to texture backed GL
>>>
>> resources.
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>
scheduler/texture_uploader.cc<https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_uploader.cc>
>> File cc/scheduler/texture_uploader.**cc (right):
>>
>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>
scheduler/texture_uploader.cc#**newcode217<https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_uploader.cc#newcode217>
>> cc/scheduler/texture_uploader.**cc:217:
>> ResourceProvider::GetStride(**format));
>> On 2013/09/13 04:56:51, piman wrote:
>>
>>> I don't understand the change here, to accomodate 4444.
>>> 1- You check that source_rect.width() is multiple of 2. Since
>>>
>> bytes_per_pixel is
>>
>>> 2, then bytes_per_pixel*source_rect.**width() is a multiple of 4 still.
>>> 2- if it wasn't, then you'd have a problem anyway. You're not changing
>>>
>> the
>>
>>> GL_UNPACK_ALIGNMENT, so if rows were not aligned on 4 bytes, you'd
>>>
>> corrupt the
>>
>>> upload and access invalid data.
>>>
>>
>>  So leaving it at 4 is actually more correct, and doesn't need the
>>>
>> GetStride.
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21159007/diff/157001/gpu/**
>>
command_buffer/service/async_**pixel_transfer_manager_**android.cc<https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/service/async_pixel_transfer_manager_android.cc>
>> File gpu/command_buffer/service/**async_pixel_transfer_manager_**
>> android.cc
>> (right):
>>
>> https://codereview.chromium.**org/21159007/diff/157001/gpu/**
>> command_buffer/service/async_**pixel_transfer_manager_**
>>
android.cc#newcode55<https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/service/async_pixel_transfer_manager_android.cc#newcode55>
>> gpu/command_buffer/service/**async_pixel_transfer_manager_**
>> android.cc:55:
>> !base::android::SysUtils::**IsLowEndDevice()) {
>> On 2013/09/13 04:56:51, piman wrote:
>>
>>> On 2013/09/13 03:43:56, kaanb wrote:
>>> > On 2013/09/13 01:42:01, piman wrote:
>>> > > Why is this needed here?
>>> >
>>> > See crbug.com/271929, I also added a TODO here and referenced the
>>>
>> bug.
>>
>>  But then it means a compromised or buggy renderer can crash the gpu
>>>
>> process (the
>>
>>> browser in the android case) on a !IsLowEndDevice(). Additional code
>>>
>> is needed
>>
>>> to prevent the renderer from sending 4444 data through the
>>>
>> AsyncPixelTransfer
>>
>>> mechanism, to make it fail instead of crashing.
>>>
>>
>> Can you clarify what you mean by "Additional code is needed"? I checked
>> with epenner@ and he tells me just a DCHECK would fire when we try to
>> send 4444 data and DCHECKs are NOOPs in production code.
>
>
> The DCHECKs become incorrect. If eglCreateImageKHR can now fail, the code
> has to correctly support it. The DCHECKs give a false indication.
> Now it's very possible that the reset of the code behaves correctly
> if egl_image_ is NULL, in which case the only thing you need to do is
> replace the DCHECKs by DLOG_IF(ERROR, ...)
>
>
I talked to epenner@ and he says:
"we can totally change it, but we figure DCHECK implies the code must still
not crash or be compromised in production." With a NULL egl_image_ we won't
upload anything, so it's hard to claim the code "works", but in itself it
wouldn't cause any other problem.


>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/raster_worker_pool.**cc<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc>
>> File cc/resources/raster_worker_**pool.cc (right):
>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/raster_worker_pool.**cc#newcode132<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode132>
>> cc/resources/raster_worker_**pool.cc:132: SkBitmap bitmap_32;
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> can we drop the _32 suffix here?
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/raster_worker_pool.**cc#newcode136<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode136>
>> cc/resources/raster_worker_**pool.cc:136: stride);
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> stride is not going to be correct here unless format is RGBA_8888 or
>>>
>> BGRA_8888.
>>
>>> you should probably move this into the switch statement and not set
>>>
>> stride in
>>
>>> the case of RGBA_4444.
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/raster_worker_pool.**cc#newcode143<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode143>
>> cc/resources/raster_worker_**pool.cc:143: case
>> ResourceProvider::LUMINANCE_8:
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> Using SkBitmap::kARGB_8888_Config for LUMINANCE_8 doesn't look right.
>>>
>> I'd rather
>>
>>> we make it explicit that this format is not supported.
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/raster_worker_pool.**cc#newcode148<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode148>
>> cc/resources/raster_worker_**pool.cc:148: SkBitmapDevice
>> device_32(bitmap_32);
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> drop _32?
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/raster_worker_pool.**cc#newcode190<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode190>
>> cc/resources/raster_worker_**pool.cc:190:
>> "RasterWorkerPoolTaskImpl::**RunRasterOnThread::Downsample"**);
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> If this is worth a trace event then I prefer if you moved it to a
>>>
>> helper
>>
>>> function.
>>>
>>
>> Done.
>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/raster_worker_pool.**cc#newcode194<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode194>
>> cc/resources/raster_worker_**pool.cc:194: }
>>
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> This is currently hardcoded to only support RGBA_4444. I think it
>>>
>> needs to be
>>
>>> generalized a bit. Maybe add a SkBitmapConfigFromFormat helper
>>>
>> function that
>>
>>> determines the config to use with copyTo and you can also use that to
>>>
>> determine
>>
>>> if the copy is needed. ie. bitmap.getConfig() !=
>>> SkBitmapConfigFromFormat(**format).
>>>
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/raster_worker_pool.**cc#newcode239<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode239>
>> cc/resources/raster_worker_**pool.cc:239: ResourceProvider::Format
>> texture_format_;
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> I don't think you need this. You can get the format from
>>>
>> resource()->format().
>>
>> Done.
>>
>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/tile_manager.cc<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc>
>> File cc/resources/tile_manager.cc (right):
>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/tile_manager.cc#**newcode111<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc#newcode111>
>> cc/resources/tile_manager.cc:**111: provider->best_texture_format(**);
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> should we instead just adjust best_texture_format() when
>>>
>> use_rgba_4444_tiles is
>>
>>> used?
>>>
>>
>> I want to make sure we only use 4444 for tile textures. I think
>> enforcing it here is cleaner.
>>
>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>
resources/tile_manager.cc#**newcode736<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc#newcode736>
>> cc/resources/tile_manager.cc:**736:
>>
>> On 2013/09/13 14:32:49, David Reveman wrote:
>>
>>> nit: no need to add this blank line.
>>>
>>
>> Done.
>>
>>
https://codereview.chromium.**org/21159007/<https://codereview.chromium.org/2...
>>
>
>


-- 
Thanks,
Kaan

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kaanb, 2013-09-13 22:35:21]

+jschuh for content/common/cc_messages.h

Refactored the Format enum out of ResourceProvider so that we could use it in
TransferableResource.

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:191: highp_threshold_min));
On 2013/09/13 20:24:31, David Reveman wrote:
> nit: looks like this fits on one line

Done.

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:950: NOTREACHED();
On 2013/09/13 20:24:31, David Reveman wrote:
> As piman already pointed out, we can't use NOTREACHED here if this is the
place
> where we check the validity of the format value. Just removing this default
case
> and using RGBA_8888 for invalid values might be fine. 

We don't need the switch statement anymore as TransferableResource uses the
ResourceFormat enum.

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1226: DCHECK(resource->format != RGBA_4444 ||
On 2013/09/13 20:24:31, David Reveman wrote:
> Can we avoid this format specific check? Shouldn't "(resource->size.width() %
2)
> == 0" be true for all formats?

There are unittests that upload 13x13 textures in RGBA format. That being said,
as far as I remember those tests were testing TextureUploader, I copied the
check from TextureUploader to here for consistency.

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1522: return GL_UNSIGNED_BYTE;
On 2013/09/13 20:24:31, David Reveman wrote:
> I think you can add a NOTREACHED() above this line.

Done.

https://codereview.chromium.org/21159007/diff/180001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1535: return GL_RGBA;
On 2013/09/13 20:24:31, David Reveman wrote:
> and NOTREACHED() here too.

Done.

[jschuh, 2013-09-13 23:02:05]

https://codereview.chromium.org/21159007/diff/186001/content/common/cc_messag...
File content/common/cc_messages.h (right):

https://codereview.chromium.org/21159007/diff/186001/content/common/cc_messag...
content/common/cc_messages.h:121: IPC_ENUM_TRAITS(cc::ResourceFormat)
Please use IPC_ENUM_TRAITS_MIN_MAX_VALUE to force range validation in the IPC
layer.

[piman, 2013-09-13 23:19:09]

On Fri, Sep 13, 2013 at 2:22 PM, Kaan Baloglu <kaanb@google.com> wrote:

>
>
>
> On Fri, Sep 13, 2013 at 1:39 PM, Antoine Labour <piman@chromium.org>wrote:
>
>>
>>
>>
>> On Fri, Sep 13, 2013 at 12:57 PM, <kaanb@chromium.org> wrote:
>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>>
resources/resource_provider.cc<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc>
>>> File cc/resources/resource_**provider.cc (right):
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>>
resources/resource_provider.**cc#newcode944<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode944>
>>> cc/resources/resource_**provider.cc:944: case 0:
>>> On 2013/09/13 14:59:48, David Reveman wrote:
>>>
>>>> On 2013/09/13 03:53:25, enne wrote:
>>>> > On 2013/09/13 03:43:56, kaanb wrote:
>>>> > > On 2013/09/13 01:39:44, enne wrote:
>>>> > > > If somebody switched the enum order, this would break.  Can you
>>>>
>>> make this
>>>
>>>> > case
>>>> > > > RGBA_8888: format = RGBA_8888, etc?
>>>> > > >
>>>> > > > Alternatively, why not just static cast and DCHECK?
>>>> > >
>>>> > > reveman@ didn't like the static_cast
>>>> >
>>>> > I think static_cast + CHECK would be the cleanest, but I think even
>>>>
>>> just using
>>>
>>>> > enum values in each case statement would be an improvement over
>>>>
>>> literals.
>>>
>>>  yes, it's the CHECK I want and I think a switch statement is the
>>>>
>>> cleanest way to
>>>
>>>> do that at the moment.
>>>>
>>>
>>> Done.
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>>
resources/resource_provider.**cc#newcode956<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode956>
>>> cc/resources/resource_**provider.cc:956: }
>>>
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> On 2013/09/13 04:56:51, piman wrote:
>>>> > On 2013/09/13 03:43:56, kaanb wrote:
>>>> > > On 2013/09/13 01:42:01, piman wrote:
>>>> > > > Why not make TransferableResource use ResourceProvider::Format?
>>>> > >
>>>> > > It looks like TransferableResource is part of an IPC message and
>>>>
>>> compilation
>>>
>>>> > > fails when I make TransferableResource use
>>>>
>>> ResourceProvider::Format
>>>
>>>> >
>>>> > Sure, so, we should fix that.
>>>> > Enums are fine to pass over IPC, you just need to declare them using
>>>> > IPC_ENUM_TRAITS (see other instances in cc_messages.h).
>>>> > It still needs to be checked on the receiving side (for security),
>>>>
>>> but at
>>>
>>>> least
>>>> > you don't need a switch.
>>>>
>>>
>>>  What's the proper way to check that enum values are valid? Ideally
>>>>
>>> we'd have an
>>>
>>>> enum_cast<Format>() impl that checks against min/max enum values but I
>>>>
>>> think
>>>
>>>> that would require something like this to be done cleanly:
>>>>
https://codereview.chromium.**org/22911037/<https://codereview.chromium.org/2...
>>>>
>>>
>>>  Instead of the above, I was thinking something like this:
>>>>
>>>
>>>  Format format = RGBA_8888;
>>>> switch (it->format) {
>>>>    case RGBA_8888:
>>>>    case RGBA_4444:
>>>>    case BGRA_8888:
>>>>    case LUMINANCE_8:
>>>>      format = static_cast<Format>(it->**format);
>>>>      break;
>>>>    default:
>>>>      NOTREACHED();
>>>> }
>>>>
>>>
>>> I implemented reveman's suggestion for now. When change the type of the
>>> format field to ResourceProvider::Format I get the following error:
>>> ../../cc/resources/**transferable_resource.h:33:3: error:
>>> 'ResourceProvider' does not name a type
>>>
>>> possibly because of the circular dependency? (i.e. resource_provider.h
>>> includes transferable_resource.h and to add the enum field I need to
>>> include resource_provider.h from transferable_resource.h)
>>
>>
>> Move the enum definition out of the class?
>>
>> I'm very uncomfortable at having an enum hidden in an int at the IPC
>> level, because that hides the security properties. The security team does
>> not audit the code in resource_provider, and so a change in the security
>> properties would be completely missed, possibly causing vulnerabilities.
>>
>> Having the IPC refuse the message if it's incorrect is much safer rather
>> than accepting it and faking bad values.
>>
>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>>
resources/resource_provider.**cc#newcode1522<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.cc#newcode1522>
>>> cc/resources/resource_**provider.cc:1522: texture_data_type =
>>> GL_UNSIGNED_SHORT_4_4_4_4;
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> nit: I'd remove the temporary variable and just "return
>>>> GL_UNSIGNED_SHORT_4_4_4_4;" here and in functions below.
>>>>
>>>
>>> Done.
>>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>>
resources/resource_provider.h<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h>
>>> File cc/resources/resource_**provider.h (right):
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>>
resources/resource_provider.h#**newcode343<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h#newcode343>
>>> cc/resources/resource_**provider.h:343: static unsigned GetStride(Format
>>> format);
>>> On 2013/09/13 04:56:51, piman wrote:
>>>
>>>> On 2013/09/13 03:43:56, kaanb wrote:
>>>> > On 2013/09/13 01:42:01, piman wrote:
>>>> > > What does this represent, and how is it related to BytesPerPixel?
>>>>
>>> "stride"
>>>
>>>> > > usually means the number of bytes per row, but that depends on the
>>>>
>>> width...
>>>
>>>> >
>>>> > It is used by the TextureUploader to decide what value to use for
>>>>
>>> rounding.
>>>
>>>  GetStrideAlignment?
>>>> Please add a comment about what it means, since it's not obvious. But,
>>>>
>>> see also
>>>
>>>> my comment there, I think this is not needed.
>>>>
>>>
>>> Removed.
>>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>>
resources/resource_provider.h#**newcode388<https://codereview.chromium.org/21159007/diff/157001/cc/resources/resource_provider.h#newcode388>
>>> cc/resources/resource_**provider.h:388: Format texture_format;
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> just "Format format;" as it doesn't just apply to texture backed GL
>>>>
>>> resources.
>>>
>>> Done.
>>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>>
scheduler/texture_uploader.cc<https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_uploader.cc>
>>> File cc/scheduler/texture_uploader.**cc (right):
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/cc/**
>>>
scheduler/texture_uploader.cc#**newcode217<https://codereview.chromium.org/21159007/diff/157001/cc/scheduler/texture_uploader.cc#newcode217>
>>> cc/scheduler/texture_uploader.**cc:217:
>>> ResourceProvider::GetStride(**format));
>>> On 2013/09/13 04:56:51, piman wrote:
>>>
>>>> I don't understand the change here, to accomodate 4444.
>>>> 1- You check that source_rect.width() is multiple of 2. Since
>>>>
>>> bytes_per_pixel is
>>>
>>>> 2, then bytes_per_pixel*source_rect.**width() is a multiple of 4 still.
>>>> 2- if it wasn't, then you'd have a problem anyway. You're not changing
>>>>
>>> the
>>>
>>>> GL_UNPACK_ALIGNMENT, so if rows were not aligned on 4 bytes, you'd
>>>>
>>> corrupt the
>>>
>>>> upload and access invalid data.
>>>>
>>>
>>>  So leaving it at 4 is actually more correct, and doesn't need the
>>>>
>>> GetStride.
>>>
>>> Done.
>>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/gpu/**
>>>
command_buffer/service/async_**pixel_transfer_manager_**android.cc<https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/service/async_pixel_transfer_manager_android.cc>
>>> File gpu/command_buffer/service/**async_pixel_transfer_manager_**
>>> android.cc
>>> (right):
>>>
>>> https://codereview.chromium.**org/21159007/diff/157001/gpu/**
>>> command_buffer/service/async_**pixel_transfer_manager_**
>>>
android.cc#newcode55<https://codereview.chromium.org/21159007/diff/157001/gpu/command_buffer/service/async_pixel_transfer_manager_android.cc#newcode55>
>>> gpu/command_buffer/service/**async_pixel_transfer_manager_**
>>> android.cc:55:
>>> !base::android::SysUtils::**IsLowEndDevice()) {
>>> On 2013/09/13 04:56:51, piman wrote:
>>>
>>>> On 2013/09/13 03:43:56, kaanb wrote:
>>>> > On 2013/09/13 01:42:01, piman wrote:
>>>> > > Why is this needed here?
>>>> >
>>>> > See crbug.com/271929, I also added a TODO here and referenced the
>>>>
>>> bug.
>>>
>>>  But then it means a compromised or buggy renderer can crash the gpu
>>>>
>>> process (the
>>>
>>>> browser in the android case) on a !IsLowEndDevice(). Additional code
>>>>
>>> is needed
>>>
>>>> to prevent the renderer from sending 4444 data through the
>>>>
>>> AsyncPixelTransfer
>>>
>>>> mechanism, to make it fail instead of crashing.
>>>>
>>>
>>> Can you clarify what you mean by "Additional code is needed"? I checked
>>> with epenner@ and he tells me just a DCHECK would fire when we try to
>>> send 4444 data and DCHECKs are NOOPs in production code.
>>
>>
>> The DCHECKs become incorrect. If eglCreateImageKHR can now fail, the code
>> has to correctly support it. The DCHECKs give a false indication.
>> Now it's very possible that the reset of the code behaves correctly
>> if egl_image_ is NULL, in which case the only thing you need to do is
>> replace the DCHECKs by DLOG_IF(ERROR, ...)
>>
>>
> I talked to epenner@ and he says:
> "we can totally change it, but we figure DCHECK implies the code must
> still not crash or be compromised in production." With a NULL egl_image_ we
> won't upload anything, so it's hard to claim the code "works", but in
> itself it wouldn't cause any other problem.
>

DCHECK is about documenting assumptions, in a way that doesn't cost
anything in production.
The assumption is incorrect, hence it shouldn't be a DCHECK.

DCHECK is to signal "the code below makes the assumption that this
condition is correct", and actually verifies it in debug. It's precisely to
/avoid/ having to handle cases where the assumption is incorrect.


>
>
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/raster_worker_pool.**cc<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc>
>>> File cc/resources/raster_worker_**pool.cc (right):
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/raster_worker_pool.**cc#newcode132<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode132>
>>> cc/resources/raster_worker_**pool.cc:132: SkBitmap bitmap_32;
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> can we drop the _32 suffix here?
>>>>
>>>
>>> Done.
>>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/raster_worker_pool.**cc#newcode136<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode136>
>>> cc/resources/raster_worker_**pool.cc:136: stride);
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> stride is not going to be correct here unless format is RGBA_8888 or
>>>>
>>> BGRA_8888.
>>>
>>>> you should probably move this into the switch statement and not set
>>>>
>>> stride in
>>>
>>>> the case of RGBA_4444.
>>>>
>>>
>>> Done.
>>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/raster_worker_pool.**cc#newcode143<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode143>
>>> cc/resources/raster_worker_**pool.cc:143: case
>>> ResourceProvider::LUMINANCE_8:
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> Using SkBitmap::kARGB_8888_Config for LUMINANCE_8 doesn't look right.
>>>>
>>> I'd rather
>>>
>>>> we make it explicit that this format is not supported.
>>>>
>>>
>>> Done.
>>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/raster_worker_pool.**cc#newcode148<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode148>
>>> cc/resources/raster_worker_**pool.cc:148: SkBitmapDevice
>>> device_32(bitmap_32);
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> drop _32?
>>>>
>>>
>>> Done.
>>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/raster_worker_pool.**cc#newcode190<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode190>
>>> cc/resources/raster_worker_**pool.cc:190:
>>> "RasterWorkerPoolTaskImpl::**RunRasterOnThread::Downsample"**);
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> If this is worth a trace event then I prefer if you moved it to a
>>>>
>>> helper
>>>
>>>> function.
>>>>
>>>
>>> Done.
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/raster_worker_pool.**cc#newcode194<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode194>
>>> cc/resources/raster_worker_**pool.cc:194: }
>>>
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> This is currently hardcoded to only support RGBA_4444. I think it
>>>>
>>> needs to be
>>>
>>>> generalized a bit. Maybe add a SkBitmapConfigFromFormat helper
>>>>
>>> function that
>>>
>>>> determines the config to use with copyTo and you can also use that to
>>>>
>>> determine
>>>
>>>> if the copy is needed. ie. bitmap.getConfig() !=
>>>> SkBitmapConfigFromFormat(**format).
>>>>
>>>
>>> Done.
>>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/raster_worker_pool.**cc#newcode239<https://codereview.chromium.org/21159007/diff/167001/cc/resources/raster_worker_pool.cc#newcode239>
>>> cc/resources/raster_worker_**pool.cc:239: ResourceProvider::Format
>>> texture_format_;
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> I don't think you need this. You can get the format from
>>>>
>>> resource()->format().
>>>
>>> Done.
>>>
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/tile_manager.cc<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc>
>>> File cc/resources/tile_manager.cc (right):
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/tile_manager.cc#**newcode111<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc#newcode111>
>>> cc/resources/tile_manager.cc:**111: provider->best_texture_format(**);
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> should we instead just adjust best_texture_format() when
>>>>
>>> use_rgba_4444_tiles is
>>>
>>>> used?
>>>>
>>>
>>> I want to make sure we only use 4444 for tile textures. I think
>>> enforcing it here is cleaner.
>>>
>>> https://codereview.chromium.**org/21159007/diff/167001/cc/**
>>>
resources/tile_manager.cc#**newcode736<https://codereview.chromium.org/21159007/diff/167001/cc/resources/tile_manager.cc#newcode736>
>>> cc/resources/tile_manager.cc:**736:
>>>
>>> On 2013/09/13 14:32:49, David Reveman wrote:
>>>
>>>> nit: no need to add this blank line.
>>>>
>>>
>>> Done.
>>>
>>>
https://codereview.chromium.**org/21159007/<https://codereview.chromium.org/2...
>>>
>>
>>
>
>
> --
> Thanks,
> Kaan
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kaanb, 2013-09-13 23:57:41]

Removed the DCHECKs for egl_image_ not being NULL in
gpu/command_buffer/service/async_pixel_transfer_manager_egl.cc and I added early
outs in methods that used egl_image_.

I enabled async uploads on a low end devices again and verified 4444 textures
didn't cause any GL related crashes in the async upload codepath.

https://codereview.chromium.org/21159007/diff/186001/content/common/cc_messag...
File content/common/cc_messages.h (right):

https://codereview.chromium.org/21159007/diff/186001/content/common/cc_messag...
content/common/cc_messages.h:121: IPC_ENUM_TRAITS(cc::ResourceFormat)
On 2013/09/13 23:02:07, Justin Schuh wrote:
> Please use IPC_ENUM_TRAITS_MIN_MAX_VALUE to force range validation in the IPC
> layer.

Done.

[piman, 2013-09-14 00:14:15]

LGTM for gpu and content, with 1 nit.

https://codereview.chromium.org/21159007/diff/189001/cc/resources/resource_fo...
File cc/resources/resource_format.h (right):

https://codereview.chromium.org/21159007/diff/189001/cc/resources/resource_fo...
cc/resources/resource_format.h:14: LUMINANCE_8,
nit: can you add RESOURCE_FORMAT_MAX = LUMINANCE_8, and use RESOURCE_FORMAT_MAX
in cc_messages.h? I don't want someone adding a value here and forgetting to
update cc_messages.h, which would cause some values to fail with the delegated
renderer.

[enne (OOO), 2013-09-14 00:21:03]

cc/ lgtm

[kaanb, 2013-09-14 00:36:23]

https://codereview.chromium.org/21159007/diff/189001/cc/resources/resource_fo...
File cc/resources/resource_format.h (right):

https://codereview.chromium.org/21159007/diff/189001/cc/resources/resource_fo...
cc/resources/resource_format.h:14: LUMINANCE_8,
On 2013/09/14 00:14:17, piman wrote:
> nit: can you add RESOURCE_FORMAT_MAX = LUMINANCE_8, and use
RESOURCE_FORMAT_MAX
> in cc_messages.h? I don't want someone adding a value here and forgetting to
> update cc_messages.h, which would cause some values to fail with the delegated
> renderer.

Done.

[reveman, 2013-09-14 00:58:37]

https://codereview.chromium.org/21159007/diff/189001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/189001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1717: settings_->use_rgba_4444_textures ? RGBA_4444 :
RGBA_8888);
I think this should be using resource_provider->best_tile_texture_format()
instead?

https://codereview.chromium.org/21159007/diff/189001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1727: if (settings_->use_rgba_4444_textures) {
I think this code should be similar to PBRWP code and not use
use_rgba_4444_textures directly. You might want to move the helper function you
added to PBRWP to resource_format.h.

https://codereview.chromium.org/21159007/diff/189001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1731: on_demand_tile_raster_bitmap_4444_.getPixels());
does caching on_demand_tile_raster_bitmap_4444_ really make a difference? I'd
prefer if this was just a temporary variable like in the case of PBRWP for now.

https://codereview.chromium.org/21159007/diff/189001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/189001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:229: return format_;
resource_provider()->best_tile_texture_format()

https://codereview.chromium.org/21159007/diff/189001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/189001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:66: void ChangeBitmapConfigIfPossible(const
SkBitmap& bitmap,
IfRequired? IfNeeded?

https://codereview.chromium.org/21159007/diff/189001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:70: if (bitmap.getConfig() !=
SkBitmapConfigFromFormat(format)) {
nit: store SkBitmapConfigFromFormat result in temporary variable

https://codereview.chromium.org/21159007/diff/189001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/21159007/diff/189001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:129: bool use_rgba_4444_tiles) {
can the use_rgba_4444_tiles setting be passed to the ResourceProvider ctor
instead so that best_tile_texture_format() doesn't take a parameter?

[kaanb, 2013-09-16 07:22:23]

https://codereview.chromium.org/21159007/diff/189001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/189001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1717: settings_->use_rgba_4444_textures ? RGBA_4444 :
RGBA_8888);
On 2013/09/14 00:58:39, David Reveman wrote:
> I think this should be using resource_provider->best_tile_texture_format()
> instead?

Yes, since previously GL_RGBA was hardcoded, raster-on-demand pixel tests were
expecting the output in GL_RGBA format. When I changed this to use
best_tile_texture_format() those tests started failing since
best_tile_texture_format() was BGRA, so I added a field to LayerTreeSettings to
enforce using RGBA textures to fix the tests.

https://codereview.chromium.org/21159007/diff/189001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1727: if (settings_->use_rgba_4444_textures) {
On 2013/09/14 00:58:39, David Reveman wrote:
> I think this code should be similar to PBRWP code and not use
> use_rgba_4444_textures directly. You might want to move the helper function
you
> added to PBRWP to resource_format.h.

Done.

https://codereview.chromium.org/21159007/diff/189001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1731: on_demand_tile_raster_bitmap_4444_.getPixels());
On 2013/09/14 00:58:39, David Reveman wrote:
> does caching on_demand_tile_raster_bitmap_4444_ really make a difference? I'd
> prefer if this was just a temporary variable like in the case of PBRWP for
now.

Done.

https://codereview.chromium.org/21159007/diff/189001/cc/resources/pixel_buffe...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/189001/cc/resources/pixel_buffe...
cc/resources/pixel_buffer_raster_worker_pool.cc:229: return format_;
On 2013/09/14 00:58:39, David Reveman wrote:
> resource_provider()->best_tile_texture_format()

Done.

https://codereview.chromium.org/21159007/diff/189001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/189001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:66: void ChangeBitmapConfigIfPossible(const
SkBitmap& bitmap,
On 2013/09/14 00:58:39, David Reveman wrote:
> IfRequired? IfNeeded?

Done.

https://codereview.chromium.org/21159007/diff/189001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:70: if (bitmap.getConfig() !=
SkBitmapConfigFromFormat(format)) {
On 2013/09/14 00:58:39, David Reveman wrote:
> nit: store SkBitmapConfigFromFormat result in temporary variable

Done.

https://codereview.chromium.org/21159007/diff/189001/cc/resources/tile_manage...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/21159007/diff/189001/cc/resources/tile_manage...
cc/resources/tile_manager.cc:129: bool use_rgba_4444_tiles) {
On 2013/09/14 00:58:39, David Reveman wrote:
> can the use_rgba_4444_tiles setting be passed to the ResourceProvider ctor
> instead so that best_tile_texture_format() doesn't take a parameter?

Done.

[reveman, 2013-09-16 15:55:13]

https://codereview.chromium.org/21159007/diff/205001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/205001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:53: ResourceFormat format) {
You can remove the format parameter if you make this a member function of
RasterWorkerPoolTaskImpl. I think that would also make the name of the trace
event less cryptic.

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_fo...
File cc/resources/resource_format.h (right):

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_fo...
cc/resources/resource_format.h:18: RESOURCE_FORMAT_MAX = LUMINANCE_8,
Note: we typically use NUM_RESOURCE_FORMATS in cc/ code but I prefer this and
there seems to be a mix of this and _NUM throughout the chromium code so I think
it's OK to keep this as is. I'm hoping we can switch to something like this in
the long term: https://codereview.chromium.org/22911037/

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
cc/resources/resource_provider.h:69: ResourceFormat best_tile_texture_format()
const {
Sorry for not thinking about this earlier. The concept of a "tile" probably
doesn't belong in the RP interface. I think this would be better named something
like "memory_efficient_texture_format()".

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
cc/resources/resource_provider.h:462: bool use_rgba_4444_tiles_;
nit: use_rgba_4444_texture_format

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
cc/resources/resource_provider.h:463: bool force_rgba_tiles_;
Not clear to me exactly why we need |force_rgba_tiles_|. Is
ContextProvider::Capabilities::texture_format_bgra8888 true when it shouldn't
be? Or is there a bug related to how we handle this texture format when running
pixel tests?

[kaanb, 2013-09-16 18:36:35]

https://codereview.chromium.org/21159007/diff/205001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/205001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:53: ResourceFormat format) {
On 2013/09/16 15:55:14, David Reveman wrote:
> You can remove the format parameter if you make this a member function of
> RasterWorkerPoolTaskImpl. I think that would also make the name of the trace
> event less cryptic.

Done.

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_fo...
File cc/resources/resource_format.h (right):

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_fo...
cc/resources/resource_format.h:18: RESOURCE_FORMAT_MAX = LUMINANCE_8,
On 2013/09/16 15:55:14, David Reveman wrote:
> Note: we typically use NUM_RESOURCE_FORMATS in cc/ code but I prefer this and
> there seems to be a mix of this and _NUM throughout the chromium code so I
think
> it's OK to keep this as is. I'm hoping we can switch to something like this in
> the long term: https://codereview.chromium.org/22911037/

Acknowledged.

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
cc/resources/resource_provider.h:69: ResourceFormat best_tile_texture_format()
const {
On 2013/09/16 15:55:14, David Reveman wrote:
> Sorry for not thinking about this earlier. The concept of a "tile" probably
> doesn't belong in the RP interface. I think this would be better named
something
> like "memory_efficient_texture_format()". 

Done.

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
cc/resources/resource_provider.h:462: bool use_rgba_4444_tiles_;
On 2013/09/16 15:55:14, David Reveman wrote:
> nit: use_rgba_4444_texture_format

Done.

https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
cc/resources/resource_provider.h:463: bool force_rgba_tiles_;
On 2013/09/16 15:55:14, David Reveman wrote:
> Not clear to me exactly why we need |force_rgba_tiles_|. Is
> ContextProvider::Capabilities::texture_format_bgra8888 true when it shouldn't
> be? Or is there a bug related to how we handle this texture format when
running
> pixel tests?

Pixel tests use the InProcess context and
ContextProvider::Capabilities::texture_format_bgra8888 is always set to true for
the InProcess context here:
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/common/gpu/...

Previously GLRenderer hardcoded GL_RGBA for the format of the raster-on-demand
tile, so the tests were expecting the output to be in RGBA. When I changed the
format to be the memory_efficient_texture_format() ResourceProvider started
using BGRA_8888 since |texture_format_bgra8888| is true for the InProcess
context. The cleanest way seemed to add a LayerTreeSettings parameter so
unittests can override format specified by the context.

[reveman, 2013-09-16 19:29:25]

On 2013/09/16 18:36:35, kaanb wrote:
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/raster_work...
> File cc/resources/raster_worker_pool.cc (right):
> 
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/raster_work...
> cc/resources/raster_worker_pool.cc:53: ResourceFormat format) {
> On 2013/09/16 15:55:14, David Reveman wrote:
> > You can remove the format parameter if you make this a member function of
> > RasterWorkerPoolTaskImpl. I think that would also make the name of the trace
> > event less cryptic.
> 
> Done.
> 
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_fo...
> File cc/resources/resource_format.h (right):
> 
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_fo...
> cc/resources/resource_format.h:18: RESOURCE_FORMAT_MAX = LUMINANCE_8,
> On 2013/09/16 15:55:14, David Reveman wrote:
> > Note: we typically use NUM_RESOURCE_FORMATS in cc/ code but I prefer this
and
> > there seems to be a mix of this and _NUM throughout the chromium code so I
> think
> > it's OK to keep this as is. I'm hoping we can switch to something like this
in
> > the long term: https://codereview.chromium.org/22911037/
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
> File cc/resources/resource_provider.h (right):
> 
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
> cc/resources/resource_provider.h:69: ResourceFormat best_tile_texture_format()
> const {
> On 2013/09/16 15:55:14, David Reveman wrote:
> > Sorry for not thinking about this earlier. The concept of a "tile" probably
> > doesn't belong in the RP interface. I think this would be better named
> something
> > like "memory_efficient_texture_format()". 
> 
> Done.
> 
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
> cc/resources/resource_provider.h:462: bool use_rgba_4444_tiles_;
> On 2013/09/16 15:55:14, David Reveman wrote:
> > nit: use_rgba_4444_texture_format
> 
> Done.
> 
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
> cc/resources/resource_provider.h:463: bool force_rgba_tiles_;
> On 2013/09/16 15:55:14, David Reveman wrote:
> > Not clear to me exactly why we need |force_rgba_tiles_|. Is
> > ContextProvider::Capabilities::texture_format_bgra8888 true when it
shouldn't
> > be? Or is there a bug related to how we handle this texture format when
> running
> > pixel tests?
> 
> Pixel tests use the InProcess context and
> ContextProvider::Capabilities::texture_format_bgra8888 is always set to true
for
> the InProcess context here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/common/gpu/...
> 
> Previously GLRenderer hardcoded GL_RGBA for the format of the raster-on-demand
> tile, so the tests were expecting the output to be in RGBA. When I changed the
> format to be the memory_efficient_texture_format() ResourceProvider started
> using BGRA_8888 since |texture_format_bgra8888| is true for the InProcess
> context. The cleanest way seemed to add a LayerTreeSettings parameter so
> unittests can override format specified by the context.

Hm, is the problem not just that ContentsDrawQuadBase::swizzle_contents is set
incorrectly? Best would be to set it correctly as that would probably improve
performance of on-demand raster. Alternatively, we can limit the texture formats
used in GLRenderer::DrawPictureQuad based on quad->swizzle_contents but that's a
bit awkward.

[jschuh, 2013-09-16 19:43:48]

ipc security lgtm

[kaanb, 2013-09-16 22:24:18]

On 2013/09/16 19:29:25, David Reveman wrote:
> On 2013/09/16 18:36:35, kaanb wrote:
> >
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/raster_work...
> > File cc/resources/raster_worker_pool.cc (right):
> > 
> >
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/raster_work...
> > cc/resources/raster_worker_pool.cc:53: ResourceFormat format) {
> > On 2013/09/16 15:55:14, David Reveman wrote:
> > > You can remove the format parameter if you make this a member function of
> > > RasterWorkerPoolTaskImpl. I think that would also make the name of the
trace
> > > event less cryptic.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_fo...
> > File cc/resources/resource_format.h (right):
> > 
> >
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_fo...
> > cc/resources/resource_format.h:18: RESOURCE_FORMAT_MAX = LUMINANCE_8,
> > On 2013/09/16 15:55:14, David Reveman wrote:
> > > Note: we typically use NUM_RESOURCE_FORMATS in cc/ code but I prefer this
> and
> > > there seems to be a mix of this and _NUM throughout the chromium code so I
> > think
> > > it's OK to keep this as is. I'm hoping we can switch to something like
this
> in
> > > the long term: https://codereview.chromium.org/22911037/
> > 
> > Acknowledged.
> > 
> >
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
> > File cc/resources/resource_provider.h (right):
> > 
> >
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
> > cc/resources/resource_provider.h:69: ResourceFormat
best_tile_texture_format()
> > const {
> > On 2013/09/16 15:55:14, David Reveman wrote:
> > > Sorry for not thinking about this earlier. The concept of a "tile"
probably
> > > doesn't belong in the RP interface. I think this would be better named
> > something
> > > like "memory_efficient_texture_format()". 
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
> > cc/resources/resource_provider.h:462: bool use_rgba_4444_tiles_;
> > On 2013/09/16 15:55:14, David Reveman wrote:
> > > nit: use_rgba_4444_texture_format
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/21159007/diff/205001/cc/resources/resource_pr...
> > cc/resources/resource_provider.h:463: bool force_rgba_tiles_;
> > On 2013/09/16 15:55:14, David Reveman wrote:
> > > Not clear to me exactly why we need |force_rgba_tiles_|. Is
> > > ContextProvider::Capabilities::texture_format_bgra8888 true when it
> shouldn't
> > > be? Or is there a bug related to how we handle this texture format when
> > running
> > > pixel tests?
> > 
> > Pixel tests use the InProcess context and
> > ContextProvider::Capabilities::texture_format_bgra8888 is always set to true
> for
> > the InProcess context here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/common/gpu/...
> > 
> > Previously GLRenderer hardcoded GL_RGBA for the format of the
raster-on-demand
> > tile, so the tests were expecting the output to be in RGBA. When I changed
the
> > format to be the memory_efficient_texture_format() ResourceProvider started
> > using BGRA_8888 since |texture_format_bgra8888| is true for the InProcess
> > context. The cleanest way seemed to add a LayerTreeSettings parameter so
> > unittests can override format specified by the context.
> 
> Hm, is the problem not just that ContentsDrawQuadBase::swizzle_contents is set
> incorrectly? Best would be to set it correctly as that would probably improve
> performance of on-demand raster. Alternatively, we can limit the texture
formats
> used in GLRenderer::DrawPictureQuad based on quad->swizzle_contents but that's
a
> bit awkward.

Yes, apparently it is. I applied https://codereview.chromium.org/23503066 and
now tests are passing. PTAL.

[reveman, 2013-09-17 02:48:10]

https://codereview.chromium.org/21159007/diff/214001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/214001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1704: 
nit: no need for this blank line

https://codereview.chromium.org/21159007/diff/214001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1721: SkBitmap::Config config =
SkBitmapConfigFromFormat(format);
this should be using quad->texture_format

https://codereview.chromium.org/21159007/diff/214001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/214001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:228: TRACE_EVENT0("cc",
"ChangeBitmapConfigIfNeeded");
"RasterWorkerPoolTaskImpl::ChangeBitmapConfigIfNeeded"

https://codereview.chromium.org/21159007/diff/214001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/214001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:211: DCHECK(format != RGBA_4444 ||
(source_rect.width() % 2) == 0);
is this format specific dcheck required? Minimum amount of format specific code
is preferred.

https://codereview.chromium.org/21159007/diff/214001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:280: DCHECK(format != RGBA_4444 ||
(source_rect.width() % 2) == 0);
same thing here. do we need this format specific dcheck?

[kaanb, 2013-09-17 04:12:17]

Please CQ if there aren't any more comments.

https://codereview.chromium.org/21159007/diff/214001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/214001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1704: 
On 2013/09/17 02:48:12, David Reveman wrote:
> nit: no need for this blank line

Done.

https://codereview.chromium.org/21159007/diff/214001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1721: SkBitmap::Config config =
SkBitmapConfigFromFormat(format);
On 2013/09/17 02:48:12, David Reveman wrote:
> this should be using quad->texture_format

Done.

https://codereview.chromium.org/21159007/diff/214001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/214001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:228: TRACE_EVENT0("cc",
"ChangeBitmapConfigIfNeeded");
On 2013/09/17 02:48:12, David Reveman wrote:
> "RasterWorkerPoolTaskImpl::ChangeBitmapConfigIfNeeded"

Done.

https://codereview.chromium.org/21159007/diff/214001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/214001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:211: DCHECK(format != RGBA_4444 ||
(source_rect.width() % 2) == 0);
On 2013/09/17 02:48:12, David Reveman wrote:
> is this format specific dcheck required? Minimum amount of format specific
code
> is preferred.

There are unittests that upload odd-sized textures (I think 13x13) in RGBA
format.

https://codereview.chromium.org/21159007/diff/214001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:280: DCHECK(format != RGBA_4444 ||
(source_rect.width() % 2) == 0);
On 2013/09/17 02:48:12, David Reveman wrote:
> same thing here. do we need this format specific dcheck?

Same answer.

[reveman, 2013-09-17 13:52:00]

I thinks this is ready to land once I understand those DCHECKs.

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:211: DCHECK(format != RGBA_4444 ||
(source_rect.width() % 2) == 0);
I'm still confused by this. Why does width need to be a multiple of 2 when
format is RGBA_4444?

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:213: RoundUp(bytes_per_pixel *
source_rect.width(), static_cast<size_t>(4));
why change this? 4u would be preferred.

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:282: RoundUp(bytes_per_pixel *
source_rect.width(), static_cast<size_t>(4));
same here. why not 4u?

[kaanb, 2013-09-17 16:41:57]

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:211: DCHECK(format != RGBA_4444 ||
(source_rect.width() % 2) == 0);
On 2013/09/17 13:52:02, David Reveman wrote:
> I'm still confused by this. Why does width need to be a multiple of 2 when
> format is RGBA_4444?

Because we're not setting GL_UNPACK_ALIGNMENT to 2 when the format is 4444. See
skyostil's comments on Sep 6 and Sep 9.

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:213: RoundUp(bytes_per_pixel *
source_rect.width(), static_cast<size_t>(4));
On 2013/09/17 13:52:02, David Reveman wrote:
> why change this? 4u would be preferred.

I get a compiler error. I guess I could leave 4u and then static_cast the LHS to
unsigned, if that's somehow more preferred.

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:282: RoundUp(bytes_per_pixel *
source_rect.width(), static_cast<size_t>(4));
On 2013/09/17 13:52:02, David Reveman wrote:
> same here. why not 4u?

same answer.

[reveman, 2013-09-17 17:44:14]

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (left):

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:212: // Assuming that GL_UNPACK_ALIGNMENT has
not changed from default.
I don't think you should be removing this comment. It's still valid if I
understand this correctly.

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:211: DCHECK(format != RGBA_4444 ||
(source_rect.width() % 2) == 0);
On 2013/09/17 16:41:59, kaanb wrote:
> On 2013/09/17 13:52:02, David Reveman wrote:
> > I'm still confused by this. Why does width need to be a multiple of 2 when
> > format is RGBA_4444?
> 
> Because we're not setting GL_UNPACK_ALIGNMENT to 2 when the format is 4444.
See
> skyostil's comments on Sep 6 and Sep 9.

But this breaks support for uneven resource dimensions. Why not let the code
below that handles this do its job?

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:213: RoundUp(bytes_per_pixel *
source_rect.width(), static_cast<size_t>(4));
On 2013/09/17 16:41:59, kaanb wrote:
> On 2013/09/17 13:52:02, David Reveman wrote:
> > why change this? 4u would be preferred.
> 
> I get a compiler error. I guess I could leave 4u and then static_cast the LHS
to
> unsigned, if that's somehow more preferred.

I guess you get this error because you changed the type of bytes_per_pixel to
size_t. maybe cleanest to change that back to 'unsigned'.

Btw, while touching this code, can you also s/unsigned int/unsigned/ here?

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:282: RoundUp(bytes_per_pixel *
source_rect.width(), static_cast<size_t>(4));
On 2013/09/17 16:41:59, kaanb wrote:
> On 2013/09/17 13:52:02, David Reveman wrote:
> > same here. why not 4u?
> 
> same answer.

same suggestions as above.

[kaanb, 2013-09-17 18:44:24]

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (left):

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:212: // Assuming that GL_UNPACK_ALIGNMENT has
not changed from default.
On 2013/09/17 17:44:16, David Reveman wrote:
> I don't think you should be removing this comment. It's still valid if I
> understand this correctly.

Done.

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:211: DCHECK(format != RGBA_4444 ||
(source_rect.width() % 2) == 0);
On 2013/09/17 17:44:16, David Reveman wrote:
> On 2013/09/17 16:41:59, kaanb wrote:
> > On 2013/09/17 13:52:02, David Reveman wrote:
> > > I'm still confused by this. Why does width need to be a multiple of 2 when
> > > format is RGBA_4444?
> > 
> > Because we're not setting GL_UNPACK_ALIGNMENT to 2 when the format is 4444.
> See
> > skyostil's comments on Sep 6 and Sep 9.
> 
> But this breaks support for uneven resource dimensions. Why not let the code
> below that handles this do its job?

It turns out we don't have uneven resource dimensions in the compositor except
for unittests, so epenner and skyostil were fine with putting the DCHECK for
now. 

AFAIK, to remove the DCHECK we'd need to set UNPACK_ALIGNMENT to 2 before
issuing the texSubImage2D and set it back to 4 after that to keep Skia happy.
Furthermore async upload code needs to be changed in a similar fashion as well.

I don't mind removing the DCHECK if we all agree on what needs to be done to set
UNPACK_ALIGNMENT correctly for all codepaths.

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:213: RoundUp(bytes_per_pixel *
source_rect.width(), static_cast<size_t>(4));
On 2013/09/17 17:44:16, David Reveman wrote:
> On 2013/09/17 16:41:59, kaanb wrote:
> > On 2013/09/17 13:52:02, David Reveman wrote:
> > > why change this? 4u would be preferred.
> > 
> > I get a compiler error. I guess I could leave 4u and then static_cast the
LHS
> to
> > unsigned, if that's somehow more preferred.
> 
> I guess you get this error because you changed the type of bytes_per_pixel to
> size_t. maybe cleanest to change that back to 'unsigned'.
> 
> Btw, while touching this code, can you also s/unsigned int/unsigned/ here?

Done.

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:282: RoundUp(bytes_per_pixel *
source_rect.width(), static_cast<size_t>(4));
On 2013/09/17 17:44:16, David Reveman wrote:
> On 2013/09/17 16:41:59, kaanb wrote:
> > On 2013/09/17 13:52:02, David Reveman wrote:
> > > same here. why not 4u?
> > 
> > same answer.
> 
> same suggestions as above.

Done.

[reveman, 2013-09-17 19:13:11]

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
File cc/scheduler/texture_uploader.cc (right):

https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
cc/scheduler/texture_uploader.cc:211: DCHECK(format != RGBA_4444 ||
(source_rect.width() % 2) == 0);
On 2013/09/17 18:44:26, kaanb wrote:
> On 2013/09/17 17:44:16, David Reveman wrote:
> > On 2013/09/17 16:41:59, kaanb wrote:
> > > On 2013/09/17 13:52:02, David Reveman wrote:
> > > > I'm still confused by this. Why does width need to be a multiple of 2
when
> > > > format is RGBA_4444?
> > > 
> > > Because we're not setting GL_UNPACK_ALIGNMENT to 2 when the format is
4444.
> > See
> > > skyostil's comments on Sep 6 and Sep 9.
> > 
> > But this breaks support for uneven resource dimensions. Why not let the code
> > below that handles this do its job?
> 
> It turns out we don't have uneven resource dimensions in the compositor except
> for unittests, so epenner and skyostil were fine with putting the DCHECK for
> now. 

Hm, I think you can set it to an uneven size using --default-tile-width/height.
The code here is also not specific to tiles. It's general texture upload code
that shouldn't make assumptions about texture size.

> 
> AFAIK, to remove the DCHECK we'd need to set UNPACK_ALIGNMENT to 2 before
> issuing the texSubImage2D and set it back to 4 after that to keep Skia happy.

Or you perform a copy into a properly aligned buffer, which is exactly what the
code below does afaict.

> Furthermore async upload code needs to be changed in a similar fashion as
well.

Doesn't skia support 4-byte alignment for 16bpp? I thought we could use whatever
stride we want.

> 
> I don't mind removing the DCHECK if we all agree on what needs to be done to
set
> UNPACK_ALIGNMENT correctly for all codepaths.

I don't think we should use a non-default value for UNPACK_ALIGNMENT but instead
always produce contents that is 4-byte aligned.

[epenner, 2013-09-17 19:15:54]

> I don't mind removing the DCHECK if we all agree on what needs to be done to
set
> UNPACK_ALIGNMENT correctly for all codepaths.

If this is the only remaining issue now, are we okay filing
follow up bugs at this point? It's slightly less than perfect, but
this patch is already doing more than its fair share of cleanup,
and we do want some buffer time to insure this patch will stick.

[epenner, 2013-09-17 19:29:26]

Can do this as follow up, but noticed one more Nit.

https://codereview.chromium.org/21159007/diff/215076/cc/resources/resource_pr...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/21159007/diff/215076/cc/resources/resource_pr...
cc/resources/resource_provider.h:59: bool use_rgba_4444_texture_format);
Nit: Can we remove this and just pass in both 'best' formats as variables? Best
to push the boolean back as far as possible.

[kaanb, 2013-09-17 21:00:02]

On 2013/09/17 19:13:11, David Reveman wrote:
>
https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
> File cc/scheduler/texture_uploader.cc (right):
> 
>
https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
> cc/scheduler/texture_uploader.cc:211: DCHECK(format != RGBA_4444 ||
> (source_rect.width() % 2) == 0);
> On 2013/09/17 18:44:26, kaanb wrote:
> > On 2013/09/17 17:44:16, David Reveman wrote:
> > > On 2013/09/17 16:41:59, kaanb wrote:
> > > > On 2013/09/17 13:52:02, David Reveman wrote:
> > > > > I'm still confused by this. Why does width need to be a multiple of 2
> when
> > > > > format is RGBA_4444?
> > > > 
> > > > Because we're not setting GL_UNPACK_ALIGNMENT to 2 when the format is
> 4444.
> > > See
> > > > skyostil's comments on Sep 6 and Sep 9.
> > > 
> > > But this breaks support for uneven resource dimensions. Why not let the
code
> > > below that handles this do its job?
> > 
> > It turns out we don't have uneven resource dimensions in the compositor
except
> > for unittests, so epenner and skyostil were fine with putting the DCHECK for
> > now. 
> 
> Hm, I think you can set it to an uneven size using
--default-tile-width/height.
> The code here is also not specific to tiles. It's general texture upload code
> that shouldn't make assumptions about texture size.
> 
> > 
> > AFAIK, to remove the DCHECK we'd need to set UNPACK_ALIGNMENT to 2 before
> > issuing the texSubImage2D and set it back to 4 after that to keep Skia
happy.
> 
> Or you perform a copy into a properly aligned buffer, which is exactly what
the
> code below does afaict.
> 
> > Furthermore async upload code needs to be changed in a similar fashion as
> well.
> 
> Doesn't skia support 4-byte alignment for 16bpp? I thought we could use
whatever
> stride we want.
> 
> > 
> > I don't mind removing the DCHECK if we all agree on what needs to be done to
> set
> > UNPACK_ALIGNMENT correctly for all codepaths.
> 
> I don't think we should use a non-default value for UNPACK_ALIGNMENT but
instead
> always produce contents that is 4-byte aligned.

scroggo@ from Skia team tells me 4444 bitmaps aren't guaranteed to be 4-byte
aligned and it would require a code change
to guarantee that they're 4-byte aligned.

epenner and I feel that without knowing what Skia used it would be safer to
leave the DCHECK in place, as the code that does the
memcpy makes assumptions about the layout of the pixels. We can then later
discuss what needs to be done to make all code properly
work with 4-byte aligned bitmaps. How does that sound?

[reveman, 2013-09-17 21:26:11]

On 2013/09/17 21:00:02, kaanb wrote:
> On 2013/09/17 19:13:11, David Reveman wrote:
> >
>
https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
> > File cc/scheduler/texture_uploader.cc (right):
> > 
> >
>
https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
> > cc/scheduler/texture_uploader.cc:211: DCHECK(format != RGBA_4444 ||
> > (source_rect.width() % 2) == 0);
> > On 2013/09/17 18:44:26, kaanb wrote:
> > > On 2013/09/17 17:44:16, David Reveman wrote:
> > > > On 2013/09/17 16:41:59, kaanb wrote:
> > > > > On 2013/09/17 13:52:02, David Reveman wrote:
> > > > > > I'm still confused by this. Why does width need to be a multiple of
2
> > when
> > > > > > format is RGBA_4444?
> > > > > 
> > > > > Because we're not setting GL_UNPACK_ALIGNMENT to 2 when the format is
> > 4444.
> > > > See
> > > > > skyostil's comments on Sep 6 and Sep 9.
> > > > 
> > > > But this breaks support for uneven resource dimensions. Why not let the
> code
> > > > below that handles this do its job?
> > > 
> > > It turns out we don't have uneven resource dimensions in the compositor
> except
> > > for unittests, so epenner and skyostil were fine with putting the DCHECK
for
> > > now. 
> > 
> > Hm, I think you can set it to an uneven size using
> --default-tile-width/height.
> > The code here is also not specific to tiles. It's general texture upload
code
> > that shouldn't make assumptions about texture size.
> > 
> > > 
> > > AFAIK, to remove the DCHECK we'd need to set UNPACK_ALIGNMENT to 2 before
> > > issuing the texSubImage2D and set it back to 4 after that to keep Skia
> happy.
> > 
> > Or you perform a copy into a properly aligned buffer, which is exactly what
> the
> > code below does afaict.
> > 
> > > Furthermore async upload code needs to be changed in a similar fashion as
> > well.
> > 
> > Doesn't skia support 4-byte alignment for 16bpp? I thought we could use
> whatever
> > stride we want.
> > 
> > > 
> > > I don't mind removing the DCHECK if we all agree on what needs to be done
to
> > set
> > > UNPACK_ALIGNMENT correctly for all codepaths.
> > 
> > I don't think we should use a non-default value for UNPACK_ALIGNMENT but
> instead
> > always produce contents that is 4-byte aligned.
> 
> scroggo@ from Skia team tells me 4444 bitmaps aren't guaranteed to be 4-byte
> aligned and it would require a code change
> to guarantee that they're 4-byte aligned.
> 
> epenner and I feel that without knowing what Skia used it would be safer to
> leave the DCHECK in place, as the code that does the
> memcpy makes assumptions about the layout of the pixels. We can then later
> discuss what needs to be done to make all code properly
> work with 4-byte aligned bitmaps. How does that sound?

Ok, but please move the DCHECK closer to the source and perform it based on
SkBitmap::rowBytes() in a format agnostic matter. It should probably be a CHECK
and not a DCHECK as it could otherwise cause undefined behaviour in release
builds. ie. CHECK((bitmap_dest.rowBytes() % 4) == 0) in
ChangeBitmapConfigIfNeeded.

[kaanb, 2013-09-17 21:40:47]

On 2013/09/17 21:26:11, David Reveman wrote:
> On 2013/09/17 21:00:02, kaanb wrote:
> > On 2013/09/17 19:13:11, David Reveman wrote:
> > >
> >
>
https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
> > > File cc/scheduler/texture_uploader.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/21159007/diff/218001/cc/scheduler/texture_upl...
> > > cc/scheduler/texture_uploader.cc:211: DCHECK(format != RGBA_4444 ||
> > > (source_rect.width() % 2) == 0);
> > > On 2013/09/17 18:44:26, kaanb wrote:
> > > > On 2013/09/17 17:44:16, David Reveman wrote:
> > > > > On 2013/09/17 16:41:59, kaanb wrote:
> > > > > > On 2013/09/17 13:52:02, David Reveman wrote:
> > > > > > > I'm still confused by this. Why does width need to be a multiple
of
> 2
> > > when
> > > > > > > format is RGBA_4444?
> > > > > > 
> > > > > > Because we're not setting GL_UNPACK_ALIGNMENT to 2 when the format
is
> > > 4444.
> > > > > See
> > > > > > skyostil's comments on Sep 6 and Sep 9.
> > > > > 
> > > > > But this breaks support for uneven resource dimensions. Why not let
the
> > code
> > > > > below that handles this do its job?
> > > > 
> > > > It turns out we don't have uneven resource dimensions in the compositor
> > except
> > > > for unittests, so epenner and skyostil were fine with putting the DCHECK
> for
> > > > now. 
> > > 
> > > Hm, I think you can set it to an uneven size using
> > --default-tile-width/height.
> > > The code here is also not specific to tiles. It's general texture upload
> code
> > > that shouldn't make assumptions about texture size.
> > > 
> > > > 
> > > > AFAIK, to remove the DCHECK we'd need to set UNPACK_ALIGNMENT to 2
before
> > > > issuing the texSubImage2D and set it back to 4 after that to keep Skia
> > happy.
> > > 
> > > Or you perform a copy into a properly aligned buffer, which is exactly
what
> > the
> > > code below does afaict.
> > > 
> > > > Furthermore async upload code needs to be changed in a similar fashion
as
> > > well.
> > > 
> > > Doesn't skia support 4-byte alignment for 16bpp? I thought we could use
> > whatever
> > > stride we want.
> > > 
> > > > 
> > > > I don't mind removing the DCHECK if we all agree on what needs to be
done
> to
> > > set
> > > > UNPACK_ALIGNMENT correctly for all codepaths.
> > > 
> > > I don't think we should use a non-default value for UNPACK_ALIGNMENT but
> > instead
> > > always produce contents that is 4-byte aligned.
> > 
> > scroggo@ from Skia team tells me 4444 bitmaps aren't guaranteed to be 4-byte
> > aligned and it would require a code change
> > to guarantee that they're 4-byte aligned.
> > 
> > epenner and I feel that without knowing what Skia used it would be safer to
> > leave the DCHECK in place, as the code that does the
> > memcpy makes assumptions about the layout of the pixels. We can then later
> > discuss what needs to be done to make all code properly
> > work with 4-byte aligned bitmaps. How does that sound?
> 
> Ok, but please move the DCHECK closer to the source and perform it based on
> SkBitmap::rowBytes() in a format agnostic matter. It should probably be a
CHECK
> and not a DCHECK as it could otherwise cause undefined behaviour in release
> builds. ie. CHECK((bitmap_dest.rowBytes() % 4) == 0) in
> ChangeBitmapConfigIfNeeded.

Done, I used CHECK_EQ(0, bitmap_dest.rowBytes() % 4) to avoid the presubmit
warning.

[kaanb, 2013-09-17 21:42:34]

On 2013/09/17 19:29:26, epenner wrote:
> Can do this as follow up, but noticed one more Nit.
> 
>
https://codereview.chromium.org/21159007/diff/215076/cc/resources/resource_pr...
> File cc/resources/resource_provider.h (right):
> 
>
https://codereview.chromium.org/21159007/diff/215076/cc/resources/resource_pr...
> cc/resources/resource_provider.h:59: bool use_rgba_4444_texture_format);
> Nit: Can we remove this and just pass in both 'best' formats as variables?
Best
> to push the boolean back as far as possible.

I'd rather do this in a follow-up.

[reveman, 2013-09-17 22:09:37]

lgtm after addressing this last set of comments and making sure it works
properly with software compositing.

https://codereview.chromium.org/21159007/diff/224001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/224001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1724: on_demand_tile_raster_bitmap_dest.getPixels());
Add the same CHECK_EQ and TODO here.

https://codereview.chromium.org/21159007/diff/224001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/224001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:235: CHECK_EQ(0, bitmap_dest.rowBytes() % 4);
Please add a TODO here that explains why this CHECK is needed and a reference to
crbug.com/293728.

https://codereview.chromium.org/21159007/diff/224001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/224001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1062: bytes_per_pixel *
resource->size.GetArea(),
Please change this to "resource->size.height() * RoundUp(bytes_per_pixel *
resource->size.width(), 4u)".

[kaanb, 2013-09-17 22:40:00]

https://codereview.chromium.org/21159007/diff/224001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/21159007/diff/224001/cc/output/gl_renderer.cc...
cc/output/gl_renderer.cc:1724: on_demand_tile_raster_bitmap_dest.getPixels());
On 2013/09/17 22:09:38, David Reveman wrote:
> Add the same CHECK_EQ and TODO here.

Done.

https://codereview.chromium.org/21159007/diff/224001/cc/resources/raster_work...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/21159007/diff/224001/cc/resources/raster_work...
cc/resources/raster_worker_pool.cc:235: CHECK_EQ(0, bitmap_dest.rowBytes() % 4);
On 2013/09/17 22:09:38, David Reveman wrote:
> Please add a TODO here that explains why this CHECK is needed and a reference
to
> crbug.com/293728.

Done.

https://codereview.chromium.org/21159007/diff/224001/cc/resources/resource_pr...
File cc/resources/resource_provider.cc (right):

https://codereview.chromium.org/21159007/diff/224001/cc/resources/resource_pr...
cc/resources/resource_provider.cc:1062: bytes_per_pixel *
resource->size.GetArea(),
On 2013/09/17 22:09:38, David Reveman wrote:
> Please change this to "resource->size.height() * RoundUp(bytes_per_pixel *
> resource->size.width(), 4u)".

Done.

[kaanb, 2013-09-17 23:13:14]

On 2013/09/17 22:09:37, David Reveman wrote:
> lgtm after addressing this last set of comments and making sure it works
> properly with software compositing.
> 
> https://codereview.chromium.org/21159007/diff/224001/cc/output/gl_renderer.cc
> File cc/output/gl_renderer.cc (right):
> 
>
https://codereview.chromium.org/21159007/diff/224001/cc/output/gl_renderer.cc...
> cc/output/gl_renderer.cc:1724: on_demand_tile_raster_bitmap_dest.getPixels());
> Add the same CHECK_EQ and TODO here.
> 
>
https://codereview.chromium.org/21159007/diff/224001/cc/resources/raster_work...
> File cc/resources/raster_worker_pool.cc (right):
> 
>
https://codereview.chromium.org/21159007/diff/224001/cc/resources/raster_work...
> cc/resources/raster_worker_pool.cc:235: CHECK_EQ(0, bitmap_dest.rowBytes() %
4);
> Please add a TODO here that explains why this CHECK is needed and a reference
to
> crbug.com/293728.
> 
>
https://codereview.chromium.org/21159007/diff/224001/cc/resources/resource_pr...
> File cc/resources/resource_provider.cc (right):
> 
>
https://codereview.chromium.org/21159007/diff/224001/cc/resources/resource_pr...
> cc/resources/resource_provider.cc:1062: bytes_per_pixel *
> resource->size.GetArea(),
> Please change this to "resource->size.height() * RoundUp(bytes_per_pixel *
> resource->size.width(), 4u)".

LayerTreeSettings::use_rgba_4444_textures is only true for low-end Android
devices for Clank which
doesn't use software compositing. I disable 4444 textures for Android WebView as
GpuMemoryBuffers don't support 4444.

[commit-bot: I haz the power, 2013-09-17 23:43:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/21159007/222001

[commit-bot: I haz the power, 2013-09-18 00:21:09]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
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.

[kaanb, 2013-09-18 00:39:51]

On 2013/09/18 00:21:09, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on linux_chromeos_clang.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
> 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.

I fixed two build errors in perftests.

[commit-bot: I haz the power, 2013-09-18 00:48:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/21159007/226001

[kaanb, 2013-09-18 01:11:45]

On 2013/09/18 00:48:58, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/kaanb%40chromium.org/21159007/226001

fixed cc_messages_unittest and trying again

[commit-bot: I haz the power, 2013-09-18 01:15:54]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-09-18 01:18:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/21159007/239001

[commit-bot: I haz the power, 2013-09-18 01:37:10]

Failed to trigger a try job on linux_aura
HTTP Error 400: Bad Request

[commit-bot: I haz the power, 2013-09-18 01:37:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/21159007/251001

[commit-bot: I haz the power, 2013-09-18 02:49:18]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
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.

[commit-bot: I haz the power, 2013-09-18 04:17:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/21159007/111001

[kaanb1, 2013-09-18 04:18:55]

Fixed video_resource_updater.h so that it compiles under Windows


On Tue, Sep 17, 2013 at 9:17 PM, <commit-bot@chromium.org> wrote:

> CQ is trying da patch. Follow status at
> https://chromium-status.**appspot.com/cq/kaanb@chromium.**
>
org/21159007/111001<https://chromium-status.appspot.com/cq/kaanb@chromium.org/21159007/111001>
>
>
>
https://chromiumcodereview.**appspot.com/21159007/<https://chromiumcodereview...
>



-- 
Thanks,
Kaan

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-09-18 05:11:46]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win7_aura.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
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.

[commit-bot: I haz the power, 2013-09-18 05:41:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kaanb@chromium.org/21159007/116001

[kaanb, 2013-09-18 05:42:09]

On 2013/09/18 05:11:46, I haz the power (commit-bot) wrote:
> Sorry for I got bad news for ya.
> Compile failed with a clobber build on win7_aura.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win7_aura&...
> 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.

Fixed another signed/unsigned comparison in video_resource_updater.

[commit-bot: I haz the power, 2013-09-18 09:34:23]

Change committed as 223830