cc: Use gpu raster in HUD.

cc: Use gpu raster in HUD.

This changes the SW back-end for drawing HUD to use Ganesh
backed canvas and texture for drawing FPS, raster and gpu
memory info. If gpu is not available it falls back to old
SW canvas implementation.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2752833002
Cr-Commit-Position: refs/heads/master@{#477845}
Committed: https://chromium.googlesource.com/chromium/src/+/bdc4a31b6e88e8a12b0b7bb298a03b020b1c61de

[sohan, 2017-03-15 17:47:53]

Description was changed from

==========
cc: Use gpu raster in HUD.

BUG=345416
==========

to

==========
cc: Use gpu raster in HUD.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sohan, 2017-03-15 17:50:37]

Description was changed from

==========
cc: Use gpu raster in HUD.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Use gpu raster in HUD.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sohan, 2017-03-28 18:26:33]

sohan.jyoti@huawei.com changed reviewers:
+ danakj@chromium.org

[sohan, 2017-03-28 18:26:34]

Looks like i maybe missing something here, i can see FrameBuffer errors like..

03-28 21:09:43.191  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(7760)]
[.RenderWorker-0xee4b8400.GpuRasterization]GL ERROR :GL_INVALID_OPERATION :
glFramebufferTexture2D: Attachment textarget doesn't match texture target
0

03-28 21:09:44.231  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
[.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION : glClear:
framebuffer incomplete
03-28 21:09:44.231  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
[.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
glDrawElements: framebuffer incomplete
03-28 21:09:44.251  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
[.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
glDrawArrays: framebuffer incomplete
03-28 21:09:44.251  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
[.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
glDrawArrays: framebuffer incomplete
03-28 21:09:44.251  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
[.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
glDrawElements: framebuffer incomplete
03-28 21:09:44.281  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
[.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
glDrawElements: framebuffer incomplete
03-28 21:09:44.281  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
[.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
glDrawElements: framebuffer incomplete
03-28 21:09:44.301  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
[.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
glDrawElements: framebuffer incomplete
03-28 21:09:44.321  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
[.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
glDrawElements: framebuffer incomplete
03-28 21:09:44.351  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
[.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
glDrawElements: framebuffer incomplete

Putting up the rough patch. Thanks.

[sohan, 2017-03-29 19:17:55]

Patchset #2 (id:20001) has been deleted

[sohan, 2017-03-29 19:32:18]

On 2017/03/28 18:26:34, sohan wrote:
> Looks like i maybe missing something here, i can see FrameBuffer errors like..
> 
> 03-28 21:09:43.191  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(7760)]
> [.RenderWorker-0xee4b8400.GpuRasterization]GL ERROR :GL_INVALID_OPERATION :
> glFramebufferTexture2D: Attachment textarget doesn't match texture target
> 0
> 
> 03-28 21:09:44.231  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
> [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
glClear:
> framebuffer incomplete
> 03-28 21:09:44.231  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
> [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> glDrawElements: framebuffer incomplete
> 03-28 21:09:44.251  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
> [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> glDrawArrays: framebuffer incomplete
> 03-28 21:09:44.251  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
> [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> glDrawArrays: framebuffer incomplete
> 03-28 21:09:44.251  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
> [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> glDrawElements: framebuffer incomplete
> 03-28 21:09:44.281  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
> [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> glDrawElements: framebuffer incomplete
> 03-28 21:09:44.281  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
> [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> glDrawElements: framebuffer incomplete
> 03-28 21:09:44.301  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
> [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> glDrawElements: framebuffer incomplete
> 03-28 21:09:44.321  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
> [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> glDrawElements: framebuffer incomplete
> 03-28 21:09:44.351  8201  8225 E chromium: [ERROR:gles2_cmd_decoder.cc(4384)]
> [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> glDrawElements: framebuffer incomplete
> 
> Putting up the rough patch. Thanks.

Looks like we need to send the real context to HUD instead of the worker context
i was sending in PS#1, but we still need to avoid the dcheck at
https://cs.chromium.org/chromium/src/services/ui/public/cpp/gpu/context_provi...
oherwise it crashes.

This has still some work to do, hard codings which can be avoided, and the hud
flickers as we scroll. just wanted you to have the first look of it, before i
proceed further.
PTAL, thanks :)

[danakj, 2017-03-29 19:42:11]

On Wed, Mar 29, 2017 at 3:32 PM, <sohan.jyoti@huawei.com> wrote:

> On 2017/03/28 18:26:34, sohan wrote:
> > Looks like i maybe missing something here, i can see FrameBuffer errors
> like..
> >
> > 03-28 21:09:43.191 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 7760)]
> > [.RenderWorker-0xee4b8400.GpuRasterization]GL ERROR
> :GL_INVALID_OPERATION :
> > glFramebufferTexture2D: Attachment textarget doesn't match texture target
> > 0
> >
> > 03-28 21:09:44.231 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 4384)]
> > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> glClear:
> > framebuffer incomplete
> > 03-28 21:09:44.231 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 4384)]
> > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > glDrawElements: framebuffer incomplete
> > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 4384)]
> > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > glDrawArrays: framebuffer incomplete
> > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 4384)]
> > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > glDrawArrays: framebuffer incomplete
> > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 4384)]
> > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > glDrawElements: framebuffer incomplete
> > 03-28 21:09:44.281 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 4384)]
> > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > glDrawElements: framebuffer incomplete
> > 03-28 21:09:44.281 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 4384)]
> > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > glDrawElements: framebuffer incomplete
> > 03-28 21:09:44.301 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 4384)]
> > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > glDrawElements: framebuffer incomplete
> > 03-28 21:09:44.321 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 4384)]
> > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > glDrawElements: framebuffer incomplete
> > 03-28 21:09:44.351 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> 4384)]
> > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > glDrawElements: framebuffer incomplete
> >
> > Putting up the rough patch. Thanks.
>
> Looks like we need to send the real context to HUD instead of the worker
> context
> i was sending in PS#1, but we still need to avoid the dcheck at
> https://cs.chromium.org/chromium/src/services/ui/public/cpp/gpu/context_
> provider_command_buffer.cc?gsn=Allocate&l=414
> oherwise it crashes.
>

