Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
149123008:
Implement GLHelperReadbackSupport for GLHelper usage. (Closed)
|
Created:
6 years, 10 months ago by sivag Modified:
6 years, 9 months ago Reviewers:
Ken Russell (switch to Gerrit), Daniel Sievers (Google), hubbe, jam, piman, vivekg, no sievers, hubbe1 CC:
chromium-reviews, jbauman+watch_chromium.org, yusukes+watch_chromium.org, yukishiino+watch_chromium.org, penghuang+watch_chromium.org, sievers+watch_chromium.org, joi+watch-content_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su, miu+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionWe need a function which can tell us whether the requested texture readback is supported or not. As of now ARGB888 is taken as the default format, this will pass until the browser surface config is ARGB8888, in low end mode we switch to 16 bit format surface if supported, then we may need this function.
Implement IsReadBackConfigSupported using map and clean up accordingly.
Bug=323150
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254981
Patch Set 1 #Patch Set 2 : Implement IsReadBackConfigSupported using map and clean up accordingly #Patch Set 3 : Move all format checking logic to IsReadBackConfigSupported and use this #Patch Set 4 : Remove unwanted DCHECK. #Patch Set 5 : Rebasing TOT. #Patch Set 6 : Remove unnecessary space. #
Total comments: 26
Patch Set 7 : Code changed as per review comments. #Patch Set 8 : Formatted cl using git format option. #
Total comments: 22
Patch Set 9 : Code changed as per review comments. #Patch Set 10 : Fixed Build error for gl_helper_unit_test. #Patch Set 11 : Fix for win_gpu_test problem. #Patch Set 12 : Avoid using SupportsFormat in between gl operations. #
Total comments: 2
Patch Set 13 : Code changed as per review comments. #
Total comments: 4
Patch Set 14 : Created a new class, Handled review comments. #
Total comments: 11
Patch Set 15 : Code changed as per review comments. #Patch Set 16 : Remove unused gl_helper member. #Messages
Total messages: 34 (0 generated)
PTAL...
https://codereview.chromium.org/149123008/diff/170001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/149123008/diff/170001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1396: callback.Run(false, SkBitmap()); You don't need to change all of these. That's what the scoped_callback_runner is for. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:401: if (!IsReadBackConfigSupported(bitmap_config)) { See comment in ReadbackTextureAsync(). https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:513: (config == SkBitmap::kARGB_8888_Config)); See comment below. The check here should be consistent with ReadbackTextureAsync(). https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:542: if (!IsReadBackConfigSupported(bitmap_config)) This check is actually not sufficient. IsReadBackConfigSupported() tells you whether we can read back a 565 texture 1:1. However, what if |texture| here is RGBA8? Maybe we should just remove the checks in ReadbackTexture(A)Sync, and leave it to the client to make sure readbacks are not requested with implied format conversion? For the internal call sites where we create an intermediate texture, we already have the checks and create the texture in the destination format. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:916: const SkBitmap::Config& texture_format) { nit: bitmap_format https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:926: GL_UNSIGNED_SHORT_5_6_5); nit: indent https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.h (right): https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:283: bool SupportsFormat(GLint format, GLint type); nit: can be private https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:287: bool IsReadBackConfigSupported(const SkBitmap::Config& texture_format); Please describe that this does not specify whether conversion during readback is supported. Based on the test we do, we only know that the readback *should* work if the texture format/type exactly match the readback format/type. Maybe it even makes more sense to pass in the GL format/type here? https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... File content/common/gpu/client/gl_helper_unittest.cc (right): https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper_unittest.cc:1005: LOG(INFO) << "Format Not supported on this platform"<<bitmap_config ; nit: formatting You might want to look at ClangFormat: https://code.google.com/p/chromium/wiki/ClangFormat https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper_unittest.cc:1006: LOG(INFO) << "Skipping the test"; nit: can be written as one stream with the above
https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:581: if(!helper_){ nit: spaces Try git cl format. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.h (right): https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:349: std::map<SkBitmap::Config, bool> config_map_; map is way overkill. SkBitmap::Config is a finite set - just use an array?
PTAL.. https://codereview.chromium.org/149123008/diff/170001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/149123008/diff/170001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1396: callback.Run(false, SkBitmap()); On 2014/02/18 19:35:44, sievers wrote: > You don't need to change all of these. That's what the scoped_callback_runner is > for. Done. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:401: if (!IsReadBackConfigSupported(bitmap_config)) { On 2014/02/18 19:35:44, sievers wrote: > See comment in ReadbackTextureAsync(). Replied at ReadbackTextureAsync. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:513: (config == SkBitmap::kARGB_8888_Config)); On 2014/02/18 19:35:44, sievers wrote: > See comment below. The check here should be consistent with > ReadbackTextureAsync(). Done. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:542: if (!IsReadBackConfigSupported(bitmap_config)) On 2014/02/18 19:35:44, sievers wrote: > This check is actually not sufficient. > IsReadBackConfigSupported() tells you whether we can read back a 565 texture > 1:1. However, what if |texture| here is RGBA8? > > Maybe we should just remove the checks in ReadbackTexture(A)Sync, and leave it > to the client to make sure readbacks are not requested with implied format > conversion? > > For the internal call sites where we create an intermediate texture, we already > have the checks and create the texture in the destination format. I think then we should actually validate the texture also using GetTexParameteriv for the GL_TEXTURE_INTERNAL_FORMAT , and match that with the SkBitmap::Config passed. Or we should remove the config parameter itself , in both the Readbacktexturesync and async and validate just by using the texture by getting the interformat details using GetTexParameteriv. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:581: if(!helper_){ On 2014/02/18 22:23:15, piman wrote: > nit: spaces > > Try git cl format. Done. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:916: const SkBitmap::Config& texture_format) { On 2014/02/18 19:35:44, sievers wrote: > nit: bitmap_format Done. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:926: GL_UNSIGNED_SHORT_5_6_5); On 2014/02/18 19:35:44, sievers wrote: > nit: indent Done. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.h (right): https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:283: bool SupportsFormat(GLint format, GLint type); On 2014/02/18 19:35:44, sievers wrote: > nit: can be private Done. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:287: bool IsReadBackConfigSupported(const SkBitmap::Config& texture_format); On 2014/02/18 19:35:44, sievers wrote: > Please describe that this does not specify whether conversion during readback is > supported. Based on the test we do, we only know that the readback *should* work > if the texture format/type exactly match the readback format/type. > Maybe it even makes more sense to pass in the GL format/type here? The reason why i kept a SkBitmap::Config is in CopyfromCOmpositingSurface i wanted to do early return if the requested format is not supported. Any way we have the SupportsFormat API which takes format/type as the input. Do you think we have to alter the function names? https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:349: std::map<SkBitmap::Config, bool> config_map_; On 2014/02/18 22:23:15, piman wrote: > map is way overkill. > SkBitmap::Config is a finite set - just use an array? Done. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... File content/common/gpu/client/gl_helper_unittest.cc (right): https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper_unittest.cc:1005: LOG(INFO) << "Format Not supported on this platform"<<bitmap_config ; On 2014/02/18 19:35:44, sievers wrote: > nit: formatting > > You might want to look at ClangFormat: > https://code.google.com/p/chromium/wiki/ClangFormat Done. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper_unittest.cc:1006: LOG(INFO) << "Skipping the test"; On 2014/02/18 19:35:44, sievers wrote: > nit: can be written as one stream with the above Done.
lgtm with nits https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:542: if (!IsReadBackConfigSupported(bitmap_config)) On 2014/02/19 15:49:04, Sikugu wrote: > On 2014/02/18 19:35:44, sievers wrote: > > This check is actually not sufficient. > > IsReadBackConfigSupported() tells you whether we can read back a 565 texture > > 1:1. However, what if |texture| here is RGBA8? > > > > Maybe we should just remove the checks in ReadbackTexture(A)Sync, and leave it > > to the client to make sure readbacks are not requested with implied format > > conversion? > > > > For the internal call sites where we create an intermediate texture, we > already > > have the checks and create the texture in the destination format. > > I think then we should actually validate the texture also using > GetTexParameteriv for the GL_TEXTURE_INTERNAL_FORMAT , and match that with the > SkBitmap::Config passed. > > Or > > we should remove the config parameter itself , in both the Readbacktexturesync > and async and validate just by using the texture by getting the interformat > details using GetTexParameteriv. > Sounds good. Feel free to try as a follow-up since there's no need to change it functionally in this patch. But you want to do it as a DCHECK() only since it's synchronous. Also, is that check with GL_TEXTURE_INTERNAL_FORMAT even supported in OpenGL ES? https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.h (right): https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:175: FORMAT_NONE, nit: FORMAT_NONE -> FORMAT_SUPPORT_UNKNOWN https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:287: // is supported or not. nit: I think the comment could still be a bit clearer so that one does not incorrectly assume that 'IsReadbackConfigSupported(565) == true' means that you can read a RGBA8 texture back into a 565 bitmap using ReadbackTexture(A)Sync() in this class. https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... File content/common/gpu/client/gl_helper_unittest.cc (right): https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper_unittest.cc:1006: << "Skipping the test"; nit: does this need to be spaced? something like LOG(INFO) << "Format Not supported on this platform: " << bitmap_config << ". Skipping the test";
LGTM+nits https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:348: nit: no blank line https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:399: nit: no blank line https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:459: nit: no blank line https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:512: nit: no blank line https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:542: nit: no blank line https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:583: NOTREACHED(); nit: just DCHECK(helper_). https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:914: nit: no blank line.
https://codereview.chromium.org/149123008/diff/320001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/149123008/diff/320001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1469: const SkBitmap::Config& bitmap_config) { nit: As SkBitmap::Config is an enum, pass it by value as is done in [1] bool IsReadbackConfigSupported(SkBitmap::Config bitmap_config) here and all other applicable places. [1] https://code.google.com/p/chromium/codesearch#search/&q=%5C(.*SkBitmap::Confi... https://codereview.chromium.org/149123008/diff/320001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/149123008/diff/320001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:252: bool IsReadBackConfigSupported(const SkBitmap::Config& bitmap_config); nit: IsReadBackConfigSupported -> IsReadbackConfigSupported here and all other usages.
Done. https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/170001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:542: if (!IsReadBackConfigSupported(bitmap_config)) On 2014/02/19 19:09:38, sievers wrote: > On 2014/02/19 15:49:04, Sikugu wrote: > > On 2014/02/18 19:35:44, sievers wrote: > > > This check is actually not sufficient. > > > IsReadBackConfigSupported() tells you whether we can read back a 565 texture > > > 1:1. However, what if |texture| here is RGBA8? > > > > > > Maybe we should just remove the checks in ReadbackTexture(A)Sync, and leave > it > > > to the client to make sure readbacks are not requested with implied format > > > conversion? > > > > > > For the internal call sites where we create an intermediate texture, we > > already > > > have the checks and create the texture in the destination format. > > > > I think then we should actually validate the texture also using > > GetTexParameteriv for the GL_TEXTURE_INTERNAL_FORMAT , and match that with the > > SkBitmap::Config passed. > > > > Or > > > > we should remove the config parameter itself , in both the Readbacktexturesync > > and async and validate just by using the texture by getting the interformat > > details using GetTexParameteriv. > > > > Sounds good. Feel free to try as a follow-up since there's no need to change it > functionally in this patch. But you want to do it as a DCHECK() only since it's > synchronous. Also, is that check with GL_TEXTURE_INTERNAL_FORMAT even supported > in OpenGL ES? When i checked , there is no such support in the api list. I will confirm this. https://codereview.chromium.org/149123008/diff/320001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/149123008/diff/320001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.cc:1469: const SkBitmap::Config& bitmap_config) { On 2014/02/20 00:53:31, vivekg_ wrote: > nit: As SkBitmap::Config is an enum, pass it by value as is done in [1] > > bool IsReadbackConfigSupported(SkBitmap::Config bitmap_config) here and all > other applicable places. > > [1] > https://code.google.com/p/chromium/codesearch#search/&q=%255C%28.*SkBitmap::C... Done. https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:348: On 2014/02/20 00:32:18, piman (OOO back 2014-3-4) wrote: > nit: no blank line Done. https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:399: On 2014/02/20 00:32:18, piman (OOO back 2014-3-4) wrote: > nit: no blank line Done. https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:459: On 2014/02/20 00:32:18, piman (OOO back 2014-3-4) wrote: > nit: no blank line Done. https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:512: On 2014/02/20 00:32:18, piman (OOO back 2014-3-4) wrote: > nit: no blank line Done. https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:542: On 2014/02/20 00:32:18, piman (OOO back 2014-3-4) wrote: > nit: no blank line Done. https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:583: NOTREACHED(); On 2014/02/20 00:32:18, piman (OOO back 2014-3-4) wrote: > nit: just DCHECK(helper_). Done. https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.h (right): https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:175: FORMAT_NONE, On 2014/02/19 19:09:39, sievers wrote: > nit: FORMAT_NONE -> FORMAT_SUPPORT_UNKNOWN Done. https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:287: // is supported or not. On 2014/02/19 19:09:39, sievers wrote: > nit: I think the comment could still be a bit clearer so that one does not > incorrectly assume that 'IsReadbackConfigSupported(565) == true' means that you > can read a RGBA8 texture back into a 565 bitmap using ReadbackTexture(A)Sync() > in this class. Done. https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... File content/common/gpu/client/gl_helper_unittest.cc (right): https://codereview.chromium.org/149123008/diff/320001/content/common/gpu/clie... content/common/gpu/client/gl_helper_unittest.cc:1006: << "Skipping the test"; On 2014/02/19 19:09:39, sievers wrote: > nit: does this need to be spaced? something like > LOG(INFO) << "Format Not supported on this platform: " << bitmap_config << ". > Skipping the test"; Done.
Hi gpu try bots were failing when i tried testing my patch Please take a look. in 681:1799:0221/022243:2756580114704:ERROR:gl_helper_unittest.cc(805 this is failing while running the GLHelperTest.YUVReadbackOptTest failed on win_gpu_triggered_tests linux_gpu_triggered_tests mac_gpu_retina_triggered_tests mac_gpu_triggered_tests.
On 2014/02/21 11:41:42, Sikugu wrote: > Hi gpu try bots were failing when i tried testing my patch > > Please take a look. > > in 681:1799:0221/022243:2756580114704:ERROR:gl_helper_unittest.cc(805 > > this is failing while running the GLHelperTest.YUVReadbackOptTest > > failed on win_gpu_triggered_tests linux_gpu_triggered_tests > mac_gpu_retina_triggered_tests mac_gpu_triggered_tests. Did you try running on Linux to see if you can reproduce?
This looks like a legitimate failure caused by your patch. You should run this test locally. Also, please file a bug about this change and reference it from a BUG= line in your CL, so that any reverts and re-lands are documented in a single place.
Hi Sievers,Ken I tested this with linux build.The issue is observed after patching my CL. The problem is we are doing a texture creation binding and attach in supportsFormat API on demand while checking for IsReadBackConfigSupported, if supports format is called in between any gl operations it may alter the behaviour of the previous operations. So i fixed this by populating the format support array at the time of initialization when the glhelper is created . Later the populated array value is returned when any particualr format support is checked. Tested by trigggering build_bots as well. PTAL...
On 2014/02/21 17:28:04, sievers wrote: > On 2014/02/21 11:41:42, Sikugu wrote: > > Hi gpu try bots were failing when i tried testing my patch > > > > Please take a look. > > > > in 681:1799:0221/022243:2756580114704:ERROR:gl_helper_unittest.cc(805 > > > > this is failing while running the GLHelperTest.YUVReadbackOptTest > > > > failed on win_gpu_triggered_tests linux_gpu_triggered_tests > > mac_gpu_retina_triggered_tests mac_gpu_triggered_tests. > > Did you try running on Linux to see if you can reproduce? Hi Sievers, I fixed the unit test issue, PTAL..at the additional Changes. Thanks. Siva.
On 2014/02/21 17:28:04, sievers wrote: > On 2014/02/21 11:41:42, Sikugu wrote: > > Hi gpu try bots were failing when i tried testing my patch > > > > Please take a look. > > > > in 681:1799:0221/022243:2756580114704:ERROR:gl_helper_unittest.cc(805 > > > > this is failing while running the GLHelperTest.YUVReadbackOptTest > > > > failed on win_gpu_triggered_tests linux_gpu_triggered_tests > > mac_gpu_retina_triggered_tests mac_gpu_triggered_tests. > > Did you try running on Linux to see if you can reproduce? Hi Sievers, Can you please take a look at the minor modifications. Waiting for your approval. Thanks.
Sorry for the delay. Just wondering what state got broken from doing the test lazily. Shouldn't the scoped binders avoid such problems? https://codereview.chromium.org/149123008/diff/630001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/630001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:926: case FORMAT_SUPPORT_UNKNOWN: If you change things this way, I'd remove the lazy init path below and assume that InitializeReadbackSupport() was already called. You can then handle FORMAT_SUPPORT_UNKNOWN as NOTREACHED() here also.
The ReadbackPlane function got called in the ReadbackYUV while running the GLHelperTest.YUVReadbackOptTest unit_tests. I kept a check using isReadBackConfig in (ReadBackAsync) which is getting called in ReadbackPlane. I think if we are binding a texture, to a framebuffer and then we do isreadbackconfig lazily which intenally does another texture bind to the same framebuffer. Once we come back out of this previous active texture binding should be retained? I though we should not do it lazily, better initialize it before all the gl calls begin and then get it from the populated array. PTAL... https://codereview.chromium.org/149123008/diff/630001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/630001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:926: case FORMAT_SUPPORT_UNKNOWN: On 2014/02/26 20:05:43, sievers wrote: > If you change things this way, I'd remove the lazy init path below and assume > that InitializeReadbackSupport() was already called. You can then handle > FORMAT_SUPPORT_UNKNOWN as NOTREACHED() here also. Done.
On 2014/02/27 15:33:17, Sikugu wrote:
> The ReadbackPlane function got called in the ReadbackYUV while running the
> GLHelperTest.YUVReadbackOptTest unit_tests.
> I kept a check using isReadBackConfig in (ReadBackAsync) which is getting
called
> in ReadbackPlane.
>
> I think if we are binding a texture, to a framebuffer and then we do
> isreadbackconfig lazily
> which intenally does another texture bind to the same framebuffer. Once we
come
> back out of this
> previous active texture binding should be retained?
>
Ah, the ScopedBinder implementation in gl_helper.h seems a bit broken.
I was originally looking at ui/gl/scoped_binders.h which do the right thing.
However, this implementation does not restore the previous binding but 0
instead:
virtual ~ScopedBinder() { (gl_->*bind_func_)(Target, 0); }
> I though we should not do it lazily, better initialize it before all the gl
> calls begin
> and then get it from the populated array.
>
> PTAL...
>
>
https://codereview.chromium.org/149123008/diff/630001/content/common/gpu/clie...
> File content/common/gpu/client/gl_helper.cc (right):
>
>
https://codereview.chromium.org/149123008/diff/630001/content/common/gpu/clie...
> content/common/gpu/client/gl_helper.cc:926: case FORMAT_SUPPORT_UNKNOWN:
> On 2014/02/26 20:05:43, sievers wrote:
> > If you change things this way, I'd remove the lazy init path below and
assume
> > that InitializeReadbackSupport() was already called. You can then handle
> > FORMAT_SUPPORT_UNKNOWN as NOTREACHED() here also.
>
> Done.
https://codereview.chromium.org/149123008/diff/650001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/650001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:889: format_support_table_[i] = FORMAT_NOT_SUPPORTED; You can init it to NOT_SUPPORTED above then and remove UNKNOWN entirely. https://codereview.chromium.org/149123008/diff/650001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:901: return; You can remove this switch statement, since we only should be initializing once.
On Thu, Feb 27, 2014 at 12:27 PM, <sievers@chromium.org> wrote: > On 2014/02/27 15:33:17, Sikugu wrote: > >> The ReadbackPlane function got called in the ReadbackYUV while running the >> GLHelperTest.YUVReadbackOptTest unit_tests. >> I kept a check using isReadBackConfig in (ReadBackAsync) which is getting >> > called > >> in ReadbackPlane. >> > > I think if we are binding a texture, to a framebuffer and then we do >> isreadbackconfig lazily >> which intenally does another texture bind to the same framebuffer. Once we >> > come > >> back out of this >> previous active texture binding should be retained? >> > > > Ah, the ScopedBinder implementation in gl_helper.h seems a bit broken. > I was originally looking at ui/gl/scoped_binders.h which do the right > thing. > However, this implementation does not restore the previous binding but 0 > instead: > virtual ~ScopedBinder() { (gl_->*bind_func_)(Target, 0); } > > > > I though we should not do it lazily, better initialize it before all the >> gl >> calls begin >> and then get it from the populated array. >> > > PTAL... >> > > > https://codereview.chromium.org/149123008/diff/630001/ > content/common/gpu/client/gl_helper.cc > >> File content/common/gpu/client/gl_helper.cc (right): >> > > > https://codereview.chromium.org/149123008/diff/630001/ > content/common/gpu/client/gl_helper.cc#newcode926 > >> content/common/gpu/client/gl_helper.cc:926: case FORMAT_SUPPORT_UNKNOWN: >> On 2014/02/26 20:05:43, sievers wrote: >> > If you change things this way, I'd remove the lazy init path below and >> > assume > >> > that InitializeReadbackSupport() was already called. You can then handle >> > FORMAT_SUPPORT_UNKNOWN as NOTREACHED() here also. >> > > Done. >> > > > https://codereview.chromium.org/149123008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I think the original author was concerned that queriying the previous binding would be slow, as it would require a synchronous call to the gpu process... (I don't think that's cached on the client side, is it?) On Thu, Feb 27, 2014 at 12:27 PM, <sievers@chromium.org> wrote: > On 2014/02/27 15:33:17, Sikugu wrote: > >> The ReadbackPlane function got called in the ReadbackYUV while running the >> GLHelperTest.YUVReadbackOptTest unit_tests. >> I kept a check using isReadBackConfig in (ReadBackAsync) which is getting >> > called > >> in ReadbackPlane. >> > > I think if we are binding a texture, to a framebuffer and then we do >> isreadbackconfig lazily >> which intenally does another texture bind to the same framebuffer. Once we >> > come > >> back out of this >> previous active texture binding should be retained? >> > > > Ah, the ScopedBinder implementation in gl_helper.h seems a bit broken. > I was originally looking at ui/gl/scoped_binders.h which do the right > thing. > However, this implementation does not restore the previous binding but 0 > instead: > virtual ~ScopedBinder() { (gl_->*bind_func_)(Target, 0); } > > > > I though we should not do it lazily, better initialize it before all the >> gl >> calls begin >> and then get it from the populated array. >> > > PTAL... >> > > > https://codereview.chromium.org/149123008/diff/630001/ > content/common/gpu/client/gl_helper.cc > >> File content/common/gpu/client/gl_helper.cc (right): >> > > > https://codereview.chromium.org/149123008/diff/630001/ > content/common/gpu/client/gl_helper.cc#newcode926 > >> content/common/gpu/client/gl_helper.cc:926: case FORMAT_SUPPORT_UNKNOWN: >> On 2014/02/26 20:05:43, sievers wrote: >> > If you change things this way, I'd remove the lazy init path below and >> > assume > >> > that InitializeReadbackSupport() was already called. You can then handle >> > FORMAT_SUPPORT_UNKNOWN as NOTREACHED() here also. >> > > Done. >> > > > https://codereview.chromium.org/149123008/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/02/27 20:27:32, sievers wrote:
> On 2014/02/27 15:33:17, Sikugu wrote:
> > The ReadbackPlane function got called in the ReadbackYUV while running the
> > GLHelperTest.YUVReadbackOptTest unit_tests.
> > I kept a check using isReadBackConfig in (ReadBackAsync) which is getting
> called
> > in ReadbackPlane.
> >
> > I think if we are binding a texture, to a framebuffer and then we do
> > isreadbackconfig lazily
> > which intenally does another texture bind to the same framebuffer. Once we
> come
> > back out of this
> > previous active texture binding should be retained?
> >
>
> Ah, the ScopedBinder implementation in gl_helper.h seems a bit broken.
> I was originally looking at ui/gl/scoped_binders.h which do the right thing.
> However, this implementation does not restore the previous binding but 0
> instead:
> virtual ~ScopedBinder() { (gl_->*bind_func_)(Target, 0); }
>
>
Hmm, looks like we are doing a roundtrip to the decoder to get the
texture/framebuffer bindings if 'bind_generates_resources' is false for the
context.
So maybe this light binder is the right thing here to just make sure that
temporary textures don't leak.
> > I though we should not do it lazily, better initialize it before all the gl
> > calls begin
> > and then get it from the populated array.
> >
> > PTAL...
> >
> >
>
https://codereview.chromium.org/149123008/diff/630001/content/common/gpu/clie...
> > File content/common/gpu/client/gl_helper.cc (right):
> >
> >
>
https://codereview.chromium.org/149123008/diff/630001/content/common/gpu/clie...
> > content/common/gpu/client/gl_helper.cc:926: case FORMAT_SUPPORT_UNKNOWN:
> > On 2014/02/26 20:05:43, sievers wrote:
> > > If you change things this way, I'd remove the lazy init path below and
> assume
> > > that InitializeReadbackSupport() was already called. You can then handle
> > > FORMAT_SUPPORT_UNKNOWN as NOTREACHED() here also.
> >
> > Done.
On Thu, Feb 27, 2014 at 12:51 PM, Fredrik Hubinette <hubbe@google.com>wrote: > I think the original author was concerned that queriying the previous > binding would be slow, as it would require a synchronous call to the gpu > process... (I don't think that's cached on the client side, is it?) > > Yea, I was just looking at the same thing :) So we just have to be a bit careful then. > > > On Thu, Feb 27, 2014 at 12:27 PM, <sievers@chromium.org> wrote: > >> On 2014/02/27 15:33:17, Sikugu wrote: >> >>> The ReadbackPlane function got called in the ReadbackYUV while running >>> the >>> GLHelperTest.YUVReadbackOptTest unit_tests. >>> I kept a check using isReadBackConfig in (ReadBackAsync) which is getting >>> >> called >> >>> in ReadbackPlane. >>> >> >> I think if we are binding a texture, to a framebuffer and then we do >>> isreadbackconfig lazily >>> which intenally does another texture bind to the same framebuffer. Once >>> we >>> >> come >> >>> back out of this >>> previous active texture binding should be retained? >>> >> >> >> Ah, the ScopedBinder implementation in gl_helper.h seems a bit broken. >> I was originally looking at ui/gl/scoped_binders.h which do the right >> thing. >> However, this implementation does not restore the previous binding but 0 >> instead: >> virtual ~ScopedBinder() { (gl_->*bind_func_)(Target, 0); } >> >> >> >> I though we should not do it lazily, better initialize it before all the >>> gl >>> calls begin >>> and then get it from the populated array. >>> >> >> PTAL... >>> >> >> >> https://codereview.chromium.org/149123008/diff/630001/ >> content/common/gpu/client/gl_helper.cc >> >>> File content/common/gpu/client/gl_helper.cc (right): >>> >> >> >> https://codereview.chromium.org/149123008/diff/630001/ >> content/common/gpu/client/gl_helper.cc#newcode926 >> >>> content/common/gpu/client/gl_helper.cc:926: case FORMAT_SUPPORT_UNKNOWN: >>> On 2014/02/26 20:05:43, sievers wrote: >>> > If you change things this way, I'd remove the lazy init path below and >>> >> assume >> >>> > that InitializeReadbackSupport() was already called. You can then >>> handle >>> > FORMAT_SUPPORT_UNKNOWN as NOTREACHED() here also. >>> >> >> Done. >>> >> >> >> https://codereview.chromium.org/149123008/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Sievers, Code corrected as per the suggestions, along with this as there is more functionality related to readback i have moved all the readback support into a new class GLHelperReadbackSupport and moved it into a new file gl_helper_readback*. PTAL.. https://codereview.chromium.org/149123008/diff/650001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.cc (right): https://codereview.chromium.org/149123008/diff/650001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:889: format_support_table_[i] = FORMAT_NOT_SUPPORTED; On 2014/02/27 20:42:50, sievers wrote: > You can init it to NOT_SUPPORTED above then and remove UNKNOWN entirely. Done. https://codereview.chromium.org/149123008/diff/650001/content/common/gpu/clie... content/common/gpu/client/gl_helper.cc:901: return; On 2014/02/27 20:42:50, sievers wrote: > You can remove this switch statement, since we only should be initializing once. Done.
+Jam For content/content_common.gypi changes. ptal..
On 2014/02/28 11:34:01, Sikugu wrote: > +Jam > For content/content_common.gypi changes. > > ptal.. lgtm. feel free to tbr simple changes to gypi files
lgtm with nits https://codereview.chromium.org/149123008/diff/670001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/149123008/diff/670001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:249: bool IsReadbackConfigSupported(SkBitmap::Config bitmap_config); private https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.h (right): https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:328: Can you still put a comment here saying that it checks whether a texture matching the format can be read back 1:1? https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... File content/common/gpu/client/gl_helper_readback_support.cc (right): https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... content/common/gpu/client/gl_helper_readback_support.cc:50: } just to be paranoid since you use SkBitmap::Config as an index: DCHECK(texture_format < arraysize(format_support_table_)); https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... content/common/gpu/client/gl_helper_readback_support.cc:59: gl_->GenTextures(1, &dst_texture); nit: instead of Gen/Delete you can use ScopedTexture here, since you are also using ScopedFramebuffer below. https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... content/common/gpu/client/gl_helper_readback_support.cc:87: case FORMAT_NOT_SUPPORTED: case FORMAT_NOT_SUPPORTED: return false; default: NOTREACHED(); return false;
https://codereview.chromium.org/149123008/diff/670001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_android.h (right): https://codereview.chromium.org/149123008/diff/670001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_android.h:249: bool IsReadbackConfigSupported(SkBitmap::Config bitmap_config); On 2014/02/28 19:25:53, sievers wrote: > private Done. https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... File content/common/gpu/client/gl_helper.h (right): https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... content/common/gpu/client/gl_helper.h:328: On 2014/02/28 19:25:53, sievers wrote: > Can you still put a comment here saying that it checks whether a texture > matching the format can be read back 1:1? Done. https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... File content/common/gpu/client/gl_helper_readback_support.cc (right): https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... content/common/gpu/client/gl_helper_readback_support.cc:50: } On 2014/02/28 19:25:53, sievers wrote: > just to be paranoid since you use SkBitmap::Config as an index: > DCHECK(texture_format < arraysize(format_support_table_)); Done. https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... content/common/gpu/client/gl_helper_readback_support.cc:59: gl_->GenTextures(1, &dst_texture); On 2014/02/28 19:25:53, sievers wrote: > nit: instead of Gen/Delete you can use ScopedTexture here, since you are also > using ScopedFramebuffer below. Done. https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... content/common/gpu/client/gl_helper_readback_support.cc:87: case FORMAT_NOT_SUPPORTED: On 2014/02/28 19:25:53, sievers wrote: > case FORMAT_NOT_SUPPORTED: > return false; > default: > NOTREACHED(); > return false; Done. https://codereview.chromium.org/149123008/diff/670001/content/common/gpu/clie... content/common/gpu/client/gl_helper_readback_support.cc:87: case FORMAT_NOT_SUPPORTED: On 2014/02/28 19:25:53, sievers wrote: > case FORMAT_NOT_SUPPORTED: > return false; > default: > NOTREACHED(); > return false; Done.
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/149123008/710001
The CQ bit was unchecked by commit-bot@chromium.org
The commit queue went berserk retrying too often for a seemingly flaky test on builder android_dbg: http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db... http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
The CQ bit was checked by siva.gunturi@samsung.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/siva.gunturi@samsung.com/149123008/710001
Message was sent while issue was closed.
Change committed as 254981 |
