Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1484473003:
gl, ozone: enable GLImageBindTest unittests
Descriptiongl, ozone: enable GLImageBindTest unittests
TEST=gl_unittests --gtest_filter=GLImage* --ozone-platform=gbm
BUG=475633
Patch Set 1 #
Total comments: 1
Patch Set 2 : add GLImageBindTest for ozone #
Total comments: 2
Patch Set 3 : improved #
Total comments: 6
Patch Set 4 : address reveman's comments #Patch Set 5 : build fix for win and mac, but cros need crrev.com/1208603002 #
Total comments: 10
Patch Set 6 : split CL into thie preparation and GLImageBindTest CL #Patch Set 7 : add new GLImageBindTest unittests. add R_8 test. give up dynamic way #
Total comments: 6
Patch Set 8 : build fix #Patch Set 9 : rebase based on crrev.com/1879243002 #
Total comments: 1
Messages
Total messages: 43 (13 generated)
Description was changed from ========== gl: add new GLImageOzoneNativePixmap unittests. TEST=gl_unittests --gtest_filter=GLImageOzoneNativePixmap* --ozone-platform=gbm BUG=475633 ========== to ========== gl: add new GLImageOzoneNativePixmap unittests. TEST=gl_unittests --gtest_filter=GLImageOzoneNativePixmap* --ozone-platform=gbm BUG=475633 ==========
dongseong.hwang@intel.com changed reviewers: + reveman@chromium.org, spang@chromium.org, tiago.vignatti@intel.com
reveman, could you review? It's what you asked in BlinkOn :) spang, could you review? To run this test, it need https://codereview.chromium.org/1208603002/ to land.
https://codereview.chromium.org/1484473003/diff/1/ui/gl/gl_image_ozone_native... File ui/gl/gl_image_ozone_native_pixmap_unittest.cc (left): https://codereview.chromium.org/1484473003/diff/1/ui/gl/gl_image_ozone_native... ui/gl/gl_image_ozone_native_pixmap_unittest.cc:15: class GLImageOzoneNativePixmapTestDelegate { Note: this previous test is only useful for chromecast where we're not creating a real native pixmap and is not very valuable as a test for the normal usage. Can we add more useful tests for the normal usage case. Tests that bind/copy the image and validate the result?
Hi I'm back. I add new test GLImageBindTest to verify GLImage::BindTexImage It takes time because it needs many prerequisite. First of all, to run it, it needs on-going two CLs; https://codereview.chromium.org/1483633002/ https://codereview.chromium.org/1208603002/ https://codereview.chromium.org/1484473003/diff/20001/ui/gl/gl_image_ozone_na... File ui/gl/gl_image_ozone_native_pixmap_unittest.cc (right): https://codereview.chromium.org/1484473003/diff/20001/ui/gl/gl_image_ozone_na... ui/gl/gl_image_ozone_native_pixmap_unittest.cc:59: ->IsConfigurationSupported(format, usage); It's because ozone knows supported configuration in runtime. See below "for statement" https://codereview.chromium.org/1484473003/diff/20001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/1484473003/diff/20001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:58: for (auto format : gfx::GetBufferFormatsForTesting()) { Second, I change gl_image_test_template.h to match gpu_memory_buffer_factory_test_template.h https://code.google.com/p/chromium/codesearch#chromium/src/content/test/gpu_m... "for statement" try all BufferFormat and each test decides which format is supported.
https://codereview.chromium.org/1484473003/diff/40001/ui/gl/gl_image_memory.h File ui/gl/gl_image_memory.h (left): https://codereview.chromium.org/1484473003/diff/40001/ui/gl/gl_image_memory.h... ui/gl/gl_image_memory.h:39: static unsigned GetInternalFormatForTesting(gfx::BufferFormat format); can we just move this to GLImage instead but keep it a function only for testing? https://codereview.chromium.org/1484473003/diff/40001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_support.h (right): https://codereview.chromium.org/1484473003/diff/40001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_support.h:31: static GLuint CreateSingleTextureProgram(); instead of adding this, can we keep this code in ui/gl/test/gl_image_test_template.h for now until it's needed somewhere else? https://codereview.chromium.org/1484473003/diff/40001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/1484473003/diff/40001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:59: if (!TypeParam::IsSupported(format)) { can we keep requiring each implementation to decide what formats to instantiate the test for instead of this? I think that's much cleaner than a conditional like this.
sorry for delaying update. reveman, could you review it again? https://codereview.chromium.org/1484473003/diff/40001/ui/gl/gl_image_memory.h File ui/gl/gl_image_memory.h (left): https://codereview.chromium.org/1484473003/diff/40001/ui/gl/gl_image_memory.h... ui/gl/gl_image_memory.h:39: static unsigned GetInternalFormatForTesting(gfx::BufferFormat format); On 2015/12/18 21:51:04, reveman wrote: > can we just move this to GLImage instead but keep it a function only for > testing? Currently CL merge this and TextureFormat() in gl_image_memory.cc into GetTextureFormatFrom() in gl_utils.h GetTextureFormatFrom() can be located in gl_image.h. Do you want to move it to gl_image.h? https://codereview.chromium.org/1484473003/diff/40001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_support.h (right): https://codereview.chromium.org/1484473003/diff/40001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_support.h:31: static GLuint CreateSingleTextureProgram(); On 2015/12/18 21:51:04, reveman wrote: > instead of adding this, can we keep this code in > ui/gl/test/gl_image_test_template.h for now until it's needed somewhere else? Done. https://codereview.chromium.org/1484473003/diff/40001/ui/gl/test/gl_image_tes... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/1484473003/diff/40001/ui/gl/test/gl_image_tes... ui/gl/test/gl_image_test_template.h:59: if (!TypeParam::IsSupported(format)) { On 2015/12/18 21:51:04, reveman wrote: > can we keep requiring each implementation to decide what formats to instantiate > the test for instead of this? I think that's much cleaner than a conditional > like this. I agree, but we cannot. each ozone backend supports different format list, so ozone knows it in runtime. I actually copy this pattern from gpu_memory_buffer_factory_test_template.h https://code.google.com/p/chromium/codesearch#chromium/src/content/test/gpu_m... See how ozone check it in runtime. static bool IsSupported(gfx::BufferFormat format) { return ui::ClientNativePixmapFactory::GetInstance() ->IsConfigurationSupported(format, usage); }
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/1484473003/diff/100001/ui/gl/gl_utils.h File ui/gl/gl_utils.h (right): https://codereview.chromium.org/1484473003/diff/100001/ui/gl/gl_utils.h#newco... ui/gl/gl_utils.h:16: GL_EXPORT GLenum GetTextureFormatFrom(gfx::BufferFormat format); Not sure we need this change as part of this patch but it we do then I'd rather see the TextureFormat function in GLImageMemory moved to the GLImage class. https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/gl_image_te... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/gl_image_te... ui/gl/test/gl_image_test_template.h:107: for (auto format : gfx::GetBufferFormatsForTesting()) { I think it was much better to have the format to test part of the test template type and let each unit test decide what formats to test. https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/gl_image_te... ui/gl/test/gl_image_test_template.h:218: TYPED_TEST_P(GLImageBindTest, BindTexImage) { Please add this in a separate patch. Easier to review that way. https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/run_all_uni... File ui/gl/test/run_all_unittests.cc (right): https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/run_all_uni... ui/gl/test/run_all_unittests.cc:56: scoped_ptr<ui::OzoneInitializerForTest> ozone_; |ozone_| is not very descriptive of what this is. Can we use a better name for this that describes what it is?
Description was changed from ========== gl: add new GLImageOzoneNativePixmap unittests. TEST=gl_unittests --gtest_filter=GLImageOzoneNativePixmap* --ozone-platform=gbm BUG=475633 ========== to ========== gl: make GLImage unittests choose the supported format in runtime Ozone cannot decide which format is supported in build time, because Ozone backend is decided in runtime and each backend supports different format list. This CL changes ui/gl/test/gl_image_test_template.h similar to content/test/gpu_memory_buffer_factory_test_template.h TEST=gl_unittests --gtest_filter=GLImage* BUG=475633 ==========
I split the CL as you requested. As slitting, the purpose of this CL is changed. I changed the CL's description. https://codereview.chromium.org/1484473003/diff/100001/ui/gl/gl_utils.h File ui/gl/gl_utils.h (right): https://codereview.chromium.org/1484473003/diff/100001/ui/gl/gl_utils.h#newco... ui/gl/gl_utils.h:16: GL_EXPORT GLenum GetTextureFormatFrom(gfx::BufferFormat format); On 2016/01/13 15:50:09, reveman wrote: > Not sure we need this change as part of this patch but it we do then I'd rather > see the TextureFormat function in GLImageMemory moved to the GLImage class. Done. we need, because both GLImageMemory and unittests need it. Unlik GLImageSharedMemoryTestDelegate and GLImageRefCountedMemoryTestDelegate, GLImageOzoneNativePixmapTestDelegate cannot use GLImageMemory::GetInternalFormatForTesting because GLImageOzoneNativePixmap doesn't inherit GLImageMemory. https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/gl_image_te... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/gl_image_te... ui/gl/test/gl_image_test_template.h:107: for (auto format : gfx::GetBufferFormatsForTesting()) { On 2016/01/13 15:50:09, reveman wrote: > I think it was much better to have the format to test part of the test template > type and let each unit test decide what formats to test. That's difficult because ozone knows the supported format list in runtime. It's why gpu_memory_buffer_factory_test_template.h has the same code. https://code.google.com/p/chromium/codesearch#chromium/src/content/test/gpu_m... https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/gl_image_te... ui/gl/test/gl_image_test_template.h:218: TYPED_TEST_P(GLImageBindTest, BindTexImage) { On 2016/01/13 15:50:09, reveman wrote: > Please add this in a separate patch. Easier to review that way. got it. I extract it to https://codereview.chromium.org/1576353007 Now this CL doesn't depend on https://codereview.chromium.org/1208603002/ https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/run_all_uni... File ui/gl/test/run_all_unittests.cc (right): https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/run_all_uni... ui/gl/test/run_all_unittests.cc:56: scoped_ptr<ui::OzoneInitializerForTest> ozone_; On 2016/01/13 15:50:09, reveman wrote: > |ozone_| is not very descriptive of what this is. Can we use a better name for > this that describes what it is? rename to |ozone_backend_for_test_|
https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/gl_image_te... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/gl_image_te... ui/gl/test/gl_image_test_template.h:107: for (auto format : gfx::GetBufferFormatsForTesting()) { On 2016/01/13 at 17:07:42, dshwang wrote: > On 2016/01/13 15:50:09, reveman wrote: > > I think it was much better to have the format to test part of the test template > > type and let each unit test decide what formats to test. > > That's difficult because ozone knows the supported format list in runtime. > It's why gpu_memory_buffer_factory_test_template.h has the same code. > https://code.google.com/p/chromium/codesearch#chromium/src/content/test/gpu_m... I'd like us to move away from that type of test structure as it makes it much less clear what's not working when a test fails. Can you limit this test to configurations that are guaranteed to be supported for now and we can later figure out how to decide what tests to use at runtime.
https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/gl_image_te... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/1484473003/diff/100001/ui/gl/test/gl_image_te... ui/gl/test/gl_image_test_template.h:107: for (auto format : gfx::GetBufferFormatsForTesting()) { On 2016/01/13 19:48:47, reveman wrote: > > That's difficult because ozone knows the supported format list in runtime. > > It's why gpu_memory_buffer_factory_test_template.h has the same code. > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/test/gpu_m... > > I'd like us to move away from that type of test structure as it makes it much > less clear what's not working when a test fails. It's unfortunate. following code minimizes less clearness. ASSERT_TRUE(small_image) << format; > Can you limit this test to > configurations that are guaranteed to be supported for now and we can later > figure out how to decide what tests to use at runtime. For example, I want to cover GLImageOzoneNativePixmapTestDelegate<gfx::BufferFormat::RGBA_8888, gfx::BufferUsage::GPU_READ_CPU_READ_WRITE> However, only Ozone GBM supports this configuration. If I add this test, Ozone cast, caca, egltest and headless fails the test. The nature of ozone platform is in runtime, so we can not use #ifdef.
Description was changed from ========== gl: make GLImage unittests choose the supported format in runtime Ozone cannot decide which format is supported in build time, because Ozone backend is decided in runtime and each backend supports different format list. This CL changes ui/gl/test/gl_image_test_template.h similar to content/test/gpu_memory_buffer_factory_test_template.h TEST=gl_unittests --gtest_filter=GLImage* BUG=475633 ========== to ========== gl, ozone: add new GLImageBindTest unittests and make ozone run it. TEST=gl_unittests --gtest_filter=GLImage* --ozone-platform=gbm BUG=475633 ==========
The CQ bit was checked by dongseong.hwang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1484473003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1484473003/140001
reveman, could you review again? sorry for long time inactive. As https://codereview.chromium.org/1869793002/ introduce R_8, I cannot postpone it anymore. I resolved all your concerns. https://codereview.chromium.org/1484473003/diff/140001/ui/gl/gl_image_ozone_n... File ui/gl/gl_image_ozone_native_pixmap_unittest.cc (right): https://codereview.chromium.org/1484473003/diff/140001/ui/gl/gl_image_ozone_n... ui/gl/gl_image_ozone_native_pixmap_unittest.cc:100: bool IsSupported() { ozone knows supported format in runtime https://codereview.chromium.org/1484473003/diff/140001/ui/gl/gl_image_ozone_n... ui/gl/gl_image_ozone_native_pixmap_unittest.cc:123: gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT>, R_8 is ok, because IsSupported() return false. After https://codereview.chromium.org/1869793002/ is landed, R_8 is automatically covered. https://codereview.chromium.org/1484473003/diff/140001/ui/gl/gl_image_ozone_n... ui/gl/gl_image_ozone_native_pixmap_unittest.cc:130: GLImageBindTestTypes); Introduce GLImageBindTest test. Note: ozone fails GLImageCopyTest because ozone doesn't need GLImage::CopyTexImage hack. https://codereview.chromium.org/1484473003/diff/140001/ui/gl/test/gl_image_te... File ui/gl/test/gl_image_test_template.h (right): https://codereview.chromium.org/1484473003/diff/140001/ui/gl/test/gl_image_te... ui/gl/test/gl_image_test_template.h:190: if (!this->delegate_.IsSupported()) reveman suggested to keep static way, while ozone knows it in runtime. so let me introduce this if statement for only ozone. https://codereview.chromium.org/1484473003/diff/140001/ui/gl/test/gl_image_te... ui/gl/test/gl_image_test_template.h:284: bool rv = image->CopyTexImage(target); ozone fails this line. copy test is not relevant to ozone. https://codereview.chromium.org/1484473003/diff/140001/ui/gl/test/gl_image_te... ui/gl/test/gl_image_test_template.h:309: TYPED_TEST_P(GLImageBindTest, BindTexImage) { android can reuse this test also.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by dongseong.hwang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1484473003/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1484473003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reveman, PTAL?
dcastagna@chromium.org changed reviewers: + dcastagna@chromium.org
Hi DS! Sorry, I didn't notice this CL until now. :( We landed crrev.com/1879243002 that is basically a subset of this CL (but runs the test on Mac). Do you mind rebasing this on ToT and leave only the changes specific for gbm/ozone? I'd also start testing only one format that is available everywhere, so you can avoid the runtime check.
The CQ bit was checked by dongseong.hwang@intel.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1484473003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1484473003/180001
Sorry for delaying update. reveman, could you review? On 2016/04/13 22:39:21, Daniele Castagna wrote: > We landed crrev.com/1879243002 that is basically a subset of this CL (but runs > the test on Mac). Do you mind rebasing this on ToT and leave only the changes > specific for gbm/ozone? Hi, thx for improving. I rebased it based on your change. > I'd also start testing only one format that is available everywhere, so you can > avoid the runtime check. However, ozone wants to test R8 as well as each ozone platform supports different formats. I remain the runtime check.
Description was changed from ========== gl, ozone: add new GLImageBindTest unittests and make ozone run it. TEST=gl_unittests --gtest_filter=GLImage* --ozone-platform=gbm BUG=475633 ========== to ========== gl, ozone: enable GLImageBindTest unittests TEST=gl_unittests --gtest_filter=GLImage* --ozone-platform=gbm BUG=475633 ==========
On 2016/04/22 at 11:42:49, dongseong.hwang wrote: > Sorry for delaying update. reveman, could you review? > > On 2016/04/13 22:39:21, Daniele Castagna wrote: > > We landed crrev.com/1879243002 that is basically a subset of this CL (but runs > > the test on Mac). Do you mind rebasing this on ToT and leave only the changes > > specific for gbm/ozone? > > Hi, thx for improving. I rebased it based on your change. > > > I'd also start testing only one format that is available everywhere, so you can > > avoid the runtime check. > > However, ozone wants to test R8 as well as each ozone platform supports different formats. I remain the runtime check. We also realized that gl_unittests was not run on the ozone trybot. We enabled them (crrev.com/1905833002) but on the trybots we're using X11 ozone backend, so I'm afraid this test will fail. If that's the case, we could land it with a DISABLED prefix.
On 2016/04/22 11:51:21, Daniele Castagna wrote: > On 2016/04/22 at 11:42:49, dongseong.hwang wrote: > > Sorry for delaying update. reveman, could you review? > > > > On 2016/04/13 22:39:21, Daniele Castagna wrote: > > > We landed crrev.com/1879243002 that is basically a subset of this CL (but > runs > > > the test on Mac). Do you mind rebasing this on ToT and leave only the > changes > > > specific for gbm/ozone? > > > > Hi, thx for improving. I rebased it based on your change. > > > > > I'd also start testing only one format that is available everywhere, so you > can > > > avoid the runtime check. > > > > However, ozone wants to test R8 as well as each ozone platform supports > different formats. I remain the runtime check. > > We also realized that gl_unittests was not run on the ozone trybot. We enabled > them (crrev.com/1905833002) but on the trybots we're using X11 ozone backend, so > I'm afraid this test will fail. If that's the case, we could land it with a > DISABLED prefix. I checked --ozone-platform=gbm as well as --ozone-platform=test. Both were fine as ozone test allows only supported format in runtime.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
reveman, PTAL?
I don't like the IsSupported part. Why do we need it? Can't we limit the test run on each platform to the tests we know are supported?
https://codereview.chromium.org/1484473003/diff/180001/ui/gl/gl_image_ozone_n... File ui/gl/gl_image_ozone_native_pixmap_unittest.cc (right): https://codereview.chromium.org/1484473003/diff/180001/ui/gl/gl_image_ozone_n... ui/gl/gl_image_ozone_native_pixmap_unittest.cc:126: gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT>>; > I don't like the IsSupported part. Why do we need it? Can't we limit the test run on each platform to the tests we know are supported? R_8 and BGRA_8888 bind test passes on only ozone gbm. ozone test, cast and others crash. Except for GBM, no platform supports GPU_READ_CPU_READ_WRITE. We don't know which configuration will supported by each platform. Even if we know, hard coding here is not good idea. Someone must keep eyes on which configuration will each platform support.
On 2016/05/03 at 11:40:25, dongseong.hwang wrote: > https://codereview.chromium.org/1484473003/diff/180001/ui/gl/gl_image_ozone_n... > File ui/gl/gl_image_ozone_native_pixmap_unittest.cc (right): > > https://codereview.chromium.org/1484473003/diff/180001/ui/gl/gl_image_ozone_n... > ui/gl/gl_image_ozone_native_pixmap_unittest.cc:126: gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT>>; > > I don't like the IsSupported part. Why do we need it? Can't we limit the test run on each platform to the tests we know are supported? > > R_8 and BGRA_8888 bind test passes on only ozone gbm. ozone test, cast and others crash. Except for GBM, no platform supports GPU_READ_CPU_READ_WRITE. > We don't know which configuration will supported by each platform. Even if we know, hard coding here is not good idea. Someone must keep eyes on which configuration will each platform support. I prefer ifdefs if possible over a runtime check. The way we've setup these test are so that it's very clear what formats and use cases are supported on each platform and I think that's really useful so I'd like to keep it that way.
On 2016/05/03 11:49:48, reveman wrote: > On 2016/05/03 at 11:40:25, dongseong.hwang wrote: > > > https://codereview.chromium.org/1484473003/diff/180001/ui/gl/gl_image_ozone_n... > > File ui/gl/gl_image_ozone_native_pixmap_unittest.cc (right): > > > > > https://codereview.chromium.org/1484473003/diff/180001/ui/gl/gl_image_ozone_n... > > ui/gl/gl_image_ozone_native_pixmap_unittest.cc:126: > gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT>>; > > > I don't like the IsSupported part. Why do we need it? Can't we limit the > test run on each platform to the tests we know are supported? > > > > R_8 and BGRA_8888 bind test passes on only ozone gbm. ozone test, cast and > others crash. Except for GBM, no platform supports GPU_READ_CPU_READ_WRITE. > > We don't know which configuration will supported by each platform. Even if we > know, hard coding here is not good idea. Someone must keep eyes on which > configuration will each platform support. > > I prefer ifdefs if possible over a runtime check. The way we've setup these test > are so that it's very clear what formats and use cases are supported on each > platform and I think that's really useful so I'd like to keep it that way. Unfortunately, ifdefs is impossible because ozone platform's nature is runtime. Single chrome or gl_unittests executable works as various ozone platforms according to command line. e.g. chrome --ozone-platform=gbm, chrome --ozone-platform=test, chrome --ozone-platform=cast, chrome --ozone-platform=wayland
On 2016/05/03 at 11:59:20, dongseong.hwang wrote: > On 2016/05/03 11:49:48, reveman wrote: > > On 2016/05/03 at 11:40:25, dongseong.hwang wrote: > > > > > https://codereview.chromium.org/1484473003/diff/180001/ui/gl/gl_image_ozone_n... > > > File ui/gl/gl_image_ozone_native_pixmap_unittest.cc (right): > > > > > > > > https://codereview.chromium.org/1484473003/diff/180001/ui/gl/gl_image_ozone_n... > > > ui/gl/gl_image_ozone_native_pixmap_unittest.cc:126: > > gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT>>; > > > > I don't like the IsSupported part. Why do we need it? Can't we limit the > > test run on each platform to the tests we know are supported? > > > > > > R_8 and BGRA_8888 bind test passes on only ozone gbm. ozone test, cast and > > others crash. Except for GBM, no platform supports GPU_READ_CPU_READ_WRITE. > > > We don't know which configuration will supported by each platform. Even if we > > know, hard coding here is not good idea. Someone must keep eyes on which > > configuration will each platform support. > > > > I prefer ifdefs if possible over a runtime check. The way we've setup these test > > are so that it's very clear what formats and use cases are supported on each > > platform and I think that's really useful so I'd like to keep it that way. > > Unfortunately, ifdefs is impossible because ozone platform's nature is runtime. Single chrome or gl_unittests executable works as various ozone platforms according to command line. > e.g. chrome --ozone-platform=gbm, chrome --ozone-platform=test, chrome --ozone-platform=cast, chrome --ozone-platform=wayland Then I think this is the wrong location for these tests and they should probably live in ozone platform specific code.
On 2016/05/03 16:01:42, reveman wrote: > On 2016/05/03 at 11:59:20, dongseong.hwang wrote: > > On 2016/05/03 11:49:48, reveman wrote: > > > On 2016/05/03 at 11:40:25, dongseong.hwang wrote: > > > > > > > > https://codereview.chromium.org/1484473003/diff/180001/ui/gl/gl_image_ozone_n... > > > > File ui/gl/gl_image_ozone_native_pixmap_unittest.cc (right): > > > > > > > > > > > > https://codereview.chromium.org/1484473003/diff/180001/ui/gl/gl_image_ozone_n... > > > > ui/gl/gl_image_ozone_native_pixmap_unittest.cc:126: > > > gfx::BufferUsage::GPU_READ_CPU_READ_WRITE_PERSISTENT>>; > > > > > I don't like the IsSupported part. Why do we need it? Can't we limit the > > > test run on each platform to the tests we know are supported? > > > > > > > > R_8 and BGRA_8888 bind test passes on only ozone gbm. ozone test, cast and > > > others crash. Except for GBM, no platform supports GPU_READ_CPU_READ_WRITE. > > > > We don't know which configuration will supported by each platform. Even if > we > > > know, hard coding here is not good idea. Someone must keep eyes on which > > > configuration will each platform support. > > > > > > I prefer ifdefs if possible over a runtime check. The way we've setup these > test > > > are so that it's very clear what formats and use cases are supported on each > > > platform and I think that's really useful so I'd like to keep it that way. > > > > Unfortunately, ifdefs is impossible because ozone platform's nature is > runtime. Single chrome or gl_unittests executable works as various ozone > platforms according to command line. > > e.g. chrome --ozone-platform=gbm, chrome --ozone-platform=test, chrome > --ozone-platform=cast, chrome --ozone-platform=wayland > > Then I think this is the wrong location for these tests and they should probably > live in ozone platform specific code. spang, what do you think? As ui/gl depends on ui/ozone/public and ui/ozone provides ui/gl's backend, IMO it's the proper location to test GLImageOzoneNativePixmap.
Hi, what is the status here ? Thx
On 2017/03/09 at 13:15:56, julien.isorce wrote: > Hi, what is the status here ? Thx These tests changed significantly since this patch. They now live in ui/ozone/gl/gl_image_ozone_native_pixmap_unittest.cc. You can build them for cros with the target ozone_gl_unittests. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