The compositor context is only used on the compositor thread, it does not
need to be locked, and that's why it's dchecking.


> This has still some work to do, hard codings which can be avoided, and the
> hud
> flickers as we scroll. just wanted you to have the first look of it,
> before i
> proceed further.
> PTAL, thanks :)
>
> https://codereview.chromium.org/2752833002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sohan, 2017-03-30 15:44:07]

On 2017/03/29 19:42:11, danakj wrote:
> On Wed, Mar 29, 2017 at 3:32 PM, <mailto:sohan.jyoti@huawei.com> wrote:
> 
> > On 2017/03/28 18:26:34, sohan wrote:
> > > Looks like i maybe missing something here, i can see FrameBuffer errors
> > like..
> > >
> > > 03-28 21:09:43.191 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 7760)]
> > > [.RenderWorker-0xee4b8400.GpuRasterization]GL ERROR
> > :GL_INVALID_OPERATION :
> > > glFramebufferTexture2D: Attachment textarget doesn't match texture target
> > > 0
> > >
> > > 03-28 21:09:44.231 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 4384)]
> > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > glClear:
> > > framebuffer incomplete
> > > 03-28 21:09:44.231 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 4384)]
> > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > > glDrawElements: framebuffer incomplete
> > > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 4384)]
> > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > > glDrawArrays: framebuffer incomplete
> > > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 4384)]
> > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > > glDrawArrays: framebuffer incomplete
> > > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 4384)]
> > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > > glDrawElements: framebuffer incomplete
> > > 03-28 21:09:44.281 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 4384)]
> > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > > glDrawElements: framebuffer incomplete
> > > 03-28 21:09:44.281 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 4384)]
> > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > > glDrawElements: framebuffer incomplete
> > > 03-28 21:09:44.301 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 4384)]
> > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > > glDrawElements: framebuffer incomplete
> > > 03-28 21:09:44.321 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 4384)]
> > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > > glDrawElements: framebuffer incomplete
> > > 03-28 21:09:44.351 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > 4384)]
> > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION :
> > > glDrawElements: framebuffer incomplete
> > >
> > > Putting up the rough patch. Thanks.
> >
> > Looks like we need to send the real context to HUD instead of the worker
> > context
> > i was sending in PS#1, but we still need to avoid the dcheck at
> > https://cs.chromium.org/chromium/src/services/ui/public/cpp/gpu/context_
> > provider_command_buffer.cc?gsn=Allocate&l=414
> > oherwise it crashes.
> >
> 
> The compositor context is only used on the compositor thread, it does not
> need to be locked, and that's why it's dchecking.
> 
> 
> > This has still some work to do, hard codings which can be avoided, and the
> > hud
> > flickers as we scroll. just wanted you to have the first look of it,
> > before i
> > proceed further.
> > PTAL, thanks :)
> >
> > https://codereview.chromium.org/2752833002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
Thanks. yes that solves the crash. but the hud flickers as we scroll, and
disappears sometimes.
Should we change the quad append/drawing logic, param in some way ?
PTAL. Thanks.

[danakj, 2017-03-30 18:12:30]

On Thu, Mar 30, 2017 at 11:44 AM, <sohan.jyoti@huawei.com> wrote:

