Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
635543002:
cc: Make ResourceProvider use bindless Produce/ConsumeTextureCHROMIUM (Closed)
Descriptioncc: Make ResourceProvider use bindless Produce/ConsumeTextureCHROMIUM
This makes ResourceProvider use ProduceTextureDirectCHROMIUM and
CreateAndConsumeTextureCHROMIUM, and reduce some redundant gl
operations.
BUG=363782
Committed: https://crrev.com/baa78362521106b063a6fc306a8c0f4462c93d02
Cr-Commit-Position: refs/heads/master@{#319237}
Patch Set 1 #
Total comments: 2
Patch Set 2 : dont lose the tex id returned from CreateAndConsumeTex. #Patch Set 3 : tests updated. #
Total comments: 14
Patch Set 4 : review comments addressed. #Patch Set 5 : fix test fails in LTH unittest. #
Total comments: 11
Patch Set 6 : rebase + fix test expectations. #
Total comments: 2
Patch Set 7 : review comments addressed. #
Total comments: 2
Patch Set 8 : remove redundant func. #
Total comments: 8
Patch Set 9 : use MOCK method to test. #Patch Set 10 : rebase to ToT #
Total comments: 6
Patch Set 11 : remove redundant func. #
Messages
Total messages: 41 (7 generated)
sohan.jyoti@samsung.com changed reviewers: + danakj@chromium.org, vmiura@chromium.org
Is this what we are looking for ? Just wanted to check, before i update the tests ? Thanks.
https://codereview.chromium.org/635543002/diff/1/cc/resources/resource_provid... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/635543002/diff/1/cc/resources/resource_provid... cc/resources/resource_provider.cc:848: gl->CreateAndConsumeTextureCHROMIUM(resource->mailbox.target(), This returns the texture id, you're just setting it to 0 right now and throwing away the id
please take a look, thanks. https://codereview.chromium.org/635543002/diff/1/cc/resources/resource_provid... File cc/resources/resource_provider.cc (right): https://codereview.chromium.org/635543002/diff/1/cc/resources/resource_provid... cc/resources/resource_provider.cc:848: gl->CreateAndConsumeTextureCHROMIUM(resource->mailbox.target(), On 2014/10/06 15:25:28, danakj wrote: > This returns the texture id, you're just setting it to 0 right now and throwing > away the id Done.
This looks good, you said there will be test changes?
On 2014/10/07 16:00:48, danakj wrote: > This looks good, you said there will be test changes? seems its not too straight-forward, we have to add non bound texture class/api's to test web graphics context to make resource provider tests use bindless Produce/ConsumeTextureCHROMIUM, i'll update them soon.
Can you please take a look at the tests, i had to let go of some GetPixel code, as we are working with unbound texture, please check the approach. thanks.
Sorry I'm not really sure what's going on in these test changes. You're adding new GL API looking methods to the context, which doesn't make sense. The target seems to be getting dropped along the way, should we be testing that somehow? https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... File cc/resources/resource_provider_unittest.cc (left): https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:197: CheckTextureIsBound(target); why is this removed? this function is not bindless? https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:222: CheckTextureIsBound(target); same https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:242: CheckTextureIsBound(target); same https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:275: base::AutoLock lock_for_texture_access(namespace_->lock); you need to lock if you're going to access the namespace_->textures https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:284: CheckTextureIsBound(GL_TEXTURE_2D); same https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:226: AllocateTexture(gfx::Size(width, height), format); why don't you make this function get the texture and pass it to Allocate and SetPixels if you need them in the API? You don't need to redefine those functions to do that? https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:231: virtual void texImage2DEXT(GLenum target, this isn't a member of TestWGC3D, we shouldn't add new GL apis that don't exist
Sorry for the tests being not too clear. My intentions were to, 1. Change all tests to use produceTextureDirectCHROMIUM instead of produceTextureCHROMIUM, and avoid the associated Bind. 2. Change all tests to use createAndConsumeTextureCHROMIUM instead of consumeTextureCHROMIUM, and avoid the separate texture creation. But, the tests were all working on bound textures and related TextureTargets etc. So testing for unbound textures have made it cumbersome. Please advice if you have some suggestion. And regarding testing the target, as we are trying to test with unbound textures, i am not sure how we can maintain the target :\ please suggest. thanks. https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... File cc/resources/resource_provider_unittest.cc (left): https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:197: CheckTextureIsBound(target); On 2014/10/15 16:46:19, danakj wrote: > why is this removed? this function is not bindless? Done. Yea, i had just removed all occurrences of CheckTextureIsBound in the file, w/o really going through them. sorry! https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:222: CheckTextureIsBound(target); On 2014/10/15 16:46:19, danakj wrote: > same Done. https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:242: CheckTextureIsBound(target); On 2014/10/15 16:46:19, danakj wrote: > same Done. https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:275: base::AutoLock lock_for_texture_access(namespace_->lock); On 2014/10/15 16:46:19, danakj wrote: > you need to lock if you're going to access the namespace_->textures Done. https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:284: CheckTextureIsBound(GL_TEXTURE_2D); On 2014/10/15 16:46:19, danakj wrote: > same Done. https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:226: AllocateTexture(gfx::Size(width, height), format); On 2014/10/15 16:46:19, danakj wrote: > why don't you make this function get the texture and pass it to Allocate and > SetPixels if you need them in the API? You don't need to redefine those > functions to do that? Acknowledged. I was trying to remove the bind call before calling texImage2D in TransferMailboxResources test hence these changes, but calling texImage2D on unbound texture is not meaningful, so it wont be required. https://codereview.chromium.org/635543002/diff/50001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:231: virtual void texImage2DEXT(GLenum target, On 2014/10/15 16:46:19, danakj wrote: > this isn't a member of TestWGC3D, we shouldn't add new GL apis that don't exist Acknowledged. Removed.
Can you please take a look at this change, and comments above, and provide your feedback. thanks. https://codereview.chromium.org/635543002/diff/90001/cc/trees/layer_tree_host... File cc/trees/layer_tree_host_unittest.cc (left): https://codereview.chromium.org/635543002/diff/90001/cc/trees/layer_tree_host... cc/trees/layer_tree_host_unittest.cc:1268: context->ResetUsedTextures(); This reset needs to be avoided as used texture is inserted when bindTexture happens, as we are avoiding the bind, we need to avoid this reset, otherwise in DrawLayersOnThread we will have 0 used textures.
On 2014/11/05 12:46:43, sohanjg wrote: > Can you please take a look at this change, and comments above, and provide your > feedback. thanks. > > https://codereview.chromium.org/635543002/diff/90001/cc/trees/layer_tree_host... > File cc/trees/layer_tree_host_unittest.cc (left): > > https://codereview.chromium.org/635543002/diff/90001/cc/trees/layer_tree_host... > cc/trees/layer_tree_host_unittest.cc:1268: context->ResetUsedTextures(); > This reset needs to be avoided as used texture is inserted when bindTexture > happens, as we are avoiding the bind, we need to avoid this reset, otherwise in > DrawLayersOnThread we will have 0 used textures. ping :) has this lost context ? should we plan to drop this CL?
https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... File cc/resources/resource_provider_unittest.cc (left): https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:722: GetResourcePixels( We should not remove test expectations https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:1866: context()->GetPixels( We should not remove test expectations. https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:1917: context()->GetPixels( ditto... https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:279: namespace_->textures.Replace(texture_id, UnboundTexture(texture_id)); We still need to do what ConsumeTexture was doing. https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:340: void SetPixelsEXT(int xoffset, Nothing uses this
Patchset #6 (id:110001) has been deleted
Patchset #6 (id:130001) has been deleted
Please take a look, thanks! https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... File cc/resources/resource_provider_unittest.cc (left): https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:722: GetResourcePixels( On 2014/12/01 16:20:05, danakj wrote: > We should not remove test expectations Done. https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:1866: context()->GetPixels( On 2014/12/01 16:20:04, danakj wrote: > We should not remove test expectations. Done. https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:1917: context()->GetPixels( On 2014/12/01 16:20:05, danakj wrote: > ditto... Done. https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:279: namespace_->textures.Replace(texture_id, UnboundTexture(texture_id)); On 2014/12/01 16:20:04, danakj wrote: > We still need to do what ConsumeTexture was doing. Done. https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... cc/resources/resource_provider_unittest.cc:340: void SetPixelsEXT(int xoffset, On 2014/12/01 16:20:05, danakj wrote: > Nothing uses this Done.
On 2014/12/30 14:45:55, sohanjg wrote: > Please take a look, thanks! > > https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... > File cc/resources/resource_provider_unittest.cc (left): > > https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... > cc/resources/resource_provider_unittest.cc:722: GetResourcePixels( > On 2014/12/01 16:20:05, danakj wrote: > > We should not remove test expectations > > Done. > > https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... > cc/resources/resource_provider_unittest.cc:1866: context()->GetPixels( > On 2014/12/01 16:20:04, danakj wrote: > > We should not remove test expectations. > > Done. > > https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... > cc/resources/resource_provider_unittest.cc:1917: context()->GetPixels( > On 2014/12/01 16:20:05, danakj wrote: > > ditto... > > Done. > > https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... > File cc/resources/resource_provider_unittest.cc (right): > > https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... > cc/resources/resource_provider_unittest.cc:279: > namespace_->textures.Replace(texture_id, UnboundTexture(texture_id)); > On 2014/12/01 16:20:04, danakj wrote: > > We still need to do what ConsumeTexture was doing. > > Done. > > https://codereview.chromium.org/635543002/diff/90001/cc/resources/resource_pr... > cc/resources/resource_provider_unittest.cc:340: void SetPixelsEXT(int xoffset, > On 2014/12/01 16:20:05, danakj wrote: > > Nothing uses this > > Done. can you have a look at this when time permits :)
https://codereview.chromium.org/635543002/diff/150001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/150001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:282: void consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { Why do we need to remove the prior code? i.e. CheckTextureIsBound(target); ... namespace_->textures.Replace(BoundTextureId(target), texture);
https://codereview.chromium.org/635543002/diff/150001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/150001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:282: void consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { On 2015/02/04 06:29:52, vmiura wrote: > Why do we need to remove the prior code? > > i.e. > > CheckTextureIsBound(target); > ... > namespace_->textures.Replace(BoundTextureId(target), texture); Sorry for being late. no we dont need that. We wont need the Bound texture check for createAndConsumeTextureCHROMIUM as we avoid the extra binds, somehow it creep-ed in here too.
On 2015/02/05 09:50:33, sohanjg wrote: > https://codereview.chromium.org/635543002/diff/150001/cc/resources/resource_p... > File cc/resources/resource_provider_unittest.cc (right): > > https://codereview.chromium.org/635543002/diff/150001/cc/resources/resource_p... > cc/resources/resource_provider_unittest.cc:282: void > consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { > On 2015/02/04 06:29:52, vmiura wrote: > > Why do we need to remove the prior code? > > > > i.e. > > > > CheckTextureIsBound(target); > > ... > > namespace_->textures.Replace(BoundTextureId(target), texture); > > Sorry for being late. no we dont need that. > We wont need the Bound texture check for createAndConsumeTextureCHROMIUM as we > avoid the extra binds, somehow it creep-ed in here too. But "consumeTextureCHROMIUM" would need the texture to be bound would it not?
On 2015/02/06 06:29:53, vmiura wrote: > On 2015/02/05 09:50:33, sohanjg wrote: > > > https://codereview.chromium.org/635543002/diff/150001/cc/resources/resource_p... > > File cc/resources/resource_provider_unittest.cc (right): > > > > > https://codereview.chromium.org/635543002/diff/150001/cc/resources/resource_p... > > cc/resources/resource_provider_unittest.cc:282: void > > consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { > > On 2015/02/04 06:29:52, vmiura wrote: > > > Why do we need to remove the prior code? > > > > > > i.e. > > > > > > CheckTextureIsBound(target); > > > ... > > > namespace_->textures.Replace(BoundTextureId(target), texture); > > > > Sorry for being late. no we dont need that. > > We wont need the Bound texture check for createAndConsumeTextureCHROMIUM as we > > avoid the extra binds, somehow it creep-ed in here too. > > But "consumeTextureCHROMIUM" would need the texture to be bound would it not? Yes, you are right, consumeTextureCHROMIUM would need texture to be bound, i have updated the patch-set.
LGTM, but danakj@ needs to approve.
https://codereview.chromium.org/635543002/diff/170001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/170001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:304: unsigned id) { What is the point of this method? |id| is not used in it.
New patchsets have been uploaded after l-g-t-m from vmiura@chromium.org
Thanks for the input. PTAL. https://codereview.chromium.org/635543002/diff/170001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/170001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:304: unsigned id) { On 2015/02/12 21:37:07, danakj wrote: > What is the point of this method? |id| is not used in it. Done. Yeah, this func is no longer required, was needed in previous patch-set. thanks.
https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (left): https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:102: MOCK_METHOD2(consumeTextureCHROMIUM, dont we want consume direct here so we can test that the function is being called still? https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:1739: EXPECT_CALL(*parent_context, consumeTextureCHROMIUM(GL_TEXTURE_2D, _)); shouldnt we be getting a consumetexturedirectchromium here? https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:282: void consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { no longer used?
https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (left): https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:102: MOCK_METHOD2(consumeTextureCHROMIUM, On 2015/02/13 18:12:22, danakj wrote: > dont we want consume direct here so we can test that the function is being > called still? Hmm..the thing is we have 2 implementation of createAndConsumeTextureCHROMIUM, in TestWebGraphicsContext3D and ResourceProviderContext. this is required because, it is expected to return the tex id, and hence cannot be kept as empty virtual like consumeTextureCHROMIUM in TestWebGraphicsContext3D . I had tried removing the impl in TestWebGraphicsContext3D, but it seems some gl_renderer_unittests (OutputSurfaceMockContext) needs it to be there, otherwise it fails linking. For most of the tests here the TestWebGraphicsContext3D version is invoked, and some uses the ResourceProviderContext implementation. If we add it as a MOCK here, then from TestGLES2Interface, it is unable to decide which implementation to use, and it fails. https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:1739: EXPECT_CALL(*parent_context, consumeTextureCHROMIUM(GL_TEXTURE_2D, _)); On 2015/02/13 18:12:22, danakj wrote: > shouldnt we be getting a consumetexturedirectchromium here? We have to make it MOCK method then, and there seems to be some issues with that as mentioned above. https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:282: void consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { On 2015/02/13 18:12:22, danakj wrote: > no longer used? it is still used internally by TestWebGraphicsContext3D and ResourceProviderContext implementation of createAndConsumeTextureCHROMIUM.
https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (left): https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:102: MOCK_METHOD2(consumeTextureCHROMIUM, On 2015/02/16 13:39:56, sohanjg wrote: > On 2015/02/13 18:12:22, danakj wrote: > > dont we want consume direct here so we can test that the function is being > > called still? > > Hmm..the thing is we have 2 implementation of createAndConsumeTextureCHROMIUM, > in TestWebGraphicsContext3D and ResourceProviderContext. This would override the one from TestWebGraphicsContext3D, the same way insertSyncPoint does. ResourceProvderContext does not inherit from this class, so that seems unrelated here? > this is required because, it is expected to return the tex id, and hence cannot > be kept as empty virtual like consumeTextureCHROMIUM in TestWebGraphicsContext3D > . Why does it need to be an empty virtual? Should it not be declared as GLuint(GLenum target, const GLbyte* mailbox) in the mock here? Are you trying to use a void return value for it still so it doesn't match what it's overriding? > I had tried removing the impl in TestWebGraphicsContext3D, but it seems some > gl_renderer_unittests (OutputSurfaceMockContext) needs it to be there, otherwise > it fails linking. > > > For most of the tests here the TestWebGraphicsContext3D version is invoked, and > some uses the ResourceProviderContext implementation. > > If we add it as a MOCK here, then from TestGLES2Interface, it is unable to > decide which implementation to use, and it fails.
https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (left): https://codereview.chromium.org/635543002/diff/190001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:102: MOCK_METHOD2(consumeTextureCHROMIUM, On 2015/02/18 18:37:28, danakj wrote: > On 2015/02/16 13:39:56, sohanjg wrote: > > On 2015/02/13 18:12:22, danakj wrote: > > > dont we want consume direct here so we can test that the function is being > > > called still? > > > > Hmm..the thing is we have 2 implementation of createAndConsumeTextureCHROMIUM, > > in TestWebGraphicsContext3D and ResourceProviderContext. > > This would override the one from TestWebGraphicsContext3D, the same way > insertSyncPoint does. > > ResourceProvderContext does not inherit from this class, so that seems unrelated > here? > > > this is required because, it is expected to return the tex id, and hence > cannot > > be kept as empty virtual like consumeTextureCHROMIUM in > TestWebGraphicsContext3D > > . > > Why does it need to be an empty virtual? Should it not be declared as > GLuint(GLenum target, const GLbyte* mailbox) in the mock here? Are you trying to > use a void return value for it still so it doesn't match what it's overriding? > > > I had tried removing the impl in TestWebGraphicsContext3D, but it seems some > > gl_renderer_unittests (OutputSurfaceMockContext) needs it to be there, > otherwise > > it fails linking. > > > > > > For most of the tests here the TestWebGraphicsContext3D version is invoked, > and > > some uses the ResourceProviderContext implementation. > > > > If we add it as a MOCK here, then from TestGLES2Interface, it is unable to > > decide which implementation to use, and it fails. > Sorry for not being clear. What i meant was, If we use MOCK method here we will have some failures. Eg. In class ResourceProviderTestTextureFilters we are creating context of type TextureStateTrackingContext (line:1648) As a result, from line:1743, when we invoke ScopedReadLockGL, it invokes LockForRead, which calls CreateAndConsumeTextureCHROMIUM, it ends up in the MOCK method (from TestGLES2Interface), and we fail the DCHECK in ScopedReadLockGL where it expects non-zero texture id. https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/resou... If we dont have the MOCK method here, then TestWebGraphicsContext3D::CreateAndConsumeTextureCHROMIUM is invoked. Same for, ResourceProviderTestTextureMailboxGLFilters class and TextureMailbox_GLTextureExternalOES test which uses TextureStateTrackingContext.
Some googling got me here. Its what you're looking for? https://code.google.com/p/googlemock/wiki/ForDummies#Actions:_What_Should_It_Do ? On Feb 19, 2015 6:54 AM, <sohan.jyoti@samsung.com> wrote: > > https://codereview.chromium.org/635543002/diff/190001/cc/ > resources/resource_provider_unittest.cc > File cc/resources/resource_provider_unittest.cc (left): > > https://codereview.chromium.org/635543002/diff/190001/cc/ > resources/resource_provider_unittest.cc#oldcode102 > cc/resources/resource_provider_unittest.cc:102: > MOCK_METHOD2(consumeTextureCHROMIUM, > On 2015/02/18 18:37:28, danakj wrote: > >> On 2015/02/16 13:39:56, sohanjg wrote: >> > On 2015/02/13 18:12:22, danakj wrote: >> > > dont we want consume direct here so we can test that the function >> > is being > >> > > called still? >> > >> > Hmm..the thing is we have 2 implementation of >> > createAndConsumeTextureCHROMIUM, > >> > in TestWebGraphicsContext3D and ResourceProviderContext. >> > > This would override the one from TestWebGraphicsContext3D, the same >> > way > >> insertSyncPoint does. >> > > ResourceProvderContext does not inherit from this class, so that seems >> > unrelated > >> here? >> > > > this is required because, it is expected to return the tex id, and >> > hence > >> cannot >> > be kept as empty virtual like consumeTextureCHROMIUM in >> TestWebGraphicsContext3D >> > . >> > > Why does it need to be an empty virtual? Should it not be declared as >> GLuint(GLenum target, const GLbyte* mailbox) in the mock here? Are you >> > trying to > >> use a void return value for it still so it doesn't match what it's >> > overriding? > > > I had tried removing the impl in TestWebGraphicsContext3D, but it >> > seems some > >> > gl_renderer_unittests (OutputSurfaceMockContext) needs it to be >> > there, > >> otherwise >> > it fails linking. >> > >> > >> > For most of the tests here the TestWebGraphicsContext3D version is >> > invoked, > >> and >> > some uses the ResourceProviderContext implementation. >> > >> > If we add it as a MOCK here, then from TestGLES2Interface, it is >> > unable to > >> > decide which implementation to use, and it fails. >> > > > Sorry for not being clear. > > What i meant was, If we use MOCK method here we will have some failures. > Eg. In class ResourceProviderTestTextureFilters we are creating context > of type TextureStateTrackingContext (line:1648) > As a result, from line:1743, when we invoke ScopedReadLockGL, it invokes > LockForRead, which calls CreateAndConsumeTextureCHROMIUM, it ends up in > the MOCK method (from TestGLES2Interface), and we fail the DCHECK in > ScopedReadLockGL where it expects non-zero texture id. > https://code.google.com/p/chromium/codesearch#chromium/ > src/cc/resources/resource_provider.cc&q=resource_provider.cc&sq=package: > chromium&type=cs&l=949 > > If we dont have the MOCK method here, then > TestWebGraphicsContext3D::CreateAndConsumeTextureCHROMIUM is invoked. > > Same for, ResourceProviderTestTextureMailboxGLFilters class and > TextureMailbox_GLTextureExternalOES test which uses > TextureStateTrackingContext. > > https://codereview.chromium.org/635543002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Can you please take a look, thanks for your patience on this. :) https://codereview.chromium.org/635543002/diff/230001/cc/test/ordered_texture... File cc/test/ordered_texture_map.cc (right): https://codereview.chromium.org/635543002/diff/230001/cc/test/ordered_texture... cc/test/ordered_texture_map.cc:36: // nothing to remove in that case. as we are returning dummy tex id from MOCK CreateandConsumeTexture.. we dont have the entry in texture map. Will this be ok ?
LGTM w/ 1 question.. https://codereview.chromium.org/635543002/diff/230001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/230001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:284: void consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { You said this is called from inside createAndConsumeTextureCHROMIUM? I don't see how. shared_data_->ConsumeTexture() does not call it. namespace_->textures.Replace() does not call it. Where is it being used in these tests? What fails if you delete it? https://codereview.chromium.org/635543002/diff/230001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:2688: EXPECT_CALL(*context, bindTexture(target, texture_id)); can you move this below the EXPECT_CALL(createAndConsumeTextureCHROMIUM)? that reflects how it'll be ordered in the resource provider right?
https://codereview.chromium.org/635543002/diff/230001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/230001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:284: void consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { On 2015/03/03 19:27:30, danakj wrote: > You said this is called from inside createAndConsumeTextureCHROMIUM? I don't see > how. shared_data_->ConsumeTexture() does not call it. > namespace_->textures.Replace() does not call it. Where is it being used in these > tests? What fails if you delete it? createAndConsumeTextureCHROMIUM, of TestWebGraphicsContext3D was using it. But, after we have made it MOCK and handling it in TextureStateTrackingContext, that will no longer be used in ResourceProviderTests. For other tests (GLRenderer, DeleGatedFrame etc) which use createAndConsumeTextureCHROMIUM of TestWebGraphicsContext3D, they are not expecting a Consumetexture, and is happy with only a created texture. I think we can remove it here, and make TestWebGraphicsContext3D::createAndConsumeTextureCHROMIUM only create a texture and return if thats what we want? Please let me know your opinion ?
On Tue, Mar 3, 2015 at 9:49 PM, <sohan.jyoti@samsung.com> wrote: > > https://codereview.chromium.org/635543002/diff/230001/cc/ > resources/resource_provider_unittest.cc > File cc/resources/resource_provider_unittest.cc (right): > > https://codereview.chromium.org/635543002/diff/230001/cc/ > resources/resource_provider_unittest.cc#newcode284 > cc/resources/resource_provider_unittest.cc:284: void > consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { > On 2015/03/03 19:27:30, danakj wrote: > >> You said this is called from inside createAndConsumeTextureCHROMIUM? I >> > don't see > >> how. shared_data_->ConsumeTexture() does not call it. >> namespace_->textures.Replace() does not call it. Where is it being >> > used in these > >> tests? What fails if you delete it? >> > > createAndConsumeTextureCHROMIUM, of TestWebGraphicsContext3D was using > it. But, after we have made it MOCK and handling it in > TextureStateTrackingContext, that will no longer be used in > ResourceProviderTests. > > For other tests (GLRenderer, DeleGatedFrame etc) which use > createAndConsumeTextureCHROMIUM of TestWebGraphicsContext3D, they are > not expecting a Consumetexture, and is happy with only a created > texture. > I think we can remove it here, and make > TestWebGraphicsContext3D::createAndConsumeTextureCHROMIUM only create a > texture and return if thats what we want? > Please let me know your opinion ? > Tests outside of this file can't see this subclass so they can't possibly be using it. I think it is dead code so I'd just like to remove it if that is the case. Why do we need to change TestWGC3D to do so? What would break from just removing it? > > https://codereview.chromium.org/635543002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/03/04 16:41:26, danakj wrote: > On Tue, Mar 3, 2015 at 9:49 PM, <mailto:sohan.jyoti@samsung.com> wrote: > > > > > https://codereview.chromium.org/635543002/diff/230001/cc/ > > resources/resource_provider_unittest.cc > > File cc/resources/resource_provider_unittest.cc (right): > > > > https://codereview.chromium.org/635543002/diff/230001/cc/ > > resources/resource_provider_unittest.cc#newcode284 > > cc/resources/resource_provider_unittest.cc:284: void > > consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { > > On 2015/03/03 19:27:30, danakj wrote: > > > >> You said this is called from inside createAndConsumeTextureCHROMIUM? I > >> > > don't see > > > >> how. shared_data_->ConsumeTexture() does not call it. > >> namespace_->textures.Replace() does not call it. Where is it being > >> > > used in these > > > >> tests? What fails if you delete it? > >> > > > > createAndConsumeTextureCHROMIUM, of TestWebGraphicsContext3D was using > > it. But, after we have made it MOCK and handling it in > > TextureStateTrackingContext, that will no longer be used in > > ResourceProviderTests. > > > > For other tests (GLRenderer, DeleGatedFrame etc) which use > > createAndConsumeTextureCHROMIUM of TestWebGraphicsContext3D, they are > > not expecting a Consumetexture, and is happy with only a created > > texture. > > I think we can remove it here, and make > > TestWebGraphicsContext3D::createAndConsumeTextureCHROMIUM only create a > > texture and return if thats what we want? > > Please let me know your opinion ? > > > > Tests outside of this file can't see this subclass so they can't possibly > be using it. I think it is dead code so I'd just like to remove it if that > is the case. Why do we need to change TestWGC3D to do so? What would break > from just removing it? > > > > > > https://codereview.chromium.org/635543002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Nothing would break, but as part of this CL we had added Consumetexture code in TestWGC3D::Createandconsumetexture, that ConsumeTexture would be a no-op. I'll remove this func.
Thanks. https://codereview.chromium.org/635543002/diff/230001/cc/resources/resource_p... File cc/resources/resource_provider_unittest.cc (right): https://codereview.chromium.org/635543002/diff/230001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:284: void consumeTextureCHROMIUM(GLenum target, const GLbyte* mailbox) override { On 2015/03/04 05:49:50, sohanjg wrote: > On 2015/03/03 19:27:30, danakj wrote: > > You said this is called from inside createAndConsumeTextureCHROMIUM? I don't > see > > how. shared_data_->ConsumeTexture() does not call it. > > namespace_->textures.Replace() does not call it. Where is it being used in > these > > tests? What fails if you delete it? > > createAndConsumeTextureCHROMIUM, of TestWebGraphicsContext3D was using it. But, > after we have made it MOCK and handling it in TextureStateTrackingContext, that > will no longer be used in ResourceProviderTests. > > For other tests (GLRenderer, DeleGatedFrame etc) which use > createAndConsumeTextureCHROMIUM of TestWebGraphicsContext3D, they are not > expecting a Consumetexture, and is happy with only a created texture. > I think we can remove it here, and make > TestWebGraphicsContext3D::createAndConsumeTextureCHROMIUM only create a texture > and return if thats what we want? > Please let me know your opinion ? Removed the func. https://codereview.chromium.org/635543002/diff/230001/cc/resources/resource_p... cc/resources/resource_provider_unittest.cc:2688: EXPECT_CALL(*context, bindTexture(target, texture_id)); On 2015/03/03 19:27:30, danakj wrote: > can you move this below the EXPECT_CALL(createAndConsumeTextureCHROMIUM)? that > reflects how it'll be ordered in the resource provider right? Done.
The CQ bit was checked by sohan.jyoti@samsung.com
The patchset sent to the CQ was uploaded after l-g-t-m from vmiura@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/635543002/#ps250001 (title: "remove redundant func.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/635543002/250001
Message was sent while issue was closed.
Committed patchset #11 (id:250001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/baa78362521106b063a6fc306a8c0f4462c93d02 Cr-Commit-Position: refs/heads/master@{#319237}
Message was sent while issue was closed.
Description was changed from ========== cc: Make ResourceProvider use bindless Produce/ConsumeTextureCHROMIUM This makes ResourceProvider use ProduceTextureDirectCHROMIUM and CreateAndConsumeTextureCHROMIUM, and reduce some redundant gl operations. BUG=363782 Committed: https://crrev.com/baa78362521106b063a6fc306a8c0f4462c93d02 Cr-Commit-Position: refs/heads/master@{#319237} ========== to ========== cc: Make ResourceProvider use bindless Produce/ConsumeTextureCHROMIUM This makes ResourceProvider use ProduceTextureDirectCHROMIUM and CreateAndConsumeTextureCHROMIUM, and reduce some redundant gl operations. BUG=363782 Committed: https://crrev.com/baa78362521106b063a6fc306a8c0f4462c93d02 Cr-Commit-Position: refs/heads/master@{#319237} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
