Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1139903005:
Add PERSISTENT_MAP usage for GpuMemoryBuffers. (Closed)
|
Created:
5 years, 7 months ago by danakj Modified:
5 years, 7 months ago CC:
cc-bugs_chromium.org, chromium-reviews, creis+watch_chromium.org, danakj+watch_chromium.org, darin-cc_chromium.org, enne (OOO), jam, jbauman+watch_chromium.org, kalyank, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, piman+watch_chromium.org, piman, sadrul, sievers+watch_chromium.org, sky, Ian Vollick Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd PERSISTENT_MAP usage for GpuMemoryBuffers.
A GpuMemoryBuffer with this usage flag will always point at the
same memory contents each time it is mapped. This will enable
partial tile updates by avoiding rastering content from the
previous frame again in the compositor.
R=reveman,piman
BUG=489447
Committed: https://crrev.com/77180b4434d93f45022b8c65110344cdced4b19d
Cr-Commit-Position: refs/heads/master@{#330987}
Committed: https://crrev.com/9966900efc1025704645b673fdd63dbf1f2ad189
Cr-Commit-Position: refs/heads/master@{#331416}
Patch Set 1 : persistentmap: . #
Total comments: 18
Patch Set 2 : persistentmap: reviewed #Patch Set 3 : persistentmap: fixtests #Patch Set 4 : persistentmap: fixmandolinebetter #
Total comments: 28
Patch Set 5 : persistentmap: . #Patch Set 6 : persistentmap: onevar #Patch Set 7 : persistentmap: . #
Total comments: 6
Patch Set 8 : persistentmap: nits #Patch Set 9 : persistentmap: typo #
Total comments: 2
Patch Set 10 : persistentmap: . #Patch Set 11 : persistentmap: unmap #Patch Set 12 : persistentmap: rebase #Messages
Total messages: 51 (17 generated)
danakj@chromium.org changed reviewers: + piman@chromium.org
Patchset #1 (id:1) has been deleted
https://codereview.chromium.org/1139903005/diff/20001/cc/trees/layer_tree_set... File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/1139903005/diff/20001/cc/trees/layer_tree_set... cc/trees/layer_tree_settings.h:78: unsigned persistent_map_image_texture_target; How are we going to tell the compositor to use partial updates vs no partial updates? If that's going to be a setting then perhaps use_image_texture_target is enough? https://codereview.chromium.org/1139903005/diff/20001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:472: // TODO(reveman): No support for native PERSISTENT_MAP. crbug.com/489438 It shouldn't hurt to enable PERSISTENT_MAP here too right now. The current idea is that kEnableNativeGpuMemoryBuffers enables all native buffer support available. https://codereview.chromium.org/1139903005/diff/20001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:263: GpuMemoryBufferFactory::GetSupportedTypes(usage, &supported_types); Instead of changing GetSupportedTypes (which might be removed soon) I think we should use IsGpuMemoryBufferConfigurationSupported here to determine what type is actually going to be used. Requires adding gfx::GpuMemoryBuffer::Format as the first param to this function but I think that's what we want. It will return the target to use for a specific combination of format and usage. https://codereview.chromium.org/1139903005/diff/20001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_memory_buffer_manager.cc:105: << usage; Do you mind adding a GpuMemoryBufferImplSharedMemory::IsUsageSupported function to clean this up a bit? https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:144: gfx::Size buffer_size(2, 2); Fyi, nothing new with this patch so I don't mind if you keep it consistent with the code above but we should probably be using dimensions that are multiple of 4 here to ensure this will work with compressed formats. https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:214: EXPECT_EQ(strides[plane], remap_strides[plane]); I don't think we should expect this to be true. An implementation could move the storage and the stride might change. https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:219: EXPECT_EQ(row_sizes_in_bytes[plane], row_size_in_bytes); Why are you checking this? row_size_in_bytes can't change as it's unrelated to the buffer instance. https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/gpu_... File content/common/gpu/gpu_memory_buffer_factory.h (right): https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/gpu_... content/common/gpu/gpu_memory_buffer_factory.h:39: std::vector<gfx::GpuMemoryBufferType>* types); I'd rather not change this function. It might be removed in the near future and the current idea is that it shouldn't take format or usage into account. Let me know if using IsGpuMemoryBufferConfigurationSupported in GetImageTextureTarget doesn't solve this problem. https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/gpu_... File content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/gpu_... content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc:33: {gfx::GpuMemoryBuffer::YUV_420, gfx::GpuMemoryBuffer::PERSISTENT_MAP}}; nit: let's sort these lines alphabetically
https://codereview.chromium.org/1139903005/diff/20001/cc/trees/layer_tree_set... File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/1139903005/diff/20001/cc/trees/layer_tree_set... cc/trees/layer_tree_settings.h:78: unsigned persistent_map_image_texture_target; On 2015/05/19 16:12:12, reveman wrote: > How are we going to tell the compositor to use partial updates vs no partial > updates? If that's going to be a setting then perhaps use_image_texture_target > is enough? The compositor decides in LTHI to use zero or one copy, and needs to know map vs persistent map when making that decision. https://codereview.chromium.org/1139903005/diff/20001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:263: GpuMemoryBufferFactory::GetSupportedTypes(usage, &supported_types); On 2015/05/19 16:12:12, reveman wrote: > Instead of changing GetSupportedTypes (which might be removed soon) I think we > should use IsGpuMemoryBufferConfigurationSupported here to determine what type > is actually going to be used. > > Requires adding gfx::GpuMemoryBuffer::Format as the first param to this function > but I think that's what we want. It will return the target to use for a specific > combination of format and usage. I don't understand this request :( IsGpuMemoryBufferConfigurationSupported(format, usage) returns true if a format,usage pair is valid. It doesn't take a type as a parameter. Is that what you are asking for? The IsGpuMemoryBufferConfigurationSupported() method has no comments explaining what it is doing so I'm not sure TBH. But for SHARED_MEMORY type it always returns false. So then how would GetImageTextureTarget decided we're going to use SHARED_MEMORY ever?
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/1139903005/diff/20001/content/browser/browser... File content/browser/browser_main_loop.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/browser/browser... content/browser/browser_main_loop.cc:472: // TODO(reveman): No support for native PERSISTENT_MAP. crbug.com/489438 On 2015/05/19 16:12:12, reveman wrote: > It shouldn't hurt to enable PERSISTENT_MAP here too right now. The current idea > is that kEnableNativeGpuMemoryBuffers enables all native buffer support > available. Done. https://codereview.chromium.org/1139903005/diff/20001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_channel_host_factory.cc:263: GpuMemoryBufferFactory::GetSupportedTypes(usage, &supported_types); On 2015/05/19 20:11:52, danakj wrote: > On 2015/05/19 16:12:12, reveman wrote: > > Instead of changing GetSupportedTypes (which might be removed soon) I think we > > should use IsGpuMemoryBufferConfigurationSupported here to determine what type > > is actually going to be used. > > > > Requires adding gfx::GpuMemoryBuffer::Format as the first param to this > function > > but I think that's what we want. It will return the target to use for a > specific > > combination of format and usage. > > I don't understand this request :( > > IsGpuMemoryBufferConfigurationSupported(format, usage) returns true if a > format,usage pair is valid. It doesn't take a type as a parameter. Is that what > you are asking for? > > The IsGpuMemoryBufferConfigurationSupported() method has no comments explaining > what it is doing so I'm not sure TBH. But for SHARED_MEMORY type it always > returns false. So then how would GetImageTextureTarget decided we're going to > use SHARED_MEMORY ever? OK thanks for the discussion about this! I've passed format to this method (hardcoded at the callsites with TODOs) and am using a static IsGMBConfigSupported method to decide to use native or shared memory buffers. https://codereview.chromium.org/1139903005/diff/20001/content/browser/gpu/bro... File content/browser/gpu/browser_gpu_memory_buffer_manager.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/browser/gpu/bro... content/browser/gpu/browser_gpu_memory_buffer_manager.cc:105: << usage; On 2015/05/19 16:12:12, reveman wrote: > Do you mind adding a GpuMemoryBufferImplSharedMemory::IsUsageSupported function > to clean this up a bit? Done. https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/clie... File content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:144: gfx::Size buffer_size(2, 2); On 2015/05/19 16:12:12, reveman wrote: > Fyi, nothing new with this patch so I don't mind if you keep it consistent with > the code above but we should probably be using dimensions that are multiple of 4 > here to ensure this will work with compressed formats. Do you want me to change both tests to 4? I'll do that. https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:214: EXPECT_EQ(strides[plane], remap_strides[plane]); On 2015/05/19 16:12:12, reveman wrote: > I don't think we should expect this to be true. An implementation could move the > storage and the stride might change. OK, done. https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/clie... content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:219: EXPECT_EQ(row_sizes_in_bytes[plane], row_size_in_bytes); On 2015/05/19 16:12:12, reveman wrote: > Why are you checking this? row_size_in_bytes can't change as it's unrelated to > the buffer instance. I see, right. Removed. https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/gpu_... File content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc (right): https://codereview.chromium.org/1139903005/diff/20001/content/common/gpu/gpu_... content/common/gpu/gpu_memory_buffer_factory_shared_memory.cc:33: {gfx::GpuMemoryBuffer::YUV_420, gfx::GpuMemoryBuffer::PERSISTENT_MAP}}; On 2015/05/19 16:12:12, reveman wrote: > nit: let's sort these lines alphabetically Done.
danakj@chromium.org changed reviewers: + alexst@chromium.org
+alexst for ui/ozone
danakj@chromium.org changed reviewers: + sky@chromium.org
+sky for mandoline/
mandoline LGTM
mandoline LGTM
https://codereview.chromium.org/1139903005/diff/120001/ui/ozone/gpu/gpu_memor... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1139903005/diff/120001/ui/ozone/gpu/gpu_memor... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:119: return SurfaceFactoryOzone::MAP; Add a new enum type to SurfaceFactoryOzone, please. We have a way to query support from the platform via CanCreateNativePixmap(BufferUsage usage). It is valid to return SurfaceFactoryOzone::PERSISTENT_MAP here.
I would like to see this CL split into 3 different patches. 1. GpuMemoryBuffer changes that add PERSISTENT_MAP (no compositor changes). 2. Browser compositor support. 3. Renderer compositor support. I think 1 is ready to land once you've addressed my non-compositor comments on this patch. 2, 3 needs more discussion but I suspect that 2 is going to be easier so it might be good to land that first. https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... cc/trees/layer_tree_settings.h:78: unsigned persistent_map_image_texture_target; I don't think this mechanism where we have a different setting field for each configuration works long term. A setting field for each combination of format and usage would explode. Options: 1. unsigned use_image_texture_target[gfx::GpuMemoryBuffer::FORMAT_LAST + 1][gfx::GpuMemoryBuffer::USAGE_LAST + 1]; 2. unsigned use_image_texture_target; and the embedder needs to figure out what target makes sense based on its usage of the compositor. e.g. use_one_copy=true + use_partial_updates=true means GetImageTextureTarget(BGRA_8888, PERSISTENT_MAP). The second option is basically what ToT is doing but BGRA_8888 is implicit. If you want to limit the scope of this patch then this option might be better. https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.cc:272: // we must fallback to shared memory buffers, which do support them. The condition for |format| and |usage| not being supported is exactly the same as the UsageEnabled(usage) check above. If the factory is not going to be used for this usage+format then GL_TEXTURE_2D should be returned. No need to mention anything about a shared memory implementation here. https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.cc:276: type = gfx::SHARED_MEMORY_BUFFER; I don't see a good reason for handling this differently than the UsageEnabled(usage) check. Please replace this line with "return GL_TEXTURE_2D;" and remove the DCHECKs. https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.cc:490: if (!IsGpuMemoryBufferFactoryUsageEnabled(usage)) Please remove this check from this helper function so it's not done twice. https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.cc:494: GpuMemoryBufferFactory::GetSupportedTypes(&supported_types); Please pass the type as a parameter to this helper function instead of calling ::GetSupportedTypes twice. https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.h:80: static bool IsGpuMemoryBufferConfigurationSupportedInternal( If you can move this to the anonymous namespace of the cc file then that would be preferred. Either way, IsGpuMemoryBufferFactoryConfigurationSupported is preferred over the "Internal" suffix as that's a more accurate description. https://codereview.chromium.org/1139903005/diff/120001/content/common/child_p... File content/common/child_process_host_impl.cc (left): https://codereview.chromium.org/1139903005/diff/120001/content/common/child_p... content/common/child_process_host_impl.cc:326: usage == gfx::GpuMemoryBuffer::MAP) { just change this line to "GpuMemoryBufferImplSharedMemory::IsUsageSupported(usage)) {" instead https://codereview.chromium.org/1139903005/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc (right): https://codereview.chromium.org/1139903005/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc:131: } NOTREACHED(); return false; https://codereview.chromium.org/1139903005/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc (right): https://codereview.chromium.org/1139903005/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:182: } nit: please remove this loop and move the code into the loops below instead. that's more consistent with the use of "data" in the loops below and the Map test above. https://codereview.chromium.org/1139903005/diff/120001/content/public/common/... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1139903005/diff/120001/content/public/common/... content/public/common/content_switches.h:232: CONTENT_EXPORT extern const char kPersistentMapImageTextureTarget[]; See my content/renderer/gpu/compositor_dependencies.h comment. I don't think a different command line switch for each combination of format and usage is an acceptable solution. https://codereview.chromium.org/1139903005/diff/120001/content/renderer/gpu/c... File content/renderer/gpu/compositor_dependencies.h (right): https://codereview.chromium.org/1139903005/diff/120001/content/renderer/gpu/c... content/renderer/gpu/compositor_dependencies.h:46: virtual uint32 GetPersistentMapImageTextureTarget() = 0; Having a different function for each configuration doesn't work long term. It would be a mess to add a function for each combination of format and usage. Can we pass format and usage as parameters instead? https://codereview.chromium.org/1139903005/diff/120001/ui/compositor/composit... File ui/compositor/compositor.h (right): https://codereview.chromium.org/1139903005/diff/120001/ui/compositor/composit... ui/compositor/compositor.h:101: virtual uint32 GetPersistentMapImageTextureTarget() = 0; Same here. Having a different function for each configuration doesn't work long term. https://codereview.chromium.org/1139903005/diff/120001/ui/ozone/gpu/gpu_memor... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1139903005/diff/120001/ui/ozone/gpu/gpu_memor... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:117: // TODO(alexst): This is not supported by ozone (yet?). I think you should add the SurfaceFactoryOzone::PERSISTENT_MAP enum value as part of this patch. We have SurfaceFactoryOzone::MAP already and that's not supported yet.
https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... cc/trees/layer_tree_settings.h:78: unsigned persistent_map_image_texture_target; On 2015/05/20 14:26:07, reveman wrote: > I don't think this mechanism where we have a different setting field for each > configuration works long term. A setting field for each combination of format > and usage would explode. Options: > > 1. unsigned use_image_texture_target[gfx::GpuMemoryBuffer::FORMAT_LAST + > 1][gfx::GpuMemoryBuffer::USAGE_LAST + 1]; > > 2. unsigned use_image_texture_target; and the embedder needs to figure out what > target makes sense based on its usage of the compositor. e.g. use_one_copy=true > + use_partial_updates=true means GetImageTextureTarget(BGRA_8888, > PERSISTENT_MAP). > > The second option is basically what ToT is doing but BGRA_8888 is implicit. If > you want to limit the scope of this patch then this option might be better. I agree. I'm just maintaining the "single target" that we have now, but also allowing us to use partial texture update. I don't think we need anything more complicated than that for this patch. I also think we need both because the embedder is not deciding which mode the compositor is using. This is simply renaming the "use_image_texture_target" and providing the same simple thing for partial updates. https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_channel_host_factory.cc (right): https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.cc:272: // we must fallback to shared memory buffers, which do support them. On 2015/05/20 14:26:07, reveman wrote: > The condition for |format| and |usage| not being supported is exactly the same > as the UsageEnabled(usage) check above. If the factory is not going to be used > for this usage+format then GL_TEXTURE_2D should be returned. No need to mention > anything about a shared memory implementation here. Done. https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.cc:276: type = gfx::SHARED_MEMORY_BUFFER; On 2015/05/20 14:26:08, reveman wrote: > I don't see a good reason for handling this differently than the > UsageEnabled(usage) check. Please replace this line with "return GL_TEXTURE_2D;" > and remove the DCHECKs. Done. https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.cc:490: if (!IsGpuMemoryBufferFactoryUsageEnabled(usage)) On 2015/05/20 14:26:07, reveman wrote: > Please remove this check from this helper function so it's not done twice. Done. https://codereview.chromium.org/1139903005/diff/120001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.cc:494: GpuMemoryBufferFactory::GetSupportedTypes(&supported_types); On 2015/05/20 14:26:07, reveman wrote: > Please pass the type as a parameter to this helper function instead of calling > ::GetSupportedTypes twice. Done. https://codereview.chromium.org/1139903005/diff/120001/content/common/child_p... File content/common/child_process_host_impl.cc (left): https://codereview.chromium.org/1139903005/diff/120001/content/common/child_p... content/common/child_process_host_impl.cc:326: usage == gfx::GpuMemoryBuffer::MAP) { On 2015/05/20 14:26:08, reveman wrote: > just change this line to > "GpuMemoryBufferImplSharedMemory::IsUsageSupported(usage)) {" > instead Done. https://codereview.chromium.org/1139903005/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc (right): https://codereview.chromium.org/1139903005/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_shared_memory.cc:131: } On 2015/05/20 14:26:08, reveman wrote: > NOTREACHED(); > return false; Done. https://codereview.chromium.org/1139903005/diff/120001/content/common/gpu/cli... File content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc (right): https://codereview.chromium.org/1139903005/diff/120001/content/common/gpu/cli... content/common/gpu/client/gpu_memory_buffer_impl_unittest.cc:182: } On 2015/05/20 14:26:08, reveman wrote: > nit: please remove this loop and move the code into the loops below instead. > that's more consistent with the use of "data" in the loops below and the Map > test above. Done. https://codereview.chromium.org/1139903005/diff/120001/content/public/common/... File content/public/common/content_switches.h (right): https://codereview.chromium.org/1139903005/diff/120001/content/public/common/... content/public/common/content_switches.h:232: CONTENT_EXPORT extern const char kPersistentMapImageTextureTarget[]; On 2015/05/20 14:26:08, reveman wrote: > See my content/renderer/gpu/compositor_dependencies.h comment. I don't think a > different command line switch for each combination of format and usage is an > acceptable solution. This is just to support partial update vs not. I agree with you, but the compositor only uses a single format and we don't know what it is in the embedder. This is only going as far as we need to go for supporting partial tile updates. Please don't make me rewrite everything about choosing GpuMemoryBuffer formats in order to implement partial tile updates. This is blocking implside painting and I just don't see the need. https://codereview.chromium.org/1139903005/diff/120001/ui/ozone/gpu/gpu_memor... File ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc (right): https://codereview.chromium.org/1139903005/diff/120001/ui/ozone/gpu/gpu_memor... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:117: // TODO(alexst): This is not supported by ozone (yet?). On 2015/05/20 14:26:08, reveman wrote: > I think you should add the SurfaceFactoryOzone::PERSISTENT_MAP enum value as > part of this patch. We have SurfaceFactoryOzone::MAP already and that's not > supported yet. Done. https://codereview.chromium.org/1139903005/diff/120001/ui/ozone/gpu/gpu_memor... ui/ozone/gpu/gpu_memory_buffer_factory_ozone_native_buffer.cc:119: return SurfaceFactoryOzone::MAP; On 2015/05/20 12:43:37, alexst wrote: > Add a new enum type to SurfaceFactoryOzone, please. We have a way to query > support from the platform via CanCreateNativePixmap(BufferUsage usage). It is > valid to return SurfaceFactoryOzone::PERSISTENT_MAP here. Done.
https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... cc/trees/layer_tree_settings.h:78: unsigned persistent_map_image_texture_target; On 2015/05/20 14:26:07, reveman wrote: > I don't think this mechanism where we have a different setting field for each > configuration works long term. A setting field for each combination of format > and usage would explode. Options: > > 1. unsigned use_image_texture_target[gfx::GpuMemoryBuffer::FORMAT_LAST + > 1][gfx::GpuMemoryBuffer::USAGE_LAST + 1]; > > 2. unsigned use_image_texture_target; and the embedder needs to figure out what > target makes sense based on its usage of the compositor. e.g. use_one_copy=true > + use_partial_updates=true means GetImageTextureTarget(BGRA_8888, > PERSISTENT_MAP). OK I'm going to go with option 2 here. This will mean you cannot turn on native buffers without breaking partial tile updates, which I was trying to avoid for you. But this seems contentious so I'll just leave us with 1 var for now instead.
https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... cc/trees/layer_tree_settings.h:78: unsigned persistent_map_image_texture_target; On 2015/05/20 17:55:11, danakj wrote: > On 2015/05/20 14:26:07, reveman wrote: > > I don't think this mechanism where we have a different setting field for each > > configuration works long term. A setting field for each combination of format > > and usage would explode. Options: > > > > 1. unsigned use_image_texture_target[gfx::GpuMemoryBuffer::FORMAT_LAST + > > 1][gfx::GpuMemoryBuffer::USAGE_LAST + 1]; > > > > 2. unsigned use_image_texture_target; and the embedder needs to figure out > what > > target makes sense based on its usage of the compositor. e.g. > use_one_copy=true > > + use_partial_updates=true means GetImageTextureTarget(BGRA_8888, > > PERSISTENT_MAP). > > OK I'm going to go with option 2 here. This will mean you cannot turn on native > buffers without breaking partial tile updates, which I was trying to avoid for > you. But this seems contentious so I'll just leave us with 1 var for now > instead. Sounds good. We're mostly interested in using native buffers for the renderer compositor, which I'm expecting not to be using partial updates. https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... cc/trees/layer_tree_settings.h:78: unsigned persistent_map_image_texture_target; On 2015/05/20 17:51:14, danakj wrote: > On 2015/05/20 14:26:07, reveman wrote: > > I don't think this mechanism where we have a different setting field for each > > configuration works long term. A setting field for each combination of format > > and usage would explode. Options: > > > > 1. unsigned use_image_texture_target[gfx::GpuMemoryBuffer::FORMAT_LAST + > > 1][gfx::GpuMemoryBuffer::USAGE_LAST + 1]; > > > > 2. unsigned use_image_texture_target; and the embedder needs to figure out > what > > target makes sense based on its usage of the compositor. e.g. > use_one_copy=true > > + use_partial_updates=true means GetImageTextureTarget(BGRA_8888, > > PERSISTENT_MAP). > > > > The second option is basically what ToT is doing but BGRA_8888 is implicit. If > > you want to limit the scope of this patch then this option might be better. > > I agree. I'm just maintaining the "single target" that we have now, but also > allowing us to use partial texture update. I don't think we need anything more > complicated than that for this patch. I also think we need both because the > embedder is not deciding which mode the compositor is using. This is simply > renaming the "use_image_texture_target" and providing the same simple thing for > partial updates. Not asking you to fix the format problem. I simply like us to not move in a direction that is known to not work long term. We should either allow just a single format+usage configuration to be used (like ToT) or any configuration. map_/persisten_map_ puts us somewhere in between, which I think is confusing and makes it hard for someone to know what to do if they like to add another usage mode and/or format.
On Wed, May 20, 2015 at 11:58 AM, <reveman@chromium.org> wrote: > > > https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... > File cc/trees/layer_tree_settings.h (right): > > > https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... > cc/trees/layer_tree_settings.h:78: unsigned > persistent_map_image_texture_target; > On 2015/05/20 17:55:11, danakj wrote: > >> On 2015/05/20 14:26:07, reveman wrote: >> > I don't think this mechanism where we have a different setting field >> > for each > >> > configuration works long term. A setting field for each combination >> > of format > >> > and usage would explode. Options: >> > >> > 1. unsigned >> > use_image_texture_target[gfx::GpuMemoryBuffer::FORMAT_LAST + > >> > 1][gfx::GpuMemoryBuffer::USAGE_LAST + 1]; >> > >> > 2. unsigned use_image_texture_target; and the embedder needs to >> > figure out > >> what >> > target makes sense based on its usage of the compositor. e.g. >> use_one_copy=true >> > + use_partial_updates=true means GetImageTextureTarget(BGRA_8888, >> > PERSISTENT_MAP). >> > > OK I'm going to go with option 2 here. This will mean you cannot turn >> > on native > >> buffers without breaking partial tile updates, which I was trying to >> > avoid for > >> you. But this seems contentious so I'll just leave us with 1 var for >> > now > >> instead. >> > > Sounds good. We're mostly interested in using native buffers for the > renderer compositor, which I'm expecting not to be using partial > updates. I would like it too, though, in some future. Savings are real. > > > > https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... > cc/trees/layer_tree_settings.h:78: unsigned > persistent_map_image_texture_target; > On 2015/05/20 17:51:14, danakj wrote: > >> On 2015/05/20 14:26:07, reveman wrote: >> > I don't think this mechanism where we have a different setting field >> > for each > >> > configuration works long term. A setting field for each combination >> > of format > >> > and usage would explode. Options: >> > >> > 1. unsigned >> > use_image_texture_target[gfx::GpuMemoryBuffer::FORMAT_LAST + > >> > 1][gfx::GpuMemoryBuffer::USAGE_LAST + 1]; >> > >> > 2. unsigned use_image_texture_target; and the embedder needs to >> > figure out > >> what >> > target makes sense based on its usage of the compositor. e.g. >> use_one_copy=true >> > + use_partial_updates=true means GetImageTextureTarget(BGRA_8888, >> > PERSISTENT_MAP). >> > >> > The second option is basically what ToT is doing but BGRA_8888 is >> > implicit. If > >> > you want to limit the scope of this patch then this option might be >> > better. > > I agree. I'm just maintaining the "single target" that we have now, >> > but also > >> allowing us to use partial texture update. I don't think we need >> > anything more > >> complicated than that for this patch. I also think we need both >> > because the > >> embedder is not deciding which mode the compositor is using. This is >> > simply > >> renaming the "use_image_texture_target" and providing the same simple >> > thing for > >> partial updates. >> > > Not asking you to fix the format problem. I simply like us to not move > in a direction that is known to not work long term. > > We should either allow just a single format+usage configuration to be > used (like ToT) or any configuration. map_/persisten_map_ puts us > somewhere in between, which I think is confusing and makes it hard for > someone to know what to do if they like to add another usage mode and/or > format. > > https://codereview.chromium.org/1139903005/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, May 20, 2015 at 12:00 PM, Antoine Labour <piman@chromium.org> wrote: > > > On Wed, May 20, 2015 at 11:58 AM, <reveman@chromium.org> wrote: > >> >> >> https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... >> File cc/trees/layer_tree_settings.h (right): >> >> >> https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... >> cc/trees/layer_tree_settings.h:78: unsigned >> persistent_map_image_texture_target; >> On 2015/05/20 17:55:11, danakj wrote: >> >>> On 2015/05/20 14:26:07, reveman wrote: >>> > I don't think this mechanism where we have a different setting field >>> >> for each >> >>> > configuration works long term. A setting field for each combination >>> >> of format >> >>> > and usage would explode. Options: >>> > >>> > 1. unsigned >>> >> use_image_texture_target[gfx::GpuMemoryBuffer::FORMAT_LAST + >> >>> > 1][gfx::GpuMemoryBuffer::USAGE_LAST + 1]; >>> > >>> > 2. unsigned use_image_texture_target; and the embedder needs to >>> >> figure out >> >>> what >>> > target makes sense based on its usage of the compositor. e.g. >>> use_one_copy=true >>> > + use_partial_updates=true means GetImageTextureTarget(BGRA_8888, >>> > PERSISTENT_MAP). >>> >> >> OK I'm going to go with option 2 here. This will mean you cannot turn >>> >> on native >> >>> buffers without breaking partial tile updates, which I was trying to >>> >> avoid for >> >>> you. But this seems contentious so I'll just leave us with 1 var for >>> >> now >> >>> instead. >>> >> >> Sounds good. We're mostly interested in using native buffers for the >> renderer compositor, which I'm expecting not to be using partial >> updates. > > > I would like it too, though, in some future. Savings are real. > We will get partial updates in the renderer too. We can't go quite as far but we do get most of it. 1) For one-copy we get the same wins in the renderer as the browser since it's the staging buffers. (~10x win) 2) For zero-copy/gpu raster/software compositing, we don't get the full wins as we can't just re-use the resource. We'll have to copy the old resource then raster the new part of it. (~3x win) To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Patchset #7 (id:180001) has been deleted
Patchset #7 (id:200001) has been deleted
Patchset #7 (id:220001) has been deleted
Patchset #7 (id:240001) has been deleted
PTAL. Depends on https://codereview.chromium.org/1149803002/ now.
lgtm with nit https://codereview.chromium.org/1139903005/diff/260001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/1139903005/diff/260001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.h:80: static bool IsGpuMemoryBufferConfigurationSupportedInternal( nit: IsGpuMemoryBufferFactoryConfigurationSupported and move to anonymous namespace if possible https://codereview.chromium.org/1139903005/diff/260001/mandoline/ui/aura/surf... File mandoline/ui/aura/surface_context_factory.cc (right): https://codereview.chromium.org/1139903005/diff/260001/mandoline/ui/aura/surf... mandoline/ui/aura/surface_context_factory.cc:74: uint32 SurfaceContextFactory::GetPersistentMapImageTextureTarget() { ditto https://codereview.chromium.org/1139903005/diff/260001/mandoline/ui/aura/surf... File mandoline/ui/aura/surface_context_factory.h (right): https://codereview.chromium.org/1139903005/diff/260001/mandoline/ui/aura/surf... mandoline/ui/aura/surface_context_factory.h:34: uint32 GetPersistentMapImageTextureTarget() override; I assume this is a leftover from previous patch and should not be here anymore.
ui/ozone lgtm
On 2015/05/20 at 19:05:45, danakj wrote: > On Wed, May 20, 2015 at 12:00 PM, Antoine Labour <piman@chromium.org> wrote: > > > > > > > On Wed, May 20, 2015 at 11:58 AM, <reveman@chromium.org> wrote: > > > >> > >> > >> https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... > >> File cc/trees/layer_tree_settings.h (right): > >> > >> > >> https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... > >> cc/trees/layer_tree_settings.h:78: unsigned > >> persistent_map_image_texture_target; > >> On 2015/05/20 17:55:11, danakj wrote: > >> > >>> On 2015/05/20 14:26:07, reveman wrote: > >>> > I don't think this mechanism where we have a different setting field > >>> > >> for each > >> > >>> > configuration works long term. A setting field for each combination > >>> > >> of format > >> > >>> > and usage would explode. Options: > >>> > > >>> > 1. unsigned > >>> > >> use_image_texture_target[gfx::GpuMemoryBuffer::FORMAT_LAST + > >> > >>> > 1][gfx::GpuMemoryBuffer::USAGE_LAST + 1]; > >>> > > >>> > 2. unsigned use_image_texture_target; and the embedder needs to > >>> > >> figure out > >> > >>> what > >>> > target makes sense based on its usage of the compositor. e.g. > >>> use_one_copy=true > >>> > + use_partial_updates=true means GetImageTextureTarget(BGRA_8888, > >>> > PERSISTENT_MAP). > >>> > >> > >> OK I'm going to go with option 2 here. This will mean you cannot turn > >>> > >> on native > >> > >>> buffers without breaking partial tile updates, which I was trying to > >>> > >> avoid for > >> > >>> you. But this seems contentious so I'll just leave us with 1 var for > >>> > >> now > >> > >>> instead. > >>> > >> > >> Sounds good. We're mostly interested in using native buffers for the > >> renderer compositor, which I'm expecting not to be using partial > >> updates. > > > > > > I would like it too, though, in some future. Savings are real. > > > > We will get partial updates in the renderer too. We can't go quite as far > but we do get most of it. > > 1) For one-copy we get the same wins in the renderer as the browser since > it's the staging buffers. (~10x win) ~10x in what situation? there will be 3 options in the future and it's not clear which one is going to be best. 1. partial updates and with texture uploads (non-native GMBs) 2. no partial updates without texture uploads (native GMBs) 3. partial updates without texture uploads (native GMBs that support persistent map usage) 3 is likely the best if supported unless supporting persistent mapping makes the implementation harder to optimize.
On Wed, May 20, 2015 at 12:43 PM, <reveman@chromium.org> wrote: > On 2015/05/20 at 19:05:45, danakj wrote: > >> On Wed, May 20, 2015 at 12:00 PM, Antoine Labour <piman@chromium.org> >> wrote: >> > > > >> > >> > On Wed, May 20, 2015 at 11:58 AM, <reveman@chromium.org> wrote: >> > >> >> >> >> >> >> >> > > https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... > >> >> File cc/trees/layer_tree_settings.h (right): >> >> >> >> >> >> >> > > https://codereview.chromium.org/1139903005/diff/120001/cc/trees/layer_tree_se... > >> >> cc/trees/layer_tree_settings.h:78: unsigned >> >> persistent_map_image_texture_target; >> >> On 2015/05/20 17:55:11, danakj wrote: >> >> >> >>> On 2015/05/20 14:26:07, reveman wrote: >> >>> > I don't think this mechanism where we have a different setting field >> >>> >> >> for each >> >> >> >>> > configuration works long term. A setting field for each combination >> >>> >> >> of format >> >> >> >>> > and usage would explode. Options: >> >>> > >> >>> > 1. unsigned >> >>> >> >> use_image_texture_target[gfx::GpuMemoryBuffer::FORMAT_LAST + >> >> >> >>> > 1][gfx::GpuMemoryBuffer::USAGE_LAST + 1]; >> >>> > >> >>> > 2. unsigned use_image_texture_target; and the embedder needs to >> >>> >> >> figure out >> >> >> >>> what >> >>> > target makes sense based on its usage of the compositor. e.g. >> >>> use_one_copy=true >> >>> > + use_partial_updates=true means GetImageTextureTarget(BGRA_8888, >> >>> > PERSISTENT_MAP). >> >>> >> >> >> >> OK I'm going to go with option 2 here. This will mean you cannot turn >> >>> >> >> on native >> >> >> >>> buffers without breaking partial tile updates, which I was trying to >> >>> >> >> avoid for >> >> >> >>> you. But this seems contentious so I'll just leave us with 1 var for >> >>> >> >> now >> >> >> >>> instead. >> >>> >> >> >> >> Sounds good. We're mostly interested in using native buffers for the >> >> renderer compositor, which I'm expecting not to be using partial >> >> updates. >> > >> > >> > I would like it too, though, in some future. Savings are real. >> > >> > > We will get partial updates in the renderer too. We can't go quite as far >> but we do get most of it. >> > > 1) For one-copy we get the same wins in the renderer as the browser since >> it's the staging buffers. (~10x win) >> > > ~10x in what situation? there will be 3 options in the future and it's not > clear > which one is going to be best. > > 1. partial updates and with texture uploads (non-native GMBs) > 2. no partial updates without texture uploads (native GMBs) > 3. partial updates without texture uploads (native GMBs that support > persistent > map usage) > > 3 is likely the best if supported unless supporting persistent mapping > makes the > implementation harder to optimize. > I'm talking only about the raster time with those numbers. I haven't looked at partial uploads at all yet outside of some measurements on that bug from a while ago. We get 10x when we can reuse the previous buffer completely and just raster the new area. We get 3x when we can copy the old buffer's contents into a new buffer and just raster the new area. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
piman can you OWNERS review please? https://codereview.chromium.org/1139903005/diff/260001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/1139903005/diff/260001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.h:80: static bool IsGpuMemoryBufferConfigurationSupportedInternal( On 2015/05/20 19:25:59, reveman wrote: > nit: IsGpuMemoryBufferFactoryConfigurationSupported and move to anonymous > namespace if possible Done. https://codereview.chromium.org/1139903005/diff/260001/mandoline/ui/aura/surf... File mandoline/ui/aura/surface_context_factory.cc (right): https://codereview.chromium.org/1139903005/diff/260001/mandoline/ui/aura/surf... mandoline/ui/aura/surface_context_factory.cc:74: uint32 SurfaceContextFactory::GetPersistentMapImageTextureTarget() { On 2015/05/20 19:25:59, reveman wrote: > ditto ditto :) https://codereview.chromium.org/1139903005/diff/260001/mandoline/ui/aura/surf... File mandoline/ui/aura/surface_context_factory.h (right): https://codereview.chromium.org/1139903005/diff/260001/mandoline/ui/aura/surf... mandoline/ui/aura/surface_context_factory.h:34: uint32 GetPersistentMapImageTextureTarget() override; On 2015/05/20 19:25:59, reveman wrote: > I assume this is a leftover from previous patch and should not be here anymore. Oops ya
https://codereview.chromium.org/1139903005/diff/290001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/1139903005/diff/290001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.h:83: gfx::GpuMemoryBufferType type); unused now, please remove it before you land the patch
lgtm
https://codereview.chromium.org/1139903005/diff/290001/content/browser/gpu/br... File content/browser/gpu/browser_gpu_channel_host_factory.h (right): https://codereview.chromium.org/1139903005/diff/290001/content/browser/gpu/br... content/browser/gpu/browser_gpu_channel_host_factory.h:83: gfx::GpuMemoryBufferType type); On 2015/05/20 21:32:53, reveman wrote: > unused now, please remove it before you land the patch Done.
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org, alexst@chromium.org, reveman@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1139903005/#ps310001 (title: "persistentmap: .")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139903005/310001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org, reveman@chromium.org, sky@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1139903005/#ps330001 (title: "persistentmap: unmap")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139903005/330001
Message was sent while issue was closed.
Committed patchset #11 (id:330001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/77180b4434d93f45022b8c65110344cdced4b19d Cr-Commit-Position: refs/heads/master@{#330987}
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:330001) has been created in https://codereview.chromium.org/1151943003/ by chirantan@chromium.org. The reason for reverting is: The is causing the freon chrome on chrome os build to fail. Looks like a case for PERSISTENT_MAP also needs to be added to ui::GbmSurfaceFactory::CanCreateNativePixmap() in ui/ozone/platform/drm/gbm_surface_factory.cc. Here is a link to a failing build: https://uberchromegw.corp.google.com/i/chromeos/builders/amd64-generic_freon%....
The CQ bit was checked by danakj@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alexst@chromium.org, reveman@chromium.org, sky@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1139903005/#ps350001 (title: "persistentmap: rebase")
Added the gbm fix and rebased. CQing
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1139903005/350001
Message was sent while issue was closed.
Committed patchset #12 (id:350001)
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/9966900efc1025704645b673fdd63dbf1f2ad189 Cr-Commit-Position: refs/heads/master@{#331416} |