> On 2017/03/29 19:42:11, danakj wrote:
>
> > On Wed, Mar 29, 2017 at 3:32 PM, <mailto:sohan.jyoti@huawei.com> wrote:
> >
> > > On 2017/03/28 18:26:34, sohan wrote:
> > > > Looks like i maybe missing something here, i can see FrameBuffer
> errors
> > > like..
> > > >
> > > > 03-28 21:09:43.191 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 7760)]
> > > > [.RenderWorker-0xee4b8400.GpuRasterization]GL ERROR
> > > :GL_INVALID_OPERATION :
> > > > glFramebufferTexture2D: Attachment textarget doesn't match texture
> target
> > > > 0
> > > >
> > > > 03-28 21:09:44.231 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 4384)]
> > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> :
> > > glClear:
> > > > framebuffer incomplete
> > > > 03-28 21:09:44.231 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 4384)]
> > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> :
> > > > glDrawElements: framebuffer incomplete
> > > > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 4384)]
> > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> :
> > > > glDrawArrays: framebuffer incomplete
> > > > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 4384)]
> > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> :
> > > > glDrawArrays: framebuffer incomplete
> > > > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 4384)]
> > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> :
> > > > glDrawElements: framebuffer incomplete
> > > > 03-28 21:09:44.281 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 4384)]
> > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> :
> > > > glDrawElements: framebuffer incomplete
> > > > 03-28 21:09:44.281 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 4384)]
> > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> :
> > > > glDrawElements: framebuffer incomplete
> > > > 03-28 21:09:44.301 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 4384)]
> > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> :
> > > > glDrawElements: framebuffer incomplete
> > > > 03-28 21:09:44.321 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 4384)]
> > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> :
> > > > glDrawElements: framebuffer incomplete
> > > > 03-28 21:09:44.351 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > 4384)]
> > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> :
> > > > glDrawElements: framebuffer incomplete
> > > >
> > > > Putting up the rough patch. Thanks.
> > >
> > > Looks like we need to send the real context to HUD instead of the
> worker
> > > context
> > > i was sending in PS#1, but we still need to avoid the dcheck at
> > > https://cs.chromium.org/chromium/src/services/ui/public/cpp/
> gpu/context_
> > > provider_command_buffer.cc?gsn=Allocate&l=414
> > > oherwise it crashes.
> > >
> >
> > The compositor context is only used on the compositor thread, it does not
> > need to be locked, and that's why it's dchecking.
> >
> >
> > > This has still some work to do, hard codings which can be avoided, and
> the
> > > hud
> > > flickers as we scroll. just wanted you to have the first look of it,
> > > before i
> > > proceed further.
> > > PTAL, thanks :)
> > >
> > > https://codereview.chromium.org/2752833002/
> > >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "Chromium-reviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> email
> > to mailto:chromium-reviews+unsubscribe@chromium.org.
> Thanks. yes that solves the crash. but the hud flickers as we scroll, and
> disappears sometimes.
> Should we change the quad append/drawing logic, param in some way ?
> PTAL. Thanks.
>

Sounds like maybe you're changing the texture while its being shown in the
display instead of double buffering, or something.


>
> https://codereview.chromium.org/2752833002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sohan, 2017-04-06 15:47:55]

On 2017/03/30 18:12:30, danakj wrote:
> On Thu, Mar 30, 2017 at 11:44 AM, <mailto:sohan.jyoti@huawei.com> wrote:
> 
> > On 2017/03/29 19:42:11, danakj wrote:
> >
> > > On Wed, Mar 29, 2017 at 3:32 PM, <mailto:sohan.jyoti@huawei.com> wrote:
> > >
> > > > On 2017/03/28 18:26:34, sohan wrote:
> > > > > Looks like i maybe missing something here, i can see FrameBuffer
> > errors
> > > > like..
> > > > >
> > > > > 03-28 21:09:43.191 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 7760)]
> > > > > [.RenderWorker-0xee4b8400.GpuRasterization]GL ERROR
> > > > :GL_INVALID_OPERATION :
> > > > > glFramebufferTexture2D: Attachment textarget doesn't match texture
> > target
> > > > > 0
> > > > >
> > > > > 03-28 21:09:44.231 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 4384)]
> > > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> > :
> > > > glClear:
> > > > > framebuffer incomplete
> > > > > 03-28 21:09:44.231 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 4384)]
> > > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> > :
> > > > > glDrawElements: framebuffer incomplete
> > > > > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 4384)]
> > > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> > :
> > > > > glDrawArrays: framebuffer incomplete
> > > > > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 4384)]
> > > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> > :
> > > > > glDrawArrays: framebuffer incomplete
> > > > > 03-28 21:09:44.251 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 4384)]
> > > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> > :
> > > > > glDrawElements: framebuffer incomplete
> > > > > 03-28 21:09:44.281 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 4384)]
> > > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> > :
> > > > > glDrawElements: framebuffer incomplete
> > > > > 03-28 21:09:44.281 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 4384)]
> > > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> > :
> > > > > glDrawElements: framebuffer incomplete
> > > > > 03-28 21:09:44.301 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 4384)]
> > > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> > :
> > > > > glDrawElements: framebuffer incomplete
> > > > > 03-28 21:09:44.321 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 4384)]
> > > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> > :
> > > > > glDrawElements: framebuffer incomplete
> > > > > 03-28 21:09:44.351 8201 8225 E chromium: [ERROR:gles2_cmd_decoder.cc(
> > > > 4384)]
> > > > > [.RenderWorker-0xee4b8400]GL ERROR :GL_INVALID_FRAMEBUFFER_OPERATION
> > :
> > > > > glDrawElements: framebuffer incomplete
> > > > >
> > > > > Putting up the rough patch. Thanks.
> > > >
> > > > Looks like we need to send the real context to HUD instead of the
> > worker
> > > > context
> > > > i was sending in PS#1, but we still need to avoid the dcheck at
> > > > https://cs.chromium.org/chromium/src/services/ui/public/cpp/
> > gpu/context_
> > > > provider_command_buffer.cc?gsn=Allocate&l=414
> > > > oherwise it crashes.
> > > >
> > >
> > > The compositor context is only used on the compositor thread, it does not
> > > need to be locked, and that's why it's dchecking.
> > >
> > >
> > > > This has still some work to do, hard codings which can be avoided, and
> > the
> > > > hud
> > > > flickers as we scroll. just wanted you to have the first look of it,
> > > > before i
> > > > proceed further.
> > > > PTAL, thanks :)
> > > >
> > > > https://codereview.chromium.org/2752833002/
> > > >
> > >
> > > --
> > > You received this message because you are subscribed to the Google Groups
> > > "Chromium-reviews" group.
> > > To unsubscribe from this group and stop receiving emails from it, send an
> > email
> > > to mailto:chromium-reviews+unsubscribe@chromium.org.
> > Thanks. yes that solves the crash. but the hud flickers as we scroll, and
> > disappears sometimes.
> > Should we change the quad append/drawing logic, param in some way ?
> > PTAL. Thanks.
> >
> 
> Sounds like maybe you're changing the texture while its being shown in the
> display instead of double buffering, or something.
> 
> 

