Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1)

Issue 14409006: cc: Changes to use GL API for GpuMemoryBuffers (Closed)

Created:
7 years, 8 months ago by kaanb
Modified:
7 years, 3 months ago
CC:
chromium-reviews, cc-bugs_chromium.org, klobag.chromium, jamesr, joth
Base URL:
https://chromium.googlesource.com/chromium/src.git@glapi
Visibility:
Public.

Description

cc: Changes to use GL API for GpuMemoryBuffers Corresponding gpu changes are here: https://codereview.chromium.org/14456004/ BUG=175012 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201718

Patch Set 1 #

Total comments: 6

Patch Set 2 : Incorporate code reviews #

Patch Set 3 : Rebase #

Patch Set 4 : Add unit tests and use the new GL API #

Patch Set 5 : Rebase #

Patch Set 6 : Fix a comment #

Patch Set 7 : Fix a bug in ReleasePixelBuffer and make the unit test tighter #

Patch Set 8 : Prefer resource->image_id == 0 to !use_gpu_memory_buffers_ #

Patch Set 9 : s/GL_WRITE_ONLY/GL_READ_WRITE/ for map mode #

Total comments: 24

Patch Set 10 : Incorporate code reviews #

Patch Set 11 : Cosmetic fixes #

Total comments: 37

Patch Set 12 : Incorporate code reviews #

Patch Set 13 : Incorporate code reviews #

Total comments: 4

Patch Set 14 : Incorporate code reviews #

Total comments: 4

Patch Set 15 : s/use_gpu_memory_buffers/use_map_image/ #

Patch Set 16 : Remove TODOs #

Total comments: 6

Patch Set 17 : s/use_gpu_memory_buffers/use_map_image/ for TileManager #

Patch Set 18 : Move settings check to GLRenderer::Initialize #

Total comments: 2

Patch Set 19 : s/kCCUseMapImage/kUseMapImage/ #

Patch Set 20 : Rebase #

Patch Set 21 : Alphabetize switch names in lists #

Total comments: 2

Patch Set 22 : Create TileManager after the renderer within LTHI #

Patch Set 23 : Add internalformat parameter to CreateImageCHROMIUM in fake_web_graphics_context_3d to fix clang bu… #

