GLImage::OnMemoryDump Stubs + Some Impls

Implements GLImage*Memory::OnMemoryDump and
GLImageIOSurface::OnMemoryDump and adds stubs for other
types.

Also adds code to iterate GLImages in TextureManager::OnMemoryDump.

BUG=514914
Committed: https://crrev.com/28a82a0e2228a8c2b69ab3cb9ddd05d9350643ae
Cr-Commit-Position: refs/heads/master@{#344751}

[ericrk, 2015-08-17 18:40:08]

ericrk@chromium.org changed reviewers:
+ reveman@chromium.org

[ericrk, 2015-08-18 07:29:26]

Patchset #1 (id:1) has been deleted

[ericrk, 2015-08-18 07:38:45]

Patchset #1 (id:20001) has been deleted

[ericrk, 2015-08-18 07:39:53]

https://codereview.chromium.org/1299713003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1299713003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1299: const std::string&
dump_name) const {
David, I remember you mentioning that we never had more than one sub-image in
practice? Do we need to iterate faces/levels, or is it sufficient to just dump
0/0?

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_memory.h
File ui/gl/gl_image_memory.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_memory.h...
ui/gl/gl_image_memory.h:53: BufferFormat format() const { return format_; }
Used by derived classes to calculate size.

[reveman, 2015-08-18 08:32:52]

https://codereview.chromium.org/1299713003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1299713003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2091: if
(ref->texture()->HasImages()) {
refactor this as discussed

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image.h#newcode21
ui/gl/gl_image.h:21: }  // base
nit: not sure the closing "}" comments are needed

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image.h#newcode70
ui/gl/gl_image.h:70: virtual void
OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,
All other functions have a short comment that describes them. Can you add one
here?

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_egl.h
File ui/gl/gl_image_egl.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_egl.h#ne...
ui/gl/gl_image_egl.h:38: void OnMemoryDump(base::trace_event::ProcessMemoryDump*
pmd,
This is not the final class. SurfaceTexture is and already implements this. I'd
remove it from here.

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.h:55: void
OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,
nit: move below ScheduleOverlayPlane and make sure .cc file matches.

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_ref_coun...
File ui/gl/gl_image_ref_counted_memory.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_ref_coun...
ui/gl/gl_image_ref_counted_memory.h:26: 
nit: remove blank line

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_shared_m...
File ui/gl/gl_image_shared_memory.cc (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_shared_m...
ui/gl/gl_image_shared_memory.cc:87: const std::string& dump_name) {
This function should report 0 size or early out after ::Destroy has been called.
Early out to be consistent with GLImageRefCountedMemory.

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_shared_m...
File ui/gl/gl_image_shared_memory.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_shared_m...
ui/gl/gl_image_shared_memory.h:23: 
nit: remove blank line

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_surface_...
File ui/gl/gl_image_surface_texture.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_surface_...
ui/gl/gl_image_surface_texture.h:40: 
nit: remove blank line

[ericrk, 2015-08-18 09:00:19]

Patchset #2 (id:60001) has been deleted

[ericrk, 2015-08-18 09:00:47]

Thanks for the feedback - incorporated changes from both CLs.

https://codereview.chromium.org/1299713003/diff/40001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1299713003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1299: const std::string&
dump_name) const {
On 2015/08/18 07:39:52, ericrk wrote:
> David, I remember you mentioning that we never had more than one sub-image in
> practice? Do we need to iterate faces/levels, or is it sufficient to just dump
> 0/0?

Per offline discussion, not changing this.

https://codereview.chromium.org/1299713003/diff/40001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2091: if
(ref->texture()->HasImages()) {
On 2015/08/18 08:32:52, reveman wrote:
> refactor this as discussed

Done.

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image.h
File ui/gl/gl_image.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image.h#newcode21
ui/gl/gl_image.h:21: }  // base
On 2015/08/18 08:32:52, reveman wrote:
> nit: not sure the closing "}" comments are needed

Done.

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image.h#newcode70
ui/gl/gl_image.h:70: virtual void
OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,
On 2015/08/18 08:32:52, reveman wrote:
> All other functions have a short comment that describes them. Can you add one
> here? 

Done.

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_egl.h
File ui/gl/gl_image_egl.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_egl.h#ne...
ui/gl/gl_image_egl.h:38: void OnMemoryDump(base::trace_event::ProcessMemoryDump*
pmd,
On 2015/08/18 08:32:52, reveman wrote:
> This is not the final class. SurfaceTexture is and already implements this.
I'd
> remove it from here.

Done.

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_io_surfa...
File ui/gl/gl_image_io_surface.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_io_surfa...
ui/gl/gl_image_io_surface.h:55: void
OnMemoryDump(base::trace_event::ProcessMemoryDump* pmd,
On 2015/08/18 08:32:52, reveman wrote:
> nit: move below ScheduleOverlayPlane and make sure .cc file matches.

Done.

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_ref_coun...
File ui/gl/gl_image_ref_counted_memory.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_ref_coun...
ui/gl/gl_image_ref_counted_memory.h:26: 
On 2015/08/18 08:32:52, reveman wrote:
> nit: remove blank line

Done.

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_shared_m...
File ui/gl/gl_image_shared_memory.cc (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_shared_m...
ui/gl/gl_image_shared_memory.cc:87: const std::string& dump_name) {
On 2015/08/18 08:32:52, reveman wrote:
> This function should report 0 size or early out after ::Destroy has been
called.
> Early out to be consistent with GLImageRefCountedMemory.

Switched both to report 0.

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_shared_m...
File ui/gl/gl_image_shared_memory.h (right):

https://codereview.chromium.org/1299713003/diff/40001/ui/gl/gl_image_shared_m...
ui/gl/gl_image_shared_memory.h:23: 
On 2015/08/18 08:32:52, reveman wrote:
> nit: remove blank line

Done.

[reveman, 2015-08-18 09:05:57]

https://codereview.chromium.org/1299713003/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1299713003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1304: if
(level_infos[level_index].image) {
should we dump the memory used by each level in the form "%s/face_%d/level_%d"
even if no image is attached?

https://codereview.chromium.org/1299713003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2122: if
(ref->texture()->HasImages()) {
is this "if" statement needed?

[ericrk, 2015-08-18 10:03:52]

ericrk@chromium.org changed reviewers:
+ danakj@chromium.org, piman@chromium.org

[ericrk, 2015-08-18 10:03:53]

+piman for gpu related paths
+danakj for ui/gfx

https://codereview.chromium.org/1299713003/diff/80001/gpu/command_buffer/serv...
File gpu/command_buffer/service/texture_manager.cc (right):

https://codereview.chromium.org/1299713003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:1304: if
(level_infos[level_index].image) {
On 2015/08/18 09:05:57, reveman wrote:
> should we dump the memory used by each level in the form "%s/face_%d/level_%d"
> even if no image is attached?

Dumping these, unless they are size 0. Images appear to include all mip levels,
even if not in use, leading to a bunch of 0 entries if we dump them all.

https://codereview.chromium.org/1299713003/diff/80001/gpu/command_buffer/serv...
gpu/command_buffer/service/texture_manager.cc:2122: if
(ref->texture()->HasImages()) {
On 2015/08/18 09:05:57, reveman wrote:
> is this "if" statement needed?

I guess not. Especially now that we are dumping more than images.

[reveman, 2015-08-18 10:06:24]

lgtm

[piman, 2015-08-18 15:08:45]

rs lgtm

[danakj, 2015-08-18 17:46:57]

ui/gfx lgtm

[ericrk, 2015-08-20 10:42:04]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-08-20 10:42:18]

The CQ bit was unchecked by ericrk@chromium.org

[ericrk, 2015-08-20 13:55:05]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-08-20 13:55:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1299713003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1299713003/100001

[commit-bot: I haz the power, 2015-08-20 14:15:39]

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

[commit-bot: I haz the power, 2015-08-20 14:15:40]

Try jobs failed on following builders:
  android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[ericrk, 2015-08-20 17:32:43]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-08-20 17:32:44]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
reveman@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1299713003/#ps120001
(title: "Fix android build.")

[commit-bot: I haz the power, 2015-08-20 17:34:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1299713003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1299713003/120001

[commit-bot: I haz the power, 2015-08-20 20:46:54]

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

[commit-bot: I haz the power, 2015-08-20 20:46:55]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[ericrk, 2015-08-21 10:40:17]

The CQ bit was checked by ericrk@chromium.org

[commit-bot: I haz the power, 2015-08-21 10:40:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1299713003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1299713003/120001

[commit-bot: I haz the power, 2015-08-21 13:28:41]

Committed patchset #4 (id:120001)

[commit-bot: I haz the power, 2015-08-21 13:29:12]

Patchset 4 (id:??) landed as
https://crrev.com/28a82a0e2228a8c2b69ab3cb9ddd05d9350643ae
Cr-Commit-Position: refs/heads/master@{#344751}