I tried a few things, avoiding the resource re-use in
HeadsUpDisplayLayerImpl::AcquireResource, also i tried to get resource from
ResourcePool like the tile resources.
But it still doesnt improve. Strangely i see the TextureDrawQuad resource ids in
HeadsUpDisplayLayerImpl::AppendQuad landing up in DrawTiledQuad, and not in
EnqueueTextureQuad where i was expecting them to go (it happens w/o patch too).
Also, there are some logs like,
chromium: [ERROR:surface_manager.cc(138)] Attempting to require callback on
nonexistent surface

Any thing you can point to ?

Thanks.

> >
> > https://codereview.chromium.org/2752833002/
> >
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[sohan, 2017-04-18 17:54:25]

Description was changed from

==========
cc: Use gpu raster in HUD.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Use gpu raster in HUD.

This changes the SW back-end for drawing HUD texture to use 
Ganesh backed canvas and texture for drawing FPS, raster and
gpu memory info.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sohan, 2017-04-18 17:56:55]

Sorry for being late on this, it now looks stable, please take a look. Thanks.

[sohan, 2017-04-25 03:18:00]

On 2017/04/18 17:56:55, sohan wrote:
> Sorry for being late on this, it now looks stable, please take a look. Thanks.

Ping ! :) can you please take a look. Thanks.

[danakj, 2017-05-01 22:57:33]

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:102: RGBA_8888, gfx::ColorSpace());
do you want best_render_buffer_format() instead of best_texture_format()? that
is what we return here:
https://cs.chromium.org/chromium/src/cc/raster/gpu_raster_buffer_provider.cc?...

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:163: ContextProvider* context_provider)
{
What happens with --disable-gpu where the context provider is null?

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:172: resources_.back()->id(), false);
Can you comment that you pass false here because youre using the compositor
context, not the raster context? Also use a temp var to give the "false" a name

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:173: gfx::Rect
raster_rect(internal_content_bounds_);
you could remove this var and go directly to the SkIRect

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:179:
hud_surface_->prepareForExternalIO();
Do you need this since this is the single owner of hud_surface_?

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:183: context_provider, &lock, false,
false, false, 0);
again comment to explain the false and give these literals names via temp vars
please

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:184: hud_surface_ =
scoped_surface.sk_sp_surface();
why do you keep the surface alive for gpu raster? it's used to draw into the
texture but doesnt control the lifetime of it. basically it is a means to get an
SkCanvas for gpu raster.

I suggest you figure out what surface you want to use, pull out the SkCanvas
into a variable based on that, then use that below instead of
hud_surface_->getCanvas(), so that code doesn't have to care about the surface.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:193:
hud_surface_->getCanvas()->translate(-raster_rect.x(), -raster_rect.y());
can you point me how would x and y be not 0 here?

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:194:
hud_surface_->getCanvas()->clipRect(SkRect::MakeFromIRect(raster_bounds));
how come the clip?

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:210:
gl->GenUnverifiedSyncTokenCHROMIUM(fence, sync_token.GetData());
This stuff is more heavy then you need if !async_worker_context, which is
equivalent to what you have here right?
https://cs.chromium.org/chromium/src/cc/raster/gpu_raster_buffer_provider.cc?...

