New issue
Advanced search Search tips

Issue 741213 link

Starred by 2 users
Status: Available
Owner: ----
Cc:
danakj@chromium.org
m...@chromium.org
piman@chromium.org
dsinclair@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Re-purpose src/ui/gl to become a client-side GL utilities collection

Project Member Reported by m...@chromium.org, Jul 12 2017 
danakj@ and piman@ had a discussion, prompted by miu@'s recent design explorations. The problem is this: Where should "GL utilitiy code" depended upon by src/cc, src/components/viz, and other places (src/content/...) be located?

The main "utility" that motivated this clean-up is GLHelper, which provides an extensive quality scalar, YUV/colorspace conversion, and a few other things.

The solution is to re-purpose src/ui/gl to be a library of client-side GL utilities. There are many files already in src/ui/gl that should be moved to src/gpu/command_buffer/common or .../service. The plan:

1. Things like src/ui/gl/??_renderer_layer_params.* will move to src/gpu/command_buffer/common.

2. Discover unreferenced code, such as src/ui/gl/gl_share_group.h, and delete or move as appropriate.

3. Many files are service-side (not client-side) stuff, and will move to src/gpu/command_buffer/service.

Comment 1 by piman@chromium.org, Jul 12 2017 

About #2, agreed we should absolutely get rid of unreferenced code, but ui/gl/gl_share_group.h is definitely referenced :)

$ git gs ui/gl/gl_share_group.h
gpu/command_buffer/service/gl_context_virtual_unittest.cc:11:#include "ui/gl/gl_share_group.h"
gpu/command_buffer/tests/fuzzer_main.cc:36:#include "ui/gl/gl_share_group.h"
gpu/command_buffer/tests/gl_manager.cc:41:#include "ui/gl/gl_share_group.h"
gpu/command_buffer/tests/gl_texture_mailbox_unittest.cc:18:#include "ui/gl/gl_share_group.h"
gpu/ipc/gpu_in_process_thread_service.h:13:#include "ui/gl/gl_share_group.h"
gpu/ipc/in_process_command_buffer.cc:51:#include "ui/gl/gl_share_group.h"
gpu/ipc/service/gpu_channel.h:31:#include "ui/gl/gl_share_group.h"
gpu/ipc/service/gpu_channel_manager.cc:33:#include "ui/gl/gl_share_group.h"
media/gpu/android/codec_image_unittest.cc:21:#include "ui/gl/gl_share_group.h"
media/gpu/surface_texture_gl_owner_unittest.cc:19:#include "ui/gl/gl_share_group.h"
ui/gl/gl_context.h:17:#include "ui/gl/gl_share_group.h"
ui/gl/gl_share_group.cc:5:#include "ui/gl/gl_share_group.h"
ui/gl/init/gl_factory.cc:13:#include "ui/gl/gl_share_group.h"
ui/gl/init/gl_factory_android.cc:16:#include "ui/gl/gl_share_group.h"
ui/gl/init/gl_factory_mac.cc:15:#include "ui/gl/gl_share_group.h"
ui/gl/init/gl_factory_ozone.cc:12:#include "ui/gl/gl_share_group.h"
ui/gl/init/gl_factory_win.cc:16:#include "ui/gl/gl_share_group.h"
ui/gl/init/gl_factory_x11.cc:16:#include "ui/gl/gl_share_group.h"
ui/ozone/common/gl_ozone_egl.cc:12:#include "ui/gl/gl_share_group.h"
ui/ozone/common/gl_ozone_osmesa.cc:13:#include "ui/gl/gl_share_group.h

Comment 2 by m...@chromium.org, Jul 12 2017 

Oh, sorry. The email thread I was reading indicated that gl_share_group.h was not referenced from src/cc code, and I didn't transfer that meaning into this bug's description correctly. ;-)

Comment 3 by kainino@chromium.org, Dec 7 2017

Labels: Needs-Feedback
Can you confirm whether this is still relevant?

Comment 4 by m...@chromium.org, Jan 25 2018

Cc: m...@chromium.org
Owner: ----
Status: Available (was: Started)
I dropped this. A lot of what I needed to have moved was already done for me in the meantime. :)

However, the rest of the code might still need to be moved. I myself cannot do this work at this time, so changing status back to Available.

Comment 5 by danakj@chromium.org, Jan 25 2018 

Doh, GLHelper is still in a place it wasn't meant to stay

Comment 6 by m...@chromium.org, Jan 25 2018 

re GLHelper: Wait on that one. I'm going to be deleting a lot of code that depends on it. Most of the GLHelper code may be deleted, making its remaining functionality and few remaining dependencies obvious. At that point, it'll hopefully be obvious where to move it to. :)
►

Sign in to add a comment