Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1129693004:
Disabled extensions in bug list now automatically removes the extension. (Closed)
|
Created:
5 years, 7 months ago by David Yen Modified:
4 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, piman+watch_chromium.org, vmiura Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisabled extensions in bug list now automatically removes the extension.
Bugs which simply require disabling of extensions no longer need any
extra work. Simply add the bug description to the GPU Bug list JSON
file and list the extension to disable under "disabled_extensions".
This CL also disabled GL_EXT_disjoint_timer_query for the Nexus 5 and
Nexus 6 under lollypop as a test to be sure it works. Tracing the
GPU no longer causes a crash on these devices under lollypop.
R=zmo@chromium.org
BUG=477514, 482067
Committed: https://crrev.com/4db835b2154821f7beefacf6866841fc52b6322a
Cr-Commit-Position: refs/heads/master@{#330446}
Patch Set 1 #
Total comments: 18
Patch Set 2 : Fixed bug ids #Patch Set 3 : Rebase / Initialize on dynamic bindings #Patch Set 4 : Added InitializeWithContext() call to unit test #
Messages
Total messages: 26 (3 generated)
dyen@chromium.org changed reviewers: + sievers@chromium.org
Now that the gl extensions could be disabled from the command line, I added the workaround parsing to append the correct command line. Note that I also had to defer the GL_Extensions filtering code because we have to be sure the GL Context is activated before we can call glGetString()...etc.
LGTM
https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1314: "id": 108, This collides with the same id defined above. It *should* blow up at runtime, no? Same below for 109. https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1320: "op": ">=", You are sure this used to work in 5.0 and before? https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1325: "gl_renderer": ".*330", Do you know that this works on other 3xx GPUs? In fact do you know if it works on any Qualcomm driver? There is also bug 453965, which your patch now allows us to address. (The DCHECK was happening too early for the normal workaround mechanism to deal with it.) You said you don't see this problem on Nexus 5? https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); Why this change and make this initialization lazy now?
https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1314: "id": 108, On 2015/05/15 18:31:51, sievers wrote: > This collides with the same id defined above. > It *should* blow up at runtime, no? Same below for 109. Fixed. Although this didn't blow up at runtime... Is it because I don't have "features" defined? https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1320: "op": ">=", On 2015/05/15 18:31:51, sievers wrote: > You are sure this used to work in 5.0 and before? Yes, we even have a perf bot running GPU timing metrics for nexus 5 under kit kat. https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1325: "gl_renderer": ".*330", On 2015/05/15 18:31:51, sievers wrote: > Do you know that this works on other 3xx GPUs? > In fact do you know if it works on any Qualcomm driver? > > There is also bug 453965, which your patch now allows us to address. (The DCHECK > was happening too early for the normal workaround mechanism to deal with it.) > You said you don't see this problem on Nexus 5? I believe the missing function problem only happens on Nexus 4 under lollypop and later. I would rather just remove the DCHECK because that only affects Nexus 4. The functions that are missing are ones we do not use, so adding this workaround to get around the DCHECK would disable gpu timing for nexus 4 which we currently have running on perf bots under lollypop.
https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1325: "gl_renderer": ".*330", On 2015/05/15 18:39:04, David Yen wrote: > On 2015/05/15 18:31:51, sievers wrote: > > Do you know that this works on other 3xx GPUs? > > In fact do you know if it works on any Qualcomm driver? > > > > There is also bug 453965, which your patch now allows us to address. (The > DCHECK > > was happening too early for the normal workaround mechanism to deal with it.) > > You said you don't see this problem on Nexus 5? > > I believe the missing function problem only happens on Nexus 4 under lollypop > and later. I would rather just remove the DCHECK because that only affects Nexus > 4. The functions that are missing are ones we do not use, so adding this > workaround to get around the DCHECK would disable gpu timing for nexus 4 which > we currently have running on perf bots under lollypop. Hmm, the missing functions for nexus 4 are not used but the moto-x functions listed in the bug are actually used. What GPU does the moto-x use? We should probably blacklist for that one as well.
https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1314: "id": 108, On 2015/05/15 18:39:03, David Yen wrote: > On 2015/05/15 18:31:51, sievers wrote: > > This collides with the same id defined above. > > It *should* blow up at runtime, no? Same below for 109. > > Fixed. Although this didn't blow up at runtime... Is it because I don't have > "features" defined? Mind checking with zmo@ and potentially filing a bug if that's the case? So that we don't silently end up with duplicate ids later. https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1325: "gl_renderer": ".*330", On 2015/05/15 18:45:22, David Yen wrote: > On 2015/05/15 18:39:04, David Yen wrote: > > On 2015/05/15 18:31:51, sievers wrote: > > > Do you know that this works on other 3xx GPUs? > > > In fact do you know if it works on any Qualcomm driver? > > > > > > There is also bug 453965, which your patch now allows us to address. (The > > DCHECK > > > was happening too early for the normal workaround mechanism to deal with > it.) > > > You said you don't see this problem on Nexus 5? > > > > I believe the missing function problem only happens on Nexus 4 under lollypop > > and later. I would rather just remove the DCHECK because that only affects > Nexus > > 4. The functions that are missing are ones we do not use, so adding this > > workaround to get around the DCHECK would disable gpu timing for nexus 4 which > > we currently have running on perf bots under lollypop. > > Hmm, the missing functions for nexus 4 are not used but the moto-x functions > listed in the bug are actually used. What GPU does the moto-x use? We should > probably blacklist for that one as well. Adreno 320. Can you reference that bug also in here then? I was wondering if you hit that DCHECK on Nexus 4. If these two bugs affect different devices/drivers we should probably split them up in here.
https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1314: "id": 108, On 2015/05/15 19:35:20, sievers wrote: > On 2015/05/15 18:39:03, David Yen wrote: > > On 2015/05/15 18:31:51, sievers wrote: > > > This collides with the same id defined above. > > > It *should* blow up at runtime, no? Same below for 109. > > > > Fixed. Although this didn't blow up at runtime... Is it because I don't have > > "features" defined? > > Mind checking with zmo@ and potentially filing a bug if that's the case? > So that we don't silently end up with duplicate ids later. OK, I will add a unittest for this.
https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... File gpu/config/gpu_driver_bug_list_json.cc (right): https://codereview.chromium.org/1129693004/diff/1/gpu/config/gpu_driver_bug_l... gpu/config/gpu_driver_bug_list_json.cc:1325: "gl_renderer": ".*330", On 2015/05/15 19:35:20, sievers wrote: > On 2015/05/15 18:45:22, David Yen wrote: > > On 2015/05/15 18:39:04, David Yen wrote: > > > On 2015/05/15 18:31:51, sievers wrote: > > > > Do you know that this works on other 3xx GPUs? > > > > In fact do you know if it works on any Qualcomm driver? > > > > > > > > There is also bug 453965, which your patch now allows us to address. (The > > > DCHECK > > > > was happening too early for the normal workaround mechanism to deal with > > it.) > > > > You said you don't see this problem on Nexus 5? > > > > > > I believe the missing function problem only happens on Nexus 4 under > lollypop > > > and later. I would rather just remove the DCHECK because that only affects > > Nexus > > > 4. The functions that are missing are ones we do not use, so adding this > > > workaround to get around the DCHECK would disable gpu timing for nexus 4 > which > > > we currently have running on perf bots under lollypop. > > > > Hmm, the missing functions for nexus 4 are not used but the moto-x functions > > listed in the bug are actually used. What GPU does the moto-x use? We should > > probably blacklist for that one as well. > > Adreno 320. Can you reference that bug also in here then? > > I was wondering if you hit that DCHECK on Nexus 4. If these two bugs affect > different devices/drivers we should probably split them up in here. This one is for the Nexus 5 (330) so I don't think that bug should be referenced here. I think we should figure out the state of 320, which driver versions it is failing for, and add another bug entry for that.
So what about the missing functions. I mean on all the devices you are testing, are they implemented by the driver anywhere? https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); On 2015/05/15 18:31:51, sievers wrote: > Why this change and make this initialization lazy now? Still wondering about this change.
Ah yes, the functions all exist for all the other devices (Nexus 5, Nexus 6). They don't have any missing functions. https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); On 2015/05/15 23:58:56, sievers wrote: > On 2015/05/15 18:31:51, sievers wrote: > > Why this change and make this initialization lazy now? > > Still wondering about this change. Sorry, I wrote about it in my first message. I had to defer the GL_Extensions filtering code because we have to be sure the GL Context is activated before we can call glGetString()...etc.
https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); On 2015/05/16 00:01:31, David Yen wrote: > On 2015/05/15 23:58:56, sievers wrote: > > On 2015/05/15 18:31:51, sievers wrote: > > > Why this change and make this initialization lazy now? > > > > Still wondering about this change. > > Sorry, I wrote about it in my first message. I had to defer the GL_Extensions > filtering code because we have > to be sure the GL Context is activated before we can call glGetString()...etc. Can you do it from InitializeCustomDynamicBindings() instead?
https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); On 2015/05/16 00:17:19, sievers wrote: > On 2015/05/16 00:01:31, David Yen wrote: > > On 2015/05/15 23:58:56, sievers wrote: > > > On 2015/05/15 18:31:51, sievers wrote: > > > > Why this change and make this initialization lazy now? > > > > > > Still wondering about this change. > > > > Sorry, I wrote about it in my first message. I had to defer the GL_Extensions > > filtering code because we have > > to be sure the GL Context is activated before we can call glGetString()...etc. > > Can you do it from InitializeCustomDynamicBindings() instead? InitializeCustomDynamicBindings() is a DriverGL function, do you mean InitializeDynamicGLBindingsGL? Could you give an example of what it would look like? Do you mean to make InitializeFilteredExtensions() some standard virtual function inside of GLApi which gets called from InitializeDynamicGLBindingsGL()?
https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); On 2015/05/18 17:08:48, David Yen wrote: > On 2015/05/16 00:17:19, sievers wrote: > > On 2015/05/16 00:01:31, David Yen wrote: > > > On 2015/05/15 23:58:56, sievers wrote: > > > > On 2015/05/15 18:31:51, sievers wrote: > > > > > Why this change and make this initialization lazy now? > > > > > > > > Still wondering about this change. > > > > > > Sorry, I wrote about it in my first message. I had to defer the > GL_Extensions > > > filtering code because we have > > > to be sure the GL Context is activated before we can call > glGetString()...etc. > > > > Can you do it from InitializeCustomDynamicBindings() instead? > > InitializeCustomDynamicBindings() is a DriverGL function, do you mean > InitializeDynamicGLBindingsGL? Could you give an example of what it would look > like? Do you mean to make InitializeFilteredExtensions() some standard virtual > function inside of GLApi which gets called from InitializeDynamicGLBindingsGL()? Can you call g_real_gl->InitializeWithContext() or whatever from InitializeDynamicGLBindingsGL()? Then it doesn't even have to be virtual. But wait, you raise a good point. What about the VirtualGLApi, how do we make that work? And since you are adding the Qualcomm workaround - on that driver we use virtual contexts. So does this only work accidentally to the extent that we have the real, and not virtual, API current when we first query extensions?
https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... File ui/gl/gl_gl_api_implementation.cc (right): https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); On 2015/05/18 18:19:32, sievers wrote: > On 2015/05/18 17:08:48, David Yen wrote: > > On 2015/05/16 00:17:19, sievers wrote: > > > On 2015/05/16 00:01:31, David Yen wrote: > > > > On 2015/05/15 23:58:56, sievers wrote: > > > > > On 2015/05/15 18:31:51, sievers wrote: > > > > > > Why this change and make this initialization lazy now? > > > > > > > > > > Still wondering about this change. > > > > > > > > Sorry, I wrote about it in my first message. I had to defer the > > GL_Extensions > > > > filtering code because we have > > > > to be sure the GL Context is activated before we can call > > glGetString()...etc. > > > > > > Can you do it from InitializeCustomDynamicBindings() instead? > > > > InitializeCustomDynamicBindings() is a DriverGL function, do you mean > > InitializeDynamicGLBindingsGL? Could you give an example of what it would look > > like? Do you mean to make InitializeFilteredExtensions() some standard virtual > > function inside of GLApi which gets called from > InitializeDynamicGLBindingsGL()? > > Can you call g_real_gl->InitializeWithContext() or whatever from > InitializeDynamicGLBindingsGL()? Then it doesn't even have to be virtual. > > But wait, you raise a good point. What about the VirtualGLApi, how do we make > that work? And since you are adding the Qualcomm workaround - on that driver we > use virtual contexts. So does this only work accidentally to the extent that we > have the real, and not virtual, API current when we first query extensions? I'll try the g_real_gl method. Virtual contexts work because upon Initialize() it calls real_context->GetExtensions() for the extensions.
On 2015/05/18 18:23:37, David Yen wrote: > https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... > File ui/gl/gl_gl_api_implementation.cc (right): > > https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... > ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); > On 2015/05/18 18:19:32, sievers wrote: > > On 2015/05/18 17:08:48, David Yen wrote: > > > On 2015/05/16 00:17:19, sievers wrote: > > > > On 2015/05/16 00:01:31, David Yen wrote: > > > > > On 2015/05/15 23:58:56, sievers wrote: > > > > > > On 2015/05/15 18:31:51, sievers wrote: > > > > > > > Why this change and make this initialization lazy now? > > > > > > > > > > > > Still wondering about this change. > > > > > > > > > > Sorry, I wrote about it in my first message. I had to defer the > > > GL_Extensions > > > > > filtering code because we have > > > > > to be sure the GL Context is activated before we can call > > > glGetString()...etc. > > > > > > > > Can you do it from InitializeCustomDynamicBindings() instead? > > > > > > InitializeCustomDynamicBindings() is a DriverGL function, do you mean > > > InitializeDynamicGLBindingsGL? Could you give an example of what it would > look > > > like? Do you mean to make InitializeFilteredExtensions() some standard > virtual > > > function inside of GLApi which gets called from > > InitializeDynamicGLBindingsGL()? > > > > Can you call g_real_gl->InitializeWithContext() or whatever from > > InitializeDynamicGLBindingsGL()? Then it doesn't even have to be virtual. > > > > But wait, you raise a good point. What about the VirtualGLApi, how do we make > > that work? And since you are adding the Qualcomm workaround - on that driver > we > > use virtual contexts. So does this only work accidentally to the extent that > we > > have the real, and not virtual, API current when we first query extensions? > > I'll try the g_real_gl method. > > Virtual contexts work because upon Initialize() it calls > real_context->GetExtensions() for the extensions. Yea, but VirtualGLApi::glGetStringFn() doesn't filter, right? So for example, when we initialize the decoder and FeatureInfo calls getString(), that's not filtered and we end up with broken workarounds.
On 2015/05/18 19:36:27, sievers wrote: > On 2015/05/18 18:23:37, David Yen wrote: > > > https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... > > File ui/gl/gl_gl_api_implementation.cc (right): > > > > > https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... > > ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); > > On 2015/05/18 18:19:32, sievers wrote: > > > On 2015/05/18 17:08:48, David Yen wrote: > > > > On 2015/05/16 00:17:19, sievers wrote: > > > > > On 2015/05/16 00:01:31, David Yen wrote: > > > > > > On 2015/05/15 23:58:56, sievers wrote: > > > > > > > On 2015/05/15 18:31:51, sievers wrote: > > > > > > > > Why this change and make this initialization lazy now? > > > > > > > > > > > > > > Still wondering about this change. > > > > > > > > > > > > Sorry, I wrote about it in my first message. I had to defer the > > > > GL_Extensions > > > > > > filtering code because we have > > > > > > to be sure the GL Context is activated before we can call > > > > glGetString()...etc. > > > > > > > > > > Can you do it from InitializeCustomDynamicBindings() instead? > > > > > > > > InitializeCustomDynamicBindings() is a DriverGL function, do you mean > > > > InitializeDynamicGLBindingsGL? Could you give an example of what it would > > look > > > > like? Do you mean to make InitializeFilteredExtensions() some standard > > virtual > > > > function inside of GLApi which gets called from > > > InitializeDynamicGLBindingsGL()? > > > > > > Can you call g_real_gl->InitializeWithContext() or whatever from > > > InitializeDynamicGLBindingsGL()? Then it doesn't even have to be virtual. > > > > > > But wait, you raise a good point. What about the VirtualGLApi, how do we > make > > > that work? And since you are adding the Qualcomm workaround - on that driver > > we > > > use virtual contexts. So does this only work accidentally to the extent that > > we > > > have the real, and not virtual, API current when we first query extensions? > > > > I'll try the g_real_gl method. > > > > Virtual contexts work because upon Initialize() it calls > > real_context->GetExtensions() for the extensions. > > Yea, but VirtualGLApi::glGetStringFn() doesn't filter, right? > So for example, when we initialize the decoder and FeatureInfo calls > getString(), that's not filtered and we end up with broken workarounds. Upon Initialize() Virtual contexts call real_context->GetExtensions() and store it in a string, then glGetStringFn() uses that string. Here is the function definition: const GLubyte* VirtualGLApi::glGetStringFn(GLenum name) { switch (name) { case GL_EXTENSIONS: return reinterpret_cast<const GLubyte*>(extensions_.c_str()); default: return driver_->fn.glGetStringFn(name); } }
On 2015/05/18 19:41:19, David Yen wrote: > On 2015/05/18 19:36:27, sievers wrote: > > On 2015/05/18 18:23:37, David Yen wrote: > > > > > > https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... > > > File ui/gl/gl_gl_api_implementation.cc (right): > > > > > > > > > https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... > > > ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); > > > On 2015/05/18 18:19:32, sievers wrote: > > > > On 2015/05/18 17:08:48, David Yen wrote: > > > > > On 2015/05/16 00:17:19, sievers wrote: > > > > > > On 2015/05/16 00:01:31, David Yen wrote: > > > > > > > On 2015/05/15 23:58:56, sievers wrote: > > > > > > > > On 2015/05/15 18:31:51, sievers wrote: > > > > > > > > > Why this change and make this initialization lazy now? > > > > > > > > > > > > > > > > Still wondering about this change. > > > > > > > > > > > > > > Sorry, I wrote about it in my first message. I had to defer the > > > > > GL_Extensions > > > > > > > filtering code because we have > > > > > > > to be sure the GL Context is activated before we can call > > > > > glGetString()...etc. > > > > > > > > > > > > Can you do it from InitializeCustomDynamicBindings() instead? > > > > > > > > > > InitializeCustomDynamicBindings() is a DriverGL function, do you mean > > > > > InitializeDynamicGLBindingsGL? Could you give an example of what it > would > > > look > > > > > like? Do you mean to make InitializeFilteredExtensions() some standard > > > virtual > > > > > function inside of GLApi which gets called from > > > > InitializeDynamicGLBindingsGL()? > > > > > > > > Can you call g_real_gl->InitializeWithContext() or whatever from > > > > InitializeDynamicGLBindingsGL()? Then it doesn't even have to be virtual. > > > > > > > > But wait, you raise a good point. What about the VirtualGLApi, how do we > > make > > > > that work? And since you are adding the Qualcomm workaround - on that > driver > > > we > > > > use virtual contexts. So does this only work accidentally to the extent > that > > > we > > > > have the real, and not virtual, API current when we first query > extensions? > > > > > > I'll try the g_real_gl method. > > > > > > Virtual contexts work because upon Initialize() it calls > > > real_context->GetExtensions() for the extensions. > > > > Yea, but VirtualGLApi::glGetStringFn() doesn't filter, right? > > So for example, when we initialize the decoder and FeatureInfo calls > > getString(), that's not filtered and we end up with broken workarounds. > > Upon Initialize() Virtual contexts call real_context->GetExtensions() and store > it in a string, then glGetStringFn() uses that string. Here is the function > definition: > > const GLubyte* VirtualGLApi::glGetStringFn(GLenum name) { > switch (name) { > case GL_EXTENSIONS: > return reinterpret_cast<const GLubyte*>(extensions_.c_str()); > default: > return driver_->fn.glGetStringFn(name); > } > }
On 2015/05/18 19:49:34, David Yen wrote: > On 2015/05/18 19:41:19, David Yen wrote: > > On 2015/05/18 19:36:27, sievers wrote: > > > On 2015/05/18 18:23:37, David Yen wrote: > > > > > > > > > > https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... > > > > File ui/gl/gl_gl_api_implementation.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/1129693004/diff/1/ui/gl/gl_gl_api_implementat... > > > > ui/gl/gl_gl_api_implementation.cc:424: InitializeFilteredExtensions(); > > > > On 2015/05/18 18:19:32, sievers wrote: > > > > > On 2015/05/18 17:08:48, David Yen wrote: > > > > > > On 2015/05/16 00:17:19, sievers wrote: > > > > > > > On 2015/05/16 00:01:31, David Yen wrote: > > > > > > > > On 2015/05/15 23:58:56, sievers wrote: > > > > > > > > > On 2015/05/15 18:31:51, sievers wrote: > > > > > > > > > > Why this change and make this initialization lazy now? > > > > > > > > > > > > > > > > > > Still wondering about this change. > > > > > > > > > > > > > > > > Sorry, I wrote about it in my first message. I had to defer the > > > > > > GL_Extensions > > > > > > > > filtering code because we have > > > > > > > > to be sure the GL Context is activated before we can call > > > > > > glGetString()...etc. > > > > > > > > > > > > > > Can you do it from InitializeCustomDynamicBindings() instead? > > > > > > > > > > > > InitializeCustomDynamicBindings() is a DriverGL function, do you mean > > > > > > InitializeDynamicGLBindingsGL? Could you give an example of what it > > would > > > > look > > > > > > like? Do you mean to make InitializeFilteredExtensions() some standard > > > > virtual > > > > > > function inside of GLApi which gets called from > > > > > InitializeDynamicGLBindingsGL()? > > > > > > > > > > Can you call g_real_gl->InitializeWithContext() or whatever from > > > > > InitializeDynamicGLBindingsGL()? Then it doesn't even have to be > virtual. > > > > > > > > > > But wait, you raise a good point. What about the VirtualGLApi, how do we > > > make > > > > > that work? And since you are adding the Qualcomm workaround - on that > > driver > > > > we > > > > > use virtual contexts. So does this only work accidentally to the extent > > that > > > > we > > > > > have the real, and not virtual, API current when we first query > > extensions? > > > > > > > > I'll try the g_real_gl method. > > > > > > > > Virtual contexts work because upon Initialize() it calls > > > > real_context->GetExtensions() for the extensions. > > > > > > Yea, but VirtualGLApi::glGetStringFn() doesn't filter, right? > > > So for example, when we initialize the decoder and FeatureInfo calls > > > getString(), that's not filtered and we end up with broken workarounds. > > > > Upon Initialize() Virtual contexts call real_context->GetExtensions() and > store > > it in a string, then glGetStringFn() uses that string. Here is the function > > definition: > > > > const GLubyte* VirtualGLApi::glGetStringFn(GLenum name) { > > switch (name) { > > case GL_EXTENSIONS: > > return reinterpret_cast<const GLubyte*>(extensions_.c_str()); > > default: > > return driver_->fn.glGetStringFn(name); > > } > > } I've added the InitializeWithContext() call and call it during initialization, PTAL.
lgtm!
The CQ bit was checked by dyen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zmo@chromium.org Link to the patchset: https://codereview.chromium.org/1129693004/#ps60001 (title: "Added InitializeWithContext() call to unit test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1129693004/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/4db835b2154821f7beefacf6866841fc52b6322a Cr-Commit-Position: refs/heads/master@{#330446}
Message was sent while issue was closed.
On 2015/05/18 23:04:34, commit-bot: I haz the power wrote: > Patchset 4 (id:??) landed as > https://crrev.com/4db835b2154821f7beefacf6866841fc52b6322a > Cr-Commit-Position: refs/heads/master@{#330446} Yes i have missing functions trying to fix. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