https://codereview.chromium.org/2752833002/diff/80001/cc/output/context_provi...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/2752833002/diff/80001/cc/output/context_provi...
cc/output/context_provider.h:53: class CC_EXPORT ScopedContext {
I don't think this class is doing more than if you just hold a
ContextCacheController::ScopedBusy directly, since all it does is return the
context provider you passed in. So I think I'd prefer just doing that.

Using an RAII object is helpful to make sure something is done, but in this case
the ScopedBusy already is one, and will complain if not handed back to the
context provider.

https://codereview.chromium.org/2752833002/diff/80001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/2752833002/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.h:356: sk_sp<SkSurface> sk_sp_surface() { return
sk_surface_; }
I dont think u need this

[sohan, 2017-05-03 11:03:30]

Patchset #5 (id:100001) has been deleted

[sohan, 2017-05-03 11:06:35]

Thanks for the review. Please take a look, thanks.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:102: RGBA_8888, gfx::ColorSpace());
On 2017/05/01 22:57:32, danakj wrote:
> do you want best_render_buffer_format() instead of best_texture_format()? that
> is what we return here:
>
https://cs.chromium.org/chromium/src/cc/raster/gpu_raster_buffer_provider.cc?...

Acknowledged.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:163: ContextProvider* context_provider)
{
On 2017/05/01 22:57:32, danakj wrote:
> What happens with --disable-gpu where the context provider is null?

I hope we do not want the SW raster path anymore, i am making it early return if
cxt provider is not there. is it ok ?

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:172: resources_.back()->id(), false);
On 2017/05/01 22:57:32, danakj wrote:
> Can you comment that you pass false here because youre using the compositor
> context, not the raster context? Also use a temp var to give the "false" a
name

Done.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:173: gfx::Rect
raster_rect(internal_content_bounds_);
On 2017/05/01 22:57:32, danakj wrote:
> you could remove this var and go directly to the SkIRect

Done.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:179:
hud_surface_->prepareForExternalIO();
On 2017/05/01 22:57:32, danakj wrote:
> Do you need this since this is the single owner of hud_surface_?

as we remove the surface handling based on your comment later, this resetting is
no longer reqd.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:183: context_provider, &lock, false,
false, false, 0);
On 2017/05/01 22:57:32, danakj wrote:
> again comment to explain the false and give these literals names via temp vars
> please

Done.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:184: hud_surface_ =
scoped_surface.sk_sp_surface();
On 2017/05/01 22:57:32, danakj wrote:
> why do you keep the surface alive for gpu raster? it's used to draw into the
> texture but doesnt control the lifetime of it. basically it is a means to get
an
> SkCanvas for gpu raster.
> 
> I suggest you figure out what surface you want to use, pull out the SkCanvas
> into a variable based on that, then use that below instead of
> hud_surface_->getCanvas(), so that code doesn't have to care about the
surface.

Done.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:193:
hud_surface_->getCanvas()->translate(-raster_rect.x(), -raster_rect.y());
On 2017/05/01 22:57:32, danakj wrote:
> can you point me how would x and y be not 0 here?

Yes, this was not needed, i tried to use the same canvas calls as SW raster
before. Removed.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:194:
hud_surface_->getCanvas()->clipRect(SkRect::MakeFromIRect(raster_bounds));
On 2017/05/01 22:57:32, danakj wrote:
> how come the clip?

Same as above.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:210:
gl->GenUnverifiedSyncTokenCHROMIUM(fence, sync_token.GetData());
On 2017/05/01 22:57:33, danakj wrote:
> This stuff is more heavy then you need if !async_worker_context, which is
> equivalent to what you have here right?
>
https://cs.chromium.org/chromium/src/cc/raster/gpu_raster_buffer_provider.cc?...

I am using GenSyncToken in latest PS, i hope this is what you wanted, because we
shouldnt remove sync tokens all together. right?

https://codereview.chromium.org/2752833002/diff/80001/cc/output/context_provi...
File cc/output/context_provider.h (right):