Unified diffs Side-by-side diffs Delta from patch set Stats (+325 lines, -35 lines) Patch
M cc/base/switches.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M cc/base/switches.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +3 lines, -0 lines 0 comments Download
M cc/debug/fake_web_graphics_context_3d.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +18 lines, -0 lines 0 comments Download
M cc/debug/fake_web_graphics_context_3d.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 1 chunk +11 lines, -0 lines 0 comments Download
M cc/output/delegating_renderer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +18 lines, -12 lines 0 comments Download
M cc/output/gl_renderer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +4 lines, -0 lines 0 comments Download
M cc/resources/resource_provider.h View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +17 lines, -0 lines 0 comments Download
M cc/resources/resource_provider.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 13 chunks +132 lines, -3 lines 0 comments Download
M cc/resources/resource_provider_unittest.cc View 1 2 3 4 5 6 7 8 9 3 chunks +86 lines, -1 line 0 comments Download
M cc/resources/tile_manager.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 2 chunks +4 lines, -3 lines 0 comments Download
M cc/resources/tile_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 3 chunks +6 lines, -3 lines 0 comments Download
M cc/test/fake_picture_layer_tiling_client.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -1 line 0 comments Download
M cc/test/fake_tile_manager.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -1 line 0 comments Download
M cc/trees/layer_tree_host.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/layer_tree_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -1 line 0 comments Download
M cc/trees/layer_tree_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 2 chunks +11 lines, -9 lines 0 comments Download
M cc/trees/layer_tree_settings.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +1 line, -0 lines 0 comments Download
M cc/trees/layer_tree_settings.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/chromeos/login/chrome_restart_request.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/renderer_host/render_process_host_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -0 lines 0 comments Download
M content/renderer/gpu/render_widget_compositor.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 44 (0 generated)
kaanb1
7 years, 8 months ago (2013-04-24 21:28:08 UTC) #1
kaanb1
Friendly ping!
7 years, 8 months ago (2013-04-25 21:40:15 UTC) #2
jamesr
This patch has zero tests. Why? https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provider.cc#newcode897 cc/resources/resource_provider.cc:897: static_cast<Context3DImpl*>(context3d); you shouldn't ...
7 years, 8 months ago (2013-04-25 22:28:56 UTC) #3
kaanb1
I added unit tests to the GPU patch: https://codereview.chromium.org/14456004/ Is there something specific you want ...
7 years, 8 months ago (2013-04-25 22:43:29 UTC) #4
jamesr
On 2013/04/25 22:43:29, kaanb1 wrote: > I added unit tests to the GPU patch: > ...
7 years, 8 months ago (2013-04-25 22:47:57 UTC) #5
jamesr
https://codereview.chromium.org/14409006/diff/1/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14409006/diff/1/cc/trees/layer_tree_host_impl.cc#newcode1345 cc/trees/layer_tree_host_impl.cc:1345: if (CommandLine::ForCurrentProcess()->HasSwitch( I don't think this is a good ...
7 years, 8 months ago (2013-04-25 22:50:46 UTC) #6
no sievers
https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provider.cc#newcode897 cc/resources/resource_provider.cc:897: static_cast<Context3DImpl*>(context3d); On 2013/04/25 22:43:29, kaanb1 wrote: > On 2013/04/25 ...
7 years, 8 months ago (2013-04-25 23:28:14 UTC) #7
kaanb1
On 2013/04/25 23:28:14, Daniel Sievers wrote: > https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provider.cc > File cc/resources/resource_provider.cc (right): > > https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provider.cc#newcode897 ...
7 years, 8 months ago (2013-04-25 23:30:00 UTC) #8
kaanb1
I'll add unit tests once I address the comments on the gpu/ patch. https://codereview.chromium.org/14409006/diff/1/cc/resources/resource_provider.cc File ...
7 years, 8 months ago (2013-04-25 23:55:05 UTC) #9
kaanb
I have patched in 14456004 and made modification to ResourceProvider to use the new API ...
7 years, 7 months ago (2013-05-10 01:22:43 UTC) #10
jamesr
Redirecting review for cc/ to reveman. I can stamp for owners on the rest after ...
7 years, 7 months ago (2013-05-10 01:29:08 UTC) #11
reveman
I prefer to wait for gpu side changes to land before I start looking at ...
7 years, 7 months ago (2013-05-10 18:38:51 UTC) #12
kaanb
On 2013/05/10 18:38:51, David Reveman wrote: > I prefer to wait for gpu side changes ...
7 years, 7 months ago (2013-05-16 19:12:52 UTC) #13
reveman
I don't think that masquerading these zero-copy buffers as pixel buffers is the correct approach. ...
7 years, 7 months ago (2013-05-17 01:48:07 UTC) #14
kaanb1
Sure, I can change the API but then we'll need to wait when the fix ...
7 years, 7 months ago (2013-05-17 02:28:55 UTC) #15
joth
On 2013/05/17 02:28:55, kaanb1 wrote: > Sure, I can change the API but then we'll ...
7 years, 7 months ago (2013-05-17 18:13:44 UTC) #16
reveman
On 2013/05/17 18:13:44, joth wrote: > On 2013/05/17 02:28:55, kaanb1 wrote: > > Sure, I ...
7 years, 7 months ago (2013-05-17 20:08:06 UTC) #17
kaanb
https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/14409006/diff/32001/cc/resources/resource_provider.cc#newcode926 cc/resources/resource_provider.cc:926: resource->size.width(), resource->size.height(), GL_RGBA8_OES); On 2013/05/17 01:48:08, David Reveman wrote: ...
7 years, 7 months ago (2013-05-17 21:27:36 UTC) #18
kaanb
Friendly ping!
7 years, 7 months ago (2013-05-20 22:08:28 UTC) #19
reveman
https://codereview.chromium.org/14409006/diff/44013/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/14409006/diff/44013/cc/base/switches.cc#newcode137 cc/base/switches.cc:137: // Use GPU memory buffers within the ResourceProvider. This ...
7 years, 7 months ago (2013-05-20 23:16:01 UTC) #20
kaanb
https://codereview.chromium.org/14409006/diff/44013/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/14409006/diff/44013/cc/base/switches.cc#newcode137 cc/base/switches.cc:137: // Use GPU memory buffers within the ResourceProvider. On ...
7 years, 7 months ago (2013-05-21 00:52:29 UTC) #21
reveman
https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_provider.cc#newcode1271 cc/resources/resource_provider.cc:1271: DCHECK_EQ(0u, resource->image_id); On 2013/05/21 00:52:29, kaanb wrote: > On ...
7 years, 7 months ago (2013-05-21 02:20:19 UTC) #22
kaanb
+piman as reviewer for chrome/browser/chromeos/login https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_provider.cc#newcode1271 cc/resources/resource_provider.cc:1271: DCHECK_EQ(0u, resource->image_id); On 2013/05/21 ...
7 years, 7 months ago (2013-05-21 17:53:48 UTC) #23
piman
LGTM+nits https://codereview.chromium.org/14409006/diff/61001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/14409006/diff/61001/cc/base/switches.cc#newcode138 cc/base/switches.cc:138: const char kUseGpuMemoryBuffers[] = "use-gpu-memory-buffers"; nit: the flag ...
7 years, 7 months ago (2013-05-21 18:15:14 UTC) #24
no sievers
https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_provider.cc#newcode872 cc/resources/resource_provider.cc:872: DCHECK(!source->image_id); On 2013/05/21 00:52:29, kaanb wrote: > On 2013/05/20 ...
7 years, 7 months ago (2013-05-21 19:15:46 UTC) #25
piman
https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_provider.cc#newcode872 cc/resources/resource_provider.cc:872: DCHECK(!source->image_id); On 2013/05/21 19:15:46, Daniel Sievers wrote: > On ...
7 years, 7 months ago (2013-05-21 19:50:39 UTC) #26
kaanb
https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_provider.cc File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/14409006/diff/44013/cc/resources/resource_provider.cc#newcode872 cc/resources/resource_provider.cc:872: DCHECK(!source->image_id); On 2013/05/21 19:50:39, piman wrote: > On 2013/05/21 ...
7 years, 7 months ago (2013-05-21 20:25:41 UTC) #27
piman
lgtm
7 years, 7 months ago (2013-05-21 20:27:43 UTC) #28
reveman
https://codereview.chromium.org/14409006/diff/72002/cc/resources/tile_manager.h File cc/resources/tile_manager.h (right): https://codereview.chromium.org/14409006/diff/72002/cc/resources/tile_manager.h#newcode70 cc/resources/tile_manager.h:70: bool use_gpu_memory_buffers); why not use_map_image here as well? https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_impl.cc ...
7 years, 7 months ago (2013-05-22 18:56:41 UTC) #29
kaanb
https://codereview.chromium.org/14409006/diff/72002/cc/resources/tile_manager.h File cc/resources/tile_manager.h (right): https://codereview.chromium.org/14409006/diff/72002/cc/resources/tile_manager.h#newcode70 cc/resources/tile_manager.h:70: bool use_gpu_memory_buffers); On 2013/05/22 18:56:41, David Reveman wrote: > ...
7 years, 7 months ago (2013-05-22 19:13:03 UTC) #30
reveman
https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_impl.cc#newcode1393 cc/trees/layer_tree_host_impl.cc:1393: settings_.use_map_image)); but shouldn't it be "settings_.use_map_image && capabilities().using_map_image"? I ...
7 years, 7 months ago (2013-05-22 19:51:19 UTC) #31
kaanb
https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14409006/diff/72002/cc/trees/layer_tree_host_impl.cc#newcode1393 cc/trees/layer_tree_host_impl.cc:1393: settings_.use_map_image)); On 2013/05/22 19:51:19, David Reveman wrote: > but ...
7 years, 7 months ago (2013-05-22 20:21:01 UTC) #32
reveman
lgtm, thanks for making that last change!
7 years, 7 months ago (2013-05-22 22:53:41 UTC) #33
jamesr
lgtm once you fix the switch's symbol name https://codereview.chromium.org/14409006/diff/91001/cc/base/switches.h File cc/base/switches.h (right): https://codereview.chromium.org/14409006/diff/91001/cc/base/switches.h#newcode43 cc/base/switches.h:43: CC_EXPORT ...
7 years, 7 months ago (2013-05-22 22:56:36 UTC) #34
kaanb
https://codereview.chromium.org/14409006/diff/91001/cc/base/switches.h File cc/base/switches.h (right): https://codereview.chromium.org/14409006/diff/91001/cc/base/switches.h#newcode43 cc/base/switches.h:43: CC_EXPORT extern const char kCCUseMapImage[]; On 2013/05/22 22:56:36, jamesr ...
7 years, 7 months ago (2013-05-22 23:01:23 UTC) #35
reveman
https://codereview.chromium.org/14409006/diff/86002/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14409006/diff/86002/cc/trees/layer_tree_host_impl.cc#newcode1439 cc/trees/layer_tree_host_impl.cc:1439: // Renderer may be NULL in unit tests. is ...
7 years, 7 months ago (2013-05-22 23:51:49 UTC) #36
kaanb
https://codereview.chromium.org/14409006/diff/86002/cc/trees/layer_tree_host_impl.cc File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/14409006/diff/86002/cc/trees/layer_tree_host_impl.cc#newcode1439 cc/trees/layer_tree_host_impl.cc:1439: // Renderer may be NULL in unit tests. On ...
7 years, 7 months ago (2013-05-22 23:59:27 UTC) #37
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaanb@chromium.org/14409006/103001
7 years, 7 months ago (2013-05-23 00:00:15 UTC) #38
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 7 months ago (2013-05-23 00:42:38 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kaanb@chromium.org/14409006/84002
7 years, 7 months ago (2013-05-23 00:55:07 UTC) #40
commit-bot: I haz the power
Change committed as 201718
7 years, 7 months ago (2013-05-23 06:48:44 UTC) #41
danakj
Is there a reason this guards the renderer caps "using_map_image" on the layer tree settings ...
7 years, 3 months ago (2013-09-03 17:24:55 UTC) #42
kaanb1
There's no reason. Good catch. I'll send a fix now. On Tue, Sep 3, 2013 ...
7 years, 3 months ago (2013-09-03 17:40:15 UTC) #43
danakj
7 years, 3 months ago (2013-09-03 17:45:49 UTC) #44
Ok I have one up.


On Tue, Sep 3, 2013 at 1:40 PM, Kaan Baloglu <kaanb@google.com> wrote:

> There's no reason. Good catch. I'll send a fix now.
>
>
> On Tue, Sep 3, 2013 at 10:24 AM, <danakj@chromium.org> wrote:
>
>> Is there a reason this guards the renderer caps "using_map_image" on the
>> layer
>> tree settings in gl renderer, but not in delegating renderer?
>>
>>
https://chromiumcodereview.**appspot.com/14409006/<https://chromiumcodereview...
>>
>
>
>
> --
> Thanks,
> Kaan
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698