Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2791723003:
Update GpuSurfaceTracker to include Android surfaces. (Closed)
|
Created:
3 years, 8 months ago by liberato (no reviews please) Modified:
3 years, 7 months ago CC:
chromium-reviews, avayvod+watch_chromium.org, jam, jbauman+watch_chromium.org, feature-media-reviews_chromium.org, feature-vr-reviews_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, mlamouri+watch-media_chromium.org, tguilbert Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate GpuSurfaceTracker to include Android surfaces.
Previously, GpuSurfaceTracker would add accelerated widgets and java
Surfaces via separate API calls. Now, a java surface is required
when registering an accelerated widget (ANativeWindow) on Android.
BUG=710230
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/2791723003
Cr-Commit-Position: refs/heads/master@{#468488}
Committed: https://chromium.googlesource.com/chromium/src/+/d90f48c21a3dd8c93bbfb9dc53cdc4c644277702
Patch Set 1 #Patch Set 2 : switched to registering structure #Patch Set 3 : rebased #Patch Set 4 : fixed constructors #Patch Set 5 : fixed compile on non-android #Patch Set 6 : actually fixed non-android platforms #Patch Set 7 : cleanup, fixed nullptr surface #
Total comments: 2
Patch Set 8 : split into two constructors #Patch Set 9 : made one-arg constructor implicit #
Total comments: 12
Patch Set 10 : made one-arg constructor explicit again #Patch Set 11 : rebased #
Total comments: 2
Patch Set 12 : updated comment #
Messages
Total messages: 64 (51 generated)
Description was changed from ========== Update GpuSurfaceTracker to include Android surfaces. Previously, GpuSurfaceTracker would add accelerated widgets and java Surfaces via separate API calls. Now, a java surface is required when registering an accelerated widget (ANativeWindow) on Android. BUG= ========== to ========== Update GpuSurfaceTracker to include Android surfaces. Previously, GpuSurfaceTracker would add accelerated widgets and java Surfaces via separate API calls. Now, a java surface is required when registering an accelerated widget (ANativeWindow) on Android. BUG= 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 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
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: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
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: mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)
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: This issue passed the CQ dry run.
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 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...
Description was changed from ========== Update GpuSurfaceTracker to include Android surfaces. Previously, GpuSurfaceTracker would add accelerated widgets and java Surfaces via separate API calls. Now, a java surface is required when registering an accelerated widget (ANativeWindow) on Android. BUG= 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 ========== Update GpuSurfaceTracker to include Android surfaces. Previously, GpuSurfaceTracker would add accelerated widgets and java Surfaces via separate API calls. Now, a java surface is required when registering an accelerated widget (ANativeWindow) on Android. BUG=710230 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 ==========
liberato@chromium.org changed reviewers: + boliu@chromium.org, klausw@chromium.org
boliu: before i open this up to owners review, please let me know if it's what you had in mind. klausw: this CL makes a change to mailbox...bridge..cc that i'm unsure about. your input is appreciated. thanks -fl https://codereview.chromium.org/2791723003/diff/120001/chrome/browser/android... File chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc (right): https://codereview.chromium.org/2791723003/diff/120001/chrome/browser/android... chrome/browser/android/vr_shell/mailbox_to_surface_bridge.cc:144: tracker->RemoveSurface(surface_handle_); @klausw: i converted a call of UnregisterViewSurface into one to RemoveSurface, which definitely changes the behavior. there was no call to RemoveSurface that i could find, so it seemed safe. i didn't trace farther than vr_shell_gl.cc . PTAL, and let me know if i've missed something.
yep, content lgtm https://codereview.chromium.org/2791723003/diff/120001/gpu/ipc/common/gpu_sur... File gpu/ipc/common/gpu_surface_tracker.h (right): https://codereview.chromium.org/2791723003/diff/120001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.h:43: jobject j_surface maybe just write two versions of the constructor, because the one-arg one needs explicit..
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
liberato@chromium.org changed reviewers: + dcheng@chromium.org, sky@chromium.org
liberato@chromium.org changed reviewers: - dcheng@chromium.org, sky@chromium.org
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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: This issue passed the CQ dry run.
liberato@chromium.org changed reviewers: + danakj@chromium.org, dcheng@chromium.org, mthiesse@chromium.org
liberato@chromium.org changed reviewers: + vollick@chromium.org - danakj@chromium.org
Hi all this is a small refactor of GpuSurfaceTracker that touches a few ares of ownership. i've added owners as follows: vollick: ui/compositor/test mthiesse: chrome/browser/android/vr_shell dcheng: gpu/ipc all comments are appreciated. thanks -fl
vr_shell/ lgtm
vollick@chromium.org changed reviewers: + piman@chromium.org - vollick@chromium.org
On 2017/04/11 20:53:18, mthiesse wrote: > vr_shell/ lgtm I'm not a good reviewer for that particular ui/compositor code. piman@, could you please take a look or choose someone more appropriate?
IPC LGTM with C++ nits addressed https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... File gpu/ipc/common/gpu_surface_tracker.cc (right): https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.cc:23: // a Surface from java. Presumably, nobody uses it. Does this mean that it can be fixed in a followup? =) https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.cc:51: SurfaceMap::value_type(surface_handle, std::move(record))); surface_map.emplace(surface_handle, std::move(record)); https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... File gpu/ipc/common/gpu_surface_tracker.h (right): https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.h:39: explicit Record(gfx::AcceleratedWidget widget, jobject j_surface); This doesn't need explicit https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.h:43: Record(gfx::AcceleratedWidget widget); FWIW, this is specifically against the style guide. This would also make Android and non-Android consistent with each other. (If you really want this, I guess you could overload AddSurfaceForNativeWidget... but there's few enough callsites that it shouldn't be hard to fix this) https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.h:44: #endif // !defined(OS_ANDROID) Nit: usually no ! here (since this usually describes what #if condition this is closing) https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.h:86: typedef std::map<gpu::SurfaceHandle, Record> SurfaceMap; Nit using SurfaceMap = ...; since this is changing anyway?
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: This issue passed the CQ dry run.
liberato@chromium.org changed reviewers: + danakj@chromium.org
thanks for the comments. danakj: vollick redirected to piman to look at ui/compositor/test/in_process_context_factory.cc but he's OOO. are you familiar with it, or should i wait? thanks -fl https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... File gpu/ipc/common/gpu_surface_tracker.cc (right): https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.cc:23: // a Surface from java. Presumably, nobody uses it. On 2017/04/14 00:55:24, dcheng (OOO through May 2) wrote: > Does this mean that it can be fixed in a followup? =) crbug.com/712717 . added comment to reference it. https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.cc:51: SurfaceMap::value_type(surface_handle, std::move(record))); On 2017/04/14 00:55:24, dcheng (OOO through May 2) wrote: > surface_map.emplace(surface_handle, std::move(record)); Done. https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... File gpu/ipc/common/gpu_surface_tracker.h (right): https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.h:39: explicit Record(gfx::AcceleratedWidget widget, jobject j_surface); On 2017/04/14 00:55:24, dcheng (OOO through May 2) wrote: > This doesn't need explicit Done. https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.h:43: Record(gfx::AcceleratedWidget widget); On 2017/04/14 00:55:24, dcheng (OOO through May 2) wrote: > FWIW, this is specifically against the style guide. > > This would also make Android and non-Android consistent with each other. > > (If you really want this, I guess you could overload > AddSurfaceForNativeWidget... but there's few enough callsites that it shouldn't > be hard to fix this) good point. i've made the callers explicitly construct one of these objects and pass it in. https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.h:44: #endif // !defined(OS_ANDROID) On 2017/04/14 00:55:24, dcheng (OOO through May 2) wrote: > Nit: usually no ! here (since this usually describes what #if condition this is > closing) Done, and elsewhere. https://codereview.chromium.org/2791723003/diff/160001/gpu/ipc/common/gpu_sur... gpu/ipc/common/gpu_surface_tracker.h:86: typedef std::map<gpu::SurfaceHandle, Record> SurfaceMap; On 2017/04/14 00:55:24, dcheng (OOO through May 2) wrote: > Nit using SurfaceMap = ...; > > since this is changing anyway? Done.
mailbox_to_surface_bridge LGTM, assuming that the behavior is equivalent to what it was doing manually. The new interface looks cleaner.
danakj / piman: friendly ping about ui/ thanks -fl
LGTM w/ a comment about the comment https://codereview.chromium.org/2791723003/diff/200001/ui/compositor/test/in_... File ui/compositor/test/in_process_context_factory.cc (right): https://codereview.chromium.org/2791723003/diff/200001/ui/compositor/test/in_... ui/compositor/test/in_process_context_factory.cc:340: // ContentShell probably does, but how do we get it? For now, we This class is used for unit tests, not for content shell. Content shell runs the full browser stack for graphics, with a gpu process etc.
thanks for the feedback, everyone. -fl https://codereview.chromium.org/2791723003/diff/200001/ui/compositor/test/in_... File ui/compositor/test/in_process_context_factory.cc (right): https://codereview.chromium.org/2791723003/diff/200001/ui/compositor/test/in_... ui/compositor/test/in_process_context_factory.cc:340: // ContentShell probably does, but how do we get it? For now, we On 2017/05/01 22:26:12, danakj wrote: > This class is used for unit tests, not for content shell. Content shell runs the > full browser stack for graphics, with a gpu process etc. Done.
The CQ bit was checked by liberato@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, mthiesse@chromium.org, dcheng@chromium.org, klausw@chromium.org, danakj@chromium.org Link to the patchset: https://codereview.chromium.org/2791723003/#ps220001 (title: "updated comment")
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": 220001, "attempt_start_ts": 1493678505670400,
"parent_rev": "c1e2d1ed8a403ce0e350aebadd12e0c38102e33d", "commit_rev":
"d90f48c21a3dd8c93bbfb9dc53cdc4c644277702"}
Message was sent while issue was closed.
Description was changed from ========== Update GpuSurfaceTracker to include Android surfaces. Previously, GpuSurfaceTracker would add accelerated widgets and java Surfaces via separate API calls. Now, a java surface is required when registering an accelerated widget (ANativeWindow) on Android. BUG=710230 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 ========== Update GpuSurfaceTracker to include Android surfaces. Previously, GpuSurfaceTracker would add accelerated widgets and java Surfaces via separate API calls. Now, a java surface is required when registering an accelerated widget (ANativeWindow) on Android. BUG=710230 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/2791723003 Cr-Commit-Position: refs/heads/master@{#468488} Committed: https://chromium.googlesource.com/chromium/src/+/d90f48c21a3dd8c93bbfb9dc53cd... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/d90f48c21a3dd8c93bbfb9dc53cd... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