https://codereview.chromium.org/2752833002/diff/80001/cc/output/context_provi...
cc/output/context_provider.h:53: class CC_EXPORT ScopedContext {
On 2017/05/01 22:57:33, danakj wrote:
> I don't think this class is doing more than if you just hold a
> ContextCacheController::ScopedBusy directly, since all it does is return the
> context provider you passed in. So I think I'd prefer just doing that.
> 
> Using an RAII object is helpful to make sure something is done, but in this
case
> the ScopedBusy already is one, and will complain if not handed back to the
> context provider.

Done.

https://codereview.chromium.org/2752833002/diff/80001/cc/resources/resource_p...
File cc/resources/resource_provider.h (right):

https://codereview.chromium.org/2752833002/diff/80001/cc/resources/resource_p...
cc/resources/resource_provider.h:356: sk_sp<SkSurface> sk_sp_surface() { return
sk_surface_; }
On 2017/05/01 22:57:33, danakj wrote:
> I dont think u need this

Done.

[danakj, 2017-05-03 14:44:51]

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:163: ContextProvider* context_provider)
{
On 2017/05/03 11:06:34, sohan wrote:
> On 2017/05/01 22:57:32, danakj wrote:
> > What happens with --disable-gpu where the context provider is null?
> 
> I hope we do not want the SW raster path anymore, i am making it early return
if
> cxt provider is not there. is it ok ?

That would mean you can't have a HUD when using software compositing, so no
that's not really good.

[sohan, 2017-05-03 14:50:31]

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:163: ContextProvider* context_provider)
{
On 2017/05/03 14:44:51, danakj wrote:
> On 2017/05/03 11:06:34, sohan wrote:
> > On 2017/05/01 22:57:32, danakj wrote:
> > > What happens with --disable-gpu where the context provider is null?
> > 
> > I hope we do not want the SW raster path anymore, i am making it early
return
> if
> > cxt provider is not there. is it ok ?
> 
> That would mean you can't have a HUD when using software compositing, so no
> that's not really good.

OK, so we keep the old SW raster code path too?

[danakj, 2017-05-03 14:52:08]

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:163: ContextProvider* context_provider)
{
On 2017/05/03 14:50:30, sohan wrote:
> On 2017/05/03 14:44:51, danakj wrote:
> > On 2017/05/03 11:06:34, sohan wrote:
> > > On 2017/05/01 22:57:32, danakj wrote:
> > > > What happens with --disable-gpu where the context provider is null?
> > > 
> > > I hope we do not want the SW raster path anymore, i am making it early
> return
> > if
> > > cxt provider is not there. is it ok ?
> > 
> > That would mean you can't have a HUD when using software compositing, so no
> > that's not really good.
> 
> OK, so we keep the old SW raster code path too?

Yes, we need it when there is no GPU.

[sohan, 2017-05-04 12:02:42]

Description was changed from

==========
cc: Use gpu raster in HUD.

This changes the SW back-end for drawing HUD texture to use 
Ganesh backed canvas and texture for drawing FPS, raster and
gpu memory info.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Use gpu raster in HUD.

This changes the SW back-end for drawing HUD to use 
Ganesh backed canvas and texture for drawing FPS, raster and
gpu memory info. If gpu is not available it falls back to old
SW canvas implementation.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sohan, 2017-05-04 12:03:14]

Description was changed from

==========
cc: Use gpu raster in HUD.

This changes the SW back-end for drawing HUD to use 
Ganesh backed canvas and texture for drawing FPS, raster and
gpu memory info. If gpu is not available it falls back to old
SW canvas implementation.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Use gpu raster in HUD.

This changes the SW back-end for drawing HUD to use Ganesh
backed canvas and texture for drawing FPS, raster and gpu
memory info. If gpu is not available it falls back to old
SW canvas implementation.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[sohan, 2017-05-04 12:03:57]

Please take a look, thanks.

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/2752833002/diff/80001/cc/layers/heads_up_disp...
cc/layers/heads_up_display_layer_impl.cc:163: ContextProvider* context_provider)
{
On 2017/05/03 14:52:08, danakj wrote:
> On 2017/05/03 14:50:30, sohan wrote:
> > On 2017/05/03 14:44:51, danakj wrote:
> > > On 2017/05/03 11:06:34, sohan wrote:
> > > > On 2017/05/01 22:57:32, danakj wrote:
> > > > > What happens with --disable-gpu where the context provider is null?
> > > > 
> > > > I hope we do not want the SW raster path anymore, i am making it early
> > return
> > > if
> > > > cxt provider is not there. is it ok ?
> > > 
> > > That would mean you can't have a HUD when using software compositing, so
no
> > > that's not really good.
> > 
> > OK, so we keep the old SW raster code path too?
> 
> Yes, we need it when there is no GPU.

Done.

[sohanjg_, 2017-05-08 11:28:39]

The CQ bit was checked by sohanjg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-08 11:28:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-08 11:40:29]

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

[commit-bot: I haz the power, 2017-05-08 11:40:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sohanjg_, 2017-05-09 13:41:38]

The CQ bit was checked by sohanjg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-09 13:42:05]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 13:58:53]

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

[commit-bot: I haz the power, 2017-05-09 13:58:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[sohanjg_, 2017-05-10 07:06:26]

The CQ bit was checked by sohanjg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 07:07:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-10 08:03:56]

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

[commit-bot: I haz the power, 2017-05-10 08:03:56]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[sohanjg_, 2017-05-11 14:30:57]

The CQ bit was checked by sohanjg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-11 14:31:44]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[sohanjg_, 2017-05-11 14:47:21]

Please take a look, thanks!

