Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
136583006:
Add 16-bit support for browser compositor surface (Closed)
|
Created:
6 years, 11 months ago by sivag Modified:
6 years, 11 months ago CC:
chromium-reviews, rjkroege, kalyank, ozone-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionAdd 16-bit support for browser compositor surface
BUG=272429
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246822
Patch Set 1 : WIP:: Add 16-bit support for browser compositor surface #
Total comments: 14
Patch Set 2 : Changes done as per review comments. #
Total comments: 12
Patch Set 3 : Changed as per the review comments. #
Total comments: 2
Patch Set 4 : Code Changed as per the review comments. #
Total comments: 8
Patch Set 5 : Fallback to default format when there is no lowend support. #
Total comments: 6
Patch Set 6 : Code changed as per the comments #Patch Set 7 : Remove unused alpha variable. #Patch Set 8 : Remove Unused alpha variable. #Messages
Total messages: 18 (0 generated)
WIP. Need to test on low end devices and confirm pbuffer works fine.
https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:176: const EGLint* kConfigAttribs = nit: Just use |choose_attributes| here and below. Also |kConfigAttribs| suggests it's constant, although it's really chosen at runtime. https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:203: matching_configs = (EGLConfig*)malloc(num_configs * sizeof(EGLConfig)); Instead of malloc/free, you can use: scoped_ptr<EGLConfig[]> matching_configs(new EGLConfig[num_configs]); Also put a comment that because of the EGL config sort order, we have to iterate through all of them (it'll put higher sum(R,G,B) bits first with the above attribs). https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:253: return false; We should fall back to 32bit, since not all drivers might support 565.
https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:141: static EGLint configattribs_8888[] = { nit: config_attribs_8888 https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:153: static EGLint configattribs_565[] = { nit: config_attribs_565 https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:228: { nit: { on previous line
PTAL... https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:141: static EGLint configattribs_8888[] = { On 2014/01/17 18:20:01, piman wrote: > nit: config_attribs_8888 Done. https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:153: static EGLint configattribs_565[] = { On 2014/01/17 18:20:01, piman wrote: > nit: config_attribs_565 Done. https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:176: const EGLint* kConfigAttribs = On 2014/01/17 18:05:30, sievers wrote: > nit: Just use |choose_attributes| here and below. > Also |kConfigAttribs| suggests it's constant, although it's really chosen at > runtime. Done. https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:203: matching_configs = (EGLConfig*)malloc(num_configs * sizeof(EGLConfig)); On 2014/01/17 18:05:30, sievers wrote: > Instead of malloc/free, you can use: > scoped_ptr<EGLConfig[]> matching_configs(new EGLConfig[num_configs]); > > Also put a comment that because of the EGL config sort order, we have to iterate > through all of them (it'll put higher sum(R,G,B) bits first with the above > attribs). Done. https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:228: { On 2014/01/17 18:20:01, piman wrote: > nit: { on previous line Done. https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:253: return false; On 2014/01/17 18:05:30, sievers wrote: > We should fall back to 32bit, since not all drivers might support 565. Done.
https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:15: #include "base/android/sys_utils.h" We could include this only on Android platform. https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:164: EGLint* choose_attributes = 0; choose_attributes could point to config_attribs_8888 here, which would avoid the else case below and also #else case. https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:199: scoped_ptr<EGLConfig[]> matching_configs(new EGLConfig[num_configs]); matching_configs could be defined under #if defined(OS_ANDROID) perhaps ? (Atleast it's not used otherwise) https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:200: EGLConfig *config_data = NULL; config_data could point to g_config here ? This would avoid the else case below and #else too. https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:200: EGLConfig *config_data = NULL; nit: asterisk to left i.e. EGLConfig* config_data as in rest of the code. https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:250: if ((success == EGL_TRUE) && (red == 5) && nit: as we are trying to find an exact match, we could use else here ?
https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/1/ui/gl/gl_surface_egl.cc#newc... ui/gl/gl_surface_egl.cc:15: #include "base/android/sys_utils.h" On 2014/01/20 14:48:31, kalyank wrote: > We could include this only on Android platform. sorry, ignore this. I was looking at wrong version of the patch.
PTAL... https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:164: EGLint* choose_attributes = 0; On 2014/01/20 14:48:31, kalyank wrote: > choose_attributes could point to config_attribs_8888 here, which would avoid the > else case below and also #else case. Done. https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:199: scoped_ptr<EGLConfig[]> matching_configs(new EGLConfig[num_configs]); On 2014/01/20 14:48:31, kalyank wrote: > matching_configs could be defined under #if defined(OS_ANDROID) perhaps ? > (Atleast it's not used otherwise) Done. https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:200: EGLConfig *config_data = NULL; On 2014/01/20 14:48:31, kalyank wrote: > config_data could point to g_config here ? This would avoid the else case below > and #else too. Done. https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:250: if ((success == EGL_TRUE) && (red == 5) && On 2014/01/20 14:48:31, kalyank wrote: > nit: as we are trying to find an exact match, we could use else here ? We dont know whether we found the exact match or not until we break the loop.so else condition here will not work.
https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:250: if ((success == EGL_TRUE) && (red == 5) && On 2014/01/21 07:44:05, Sikugu wrote: > On 2014/01/20 14:48:31, kalyank wrote: > > nit: as we are trying to find an exact match, we could use else here ? > > We dont know whether we found the exact match or not until we break the loop.so > else condition here will not work. Sorry, I probably wasn't clear enough. I meant something like this: if (success == EGL_TRUE) { if ((red == 8) && (green == 8) other checks) { default_format = i; } else if ((red == 5) and other checks) { g_config = matching_configs[i]; match_found = true; break; } } We cannot have a config which matches both criteria at the same time.
https://codereview.chromium.org/136583006/diff/110001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/110001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:196: EGLConfig *config_data = &g_config; nit: asterisk to left i.e. EGLConfig* config_data as in rest of the code.
PTAL... https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:200: EGLConfig *config_data = NULL; On 2014/01/20 14:48:31, kalyank wrote: > nit: asterisk to left i.e. EGLConfig* config_data as in rest of the code. Done. https://codereview.chromium.org/136583006/diff/60001/ui/gl/gl_surface_egl.cc#... ui/gl/gl_surface_egl.cc:250: if ((success == EGL_TRUE) && (red == 5) && On 2014/01/21 08:10:02, kalyank wrote: > On 2014/01/21 07:44:05, Sikugu wrote: > > On 2014/01/20 14:48:31, kalyank wrote: > > > nit: as we are trying to find an exact match, we could use else here ? > > > > We dont know whether we found the exact match or not until we break the > loop.so > > else condition here will not work. > > Sorry, I probably wasn't clear enough. I meant something like this: > > if (success == EGL_TRUE) { > if ((red == 8) && (green == 8) other checks) { > default_format = i; > } else if ((red == 5) and other checks) { > g_config = matching_configs[i]; > match_found = true; > break; > } > } > > We cannot have a config which matches both criteria at the same time. > Done. https://codereview.chromium.org/136583006/diff/110001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/110001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:196: EGLConfig *config_data = &g_config; On 2014/01/21 08:16:24, kalyank wrote: > nit: asterisk to left i.e. EGLConfig* config_data as in rest of the code. Done.
lgtm
https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:236: (green == 8) && (blue == 8) && (alpha == 8)) { Do you think it might be better to take the first config that matches this rather than the last? I'm just wondering if we could somehow end up picking a weird config (such as multisampled, I dunno). The way all the sorting rules work it might be more promising to pick the first one that matches this criteria. wdyt? https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:238: default_format = i; nit: indent https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:247: LOG (ERROR) << "Falling back to default 32 bit format"; nit: no space after LOG. Also maybe make it WARNING or INFO.
https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:236: (green == 8) && (blue == 8) && (alpha == 8)) { On 2014/01/21 19:47:46, sievers wrote: > Do you think it might be better to take the first config that matches this > rather than the last? > I'm just wondering if we could somehow end up picking a weird config (such as > multisampled, I dunno). The way all the sorting rules work it might be more > promising to pick the first one that matches this criteria. wdyt? I think we should go with the first match as per the sorting rules. And as per the current code , as we have already choosed the 565 config in lowend device mode and if we are not able to find any match for that , we may never get the fallback choice of ARGB (because the choosed attributes are for 16bit buffers).So we may have to choose it again with the right ARGB atrributes, buffer size etc ...to enter the fallback mode. I am changing the code accordingly, will update the patch by tomorrow.
PTAL.. https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:236: (green == 8) && (blue == 8) && (alpha == 8)) { On 2014/01/22 15:48:20, Sikugu wrote: > On 2014/01/21 19:47:46, sievers wrote: > > Do you think it might be better to take the first config that matches this > > rather than the last? > > I'm just wondering if we could somehow end up picking a weird config (such as > > multisampled, I dunno). The way all the sorting rules work it might be more > > promising to pick the first one that matches this criteria. wdyt? > > I think we should go with the first match as per the sorting rules. > And as per the current code , as we have already choosed the 565 config in > lowend device mode and if we are not able to find any match for that , we may > never get the fallback choice of ARGB (because the choosed attributes are for > 16bit buffers).So we may have to choose it again with the right ARGB atrributes, > buffer size etc ...to enter the fallback mode. > > I am changing the code accordingly, will update the patch by tomorrow. Done. https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:236: (green == 8) && (blue == 8) && (alpha == 8)) { On 2014/01/21 19:47:46, sievers wrote: > Do you think it might be better to take the first config that matches this > rather than the last? > I'm just wondering if we could somehow end up picking a weird config (such as > multisampled, I dunno). The way all the sorting rules work it might be more > promising to pick the first one that matches this criteria. wdyt? Done. https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:238: default_format = i; On 2014/01/21 19:47:46, sievers wrote: > nit: indent Done. https://codereview.chromium.org/136583006/diff/170001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:247: LOG (ERROR) << "Falling back to default 32 bit format"; On 2014/01/21 19:47:46, sievers wrote: > nit: no space after LOG. Also maybe make it WARNING or INFO. Done.
lgtm with nits. thanks! https://codereview.chromium.org/136583006/diff/230001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/230001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:104: EGLint* num_configs) { Can you put this in the anonymous namespace above? (Also no need for 'static' then.) https://codereview.chromium.org/136583006/diff/230001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:203: &num_configs); nit: if (!ValidateEglConfig(...)) return false; https://codereview.chromium.org/136583006/diff/230001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:257: if (!config_status) { nit: same here and no need for |config_status| variable.
Done. https://codereview.chromium.org/136583006/diff/230001/ui/gl/gl_surface_egl.cc File ui/gl/gl_surface_egl.cc (right): https://codereview.chromium.org/136583006/diff/230001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:104: EGLint* num_configs) { On 2014/01/23 18:16:18, sievers wrote: > Can you put this in the anonymous namespace above? (Also no need for 'static' > then.) Done. https://codereview.chromium.org/136583006/diff/230001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:203: &num_configs); On 2014/01/23 18:16:18, sievers wrote: > nit: if (!ValidateEglConfig(...)) > return false; Done. https://codereview.chromium.org/136583006/diff/230001/ui/gl/gl_surface_egl.cc... ui/gl/gl_surface_egl.cc:257: if (!config_status) { On 2014/01/23 18:16:18, sievers wrote: > nit: same here and no need for |config_status| variable. Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/136583006/280001
Message was sent while issue was closed.
Change committed as 246822 |
