Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1375663002:
Show GpuMemoryBuffer feature in chrome://gpu (Closed)
|
Created:
5 years, 2 months ago by vignatti (out of this project) Modified:
5 years, 1 month ago CC:
chromium-reviews, darin-cc_chromium.org, jam, piman+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow GpuMemoryBuffer information in chrome://gpu
This CL adds a new section "Native GpuMemoryBuffer Status" in the Status page
to show whether GpuMemoryBuffer is running through native buffers.
BUG=475633
TEST=start chrome with --enable-native-gpu-memory-buffers, open chrome://gpu
and see a matrix with the GpuMemoryBuffer native configurations for CPU mapping
(CPU_READ_WRITE*). Start another chrome without that flag and see that the
native configurations for mapping are not present.
Committed: https://crrev.com/48a014ec51b94693aa7de9111aef5b9134434162
Cr-Commit-Position: refs/heads/master@{#356845}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix description #
Total comments: 2
Patch Set 3 : WIP on reveman comments in #9 #
Total comments: 5
Patch Set 4 : display a matrix of formats and usages #
Total comments: 2
Patch Set 5 : use IsNativeGpuMemoryBufferConfiguration #
Total comments: 35
Patch Set 6 : address nits and change GpuMemoryBufferInfoAsDictionaryValue main loop #
Total comments: 26
Patch Set 7 : nits #Patch Set 8 : improve the look #
Total comments: 4
Patch Set 9 : cl formatted and removed "Formats"/"Native usage support" value pair #
Messages
Total messages: 38 (6 generated)
tiago.vignatti@intel.com changed reviewers: + dongseong.hwang@intel.com, kbr@chromium.org, reveman@chromium.org
We already had issues with our validation team running wrong testing images, spending useless time due the lack of an easy way to check whether native GMB was enabled or not. I think this CL captures and helps out to fix this behaviour. kbr@ and reveman@. PTAL.
lgtm if this has been tested. One comment. https://codereview.chromium.org/1375663002/diff/1/content/browser/gpu/composi... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/1375663002/diff/1/content/browser/gpu/composi... content/browser/gpu/compositor_util.cc:159: "Raster is using fallback (shm).", Could you elaborate on the description here? Perhaps at least "Raster is using fallback path (shared memory, instead of zero- or one-copy)".
I'd like to review this but is ooo today. Please wait for my review tomorrow morning. On Mon, Sep 28, 2015, 7:00 PM <kbr@chromium.org> wrote: > lgtm if this has been tested. One comment. > > > > > https://codereview.chromium.org/1375663002/diff/1/content/browser/gpu/composi... > File content/browser/gpu/compositor_util.cc (right): > > > https://codereview.chromium.org/1375663002/diff/1/content/browser/gpu/composi... > content/browser/gpu/compositor_util.cc:159 > <https://codereview.chromium.org/1375663002/diff/1/content/browser/gpu/composi...>: > "Raster is using fallback > (shm).", > Could you elaborate on the description here? Perhaps at least "Raster is > using fallback path (shared memory, instead of zero- or one-copy)". > > https://codereview.chromium.org/1375663002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Thank you, kbr. Let's wait for reveman's now. https://codereview.chromium.org/1375663002/diff/1/content/browser/gpu/composi... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/1375663002/diff/1/content/browser/gpu/composi... content/browser/gpu/compositor_util.cc:159: "Raster is using fallback (shm).", On 2015/09/28 23:00:14, Ken Russell wrote: > Could you elaborate on the description here? Perhaps at least "Raster is using > fallback path (shared memory, instead of zero- or one-copy)". Done.
https://codereview.chromium.org/1375663002/diff/20001/content/browser/gpu/com... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/1375663002/diff/20001/content/browser/gpu/com... content/browser/gpu/compositor_util.cc:160: " instead the hardware backed one.", This is not completely true. Just because native GMBs are enabled doesn't mean that they can be used. It depends on the format and usage, which is determined by a number of settings such as usage of tile compression. Video pipeline uses R8 or one of the YUV formats and compositor is likely using RGBA8888 or BGRA888. I don't think it's a good idea to expose a feature that is supposed to tell if native GMBs are used or not. It would give a lot of false positives. https://codereview.chromium.org/1375663002/diff/20001/content/browser/gpu/com... content/browser/gpu/compositor_util.cc:228: // We need buffer mapping configuration to proper support native GMB. That's why this gets messy and I don't think it's a good idea. One buffer might have a native backing but another might not and a IsNativeGpuMemoryBufferEnabled function that returns true if the configs below are happens to be supported is going to be confusing. However, it would be nice to surface whether native GMBs are enabled or not and might be used. Either as a feature or as an about:flag. I think an about:flag would be useful for debugging purposes now that we enable this on more platforms. That allows a user to change it as well as see what the current value is. The difference compared to this patch will be that there's no guarantee that native GMBs are used just because the flag is enabled. You'll need to capture a trace to be sure. Just like the current --enable-native-gpu-memory-buffers switch. The flag will just mirror that switch.
On 2015/09/29 11:58:03, reveman (OOO Sept 24-29) wrote: > https://codereview.chromium.org/1375663002/diff/20001/content/browser/gpu/com... > File content/browser/gpu/compositor_util.cc (right): > > https://codereview.chromium.org/1375663002/diff/20001/content/browser/gpu/com... > content/browser/gpu/compositor_util.cc:160: " instead the hardware backed one.", > This is not completely true. Just because native GMBs are enabled doesn't mean > that they can be used. It depends on the format and usage, which is determined > by a number of settings such as usage of tile compression. Video pipeline uses > R8 or one of the YUV formats and compositor is likely using RGBA8888 or BGRA888. > I don't think it's a good idea to expose a feature that is supposed to tell if > native GMBs are used or not. It would give a lot of false positives. > > https://codereview.chromium.org/1375663002/diff/20001/content/browser/gpu/com... > content/browser/gpu/compositor_util.cc:228: // We need buffer mapping > configuration to proper support native GMB. > That's why this gets messy and I don't think it's a good idea. One buffer might > have a native backing but another might not and a IsNativeGpuMemoryBufferEnabled > function that returns true if the configs below are happens to be supported is > going to be confusing. > > However, it would be nice to surface whether native GMBs are enabled or not and > might be used. Either as a feature or as an about:flag. I think an about:flag > would be useful for debugging purposes now that we enable this on more > platforms. That allows a user to change it as well as see what the current value > is. > > The difference compared to this patch will be that there's no guarantee that > native GMBs are used just because the flag is enabled. You'll need to capture a > trace to be sure. Just like the current --enable-native-gpu-memory-buffers > switch. The flag will just mirror that switch. if it's about mirroring this switch just then we don't need any additions cause the developer can simply look for it in the "Command Line Args" field at chrome://gpu. But that it's not useful and, actually confuses even more when for example the switch is on and displayed in that page but not really enabled in the code. So I don't think it will help to expose it as a feature in chrome://flags either. I agree with you though that it's also confusing to expose the way I suggested here. The main point is that we need a runtime output of this feature. Do you think we can use the other fields of chrome://gpu like "Version Information" or "Driver Information" to display it? Or maybe simply be more bold about the feature saying explicitly what it is: "GpuMemoryBuffer used in the Compositor for Texture Uploads" or something?
On 2015/09/29 at 15:06:42, tiago.vignatti wrote: > On 2015/09/29 11:58:03, reveman (OOO Sept 24-29) wrote: > > https://codereview.chromium.org/1375663002/diff/20001/content/browser/gpu/com... > > File content/browser/gpu/compositor_util.cc (right): > > > > https://codereview.chromium.org/1375663002/diff/20001/content/browser/gpu/com... > > content/browser/gpu/compositor_util.cc:160: " instead the hardware backed one.", > > This is not completely true. Just because native GMBs are enabled doesn't mean > > that they can be used. It depends on the format and usage, which is determined > > by a number of settings such as usage of tile compression. Video pipeline uses > > R8 or one of the YUV formats and compositor is likely using RGBA8888 or BGRA888. > > I don't think it's a good idea to expose a feature that is supposed to tell if > > native GMBs are used or not. It would give a lot of false positives. > > > > https://codereview.chromium.org/1375663002/diff/20001/content/browser/gpu/com... > > content/browser/gpu/compositor_util.cc:228: // We need buffer mapping > > configuration to proper support native GMB. > > That's why this gets messy and I don't think it's a good idea. One buffer might > > have a native backing but another might not and a IsNativeGpuMemoryBufferEnabled > > function that returns true if the configs below are happens to be supported is > > going to be confusing. > > > > However, it would be nice to surface whether native GMBs are enabled or not and > > might be used. Either as a feature or as an about:flag. I think an about:flag > > would be useful for debugging purposes now that we enable this on more > > platforms. That allows a user to change it as well as see what the current value > > is. > > > > The difference compared to this patch will be that there's no guarantee that > > native GMBs are used just because the flag is enabled. You'll need to capture a > > trace to be sure. Just like the current --enable-native-gpu-memory-buffers > > switch. The flag will just mirror that switch. > > if it's about mirroring this switch just then we don't need any additions cause the developer can simply look for it in the "Command Line Args" field at chrome://gpu. But that it's not useful and, actually confuses even more when for example the switch is on and displayed in that page but not really enabled in the code. So I don't think it will help to expose it as a feature in chrome://flags either. > > I agree with you though that it's also confusing to expose the way I suggested here. The main point is that we need a runtime output of this feature. Do you think we can use the other fields of chrome://gpu like "Version Information" or "Driver Information" to display it? Or maybe simply be more bold about the feature saying explicitly what it is: "GpuMemoryBuffer used in the Compositor for Texture Uploads" or something? The compositor will very soon use more than one format for tiles. That means that some tiles are backed by native GMBs but others are not. Showing if native GMBs are used by a specific piece of code like the compositor which is not aware of the difference between native GMBs and the alternative is going to be a pain to maintain as by changing the compositor code chrome://gpu will no longer be correct. We can show a matrix of GMB formats+usages for which native GMBs are available in chrome://gpu.
On 2015/09/29 17:06:46, reveman wrote: > We can show a matrix of GMB formats+usages for which native GMBs are available > in chrome://gpu. David, I just updated the patchset with a WIP towards this suggestion you made. I've updated also the description of the CL. PTAL and if it sounds reasonable, I'll be working on adding the format+usages matrix to about:gpu then.
https://codereview.chromium.org/1375663002/diff/40001/content/browser/gpu/com... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/1375663002/diff/40001/content/browser/gpu/com... content/browser/gpu/compositor_util.cc:33: const char* kNativeGpuMemoryBufferFeatureName = "native_gpu_memory_buffers"; s/GpuMemoryBuffer/GpuMemoryBuffers/ https://codereview.chromium.org/1375663002/diff/40001/content/browser/gpu/com... content/browser/gpu/compositor_util.cc:228: bool IsNativeGpuMemoryBufferEnabled() { The logic used to determine if enable_native_gpu_memory_buffers variable in GetNativeGpuMemoryBufferConfigurations (browser_gpu_memory_buffer_manager.cc) is true should move here. https://codereview.chromium.org/1375663002/diff/40001/content/browser/gpu/com... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/1375663002/diff/40001/content/browser/gpu/com... content/browser/gpu/compositor_util.h:24: // Returns true if native GpuMemoryBuffer can be used and is on (via flags, or nit: s/GpuMemoryBuffer/GpuMemoryBuffers/ https://codereview.chromium.org/1375663002/diff/40001/content/browser/gpu/com... content/browser/gpu/compositor_util.h:26: CONTENT_EXPORT bool IsNativeGpuMemoryBufferEnabled(); nit: s/GpuMemoryBuffer/GpuMemoryBuffers/ I think "Is" is correct as it's referring to the feature and not the buffers. https://codereview.chromium.org/1375663002/diff/40001/content/browser/resourc... File content/browser/resources/gpu/info_view.js (right): https://codereview.chromium.org/1375663002/diff/40001/content/browser/resourc... content/browser/resources/gpu/info_view.js:102: }; Why is this not just another field in featureLabelMap? It's a boolean. We're either using native buffers or not.
Patchset now has been changed considerably from the previous one, so I didn't addressed the issues you pointed. PTAL.
On 2015/10/16 21:46:53, vignatti wrote: > Patchset now has been changed considerably from the previous one, so I didn't > addressed the issues you pointed. PTAL. David, in browser_gpu_memory_buffer_manager.cc, do you think it's okay to change GpuMemoryBufferConfigurationSet to use a list instead a hash? Doing like this we'd be able to iterate through the supported native configurations to display them in the about:gpu. Also, in BrowserGpuMemoryBufferManager::GetImageTextureTarget we're getting the configuration in the local native_configurations. Is this really needed given that it was already cached previously in its ctor?
On 2015/10/19 13:39:18, vignatti wrote: > On 2015/10/16 21:46:53, vignatti wrote: > > Patchset now has been changed considerably from the previous one, so I didn't > > addressed the issues you pointed. PTAL. > > David, in browser_gpu_memory_buffer_manager.cc, do you think it's okay to change > GpuMemoryBufferConfigurationSet to use a list instead a hash? Doing like this > we'd be able to iterate through the supported native configurations to display > them in the about:gpu. > > Also, in BrowserGpuMemoryBufferManager::GetImageTextureTarget we're getting the > configuration in the local native_configurations. Is this really needed given > that it was already cached previously in its ctor? friendly ping reveman@. Let me know about these questions above. I'd need to rework this CL then with the recent GMB BufferUsage renaming.
The general idea of showing a matrix of gpumemorybuffer support in chrome://gpu seems useful. It just needs to be done without the code duplication that this patch introduces. I haven't looked at this in detail but why can't we just expose and use the existing IsNativeGpuMemoryBufferConfiguration function in BrowserGMBM?
On 2015/10/26 20:35:28, reveman wrote: > The general idea of showing a matrix of gpumemorybuffer support in chrome://gpu > seems useful. It just needs to be done without the code duplication that this > patch introduces. I haven't looked at this in detail but why can't we just > expose and use the existing IsNativeGpuMemoryBufferConfiguration function in > BrowserGMBM? if we want to use IsNativeGMBConfiguration then we'd need to change native_configurations_ to be a list instead a hash set, because in the displayed matrix we need to go through all the possible configurations which is not possible when using hash map. So I was wondering if, from the performance point of view, you see any problems in doing this changes (assuming hash seek is faster than the list).
https://codereview.chromium.org/1375663002/diff/60001/ui/gfx/buffer_types.h File ui/gfx/buffer_types.h (right): https://codereview.chromium.org/1375663002/diff/60001/ui/gfx/buffer_types.h#n... ui/gfx/buffer_types.h:62: reveman@ do you think it's worth doing this global in here?
On 2015/10/26 at 20:54:07, tiago.vignatti wrote: > On 2015/10/26 20:35:28, reveman wrote: > > The general idea of showing a matrix of gpumemorybuffer support in chrome://gpu > > seems useful. It just needs to be done without the code duplication that this > > patch introduces. I haven't looked at this in detail but why can't we just > > expose and use the existing IsNativeGpuMemoryBufferConfiguration function in > > BrowserGMBM? > > if we want to use IsNativeGMBConfiguration then we'd need to change native_configurations_ to be a list instead a hash set, because in the displayed matrix we need to go through all the possible configurations which is not possible when using hash map. So I was wondering if, from the performance point of view, you see any problems in doing this changes (assuming hash seek is faster than the list). Why do we need a list? Can't we iterate over all format/usage combinations to produce this matrix?
https://codereview.chromium.org/1375663002/diff/60001/ui/gfx/buffer_types.h File ui/gfx/buffer_types.h (right): https://codereview.chromium.org/1375663002/diff/60001/ui/gfx/buffer_types.h#n... ui/gfx/buffer_types.h:62: On 2015/10/26 at 20:55:49, vignatti wrote: > reveman@ do you think it's worth doing this global in here? I'd keep it local for now and we can move it hear as soon as we have another use-case for it.
On 2015/10/26 21:09:20, reveman wrote: > https://codereview.chromium.org/1375663002/diff/60001/ui/gfx/buffer_types.h > File ui/gfx/buffer_types.h (right): > > https://codereview.chromium.org/1375663002/diff/60001/ui/gfx/buffer_types.h#n... > ui/gfx/buffer_types.h:62: > On 2015/10/26 at 20:55:49, vignatti wrote: > > reveman@ do you think it's worth doing this global in here? > > I'd keep it local for now and we can move it hear as soon as we have another > use-case for it. PTAL in Set 5 now reveman@. Thank you.
Description was changed from
==========
Show GpuMemoryBuffer information in chrome://gpu
This CL adds a new section "GpuMemoryBuffer implementation" in the Status page
to show whether GpuMemoryBuffer is running through native buffers.
Follow-up CL will iterate through GetNativeGpuMemoryBufferConfigurations,
adding a matrix detailing which native buffer formats and usages are supported.
BUG=475633
==========
to
==========
Show GpuMemoryBuffer information in chrome://gpu
This CL adds a new section "GpuMemoryBuffer implementation" in the Status
page
to show whether GpuMemoryBuffer is running through native buffers.
BUG=475633
TEST=start chrome with --enable-native-gpu-memory-buffers, open chrome://gpu
and see a matrix with the GpuMemoryBuffer native configurations for CPU
mapping
(CPU_READ_WRITE*). Start another chrome without that flag and see that the
native configurations for mapping are not present.
==========
Description was changed from
==========
Show GpuMemoryBuffer information in chrome://gpu
This CL adds a new section "GpuMemoryBuffer implementation" in the Status
page
to show whether GpuMemoryBuffer is running through native buffers.
BUG=475633
TEST=start chrome with --enable-native-gpu-memory-buffers, open chrome://gpu
and see a matrix with the GpuMemoryBuffer native configurations for CPU
mapping
(CPU_READ_WRITE*). Start another chrome without that flag and see that the
native configurations for mapping are not present.
==========
to
==========
Show GpuMemoryBuffer information in chrome://gpu
This CL adds a new section "GpuMemoryBuffer implementation" in the Status page
to show whether GpuMemoryBuffer is running through native buffers.
BUG=475633
TEST=start chrome with --enable-native-gpu-memory-buffers, open chrome://gpu
and see a matrix with the GpuMemoryBuffer native configurations for CPU mapping
(CPU_READ_WRITE*). Start another chrome without that flag and see that the
native configurations for mapping are not present.
==========
https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_memory_buffer_manager.h:97: gfx::BufferUsage usage) const; nit: please move the function in the cc file too so the order is the same https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:236: const char* NativeBufferFormatToString(gfx::BufferFormat format) { nit: Remove "Native" prefix https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:250: default: nit: remove default case, include all formats and move NOTREACHED below the switch statement https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:256: const char* NativeBufferUsageToString(gfx::BufferUsage usage) { nit: Remove "Native" prefix https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:267: } NOTREACHED(); return nullptr; https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:269: base::DictionaryValue* GmbInfoAsDictionaryValue() { nit: Gmb -> GpuMemoryBuffer. please avoid the GMB abbreviation here and everywhere else in the patch https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:270: base::ListValue* gmb_info = new base::ListValue(); nit: gmb -> gpu_memory_buffer https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:273: BrowserGpuMemoryBufferManager* gmb_manager = nit: gmb -> gpu_memory_buffer https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:279: gfx::BufferFormat::UYVY_422, gfx::BufferFormat::YUV_420_BIPLANAR}; I don't think this code should assume that only these formats can be native https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:283: gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT}; can you use gfx::BufferUsage::LAST and gfx::BufferFormat::LAST instead of listing all these here? https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:294: info->Set("gmb_info", gmb_info); gmb -> gpu_memory_buffer https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:478: scoped_ptr<base::DictionaryValue> gmb_info_val(GmbInfoAsDictionaryValue()); gmb -> gpu_memory_buffer https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:480: // Add in blacklisting features blacklisting features? https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:481: base::DictionaryValue* gmb_status = new base::DictionaryValue; gmb -> gpu_memory_buffer https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:482: gmb_info_val->Set("gbmStatus", gmb_status); gmb -> gpu_memory_buffer https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:484: // Send GMB Info to javascript. gmb -> gpu_memory_buffer https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:486: *(gmb_info_val.get())); gmb -> gpu_memory_buffer https://codereview.chromium.org/1375663002/diff/80001/content/browser/resourc... File content/browser/resources/gpu/browser_bridge.js (right): https://codereview.chromium.org/1375663002/diff/80001/content/browser/resourc... content/browser/resources/gpu/browser_bridge.js:43: cr.dispatchSimpleEvent(this, 'gmbInfoUpdate'); gmb -> gpu_memory_buffer https://codereview.chromium.org/1375663002/diff/80001/content/browser/resourc... File content/browser/resources/gpu/info_view.html (right): https://codereview.chromium.org/1375663002/diff/80001/content/browser/resourc... content/browser/resources/gpu/info_view.html:26: <h3>GpuMemoryBuffer implementation</h3> "Native GpuMemoryBuffer Status"? https://codereview.chromium.org/1375663002/diff/80001/content/browser/resourc... content/browser/resources/gpu/info_view.html:27: <div id="gmb-info"></div> gmb -> gpu_memory_buffer
Thanks for reviewing. PTAL. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_memory_buffer_manager.h (right): https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_memory_buffer_manager.h:97: gfx::BufferUsage usage) const; On 2015/10/27 23:20:35, reveman wrote: > nit: please move the function in the cc file too so the order is the same Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:236: const char* NativeBufferFormatToString(gfx::BufferFormat format) { On 2015/10/27 23:20:36, reveman wrote: > nit: Remove "Native" prefix Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:250: default: On 2015/10/27 23:20:35, reveman wrote: > nit: remove default case, include all formats and move NOTREACHED below the > switch statement Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:256: const char* NativeBufferUsageToString(gfx::BufferUsage usage) { On 2015/10/27 23:20:36, reveman wrote: > nit: Remove "Native" prefix Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:267: } On 2015/10/27 23:20:35, reveman wrote: > NOTREACHED(); > return nullptr; Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:269: base::DictionaryValue* GmbInfoAsDictionaryValue() { On 2015/10/27 23:20:36, reveman wrote: > nit: Gmb -> GpuMemoryBuffer. please avoid the GMB abbreviation here and > everywhere else in the patch Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:270: base::ListValue* gmb_info = new base::ListValue(); On 2015/10/27 23:20:36, reveman wrote: > nit: gmb -> gpu_memory_buffer Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:273: BrowserGpuMemoryBufferManager* gmb_manager = On 2015/10/27 23:20:36, reveman wrote: > nit: gmb -> gpu_memory_buffer Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:283: gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT}; On 2015/10/27 23:20:35, reveman wrote: > can you use gfx::BufferUsage::LAST and gfx::BufferFormat::LAST instead of > listing all these here? hmm with this the code became not so clear now, but I think it's fine. Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:294: info->Set("gmb_info", gmb_info); On 2015/10/27 23:20:35, reveman wrote: > gmb -> gpu_memory_buffer Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:480: // Add in blacklisting features On 2015/10/27 23:20:36, reveman wrote: > blacklisting features? Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:481: base::DictionaryValue* gmb_status = new base::DictionaryValue; On 2015/10/27 23:20:35, reveman wrote: > gmb -> gpu_memory_buffer Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:482: gmb_info_val->Set("gbmStatus", gmb_status); On 2015/10/27 23:20:36, reveman wrote: > gmb -> gpu_memory_buffer Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:484: // Send GMB Info to javascript. On 2015/10/27 23:20:36, reveman wrote: > gmb -> gpu_memory_buffer Done. https://codereview.chromium.org/1375663002/diff/80001/content/browser/gpu/gpu... content/browser/gpu/gpu_internals_ui.cc:486: *(gmb_info_val.get())); On 2015/10/27 23:20:36, reveman wrote: > gmb -> gpu_memory_buffer Done.
https://codereview.chromium.org/1375663002/diff/100001/content/browser/gpu/gp... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1375663002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_internals_ui.cc:308: info->Set("gmb_info", gpu_memory_buffer_info); nit: "gpu_memory_buffer_info" https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... File content/browser/resources/gpu/browser_bridge.js (right): https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:39: this.gmbInfo_ = data.gmbInfo; nit: gpuMemoryBufferInfo https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:43: cr.dispatchSimpleEvent(this, 'gmbInfoUpdate'); it: gpuMemoryBufferInfo https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:100: * Get gmbInfo data. it: gpuMemoryBufferInfo https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:102: get gmbInfo() { it: gpuMemoryBufferInfo https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:103: return this.gmbInfo_; it: gpuMemoryBufferInfo https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:110: this.gmbInfo_ = gmbInfo; it: gpuMemoryBufferInfo https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:111: cr.dispatchSimpleEvent(this, 'gmbInfoUpdate'); it: gpuMemoryBufferInfo https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... File content/browser/resources/gpu/info_view.html (right): https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/info_view.html:26: <h3>GpuMemoryBuffer implementation</h3> See my comment on previous patch. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/info_view.html:27: <div id="gmb-info"></div> nit: gpu-memory-buffer-info https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... File content/browser/resources/gpu/info_view.js (right): https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/info_view.js:26: browserBridge.addEventListener('gmbInfoUpdate', this.refresh.bind(this)); and here https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/info_view.js:156: var gmbInfo = browserBridge.gmbInfo; and here https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/info_view.js:221: if (gmbInfo.gmb_info) here and below
PTAL. https://codereview.chromium.org/1375663002/diff/100001/content/browser/gpu/gp... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1375663002/diff/100001/content/browser/gpu/gp... content/browser/gpu/gpu_internals_ui.cc:308: info->Set("gmb_info", gpu_memory_buffer_info); On 2015/10/28 14:41:07, reveman wrote: > nit: "gpu_memory_buffer_info" Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... File content/browser/resources/gpu/browser_bridge.js (right): https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:39: this.gmbInfo_ = data.gmbInfo; On 2015/10/28 14:41:07, reveman wrote: > nit: gpuMemoryBufferInfo Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:43: cr.dispatchSimpleEvent(this, 'gmbInfoUpdate'); On 2015/10/28 14:41:07, reveman wrote: > it: gpuMemoryBufferInfo Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:100: * Get gmbInfo data. On 2015/10/28 14:41:07, reveman wrote: > it: gpuMemoryBufferInfo Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:102: get gmbInfo() { On 2015/10/28 14:41:07, reveman wrote: > it: gpuMemoryBufferInfo Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:103: return this.gmbInfo_; On 2015/10/28 14:41:07, reveman wrote: > it: gpuMemoryBufferInfo Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:110: this.gmbInfo_ = gmbInfo; On 2015/10/28 14:41:07, reveman wrote: > it: gpuMemoryBufferInfo Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/browser_bridge.js:111: cr.dispatchSimpleEvent(this, 'gmbInfoUpdate'); On 2015/10/28 14:41:07, reveman wrote: > it: gpuMemoryBufferInfo Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... File content/browser/resources/gpu/info_view.html (right): https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/info_view.html:26: <h3>GpuMemoryBuffer implementation</h3> On 2015/10/28 14:41:07, reveman wrote: > See my comment on previous patch. oh sorry, I didn't see that. Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/info_view.html:27: <div id="gmb-info"></div> On 2015/10/28 14:41:07, reveman wrote: > nit: gpu-memory-buffer-info Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... File content/browser/resources/gpu/info_view.js (right): https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/info_view.js:26: browserBridge.addEventListener('gmbInfoUpdate', this.refresh.bind(this)); On 2015/10/28 14:41:07, reveman wrote: > and here Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/info_view.js:156: var gmbInfo = browserBridge.gmbInfo; On 2015/10/28 14:41:07, reveman wrote: > and here Done. https://codereview.chromium.org/1375663002/diff/100001/content/browser/resour... content/browser/resources/gpu/info_view.js:221: if (gmbInfo.gmb_info) On 2015/10/28 14:41:07, reveman wrote: > here and below Done.
Description was changed from
==========
Show GpuMemoryBuffer information in chrome://gpu
This CL adds a new section "GpuMemoryBuffer implementation" in the Status page
to show whether GpuMemoryBuffer is running through native buffers.
BUG=475633
TEST=start chrome with --enable-native-gpu-memory-buffers, open chrome://gpu
and see a matrix with the GpuMemoryBuffer native configurations for CPU mapping
(CPU_READ_WRITE*). Start another chrome without that flag and see that the
native configurations for mapping are not present.
==========
to
==========
Show GpuMemoryBuffer information in chrome://gpu
This CL adds a new section "Native GpuMemoryBuffer Status" in the Status page
to show whether GpuMemoryBuffer is running through native buffers.
BUG=475633
TEST=start chrome with --enable-native-gpu-memory-buffers, open chrome://gpu
and see a matrix with the GpuMemoryBuffer native configurations for CPU mapping
(CPU_READ_WRITE*). Start another chrome without that flag and see that the
native configurations for mapping are not present.
==========
I tried this patch locally without any native GMB support and it looks weird to have a section on chrome://gpu that is empty. This is what I see: --------------------------------------------- |Native GpuMemoryBuffer Status | --------------------------------------------- |Native Formats | Usage | --------------------------------------------- how about we show something like this instead: ----------------------------------------------------- |GpuMemoryBuffer Status | ----------------------------------------------------- |Formats | Native usage support | ----------------------------------------------------- |RGBA_8888 | GPU_READ, GPU_READ_WRITE, ... | |RGBX_8888 | Unsupported | |... | ... | Wdyt?
On 2015/10/28 18:05:02, reveman wrote: > I tried this patch locally without any native GMB support and it looks weird to > have a section on chrome://gpu that is empty. This is what I see: > > --------------------------------------------- > |Native GpuMemoryBuffer Status | > --------------------------------------------- > |Native Formats | Usage | > --------------------------------------------- > > how about we show something like this instead: > > ----------------------------------------------------- > |GpuMemoryBuffer Status | > ----------------------------------------------------- > |Formats | Native usage support | > ----------------------------------------------------- > |RGBA_8888 | GPU_READ, GPU_READ_WRITE, ... | > |RGBX_8888 | Unsupported | > |... | ... | > > Wdyt? yup, I definitely agree. PTAL now. Thanks!
Thanks! LGTM after removing the "Formats"/"Native usage support" value pair as mentioned in the comment below. https://codereview.chromium.org/1375663002/diff/140001/content/browser/gpu/gp... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1375663002/diff/140001/content/browser/gpu/gp... content/browser/gpu/gpu_internals_ui.cc:289: NewDescriptionValuePair("Formats", "Native usage support")); I think it's a bit inappropriate to use a value pair row to describe the columns below. I think it's better to just remove this row and maybe instead improve the value pair strings for each format. See below. https://codereview.chromium.org/1375663002/diff/140001/content/browser/gpu/gp... content/browser/gpu/gpu_internals_ui.cc:308: native_usage_support = base::StringPrintf("Unsupported"); Maybe "Software only" instead of "Unsupported" as that would implicitly make the values represent what has native support. Leaving it as "Unsupported" is fine with me too.
nit: please cl format this patch before landing
alright, thanks David. I think I've addressed all the comments now. Let's try to land it. https://codereview.chromium.org/1375663002/diff/140001/content/browser/gpu/gp... File content/browser/gpu/gpu_internals_ui.cc (right): https://codereview.chromium.org/1375663002/diff/140001/content/browser/gpu/gp... content/browser/gpu/gpu_internals_ui.cc:289: NewDescriptionValuePair("Formats", "Native usage support")); On 2015/10/29 00:27:26, reveman wrote: > I think it's a bit inappropriate to use a value pair row to describe the columns > below. I think it's better to just remove this row and maybe instead improve the > value pair strings for each format. See below. Done. https://codereview.chromium.org/1375663002/diff/140001/content/browser/gpu/gp... content/browser/gpu/gpu_internals_ui.cc:308: native_usage_support = base::StringPrintf("Unsupported"); On 2015/10/29 00:27:26, reveman wrote: > Maybe "Software only" instead of "Unsupported" as that would implicitly make the > values represent what has native support. Leaving it as "Unsupported" is fine > with me too. yup, let's use "Software only" then. Done.
The CQ bit was checked by tiago.vignatti@intel.com
The patchset sent to the CQ was uploaded after l-g-t-m from kbr@chromium.org, reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1375663002/#ps160001 (title: "cl formatted and removed "Formats"/"Native usage support" value pair")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1375663002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1375663002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/48a014ec51b94693aa7de9111aef5b9134434162 Cr-Commit-Position: refs/heads/master@{#356845} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