https://codereview.chromium.org/2752833002/diff/200001/cc/trees/layer_tree_ho...
File cc/trees/layer_tree_host_unittest_context.cc (left):

https://codereview.chromium.org/2752833002/diff/200001/cc/trees/layer_tree_ho...
cc/trees/layer_tree_host_unittest_context.cc:1006: }
This was causing the context loss cb to be executed during updatehudtexture in
multi-threaded mode, as a result the gr resource cache was abandoned.
https://cs.chromium.org/chromium/src/cc/test/test_context_provider.cc?q=test_...


And in updatehudtexture, while acquiring the sk surface it returned NULL.
https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrResourceProvi...


I have delayed the callback till the draw completes, do you think this maintains
the expectations ?

[commit-bot: I haz the power, 2017-05-11 15:28:25]

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

[commit-bot: I haz the power, 2017-05-11 15:28:26]

Dry run: This issue passed the CQ dry run.

[sohan, 2017-05-17 17:16:33]

On 2017/05/11 14:47:21, sohanjg_ wrote:
> Please take a look, thanks!
> 
>
https://codereview.chromium.org/2752833002/diff/200001/cc/trees/layer_tree_ho...
> File cc/trees/layer_tree_host_unittest_context.cc (left):
> 
>
https://codereview.chromium.org/2752833002/diff/200001/cc/trees/layer_tree_ho...
> cc/trees/layer_tree_host_unittest_context.cc:1006: }
> This was causing the context loss cb to be executed during updatehudtexture in
> multi-threaded mode, as a result the gr resource cache was abandoned.
>
https://cs.chromium.org/chromium/src/cc/test/test_context_provider.cc?q=test_...
> 
> 
> And in updatehudtexture, while acquiring the sk surface it returned NULL.
>
https://cs.chromium.org/chromium/src/third_party/skia/src/gpu/GrResourceProvi...
> 
> 
> I have delayed the callback till the draw completes, do you think this
maintains
> the expectations ?

Ping :)

[sohanjg_, 2017-06-05 14:22:18]

ping ! :)

[sohanjg_, 2017-06-05 14:58:55]

The CQ bit was checked by sohanjg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-05 14:59:16]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-05 16:24:32]

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

[commit-bot: I haz the power, 2017-06-05 16:24:33]

Dry run: This issue passed the CQ dry run.

[sohan, 2017-06-06 18:43:04]

sohan.jyoti@huawei.com changed reviewers:
+ ericrk@chromium.org
- sohanjg@chromium.org

[sohan, 2017-06-06 18:43:05]

PTAL. Thanks.

[ericrk, 2017-06-06 20:22:25]

Thanks for taking a look at this. A few comments to start off.

https://codereview.chromium.org/2752833002/diff/200001/cc/layers/heads_up_dis...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/2752833002/diff/200001/cc/layers/heads_up_dis...
cc/layers/heads_up_display_layer_impl.cc:170:
std::unique_ptr<ContextCacheController::ScopedBusy> busy =
I wouldn't expect to need this DetachFromThread or ScopedBusy - I don't believe
we use these on the main context provider (just the worker context provider).

Given that the main context provider is not threadsafe, I'd be a bit worried if
we needed to detachfromthread. Can these lines just be removed?

https://codereview.chromium.org/2752833002/diff/200001/cc/layers/heads_up_dis...
cc/layers/heads_up_display_layer_impl.cc:192:
gpu_raster_canvas->clear(SkColorSetARGB(0, 0, 0, 0));
Can we just move the Trace, clear, save, scale, and following restore to within
DrawHudContents? this would avoid the duplication in both branch of the if
statement.

https://codereview.chromium.org/2752833002/diff/200001/cc/layers/heads_up_dis...
cc/layers/heads_up_display_layer_impl.cc:210:
context_provider->DetachFromThread();
See comment above - Drop these lines if possible...

[sohan, 2017-06-07 13:31:11]

https://codereview.chromium.org/2752833002/diff/200001/cc/layers/heads_up_dis...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/2752833002/diff/200001/cc/layers/heads_up_dis...
cc/layers/heads_up_display_layer_impl.cc:170:
std::unique_ptr<ContextCacheController::ScopedBusy> busy =
On 2017/06/06 20:22:25, ericrk wrote:
> I wouldn't expect to need this DetachFromThread or ScopedBusy - I don't
believe
> we use these on the main context provider (just the worker context provider).
> 
> Given that the main context provider is not threadsafe, I'd be a bit worried
if
> we needed to detachfromthread. Can these lines just be removed?

Done.

Yes, this was a little over engineering in main context.

https://codereview.chromium.org/2752833002/diff/200001/cc/layers/heads_up_dis...
cc/layers/heads_up_display_layer_impl.cc:192:
gpu_raster_canvas->clear(SkColorSetARGB(0, 0, 0, 0));
On 2017/06/06 20:22:25, ericrk wrote:
> Can we just move the Trace, clear, save, scale, and following restore to
within
> DrawHudContents? this would avoid the duplication in both branch of the if
> statement.

