Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
668123003:
cc: Support texture rect targets for masks (Closed)
|
Created:
6 years, 2 months ago by enne (OOO) Modified:
6 years, 1 month ago CC:
cc-bugs_chromium.org, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@base_for_texture_rect_changes Project:
chromium Visibility:
Public. |
Descriptioncc: Support texture rect targets for masks
Currently, GLRenderer supports tile resources as either texture 2d or
texture rectangle. Before this patch, it only supported texture 2d
masks. This fills the gap to handle texture rectangle masks. In
practice it means that using a zero copy raster worker pool will
now not break masks. This in turn enables impl-side painting for
layout tests.
Unfortunately, there is a lot of indirection between a test saying it
wants to test a particular target type and what needs to happen. To
make sure that tests are testing the right thing, the creation of
the raster worker and resource pools is now virtual so that tests
can create them as explicitly as needed.
R=danakj@chromium.org,reveman@chromium.org
BUG=423533
Committed: https://crrev.com/03dbe8ae71dbf674627acad91fff64a2fa9d4eb6
Cr-Commit-Position: refs/heads/master@{#302010}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Rebase on blend mode patch #Patch Set 3 : virtual CreateResourceAndRasterWorkerPool #
Total comments: 28
Patch Set 4 : Rebase #Patch Set 5 : reveman comments #
Total comments: 2
Patch Set 6 : Add a NOTREACHED #Patch Set 7 : break => return #
Total comments: 6
Patch Set 8 : Rebase #Patch Set 9 : Rebase #
Messages
Total messages: 30 (3 generated)
For correctness, this depends on: https://codereview.chromium.org/656263003 https://codereview.chromium.org/659883003 https://codereview.chromium.org/665463003 It will also need to get rebased on https://codereview.chromium.org/658483003/ when that lands, but it should be mostly mechanical changes around shader creation, cleanup, and testing.
I don't think texture rectangle target is enough. We need external oes target too. https://codereview.chromium.org/668123003/diff/1/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/output/gl_renderer.cc#new... cc/output/gl_renderer.cc:1373: if (mask_sampler == SamplerType2DRect) { sampler != SamplerType2DRect? or how is GL_TEXTURE_EXTERNAL_OES otherwise handled correctly? https://codereview.chromium.org/668123003/diff/1/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/output/shader.cc#newcode1125 cc/output/shader.cc:1125: uniform sampler2D s_texture; Is the mask shader always used with a render surface as s_texture input and that's why this can be hard coded to sampler2D? https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... cc/trees/layer_tree_host_impl.cc:82: LayerTreeSettings::TextureTargetPrefs prefs) { I find this utility function a bit confusing. Based on the name, I'd expect it to be passed a target and the ContextProvider::Capabilities and return true if target is supported. https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... cc/trees/layer_tree_host_impl.cc:2055: LayerTreeSettings::FORCE_TEXTURE_2D_TARGET) I don't think this should be a special case for zero-copy. It applies to one-copy too and possibly to Gpu raster in the future. It would be nice if we could simply pass settings_.raster_texture_target directly to each of these functions and maybe just have a check above all this if-else stuff to make sure target is supported. https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_pix... File cc/trees/layer_tree_host_pixeltest_masks.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_pix... cc/trees/layer_tree_host_pixeltest_masks.cc:32: LayerTreeSettings::FORCE_TEXTURE_RECT_TARGET)); We should be testing EXTERNAL_OES target too. https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings.h File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings... cc/trees/layer_tree_settings.h:85: FORCE_TEXTURE_RECT_TARGET, I think TEXTURE_EXTERNAL_OES should be part of this too. https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings... cc/trees/layer_tree_settings.h:87: TextureTargetPrefs texture_target_prefs; I'd prefer if this was not a preference or override but our real texture target selection mechanism and the code to set it replaces any logic we currently have to determine what target to use. This requires having access to ContextProvider::Capabilities at the time we initialize this setting so this suggestion might be misguided if that's not the case.
https://codereview.chromium.org/668123003/diff/1/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/output/gl_renderer.cc#new... cc/output/gl_renderer.cc:1373: if (mask_sampler == SamplerType2DRect) { On 2014/10/22 12:40:38, reveman wrote: > sampler != SamplerType2DRect? or how is GL_TEXTURE_EXTERNAL_OES otherwise > handled correctly? Ack. Thanks for pointing out that I had not thought about GL_TEXTURAL_EXTERNAL_OES at all. Where is that supported? https://codereview.chromium.org/668123003/diff/1/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/output/shader.cc#newcode1125 cc/output/shader.cc:1125: uniform sampler2D s_texture; On 2014/10/22 12:40:38, reveman wrote: > Is the mask shader always used with a render surface as s_texture input and > that's why this can be hard coded to sampler2D? Yeah, that's exactly it. Currently, masks only apply to render surfaces and so they're always sampler2D. I could generalize this, but I don't really see this changing any time soon. Is your worry that this assumption about sampler2D isn't correct? Or, is it that it isn't made clear enough? https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... cc/trees/layer_tree_host_impl.cc:2055: LayerTreeSettings::FORCE_TEXTURE_2D_TARGET) On 2014/10/22 12:40:38, reveman wrote: > I don't think this should be a special case for zero-copy. It applies to > one-copy too and possibly to Gpu raster in the future. It would be nice if we > could simply pass settings_.raster_texture_target directly to each of these > functions and maybe just have a check above all this if-else stuff to make sure > target is supported. Maybe I'm mistaken, but I don't think it applies to one copy. The staging resource is GetMapImageTextureTarget, but the final resource is GL_TEXTURE_2D? So, the GLRenderer code that runs will use GL_TEXTURE_2D, right? https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings.h File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings... cc/trees/layer_tree_settings.h:87: TextureTargetPrefs texture_target_prefs; On 2014/10/22 12:40:39, reveman wrote: > I'd prefer if this was not a preference or override but our real texture target > selection mechanism and the code to set it replaces any logic we currently have > to determine what target to use. This requires having access to > ContextProvider::Capabilities at the time we initialize this setting so this > suggestion might be misguided if that's not the case. The problem here is conflicting requirements. Production code wants to be flexible in the face of different capabilities. Test code wants to be strict to make sure that it's actually testing what it's testing and these preferences aren't flexibly ignored. How do you combine those two needs? Your solution helps with the flexiblity, but I doesn't meet the testing need.
https://codereview.chromium.org/668123003/diff/1/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/output/gl_renderer.cc#new... cc/output/gl_renderer.cc:1373: if (mask_sampler == SamplerType2DRect) { On 2014/10/22 19:20:44, enne wrote: > On 2014/10/22 12:40:38, reveman wrote: > > sampler != SamplerType2DRect? or how is GL_TEXTURE_EXTERNAL_OES otherwise > > handled correctly? > > Ack. Thanks for pointing out that I had not thought about > GL_TEXTURAL_EXTERNAL_OES at all. Where is that supported? It's used on Android. It's the only supported target for SurfaceTextures. It's possible that future zero-copy extensions might have similar limitations. FYI, this target restricts what you can do with the texture significantly, which makes extensions like this much easier to implement as you don't have to support the whole universe of existing texture use cases.. https://codereview.chromium.org/668123003/diff/1/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/output/shader.cc#newcode1125 cc/output/shader.cc:1125: uniform sampler2D s_texture; On 2014/10/22 19:20:44, enne wrote: > On 2014/10/22 12:40:38, reveman wrote: > > Is the mask shader always used with a render surface as s_texture input and > > that's why this can be hard coded to sampler2D? > > Yeah, that's exactly it. Currently, masks only apply to render surfaces and so > they're always sampler2D. > > I could generalize this, but I don't really see this changing any time soon. > > Is your worry that this assumption about sampler2D isn't correct? Or, is it that > it isn't made clear enough? I just wanted to make sure I understood this change correctly. Pretty sure I'm the one to blame for the incorrect SamplerType/TextureLookup usage here. Thanks for fixing it! It would be nice to explicitly pass sampler and precision parameters to the FRAGMENT_SHADER macro instead of assuming variables named |precision| and |sampler| exists. That would allow us to change the name of the "sampler" parameter above to "mask_sampler" and I think that would be enough to remove any confusion here. That could be done as a follow up though. Maybe just add a TODO here for now. https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... cc/trees/layer_tree_host_impl.cc:2055: LayerTreeSettings::FORCE_TEXTURE_2D_TARGET) On 2014/10/22 19:20:44, enne wrote: > On 2014/10/22 12:40:38, reveman wrote: > > I don't think this should be a special case for zero-copy. It applies to > > one-copy too and possibly to Gpu raster in the future. It would be nice if we > > could simply pass settings_.raster_texture_target directly to each of these > > functions and maybe just have a check above all this if-else stuff to make > sure > > target is supported. > > Maybe I'm mistaken, but I don't think it applies to one copy. The staging > resource is GetMapImageTextureTarget, but the final resource is GL_TEXTURE_2D? > So, the GLRenderer code that runs will use GL_TEXTURE_2D, right? Correct, GLRenderer is only affected by the target used for drawing. It's the addition of a new way of configuring this without removing an old that confuses me. We need to select the raster texture target based on context capabilities, that's clear. And based on this patch I assume that we need to be able to force raster texture target for testing purposes too, right? Instead of having to fuse both of these selection mechanism here is there some way we can have one replace or override the other so that we only have to deal with one setting or capability here? https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings.h File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings... cc/trees/layer_tree_settings.h:87: TextureTargetPrefs texture_target_prefs; On 2014/10/22 19:20:44, enne wrote: > On 2014/10/22 12:40:39, reveman wrote: > > I'd prefer if this was not a preference or override but our real texture > target > > selection mechanism and the code to set it replaces any logic we currently > have > > to determine what target to use. This requires having access to > > ContextProvider::Capabilities at the time we initialize this setting so this > > suggestion might be misguided if that's not the case. > > The problem here is conflicting requirements. Production code wants to be > flexible in the face of different capabilities. Test code wants to be strict to > make sure that it's actually testing what it's testing and these preferences > aren't flexibly ignored. > > How do you combine those two needs? Your solution helps with the flexiblity, but > I doesn't meet the testing need. Hm, can we adjust capabilities for our testing needs instead of adding this setting?
https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... cc/trees/layer_tree_host_impl.cc:2055: LayerTreeSettings::FORCE_TEXTURE_2D_TARGET) On 2014/10/22 20:36:47, reveman wrote: > On 2014/10/22 19:20:44, enne wrote: > And based on this patch I assume that we need to be able to force > raster texture target for testing purposes too, right? Instead of having to fuse > both of these selection mechanism here is there some way we can have one replace > or override the other so that we only have to deal with one setting or > capability here? I'm not sure that these should be the same flag, as they exercise different paths in GLRenderer depending on which raster worker pool is being used. We probably should test them, but it seems like there needs to be a different setting. If you want to test the raster worker pool via pixel tests, then we probably need to have (pool type) x (raster/staging target type) x (draw target type) configurations. https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings.h File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings... cc/trees/layer_tree_settings.h:87: TextureTargetPrefs texture_target_prefs; On 2014/10/22 20:36:48, reveman wrote: > > How do you combine those two needs? Your solution helps with the flexiblity, > but > > I doesn't meet the testing need. > > Hm, can we adjust capabilities for our testing needs instead of adding this > setting? How does this meet the testing need? A test could specify texture rect capability or not, but adding texture rect capability doesn't mean that it will be used. What I want is some way to know for sure that the different GLRenderer paths for tile contents and mask contents get exercised.
A different solution from adding settings would be to virtualize the raster worker pool creation and map image getter so that a test could clobber it as need be. Would that be more in line with what you wanted?
https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... File cc/trees/layer_tree_host_impl.cc (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_host_imp... cc/trees/layer_tree_host_impl.cc:2055: LayerTreeSettings::FORCE_TEXTURE_2D_TARGET) On 2014/10/22 21:14:30, enne wrote: > On 2014/10/22 20:36:47, reveman wrote: > > On 2014/10/22 19:20:44, enne wrote: > > > And based on this patch I assume that we need to be able to force > > raster texture target for testing purposes too, right? Instead of having to > fuse > > both of these selection mechanism here is there some way we can have one > replace > > or override the other so that we only have to deal with one setting or > > capability here? > > I'm not sure that these should be the same flag, as they exercise different > paths in GLRenderer depending on which raster worker pool is being used. We > probably should test them, but it seems like there needs to be a different > setting. > > If you want to test the raster worker pool via pixel tests, then we probably > need to have (pool type) x (raster/staging target type) x (draw target type) > configurations. I think ((pool type) * (raster target type)) + (draw target type) is a sufficient set of test configurations. If a draw target works with one pool type, there's no reason it shouldn't work with another. Also, you can't really separate raster and draw target types. Draw target type is a result of pool type + raster target type. Ideally we wouldn't need use our existing rasterizer implementations to verify that all draw target types work correctly. But we probably need to add a lot of code to avoid that. Using the 0-copy rasterizer uniquely (as it supports output to all target types) to test that all draw target types work correctly seem fine to me.
https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings.h File cc/trees/layer_tree_settings.h (right): https://codereview.chromium.org/668123003/diff/1/cc/trees/layer_tree_settings... cc/trees/layer_tree_settings.h:87: TextureTargetPrefs texture_target_prefs; On 2014/10/22 21:14:30, enne wrote: > On 2014/10/22 20:36:48, reveman wrote: > > > How do you combine those two needs? Your solution helps with the flexiblity, > > but > > > I doesn't meet the testing need. > > > > Hm, can we adjust capabilities for our testing needs instead of adding this > > setting? > > How does this meet the testing need? A test could specify texture rect > capability or not, but adding texture rect capability doesn't mean that it will > be used. We should probably add a new capability field for what target is supported with CHROMIUM_image instead of picking external_oes if available, then rectangle_arb if available and then fallback to 2d. That just happens to work but there's not really a guarantee that rectangle_arb or 2d is supported if external_oes is not. capability.image_texture_target might be what we need. > > What I want is some way to know for sure that the different GLRenderer paths for > tile contents and mask contents get exercised.
Here's an approach with a virtual resource pool/raster worker creating function. What do you think? https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.cc (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:33: // TODO(reveman): one copy not supported in unit tests yet. These tests all yell about sync_query, so I've disabled them here. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:135: void LayerTreeHostPixelResourceTest::CreateResourceAndRasterWorkerPool( This function ends up looking really long, but I don't think it's too bad on the whole. I think this is better than the settings approach, as it lets us forcibly set everything in tests as they should be.
Sure, this approach seems ok to me. Some comments below. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.cc (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:36: #if !defined(OS_ANDROID) what happens without this ifdef? what do we need to do to test this everywhere and not just on android? https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:48: } I think we should try to get this function removed and all our configurations tested on all platforms asap. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:55: } ResourcePoolTestConfig[NUM_PIXEL_RESOURCE_TEST_CASES] = { Please consider replacing this with a set of functions with switch cases instead. GetPixelTestType(PixelResourceTestCase test_case), GetRasterTextureTarget(PixelResourceTestCase test_case)... or just one function with one switch statement that returns a ResourcePoolTestConfig if you prefer that. As that would allow you to remove NUM_PIXEL_RESOURCE_TEST_CASES and make adding new configurations easier as the compiler would force us to think about all cases and all that good stuff.. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:148: if (resource_pool_option_ == RESOURCE_POOL_BITMAP_RASTER_WORKER) { Can you use a switch statement here instead of this long else-if section? https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.h (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:30: GL_ASYNC_UPLOAD_2D_DRAW, What's the naming convention here? GL_(raster-worker-pool-type)_[(staging-target)_STAGING](draw-target)_DRAW? I'd prefer if it was simply: GL_(raster-worker-pool-type)_(raster-target) See my comment related to staging/draw_texture_target_ below. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:31: NUM_PIXEL_RESOURCE_TEST_CASES, If you're going to keep this then consider using "PIXEL_RESOURCE_TEST_CASES_MAX = GL_ASYNC_UPLOAD_2D_DRAW" which I think is the more common pattern at this point. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:47: enum ResourcePoolOption { ResourcePoolOption, uh? Should it be RasterizeOption or RasterWorkerPoolOption? https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:52: RESOURCE_POOL_PIXEL_BUFFER_RASTER, Can you make these names better align with RasterWorkerPool implementations? _BITMAP, _GPU, _ZERO_COPY.. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:57: unsigned draw_texture_target_; I don't think we should have both staging and draw target here. That just allows this to be set to configurations that can never be supported. Replacing both with "raster_texture_target" is much better imo. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_test.... cc/test/layer_tree_test.cc:155: if (!*resource_pool) { Can we have a default TestHooks implementation instead of this fallback?
https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.cc (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:36: #if !defined(OS_ANDROID) On 2014/10/23 20:39:27, reveman wrote: > what happens without this ifdef? what do we need to do to test this everywhere > and not just on android? Lots of GL errors. I don't know what's needed to support this elsewhere. Actually, I don't even know that it works on Android, as all pixel tests are turned off on Android. I should just make this false with a TODO. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:48: } On 2014/10/23 20:39:27, reveman wrote: > I think we should try to get this function removed and all our configurations > tested on all platforms asap. No question! I just wanted to start somewhere. This patch is about adding testing support for texture rectangle arb and masks, not testing everything everywhere. The other obvious follow-up would be to make more pixel tests use this framework. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:55: } ResourcePoolTestConfig[NUM_PIXEL_RESOURCE_TEST_CASES] = { On 2014/10/23 20:39:27, reveman wrote: > Please consider replacing this with a set of functions with switch cases > instead. GetPixelTestType(PixelResourceTestCase test_case), > GetRasterTextureTarget(PixelResourceTestCase test_case)... or just one function > with one switch statement that returns a ResourcePoolTestConfig if you prefer > that. > > As that would allow you to remove NUM_PIXEL_RESOURCE_TEST_CASES and make adding > new configurations easier as the compiler would force us to think about all > cases and all that good stuff.. I like all the values set together, because it made it easier to understand the entire test case. So, single functions sound strictly worse to me. To be honest, I don't see much difference at all between a struct with data or a switch statement that sets values. Adding a new configuration doesn't make any difference; the compiler would complain if you added a new enum and didn't update this array initialization or didn't update a switch statement. So, sure. I'll make it a function if that's to your taste. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:148: if (resource_pool_option_ == RESOURCE_POOL_BITMAP_RASTER_WORKER) { On 2014/10/23 20:39:27, reveman wrote: > Can you use a switch statement here instead of this long else-if section? Sure. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.h (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:30: GL_ASYNC_UPLOAD_2D_DRAW, On 2014/10/23 20:39:27, reveman wrote: > What's the naming convention here? > > GL_(raster-worker-pool-type)_[(staging-target)_STAGING](draw-target)_DRAW? > > I'd prefer if it was simply: > GL_(raster-worker-pool-type)_(raster-target) > > See my comment related to staging/draw_texture_target_ below. Don't agree. See comment below. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:31: NUM_PIXEL_RESOURCE_TEST_CASES, On 2014/10/23 20:39:27, reveman wrote: > If you're going to keep this then consider using "PIXEL_RESOURCE_TEST_CASES_MAX > = GL_ASYNC_UPLOAD_2D_DRAW" which I think is the more common pattern at this > point. Removed. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:47: enum ResourcePoolOption { On 2014/10/23 20:39:27, reveman wrote: > ResourcePoolOption, uh? Should it be RasterizeOption or RasterWorkerPoolOption? Sure. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:52: RESOURCE_POOL_PIXEL_BUFFER_RASTER, On 2014/10/23 20:39:27, reveman wrote: > Can you make these names better align with RasterWorkerPool implementations? > _BITMAP, _GPU, _ZERO_COPY.. I named them identically to the class names, e.g. BITMAP_RASTER_WORKER_POOL. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:57: unsigned draw_texture_target_; On 2014/10/23 20:39:27, reveman wrote: > I don't think we should have both staging and draw target here. That just allows > this to be set to configurations that can never be supported. Replacing both > with "raster_texture_target" is much better imo. I disagree. raster_texture_target just hides the fact that if we test both zero copy and one copy with all the values of raster_texture_target that we'll test all the staging and draw combinations. If you happened to remove zero copy, then you'd be unable to test all the combinations unless you added this back in. Sure, they can be set to things that don't make sense, but that's why the variables are controlled by a higher level test case that only permits sane options. Being able to set them arbitrarily is why I wrote this code. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_test.cc File cc/test/layer_tree_test.cc (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_test.... cc/test/layer_tree_test.cc:155: if (!*resource_pool) { On 2014/10/23 20:39:27, reveman wrote: > Can we have a default TestHooks implementation instead of this fallback? Sure.
https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.cc (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:36: #if !defined(OS_ANDROID) On 2014/10/24 22:21:28, enne wrote: > On 2014/10/23 20:39:27, reveman wrote: > > what happens without this ifdef? what do we need to do to test this everywhere > > and not just on android? > > Lots of GL errors. > > I don't know what's needed to support this elsewhere. Actually, I don't even > know that it works on Android, as all pixel tests are turned off on Android. I > should just make this false with a TODO. Sounds like we're missing a check to determine if this extension is available. If we can test this or not doesn't depend on the platform but available extensions. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:48: } On 2014/10/24 22:21:28, enne wrote: > On 2014/10/23 20:39:27, reveman wrote: > > I think we should try to get this function removed and all our configurations > > tested on all platforms asap. > > No question! I just wanted to start somewhere. This patch is about adding > testing support for texture rectangle arb and masks, not testing everything > everywhere. The other obvious follow-up would be to make more pixel tests use > this framework. Sounds good. I just wanted to make sure we're on the same page. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.h (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:57: unsigned draw_texture_target_; On 2014/10/24 22:21:28, enne wrote: > On 2014/10/23 20:39:27, reveman wrote: > > I don't think we should have both staging and draw target here. That just > allows > > this to be set to configurations that can never be supported. Replacing both > > with "raster_texture_target" is much better imo. > > I disagree. raster_texture_target just hides the fact that if we test both zero > copy and one copy with all the values of raster_texture_target that we'll test > all the staging and draw combinations. If you happened to remove zero copy, > then you'd be unable to test all the combinations unless you added this back in. To pixel test draw target support independent of available RasterWorkerPool implementations I think we should add some form of pixels tests that doesn't use a RasterWorkerPool. > > Sure, they can be set to things that don't make sense, but that's why the > variables are controlled by a higher level test case that only permits sane > options. Being able to set them arbitrarily is why I wrote this code. I guess I'm failing to see what we're trying to test. If zero-copy didn't exist or if it didn't need RECT/EXTERNAL support then why would we need to test it? Wouldn't it be better to remove/DCHECK than keep supporting those draw targets? Like I mentioned above, if we need to pixel test these draw targets independent of available raster worker pool implementations, then I think we need some pixel test that doesn't use an existing raster worker pool implementation. https://codereview.chromium.org/668123003/diff/80001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.cc (right): https://codereview.chromium.org/668123003/diff/80001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:112: } nit: NOTREACHED()
https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.cc (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:36: #if !defined(OS_ANDROID) On 2014/10/27 17:19:04, reveman wrote: > On 2014/10/24 22:21:28, enne wrote: > > On 2014/10/23 20:39:27, reveman wrote: > > > what happens without this ifdef? what do we need to do to test this > everywhere > > > and not just on android? > > > > Lots of GL errors. > > > > I don't know what's needed to support this elsewhere. Actually, I don't even > > know that it works on Android, as all pixel tests are turned off on Android. > I > > should just make this false with a TODO. > > Sounds like we're missing a check to determine if this extension is available. > If we can test this or not doesn't depend on the platform but available > extensions. That seemed likely to me too. It seems like something that could be followed up on, along with turning this framework on for other patches. https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.h (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:57: unsigned draw_texture_target_; On 2014/10/27 17:19:04, reveman wrote: > To pixel test draw target support independent of available RasterWorkerPool > implementations I think we should add some form of pixels tests that doesn't use > a RasterWorkerPool. Maybe you should have mentioned that several reviews ago, if that was what you ultimately wanted. These are LayerTreeHost pixel tests, not just straight up pixel tests. So, they have layers and tile managers and raster worker pools etc. There seems to be zero cost to using a real raster worker pool and I see no use in writing a path to bypass it. Here was my sequence of thoughts. I want to test GLRenderer with different draw targets. Currently, zero/one copy raster worker pool are the paths that currently support the whole variety of them. It made sense to test our actual raster worker pools rather than write a test raster worker pool, since there's no cost to using real paths here. Then, if I was testing one raster worker pool, why not just test all of them with all combinations. This sounded useful to me to exercise all of our code paths; it sounds like you don't agree. I want to test draw targets. How about I replace all of these combinations with just a single texture draw target enum and it'll use zero copy under the hood? https://codereview.chromium.org/668123003/diff/80001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.cc (right): https://codereview.chromium.org/668123003/diff/80001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.cc:112: } On 2014/10/27 17:19:04, reveman wrote: > nit: NOTREACHED() Done.
https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... File cc/test/layer_tree_pixel_resource_test.h (right): https://codereview.chromium.org/668123003/diff/40001/cc/test/layer_tree_pixel... cc/test/layer_tree_pixel_resource_test.h:57: unsigned draw_texture_target_; On 2014/10/27 17:56:22, enne wrote: > On 2014/10/27 17:19:04, reveman wrote: > > > To pixel test draw target support independent of available RasterWorkerPool > > implementations I think we should add some form of pixels tests that doesn't > use > > a RasterWorkerPool. > > Maybe you should have mentioned that several reviews ago, if that was what you > ultimately wanted. > > These are LayerTreeHost pixel tests, not just straight up pixel tests. So, they > have layers and tile managers and raster worker pools etc. There seems to be > zero cost to using a real raster worker pool and I see no use in writing a path > to bypass it. > > Here was my sequence of thoughts. I want to test GLRenderer with different draw > targets. Currently, zero/one copy raster worker pool are the paths that > currently support the whole variety of them. It made sense to test our actual > raster worker pools rather than write a test raster worker pool, since there's > no cost to using real paths here. Then, if I was testing one raster worker > pool, why not just test all of them with all combinations. > > This sounded useful to me to exercise all of our code paths; it sounds like you > don't agree. > > I want to test draw targets. How about I replace all of these combinations with > just a single texture draw target enum and it'll use zero copy under the hood? Testing all supported draw targets sgtm. Testing all supported raster targets with all supported raster worker pool implementations sgtm. That we need test code that make it look like every combination of raster/draw target is supported is what I'm not a fan of but this lgtm if you insist that this is the best way to test all this.
On 2014/10/27 at 18:42:44, reveman wrote: > Testing all supported draw targets sgtm. Testing all supported raster targets with all supported raster worker pool implementations sgtm. Ok, good. :) > That we need test code that make it look like every combination of raster/draw target is supported is what I'm not a fan of but this lgtm if you insist that this is the best way to test all this. That's why those variables which really aren't fully independent are encapsulated by the test class and the test class's public API is the set of supported combinations. This patch still depends on all of your patches to make zero copy work with the in process command buffer, so I'll wait to land this until then.
https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... File cc/test/layer_tree_pixel_resource_test.h (right): https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... cc/test/layer_tree_pixel_resource_test.h:23: GL_GPU_RASTER_2D_DRAW, Just so I don't forget, does adding GL_GPU_RASTER_RECT_DRAW work properly? I'm curious as we might want to use that with GPU raster in the future and if it works already we we should properly include a test for it so it doesn't regress.
https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... File cc/test/layer_tree_pixel_resource_test.h (right): https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... cc/test/layer_tree_pixel_resource_test.h:23: GL_GPU_RASTER_2D_DRAW, On 2014/10/27 19:39:43, reveman wrote: > Just so I don't forget, does adding GL_GPU_RASTER_RECT_DRAW work properly? > > I'm curious as we might want to use that with GPU raster in the future and if it > works already we we should properly include a test for it so it doesn't regress. Yeah, it does work properly. What do you mean by "we should properly include a test for it" past what this patch does?
https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... File cc/test/layer_tree_pixel_resource_test.h (right): https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... cc/test/layer_tree_pixel_resource_test.h:23: GL_GPU_RASTER_2D_DRAW, On 2014/10/27 19:56:45, enne wrote: > On 2014/10/27 19:39:43, reveman wrote: > > Just so I don't forget, does adding GL_GPU_RASTER_RECT_DRAW work properly? > > > > I'm curious as we might want to use that with GPU raster in the future and if > it > > works already we we should properly include a test for it so it doesn't > regress. > > Yeah, it does work properly. > > What do you mean by "we should properly include a test for it" past what this > patch does? I was just referring to this test so just including it in this patch would be perfect.
https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... File cc/test/layer_tree_pixel_resource_test.cc (right): https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... cc/test/layer_tree_pixel_resource_test.cc:25: case GL_GPU_RASTER_2D_DRAW: And supported here too? https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... File cc/test/layer_tree_pixel_resource_test.h (right): https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... cc/test/layer_tree_pixel_resource_test.h:70: GL_GPU_RASTER_2D_DRAW, \ > > What do you mean by "we should properly include a test for it" past what this > > patch does? > > I was just referring to this test so just including it in this patch would be perfect. It's already included?
https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... File cc/test/layer_tree_pixel_resource_test.h (right): https://codereview.chromium.org/668123003/diff/120001/cc/test/layer_tree_pixe... cc/test/layer_tree_pixel_resource_test.h:70: GL_GPU_RASTER_2D_DRAW, \ On 2014/10/27 20:08:51, enne wrote: > > > What do you mean by "we should properly include a test for it" past what > this > > > patch does? > > > > I was just referring to this test so just including it in this patch would be > perfect. > > It's already included? GL_GPU_RASTER_RECT_DRAW? I'm interested in testing the GpuRasterWorkerPool with TEXTURE_RECTANGLE_ARB as raster target.
On 2014/10/28 at 20:11:01, reveman wrote: > GL_GPU_RASTER_RECT_DRAW? I'm interested in testing the GpuRasterWorkerPool with TEXTURE_RECTANGLE_ARB as raster target. Sorry, I misunderstood. I can add support for that afterwards as well? There is currently no way to enable that in code as CreateTileManager hardcodes that to GL_TEXTURE_2D, so it's not something that's shipping.
On 2014/10/28 22:18:05, enne wrote: > On 2014/10/28 at 20:11:01, reveman wrote: > > > GL_GPU_RASTER_RECT_DRAW? I'm interested in testing the GpuRasterWorkerPool > with TEXTURE_RECTANGLE_ARB as raster target. > > Sorry, I misunderstood. > > I can add support for that afterwards as well? There is currently no way to > enable that in code as CreateTileManager hardcodes that to GL_TEXTURE_2D, so > it's not something that's shipping. Right, not something we're shipping right now but would be nice to include this test case if it happens to work already as we might want to ship it in the future. Doing it afterwards is fine and definitely recommended if it doesn't work already.
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/668123003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) mac_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/75513) win_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/80540) android_aosp on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...) android_arm64_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...) android_chromium_gn_compile_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_chromium_gn_compile_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...) android_clang_dbg_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_32 on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_gn_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...) win_chromium_compile_dbg on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...) win_chromium_x64_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by enne@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/668123003/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/03dbe8ae71dbf674627acad91fff64a2fa9d4eb6 Cr-Commit-Position: refs/heads/master@{#302010} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
