Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2706653002:
Added SurfaceTextureGLOwner to create / own GL objects. (Closed)
|
Created:
3 years, 10 months ago by liberato (no reviews please) Modified:
3 years, 9 months ago CC:
chromium-reviews, posciak+watch_chromium.org, piman+watch_chromium.org, feature-media-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded SurfaceTextureGLOwner to create / own GL objects.
To simplify lifetime issues with surface textures, this L introduces
SurfaceTextureGLOwner. It is a SurfaceTexture which creates and
destroys the underlying GL objects.
This CL also switches AVDAPictureBufferManager / SharedState to use
SurfaceTextureGLOwner rather than maintaining them separately.
BUG=618368
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Review-Url: https://codereview.chromium.org/2706653002
Cr-Commit-Position: refs/heads/master@{#455180}
Committed: https://chromium.googlesource.com/chromium/src/+/2744a14dfbc08a2e286aa9d7faab5c465b1910b8
Patch Set 1 #Patch Set 2 : made destruction the same as before #
Total comments: 4
Patch Set 3 : added thread checker #Patch Set 4 : Added (broken) unit test #
Total comments: 2
Patch Set 5 : unit tests #
Total comments: 4
Patch Set 6 : renamed tests #Patch Set 7 : rebased #Patch Set 8 : stopped including gl_initializer.h #
Dependent Patchsets: Messages
Total messages: 49 (28 generated)
Description was changed from ========== Added SurfaceTextureGLOwner to create / own GL objects. To simplify lifetime issues with surface textures, this L introduces SurfaceTextureGLOwner. It is a SurfaceTexture which creates and destroys the underlying GL objects. This CL also switches AVDAPictureBufferManager / SharedState to use SurfaceTextureGLOwner rather than maintaining them separately. BUG= ========== to ========== Added SurfaceTextureGLOwner to create / own GL objects. To simplify lifetime issues with surface textures, this L introduces SurfaceTextureGLOwner. It is a SurfaceTexture which creates and destroys the underlying GL objects. This CL also switches AVDAPictureBufferManager / SharedState to use SurfaceTextureGLOwner rather than maintaining them separately. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by liberato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by liberato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Description was changed from ========== Added SurfaceTextureGLOwner to create / own GL objects. To simplify lifetime issues with surface textures, this L introduces SurfaceTextureGLOwner. It is a SurfaceTexture which creates and destroys the underlying GL objects. This CL also switches AVDAPictureBufferManager / SharedState to use SurfaceTextureGLOwner rather than maintaining them separately. BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Added SurfaceTextureGLOwner to create / own GL objects. To simplify lifetime issues with surface textures, this L introduces SurfaceTextureGLOwner. It is a SurfaceTexture which creates and destroys the underlying GL objects. This CL also switches AVDAPictureBufferManager / SharedState to use SurfaceTextureGLOwner rather than maintaining them separately. BUG=694763 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
Description was changed from ========== Added SurfaceTextureGLOwner to create / own GL objects. To simplify lifetime issues with surface textures, this L introduces SurfaceTextureGLOwner. It is a SurfaceTexture which creates and destroys the underlying GL objects. This CL also switches AVDAPictureBufferManager / SharedState to use SurfaceTextureGLOwner rather than maintaining them separately. BUG=694763 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Added SurfaceTextureGLOwner to create / own GL objects. To simplify lifetime issues with surface textures, this L introduces SurfaceTextureGLOwner. It is a SurfaceTexture which creates and destroys the underlying GL objects. This CL also switches AVDAPictureBufferManager / SharedState to use SurfaceTextureGLOwner rather than maintaining them separately. BUG=618368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
liberato@chromium.org changed reviewers: + kbr@chromium.org, watk@chromium.org
watk: this should be a no-op change to set up some of the other AVDA improvements. just lets us move the GL texture creat / destroy code out of AVDASharedState kbr: PTAL @ui/ . Refactored SurfaceTexture a little to permit a subclass. i first tried wrapping SurfaceTexture rather than extending it, but it just didn't seem as good. required folks to care if they have a SurfaceTexture or SurfaceTexture++, which almost nobody needs to care about. since ST is refcounted, it made things even more confusing. thanks -fl
nice, lgtm https://codereview.chromium.org/2706653002/diff/20001/media/gpu/surface_textu... File media/gpu/surface_texture_gl_owner.h (right): https://codereview.chromium.org/2706653002/diff/20001/media/gpu/surface_textu... media/gpu/surface_texture_gl_owner.h:6: #define MEDIA_GPU_AVDA_SURFACE_TEXTURE_GL_OWNER_H_ remove AVDA https://codereview.chromium.org/2706653002/diff/20001/media/gpu/surface_textu... media/gpu/surface_texture_gl_owner.h:19: class MEDIA_GPU_EXPORT SurfaceTextureGLOwner : public gl::SurfaceTexture { This requires that it's deleted on the thread it was constructed on right? Could you write a note about that and/or add a threadchecker to the constructor/destructor?
The CQ bit was checked by liberato@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2706653002/diff/20001/media/gpu/surface_textu... File media/gpu/surface_texture_gl_owner.h (right): https://codereview.chromium.org/2706653002/diff/20001/media/gpu/surface_textu... media/gpu/surface_texture_gl_owner.h:6: #define MEDIA_GPU_AVDA_SURFACE_TEXTURE_GL_OWNER_H_ On 2017/02/21 23:18:31, watk wrote: > remove AVDA Done. https://codereview.chromium.org/2706653002/diff/20001/media/gpu/surface_textu... media/gpu/surface_texture_gl_owner.h:19: class MEDIA_GPU_EXPORT SurfaceTextureGLOwner : public gl::SurfaceTexture { On 2017/02/21 23:18:31, watk wrote: > This requires that it's deleted on the thread it was constructed on right? Could > you write a note about that and/or add a threadchecker to the > constructor/destructor? Done.
Looks fine overall. What tests cover these code paths though?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_optional_gpu_tests_rel on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2017/02/22 01:33:31, Ken Russell wrote: > Looks fine overall. What tests cover these code paths though? gl::SurfaceTexture has none that i can find. in general, testing code that touches GL on android seems pretty sparese. i tried to write a test for SurfaceTextureGLOwner, but i haven't managed to set up GL properly. glGetError always returns no error. if you can explain why i've done wrong (please see PS4), then i'll add a few more tests to make it more robust. thanks -fl
On 2017/02/22 18:00:08, liberato wrote: > On 2017/02/22 01:33:31, Ken Russell wrote: > > Looks fine overall. What tests cover these code paths though? > > gl::SurfaceTexture has none that i can find. in general, testing code that > touches GL on android seems pretty sparese. > > i tried to write a test for SurfaceTextureGLOwner, but i haven't managed to set > up GL properly. glGetError always returns no error. if you can explain why > i've done wrong (please see PS4), then i'll add a few more tests to make it more > robust. > > thanks > -fl after fiddling with this for the better part of a day, i have no idea what's wrong with the test. traced through bunches of chrome egl bindings, verified that it's doing about what i'd expect egl init to do. looks great, just doesn't seem to fail when it should. i'm also not sure if i'm calling the real glGetError or not (tried adding tracing to the DriverEGL binding, but didn't get it to work). any advice is appreciated; i'm stuck. otherwise, i'm not sure what to do other than to remove the test. i'd really like to get some sort of gl-enabled tests running for media, though, so i'll revisit this either way. thanks -fl
Your new unittest looks good in general. See the specific comment about a
potential source of confusion. I really think you should push forward with it
rather than taking it out.
Take a look at src/ui/gl/test/gl_image_test_template.h and
src/ui/gl/test/gl_image_test_support.{cc,h} for some other ideas about how you
might add a test which can use the real GL bindings in a cross-platform manner
-- specifically, bringing up and tearing down the bindings correctly.
https://codereview.chromium.org/2706653002/diff/60001/media/gpu/surface_textu...
File media/gpu/surface_texture_gl_owner_unittest.cc (right):
https://codereview.chromium.org/2706653002/diff/60001/media/gpu/surface_textu...
media/gpu/surface_texture_gl_owner_unittest.cc:82: // Which is: 1281
This won't generate an error with real OpenGL or OpenGL ES. It's totally legal
to make up your own object names. Binding operations like glBindTexture define
the name upon the first bind call. I think this is the source of confusion. The
GLES implementation which Chromium exposes to the renderer process is more
strict, and forbids the bind-to-create-resource semantic.
You may be able to use glIsTexture to see the OpenGL-level side-effects of
deleting the SurfaceTextureGLOwner instance.
https://codereview.chromium.org/2706653002/diff/60001/media/gpu/surface_textu... File media/gpu/surface_texture_gl_owner_unittest.cc (right): https://codereview.chromium.org/2706653002/diff/60001/media/gpu/surface_textu... media/gpu/surface_texture_gl_owner_unittest.cc:82: // Which is: 1281 On 2017/02/23 02:38:09, Ken Russell wrote: > This won't generate an error with real OpenGL or OpenGL ES. It's totally legal > to make up your own object names. Binding operations like glBindTexture define > the name upon the first bind call. I think this is the source of confusion. The > GLES implementation which Chromium exposes to the renderer process is more > strict, and forbids the bind-to-create-resource semantic. > > You may be able to use glIsTexture to see the OpenGL-level side-effects of > deleting the SurfaceTextureGLOwner instance. <facepalm/> thank you! glIsTexture works! fwiw, i even checked khronos, at some point starting to suspect that maybe it was WAI. the gles docs and gl2.1 docs do not list it as an error, as you correctly point out. the gl4 docs, however, do. anybody want to guess which one comes up at the top of a google search for 'glBindTexture'? :)
kbr: thanks for all the help! PTAL @ unit tests. thanks -fl
Nice test! lgtm https://codereview.chromium.org/2706653002/diff/80001/media/gpu/surface_textu... File media/gpu/surface_texture_gl_owner_unittest.cc (right): https://codereview.chromium.org/2706653002/diff/80001/media/gpu/surface_textu... media/gpu/surface_texture_gl_owner_unittest.cc:59: surface_ = nullptr; These are all automatic because gtest instantiates a new test harness object for each test. So SetUp and Teardown run right after/before the constructor/destructor. https://codereview.chromium.org/2706653002/diff/80001/media/gpu/surface_textu... media/gpu/surface_texture_gl_owner_unittest.cc:80: TEST_F(SurfaceTextureGLOwnerTest, TestReleaseDoesntDestroyTexture) { Remove "Test" prefix from test names.
Very nice. LGTM. Please watch the bots to make sure the new tests are reliable.
Description was changed from ========== Added SurfaceTextureGLOwner to create / own GL objects. To simplify lifetime issues with surface textures, this L introduces SurfaceTextureGLOwner. It is a SurfaceTexture which creates and destroys the underlying GL objects. This CL also switches AVDAPictureBufferManager / SharedState to use SurfaceTextureGLOwner rather than maintaining them separately. BUG=618368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Added SurfaceTextureGLOwner to create / own GL objects. To simplify lifetime issues with surface textures, this L introduces SurfaceTextureGLOwner. It is a SurfaceTexture which creates and destroys the underlying GL objects. This CL also switches AVDAPictureBufferManager / SharedState to use SurfaceTextureGLOwner rather than maintaining them separately. BUG=618368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ==========
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2706653002/#ps120001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thanks for all the comments. (sorry for the delay -- waited until i was back from vacation to land.) -fl https://codereview.chromium.org/2706653002/diff/80001/media/gpu/surface_textu... File media/gpu/surface_texture_gl_owner_unittest.cc (right): https://codereview.chromium.org/2706653002/diff/80001/media/gpu/surface_textu... media/gpu/surface_texture_gl_owner_unittest.cc:59: surface_ = nullptr; On 2017/02/23 19:52:40, watk wrote: > These are all automatic because gtest instantiates a new test harness object for > each test. So SetUp and Teardown run right after/before the > constructor/destructor. i wanted to clear these down before ShutdownGL. plus, with all the raw ptrs to scoped_refptr, it's more obvious here than relying on destruction order. nobody expects scoped refptrs to care about that. https://codereview.chromium.org/2706653002/diff/80001/media/gpu/surface_textu... media/gpu/surface_texture_gl_owner_unittest.cc:80: TEST_F(SurfaceTextureGLOwnerTest, TestReleaseDoesntDestroyTexture) { On 2017/02/23 19:52:39, watk wrote: > Remove "Test" prefix from test names. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from watk@chromium.org, kbr@chromium.org Link to the patchset: https://codereview.chromium.org/2706653002/#ps140001 (title: "stopped including gl_initializer.h")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by liberato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2017/03/07 04:34:26, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...) This is http://crbug.com/698869 .
The CQ bit was checked by liberato@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1488907738317330,
"parent_rev": "889cea49e6b0ab4cb774d2aeeb1ab684d81a252b", "commit_rev":
"2744a14dfbc08a2e286aa9d7faab5c465b1910b8"}
Message was sent while issue was closed.
Description was changed from ========== Added SurfaceTextureGLOwner to create / own GL objects. To simplify lifetime issues with surface textures, this L introduces SurfaceTextureGLOwner. It is a SurfaceTexture which creates and destroys the underlying GL objects. This CL also switches AVDAPictureBufferManager / SharedState to use SurfaceTextureGLOwner rather than maintaining them separately. BUG=618368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel ========== to ========== Added SurfaceTextureGLOwner to create / own GL objects. To simplify lifetime issues with surface textures, this L introduces SurfaceTextureGLOwner. It is a SurfaceTexture which creates and destroys the underlying GL objects. This CL also switches AVDAPictureBufferManager / SharedState to use SurfaceTextureGLOwner rather than maintaining them separately. BUG=618368 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel Review-Url: https://codereview.chromium.org/2706653002 Cr-Commit-Position: refs/heads/master@{#455180} Committed: https://chromium.googlesource.com/chromium/src/+/2744a14dfbc08a2e286aa9d7faab... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/2744a14dfbc08a2e286aa9d7faab... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