Done.

Thanks, this does make the code look better !

https://codereview.chromium.org/2752833002/diff/200001/cc/layers/heads_up_dis...
cc/layers/heads_up_display_layer_impl.cc:210:
context_provider->DetachFromThread();
On 2017/06/06 20:22:25, ericrk wrote:
> See comment above - Drop these lines if possible... 

Done.

[sohan, 2017-06-07 13:31:40]

Thanks for checking this. PTAL. Thanks.

[sohanjg_, 2017-06-07 13:32:11]

The CQ bit was checked by sohanjg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 13:32:28]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 14:46:15]

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

[commit-bot: I haz the power, 2017-06-07 14:46:17]

Dry run: This issue passed the CQ dry run.

[ericrk, 2017-06-07 20:50:18]

Seems OK to me at this point. LGTM % any further comments danakj@ might have.

https://codereview.chromium.org/2752833002/diff/220001/cc/layers/heads_up_dis...
File cc/layers/heads_up_display_layer_impl.cc (right):

https://codereview.chromium.org/2752833002/diff/220001/cc/layers/heads_up_dis...
cc/layers/heads_up_display_layer_impl.cc:196: lock.set_synchronized(true);
Don't worry about it for this change, but I wonder if we could clean up some of
our code by moving this sync token generation logic into the ScopedWriteLockGL,
like have a bool needs_sync_token, which will run this on destruction?

Again, not for this change, just something that might clean things up later.

[danakj, 2017-06-08 00:15:15]

On Wed, Jun 7, 2017 at 4:50 PM, <ericrk@chromium.org> wrote:

> Seems OK to me at this point. LGTM % any further comments danakj@ might
> have.
>

I'm happy if eric's happy. Thanks for looking eric!


>
>
> https://codereview.chromium.org/2752833002/diff/220001/cc/
> layers/heads_up_display_layer_impl.cc
> File cc/layers/heads_up_display_layer_impl.cc (right):
>
> https://codereview.chromium.org/2752833002/diff/220001/cc/
> layers/heads_up_display_layer_impl.cc#newcode196
> cc/layers/heads_up_display_layer_impl.cc:196:
> lock.set_synchronized(true);
> Don't worry about it for this change, but I wonder if we could clean up
> some of our code by moving this sync token generation logic into the
> ScopedWriteLockGL, like have a bool needs_sync_token, which will run
> this on destruction?
>
> Again, not for this change, just something that might clean things up
> later.
>
> https://codereview.chromium.org/2752833002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sohanjg_, 2017-06-08 01:03:47]

The CQ bit was checked by sohanjg@chromium.org

[commit-bot: I haz the power, 2017-06-08 01:04:13]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-08 01:09:53]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1496883827606110,
"parent_rev": "37e21d50eb77c515341932f63109afdd9a27e1ff", "commit_rev":
"bdc4a31b6e88e8a12b0b7bb298a03b020b1c61de"}

[commit-bot: I haz the power, 2017-06-08 01:10:05]

Description was changed from

==========
cc: Use gpu raster in HUD.

This changes the SW back-end for drawing HUD to use Ganesh
backed canvas and texture for drawing FPS, raster and gpu
memory info. If gpu is not available it falls back to old
SW canvas implementation.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Use gpu raster in HUD.

This changes the SW back-end for drawing HUD to use Ganesh
backed canvas and texture for drawing FPS, raster and gpu
memory info. If gpu is not available it falls back to old
SW canvas implementation.

BUG=345416
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2752833002
Cr-Commit-Position: refs/heads/master@{#477845}
Committed:
https://chromium.googlesource.com/chromium/src/+/bdc4a31b6e88e8a12b0b7bb298a0...
==========

[commit-bot: I haz the power, 2017-06-08 01:10:09]

Committed patchset #10 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/bdc4a31b6e88e8a12b0b7bb298a0...

[sohan, 2017-06-08 01:15:48]

On 2017/06/08 01:10:09, commit-bot: I haz the power wrote:
> Committed patchset #10 (id:220001) as
>
https://chromium.googlesource.com/chromium/src/+/bdc4a31b6e88e8a12b0b7bb298a0...

Thanks Eric, Dana!
I will take up the scopedlockgl tocken thing seperately.

[sunnyps, 2017-06-10 01:01:38]

On 2017/06/08 01:15:48, sohan wrote:
> On 2017/06/08 01:10:09, commit-bot: I haz the power wrote:
> > Committed patchset #10 (id:220001) as
> >
>
https://chromium.googlesource.com/chromium/src/+/bdc4a31b6e88e8a12b0b7bb298a0...
> 
> Thanks Eric, Dana!
> I will take up the scopedlockgl tocken thing seperately.

drive-by from merge conflict: I'll make ScopedWriteLockGL sync token API clearer
as a part of my CL (https://codereview.chromium.org/2885533002/) as I'm already
changing the lock to support worker context allocations there.