Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2895273002:
Unified the use of BufferToTextureTargetMap for all the Processes (Closed)
DescriptionUnified the use of BufferToTextureTargetMap for all the Processes
Currently BufferToTextureTargetMap is generated seperately for
ui/renderer process. Now added forbrowser process as well and
unified the use of it.
BUG=725082
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
Review-Url: https://codereview.chromium.org/2895273002
Cr-Commit-Position: refs/heads/master@{#484833}
Committed: https://chromium.googlesource.com/chromium/src/+/c4b9cf2a67c8be392a878373d356a43ac4b3fe24
Patch Set 1 #Patch Set 2 : BufferToTextureTargetMap for Browser Process #
Total comments: 2
Patch Set 3 : Adding BufferToTextureTargetMap for browser process #
Total comments: 1
Patch Set 4 : nit taken care #
Total comments: 5
Patch Set 5 : rebased #Patch Set 6 : Unified BufferToTextureTargetMap #Patch Set 7 : Unified BufferToTextureTargetMap for all the Processes #
Total comments: 8
Patch Set 8 : Unified the use of BufferToTextureTargetMap for all the Processes #Patch Set 9 : Unified the use of BufferToTextureTargetMap for all the Processes #Patch Set 10 : texture_targets as reference #
Total comments: 7
Patch Set 11 : Unified the use of BufferToTextureTargetMap for all the Processes #
Total comments: 14
Patch Set 12 : Unified the use of BufferToTextureTargetMap for all the Processes #Patch Set 13 : c++ styling done #
Total comments: 4
Patch Set 14 : nit taken care #
Total comments: 2
Patch Set 15 : nit taken care #Messages
Total messages: 57 (16 generated)
Description was changed from ========== Create GLTextureCreate of type requested Create the GLTexture for the resource type requested in CreateGpuMemoryBufferResource. BUG=725082 Signed-off-by: sujiths.s <sujiths.s@samsung.com> ========== to ========== Create GLTextureCreate of type requested Create the GLTexture for the resource type requested in CreateGpuMemoryBufferResource. BUG=725082 Signed-off-by: sujiths.s <sujiths.s@samsung.com> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
sujiths.s@samsung.com changed reviewers: + ccameron@chromium.org, ericrk@chromium.org, reveman@chromium.org
Hi All, PTAL
Please explain why this is correct behavior. Function name CreateGpuMemoryBufferResource so it's not clear why this is correct. Looking at the bug it seems like we should instead fix so we use the correct texture target in the browser process.
On 2017/05/22 15:46:24, reveman wrote: > Please explain why this is correct behavior. Function name > CreateGpuMemoryBufferResource so it's not clear why this is correct. Looking at > the bug it seems like we should instead fix so we use the correct texture target > in the browser process. Based on the patch https://codereview.chromium.org/2120713002, only Renderer Process will assign the values to buffer_to_texture_target_map_. So for Browser Process buffer_to_texture_target_map_ will be null. Regarding the current change, since the default_resource_type is decided based on use_gpu_memory_buffer_resources in ResourceProvider constructor default_resource_type(use_gpu_memory_buffer_resources ? RESOURCE_TYPE_GPU_MEMORY_BUFFER : RESOURCE_TYPE_GL_TEXTURE) So in the current scenario use_gpu_memory_buffer_resources is false, and the default_resource_type is RESOURCE_TYPE_GL_TEXTURE, So i thought we should CreateGLTexture with RESOURCE_TYPE_GL_TEXTURE instead of RESOURCE_TYPE_GPU_MEMORY_BUFFER. Please share your opinion on this.
sujiths.s@samsung.com changed reviewers: + kinuko@chromium.org, tedchoc@chromium.org
@reveman, as you suggested i have made changes to add BufferToTextureTargetMap for browser process as well. Hi All, PTAL
Description was changed from ========== Create GLTextureCreate of type requested Create the GLTexture for the resource type requested in CreateGpuMemoryBufferResource. BUG=725082 Signed-off-by: sujiths.s <sujiths.s@samsung.com> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Currently BufferToTextureTargetMap is accessible only for RendererProcess. Browser Process its always null. So adding BufferToTextureTargetMap for Browser process as well. BUG=725082 Signed-off-by: sujiths.s <sujiths.s@samsung.com> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
tedchoc@chromium.org changed reviewers: + boliu@chromium.org
+boliu
https://codereview.chromium.org/2895273002/diff/20001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2895273002/diff/20001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:532: settings.renderer_settings.buffer_to_texture_target_map = why do we need a command line switch for this when we're already in the browser process?
sujiths.s@samsung.com changed reviewers: + shanmuga.m@samsung.com
Hi All, PTAL https://codereview.chromium.org/2895273002/diff/20001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2895273002/diff/20001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:532: settings.renderer_settings.buffer_to_texture_target_map = On 2017/05/31 15:06:09, reveman wrote: > why do we need a command line switch for this when we're already in the browser > process? I tried to follow the same way how the values are getting at render process. As you suggest i will modify it, i will populate it in compositorImplAndroid
lgtm with nit https://codereview.chromium.org/2895273002/diff/40001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2895273002/diff/40001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:544: settings.buffer_to_texture_target_map = image_targets; nit: avoid making a copy of the map by setting it directly in the loop
I'm not an expert in this area, but given that reveman's stamped lgtm (if no one disagrees)
Hi ALL, PTAL
https://codereview.chromium.org/2895273002/diff/60001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2895273002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:533: for (int usage_idx = 0; usage_idx <= static_cast<int>(gfx::BufferUsage::LAST); I think this code to generate a map from gpu::GetImageTextureTarget is now repeated 3 times: * ui/compositor * render_process_host_impl where it's then turned into a string for the renderer side * here can we unify these? https://codereview.chromium.org/2895273002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:541: settings.buffer_to_texture_target_map[std::make_pair(usage, format)] = you need a rebase I think, this setting moved into a separate ResourceSettings recently
Description was changed from ========== Currently BufferToTextureTargetMap is accessible only for RendererProcess. Browser Process its always null. So adding BufferToTextureTargetMap for Browser process as well. BUG=725082 Signed-off-by: sujiths.s <sujiths.s@samsung.com> CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Currently BufferToTextureTargetMap is accessible only for RendererProcess. Browser Process its always null. So adding BufferToTextureTargetMap for Browser process as well. BUG=725082 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Hi All, PTAL https://codereview.chromium.org/2895273002/diff/60001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2895273002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:533: for (int usage_idx = 0; usage_idx <= static_cast<int>(gfx::BufferUsage::LAST); On 2017/06/07 23:14:48, boliu wrote: > I think this code to generate a map from gpu::GetImageTextureTarget is now > repeated 3 times: > * ui/compositor > * render_process_host_impl where it's then turned into a string for the renderer > side > * here > > can we unify these? yes currently its been generated in 3 different places. All the place we are doing the same thing. Previously it was generated at LayerTreeSettings, later it got changed https://codereview.chromium.org/2120713002 So what you suggest, generate it at LayerTreeSettings, so that it will be available to all processes? https://codereview.chromium.org/2895273002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:541: settings.buffer_to_texture_target_map[std::make_pair(usage, format)] = On 2017/06/07 23:14:48, boliu wrote: > you need a rebase I think, this setting moved into a separate ResourceSettings > recently i have rebased the current patch based on the latest changes.
https://codereview.chromium.org/2895273002/diff/60001/content/browser/rendere... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2895273002/diff/60001/content/browser/rendere... content/browser/renderer_host/compositor_impl_android.cc:533: for (int usage_idx = 0; usage_idx <= static_cast<int>(gfx::BufferUsage::LAST); On 2017/06/08 10:40:30, sujith wrote: > On 2017/06/07 23:14:48, boliu wrote: > > I think this code to generate a map from gpu::GetImageTextureTarget is now > > repeated 3 times: > > * ui/compositor > > * render_process_host_impl where it's then turned into a string for the > renderer > > side > > * here > > > > can we unify these? > > yes currently its been generated in 3 different places. > All the place we are doing the same thing. > Previously it was generated at LayerTreeSettings, later it got > changed https://codereview.chromium.org/2120713002 > > So what you suggest, generate it at LayerTreeSettings, > so that it will be available to all processes? Add a method to content/browser/gpu/compositor_util.h: cc::BufferToTextureTargetMap GetBufferToTextureTargetMap() ui::CreateRendererSettings can be modified to take a ref of the map directly
Description was changed from ========== Currently BufferToTextureTargetMap is accessible only for RendererProcess. Browser Process its always null. So adding BufferToTextureTargetMap for Browser process as well. BUG=725082 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Currently BufferToTextureTargetMap is generated seperately for ui/renderer process. Now added for browser process as well and unified the use of it. BUG=725082 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Changes done as suggested. PTAL
https://codereview.chromium.org/2895273002/diff/120001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/120001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:51: CONTENT_EXPORT cc::BufferToTextureTargetMap GetBufferToTextureTargetMap(); Can this be "void GetBufferToTextureTargetMap(cc::BufferToTextureTargetMap* texture_targets)" so caller is not forced to make a copy of a map? Also, would be nice to rename settings.resource_settings.buffer_to_texture_target_map image_targets and change the name of this a related functions to GetImageTargets. A better name for the map type is BufferConfigurationTextureTargetMap imo. Fine to leave these last two things to a follow up patch if preferred. https://codereview.chromium.org/2895273002/diff/120001/ui/compositor/composit... File ui/compositor/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/120001/ui/compositor/composit... ui/compositor/compositor_util.h:21: cc::BufferToTextureTargetMap image_targets); "const cc::BufferToTextureTargetMap& image_targets" to avoid a copy
https://codereview.chromium.org/2895273002/diff/120001/ui/aura/mus/mus_contex... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2895273002/diff/120001/ui/aura/mus/mus_contex... ui/aura/mus/mus_context_factory.cc:24: content::GetBufferToTextureTargetMap()), ui can't depend on content. does this even compile?
Hi All, please check my comments and update your views. https://codereview.chromium.org/2895273002/diff/120001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/120001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:51: CONTENT_EXPORT cc::BufferToTextureTargetMap GetBufferToTextureTargetMap(); On 2017/06/09 14:28:46, reveman wrote: > Can this be "void GetBufferToTextureTargetMap(cc::BufferToTextureTargetMap* > texture_targets)" so caller is not forced to make a copy of a map? > > Also, would be nice to rename > settings.resource_settings.buffer_to_texture_target_map image_targets and change > the name of this a related functions to GetImageTargets. A better name for the > map type is BufferConfigurationTextureTargetMap imo. Fine to leave these last > two things to a follow up patch if preferred. when i was trying the modifications for GetBufferToTextureTargetMap(cc::BufferToTextureTargetMap* texture_targets) i think we need to create seperate variables each time to access it. For example GpuProcessTransportFactory::GpuProcessTransportFactory() : frame_sink_id_allocator_(kDefaultClientId), renderer_settings_( ui::CreateRendererSettings(GetBufferToTextureTargetMap())), render_settings_ get initialized in constructor, so if we move it inside, need to overload operator= so could you please suggest what approach should i follow. Regarding the name change, as you suggest i will do a follow up patch. https://codereview.chromium.org/2895273002/diff/120001/ui/aura/mus/mus_contex... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2895273002/diff/120001/ui/aura/mus/mus_contex... ui/aura/mus/mus_context_factory.cc:24: content::GetBufferToTextureTargetMap()), On 2017/06/12 16:31:39, boliu wrote: > ui can't depend on content. does this even compile? i was building for android, so it doesn't throw any error at that time. So in this case we need to duplicate the GetBufferToTextureTargetMap() in ui side right? without that i guess we cannot access it. https://codereview.chromium.org/2895273002/diff/120001/ui/compositor/composit... File ui/compositor/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/120001/ui/compositor/composit... ui/compositor/compositor_util.h:21: cc::BufferToTextureTargetMap image_targets); On 2017/06/09 14:28:46, reveman wrote: > "const cc::BufferToTextureTargetMap& image_targets" to avoid a copy Done.
https://codereview.chromium.org/2895273002/diff/120001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/120001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:51: CONTENT_EXPORT cc::BufferToTextureTargetMap GetBufferToTextureTargetMap(); On 2017/06/14 at 10:30:18, sujith wrote: > On 2017/06/09 14:28:46, reveman wrote: > > Can this be "void GetBufferToTextureTargetMap(cc::BufferToTextureTargetMap* > > texture_targets)" so caller is not forced to make a copy of a map? > > > > Also, would be nice to rename > > settings.resource_settings.buffer_to_texture_target_map image_targets and change > > the name of this a related functions to GetImageTargets. A better name for the > > map type is BufferConfigurationTextureTargetMap imo. Fine to leave these last > > two things to a follow up patch if preferred. > > > when i was trying the modifications for > GetBufferToTextureTargetMap(cc::BufferToTextureTargetMap* texture_targets) > i think we need to create seperate variables each time to access it. > For example > > GpuProcessTransportFactory::GpuProcessTransportFactory() > : frame_sink_id_allocator_(kDefaultClientId), > renderer_settings_( > ui::CreateRendererSettings(GetBufferToTextureTargetMap())), > > render_settings_ get initialized in constructor, so if we move it inside, > need to overload operator= > so could you please suggest what approach should i follow. You can remove it from the initialization list or just create a local wrapper around GetBufferToTextureTargetMap for this specific use-case. > > Regarding the name change, as you suggest i will do a follow up patch.
Hi All, Changes done as suggested. PTAL
https://codereview.chromium.org/2895273002/diff/180001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/180001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:11: #include "cc/output/buffer_to_texture_target_map.h" you can forward declare the map here, and include this header in .cc file https://codereview.chromium.org/2895273002/diff/180001/ui/compositor/composit... File ui/compositor/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/180001/ui/compositor/composit... ui/compositor/compositor_util.h:10: #include "cc/output/buffer_to_texture_target_map.h" ditto about forward declaration, after fixing comment below https://codereview.chromium.org/2895273002/diff/180001/ui/compositor/composit... ui/compositor/compositor_util.h:21: const cc::BufferToTextureTargetMap image_targets); const &
https://codereview.chromium.org/2895273002/diff/180001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/180001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:11: #include "cc/output/buffer_to_texture_target_map.h" On 2017/06/20 22:44:33, boliu wrote: > you can forward declare the map here, and include this header in .cc file are you suggesting to do a declaration like this namespace cc { using BufferToTextureTargetKey = std::pair<gfx::BufferUsage, gfx::BufferFormat>; using BufferToTextureTargetMap = std::map<BufferToTextureTargetKey, uint32_t>; }
https://codereview.chromium.org/2895273002/diff/180001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/180001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:11: #include "cc/output/buffer_to_texture_target_map.h" On 2017/06/21 13:04:59, sujith wrote: > On 2017/06/20 22:44:33, boliu wrote: > > you can forward declare the map here, and include this header in .cc file > > are you suggesting to do a declaration like this > > namespace cc { > using BufferToTextureTargetKey = std::pair<gfx::BufferUsage, gfx::BufferFormat>; > using BufferToTextureTargetMap = std::map<BufferToTextureTargetKey, uint32_t>; > } I meant this: namespace cc { class BufferToTextureTargetMap; } If that doesn't work, then include here is fine.
Hi All, PTAL https://codereview.chromium.org/2895273002/diff/180001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/180001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:11: #include "cc/output/buffer_to_texture_target_map.h" On 2017/06/21 16:01:25, boliu wrote: > On 2017/06/21 13:04:59, sujith wrote: > > On 2017/06/20 22:44:33, boliu wrote: > > > you can forward declare the map here, and include this header in .cc file > > > > are you suggesting to do a declaration like this > > > > namespace cc { > > using BufferToTextureTargetKey = std::pair<gfx::BufferUsage, > gfx::BufferFormat>; > > using BufferToTextureTargetMap = std::map<BufferToTextureTargetKey, uint32_t>; > > } > > I meant this: > > namespace cc { > class BufferToTextureTargetMap; > } > > If that doesn't work, then include here is fine. its not working since BufferToTextureTargetMap is not a class, its just a map object. https://codereview.chromium.org/2895273002/diff/180001/ui/compositor/composit... File ui/compositor/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/180001/ui/compositor/composit... ui/compositor/compositor_util.h:21: const cc::BufferToTextureTargetMap image_targets); On 2017/06/20 22:44:33, boliu wrote: > const & Done.
lgtm
sujiths.s@samsung.com changed reviewers: + danakj@chromium.org, sadrul@chromium.org
@sadrul, PTAL from aura side @danakj, PTAL from ui/compositor side.
@sadrul, @danakj gentle ping. PTAL
Seems good but some C++ style stuff, we can make use of map being a movable type better. https://codereview.chromium.org/2895273002/diff/200001/content/browser/compos... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2895273002/diff/200001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:204: GpuProcessTransportFactory::GetBufferToTextureTargetMap() { Remove this function. https://codereview.chromium.org/2895273002/diff/200001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:213: ui::CreateRendererSettings(GetBufferToTextureTargetMap())), you can call the content:: function directly here when it returns the map. https://codereview.chromium.org/2895273002/diff/200001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/200001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:52: // Populate buffer_to_texture_target_map for all buffer usage/formats. This comment refers to a name that isn't present in the code. https://codereview.chromium.org/2895273002/diff/200001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:53: CONTENT_EXPORT void GetBufferToTextureTargetMap( Return the map, instead of taking a pointer please, so we can use RVO. https://codereview.chromium.org/2895273002/diff/200001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2895273002/diff/200001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:542: &settings.resource_settings.buffer_to_texture_target_map); assign to this when the map is returned instead https://codereview.chromium.org/2895273002/diff/200001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2895273002/diff/200001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2025: GetBufferToTextureTargetMap(&image_targets); either get rid of the |image_targets| and call GetBufferToTextureTargetMap() on L2028 directly, or assign its result to |image_targets| here and std::move(image_targets) on L2028. https://codereview.chromium.org/2895273002/diff/200001/ui/aura/mus/mus_contex... File ui/aura/mus/mus_context_factory.cc (left): https://codereview.chromium.org/2895273002/diff/200001/ui/aura/mus/mus_contex... ui/aura/mus/mus_context_factory.cc:25: // TODO(sad): http://crbug.com/675431 Keep this TODO where we set them to GL_TEXTURE_2D
@danakj https://codereview.chromium.org/2895273002/diff/200001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/200001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:52: // Populate buffer_to_texture_target_map for all buffer usage/formats. On 2017/06/28 17:00:19, danakj wrote: > This comment refers to a name that isn't present in the code. Done. https://codereview.chromium.org/2895273002/diff/200001/ui/aura/mus/mus_contex... File ui/aura/mus/mus_context_factory.cc (left): https://codereview.chromium.org/2895273002/diff/200001/ui/aura/mus/mus_contex... ui/aura/mus/mus_context_factory.cc:25: // TODO(sad): http://crbug.com/675431 On 2017/06/28 17:00:19, danakj wrote: > Keep this TODO where we set them to GL_TEXTURE_2D Done.
@danakj, I have done similar changes in patch set 7. based on review comment i have modified the patch. Could you please have a look at patch set 7. Please suggest which way we should proceed.
https://codereview.chromium.org/2895273002/diff/120001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/120001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:51: CONTENT_EXPORT cc::BufferToTextureTargetMap GetBufferToTextureTargetMap(); On 2017/06/09 14:28:46, reveman wrote: > Can this be "void GetBufferToTextureTargetMap(cc::BufferToTextureTargetMap* > texture_targets)" so caller is not forced to make a copy of a map? It's a move not a copy, and it would make use of RVO. Returning is the correct way to do this in C++11. > Also, would be nice to rename > settings.resource_settings.buffer_to_texture_target_map image_targets and change > the name of this a related functions to GetImageTargets. A better name for the > map type is BufferConfigurationTextureTargetMap imo. Fine to leave these last > two things to a follow up patch if preferred.
On Thu, Jun 29, 2017 at 2:22 AM, <sujiths.s@samsung.com> wrote: > @danakj, > I have done similar changes in patch set 7. > based on review comment i have modified the patch. Could you please have a > look > at patch set 7. > > Please suggest which way we should proceed. > I stand by my previous comments. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi All, PTAL https://codereview.chromium.org/2895273002/diff/200001/content/browser/compos... File content/browser/compositor/gpu_process_transport_factory.cc (right): https://codereview.chromium.org/2895273002/diff/200001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:204: GpuProcessTransportFactory::GetBufferToTextureTargetMap() { On 2017/06/28 17:00:19, danakj wrote: > Remove this function. Done. https://codereview.chromium.org/2895273002/diff/200001/content/browser/compos... content/browser/compositor/gpu_process_transport_factory.cc:213: ui::CreateRendererSettings(GetBufferToTextureTargetMap())), On 2017/06/28 17:00:19, danakj wrote: > you can call the content:: function directly here when it returns the map. Done. https://codereview.chromium.org/2895273002/diff/200001/content/browser/gpu/co... File content/browser/gpu/compositor_util.h (right): https://codereview.chromium.org/2895273002/diff/200001/content/browser/gpu/co... content/browser/gpu/compositor_util.h:53: CONTENT_EXPORT void GetBufferToTextureTargetMap( On 2017/06/28 17:00:19, danakj wrote: > Return the map, instead of taking a pointer please, so we can use RVO. Done. https://codereview.chromium.org/2895273002/diff/200001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2895273002/diff/200001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:542: &settings.resource_settings.buffer_to_texture_target_map); On 2017/06/28 17:00:19, danakj wrote: > assign to this when the map is returned instead Done. https://codereview.chromium.org/2895273002/diff/200001/content/browser/render... File content/browser/renderer_host/render_process_host_impl.cc (right): https://codereview.chromium.org/2895273002/diff/200001/content/browser/render... content/browser/renderer_host/render_process_host_impl.cc:2025: GetBufferToTextureTargetMap(&image_targets); On 2017/06/28 17:00:19, danakj wrote: > either get rid of the |image_targets| and call GetBufferToTextureTargetMap() on > L2028 directly, or assign its result to |image_targets| here and > std::move(image_targets) on L2028. Done.
LGTM https://codereview.chromium.org/2895273002/diff/240001/content/browser/gpu/co... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/2895273002/diff/240001/content/browser/gpu/co... content/browser/gpu/compositor_util.cc:401: cc::BufferToTextureTargetMap GetBufferToTextureTargetMap() { last nit: Create rather than Get as this isn't just getting an existing thing. https://codereview.chromium.org/2895273002/diff/240001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2895273002/diff/240001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:541: settings.resource_settings.buffer_to_texture_target_map = group this with the other settings, on L359?
Hi All, PTAL https://codereview.chromium.org/2895273002/diff/240001/content/browser/gpu/co... File content/browser/gpu/compositor_util.cc (right): https://codereview.chromium.org/2895273002/diff/240001/content/browser/gpu/co... content/browser/gpu/compositor_util.cc:401: cc::BufferToTextureTargetMap GetBufferToTextureTargetMap() { On 2017/06/30 16:03:23, danakj wrote: > last nit: Create rather than Get as this isn't just getting an existing thing. Done. https://codereview.chromium.org/2895273002/diff/240001/content/browser/render... File content/browser/renderer_host/compositor_impl_android.cc (right): https://codereview.chromium.org/2895273002/diff/240001/content/browser/render... content/browser/renderer_host/compositor_impl_android.cc:541: settings.resource_settings.buffer_to_texture_target_map = On 2017/06/30 16:03:23, danakj wrote: > group this with the other settings, on L359? Hope its L539. Done
On 2017/07/01 12:30:13, sujith wrote: > Hi All, > > PTAL > > https://codereview.chromium.org/2895273002/diff/240001/content/browser/gpu/co... > File content/browser/gpu/compositor_util.cc (right): > > https://codereview.chromium.org/2895273002/diff/240001/content/browser/gpu/co... > content/browser/gpu/compositor_util.cc:401: cc::BufferToTextureTargetMap > GetBufferToTextureTargetMap() { > On 2017/06/30 16:03:23, danakj wrote: > > last nit: Create rather than Get as this isn't just getting an existing thing. > > Done. > > https://codereview.chromium.org/2895273002/diff/240001/content/browser/render... > File content/browser/renderer_host/compositor_impl_android.cc (right): > > https://codereview.chromium.org/2895273002/diff/240001/content/browser/render... > content/browser/renderer_host/compositor_impl_android.cc:541: > settings.resource_settings.buffer_to_texture_target_map = > On 2017/06/30 16:03:23, danakj wrote: > > group this with the other settings, on L359? > > Hope its L539. Done Yes, thanks :) LGTM
@sadrul, PTAL from ui/aura side
lgtm https://codereview.chromium.org/2895273002/diff/260001/ui/aura/mus/mus_contex... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2895273002/diff/260001/ui/aura/mus/mus_contex... ui/aura/mus/mus_context_factory.cc:21: cc::BufferToTextureTargetMap CreateBufferToTextureTargetMap() { Put in anon namespace.
side note: consider updating the CL description to follow https://chris.beams.io/posts/git-commit/#separate. (and also note that the CL subject does not get included in the commit message)
Description was changed from ========== Currently BufferToTextureTargetMap is generated seperately for ui/renderer process. Now added for browser process as well and unified the use of it. BUG=725082 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Unified the use of BufferToTextureTargetMap for all the Processes Currently BufferToTextureTargetMap is generated seperately for ui/renderer process. Now added for browser process as well and unified the use of it. BUG=725082 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
Description was changed from ========== Unified the use of BufferToTextureTargetMap for all the Processes Currently BufferToTextureTargetMap is generated seperately for ui/renderer process. Now added for browser process as well and unified the use of it. BUG=725082 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Unified the use of BufferToTextureTargetMap for all the Processes Currently BufferToTextureTargetMap is generated seperately for ui/renderer process. Now added forbrowser process as well and unified the use of it. BUG=725082 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ==========
changes done https://codereview.chromium.org/2895273002/diff/260001/ui/aura/mus/mus_contex... File ui/aura/mus/mus_context_factory.cc (right): https://codereview.chromium.org/2895273002/diff/260001/ui/aura/mus/mus_contex... ui/aura/mus/mus_context_factory.cc:21: cc::BufferToTextureTargetMap CreateBufferToTextureTargetMap() { On 2017/07/05 15:21:36, sadrul wrote: > Put in anon namespace. Done.
The CQ bit was checked by sujiths.s@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org, reveman@chromium.org, boliu@chromium.org, danakj@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/2895273002/#ps280001 (title: "nit taken care")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1499398043115440,
"parent_rev": "daf95c19c43619e2b49d807e2b31e771512f2a4b", "commit_rev":
"8b779c37873fe980ce6acaa0672bbd86fc6e40a4"}
CQ is committing da patch.
Bot data: {"patchset_id": 280001, "attempt_start_ts": 1499398043115440,
"parent_rev": "a58703a885159856b5bce37a4c7798a7c481ae39", "commit_rev":
"c4b9cf2a67c8be392a878373d356a43ac4b3fe24"}
Message was sent while issue was closed.
Description was changed from ========== Unified the use of BufferToTextureTargetMap for all the Processes Currently BufferToTextureTargetMap is generated seperately for ui/renderer process. Now added forbrowser process as well and unified the use of it. BUG=725082 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel ========== to ========== Unified the use of BufferToTextureTargetMap for all the Processes Currently BufferToTextureTargetMap is generated seperately for ui/renderer process. Now added forbrowser process as well and unified the use of it. BUG=725082 CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel Review-Url: https://codereview.chromium.org/2895273002 Cr-Commit-Position: refs/heads/master@{#484833} Committed: https://chromium.googlesource.com/chromium/src/+/c4b9cf2a67c8be392a878373d356... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:280001) as https://chromium.googlesource.com/chromium/src/+/c4b9cf2a67c8be392a878373d356... |
