Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
413103002:
Android WebView: Fall back to idle uploads if draw functor table not set (Closed)
|
Created:
6 years, 5 months ago by no sievers Modified:
6 years, 4 months ago Reviewers:
boliu CC:
chromium-reviews, piman+watch_chromium.org, android-webview-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionAndroid WebView: Fall back to idle uploads if draw functor table not set
Disable map_image if draw functor table is not set,
and disallow EGL async uploads if MailboxSync is on (doesn't work).
R=boliu@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286501
Patch Set 1 #
Total comments: 7
Patch Set 2 : address comment #
Total comments: 2
Patch Set 3 : #
Messages
Total messages: 21 (0 generated)
ptal
https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right): https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:68: !gles2::MailboxSynchronizer::GetInstance()) { What does the rest of this change have to do with MailboxSynchronizer? That's initialized in aw_browser_main_parts.cc
https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right): https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:68: !gles2::MailboxSynchronizer::GetInstance()) { On 2014/07/24 01:29:07, boliu wrote: > What does the rest of this change have to do with MailboxSynchronizer? That's > initialized in aw_browser_main_parts.cc This async upload impl does not work with mailbox sync because it creates EGL images for the textures also. Disabling it here makes it fall back to the idle uploader.
https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right): https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:68: !gles2::MailboxSynchronizer::GetInstance()) { On 2014/07/24 16:59:39, sievers wrote: > On 2014/07/24 01:29:07, boliu wrote: > > What does the rest of this change have to do with MailboxSynchronizer? That's > > initialized in aw_browser_main_parts.cc > > This async upload impl does not work with mailbox sync because it creates EGL > images for the textures also. > Disabling it here makes it fall back to the idle uploader. I don't see how the functor table not being set leads to MailboxSynchronizer not being initialized though.
https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right): https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:68: !gles2::MailboxSynchronizer::GetInstance()) { On 2014/07/24 17:00:59, boliu wrote: > On 2014/07/24 16:59:39, sievers wrote: > > On 2014/07/24 01:29:07, boliu wrote: > > > What does the rest of this change have to do with MailboxSynchronizer? > That's > > > initialized in aw_browser_main_parts.cc > > > > This async upload impl does not work with mailbox sync because it creates EGL > > images for the textures also. > > Disabling it here makes it fall back to the idle uploader. > > I don't see how the functor table not being set leads to MailboxSynchronizer not > being initialized though. It has nothing to do with the draw functor table. The draw functor table not being set falls back to async uploads. MailboxSync enabled disallows AsyncPixelTransferManagerEGL.
lgtm % one change https://codereview.chromium.org/413103002/diff/1/android_webview/lib/main/aw_... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/413103002/diff/1/android_webview/lib/main/aw_... android_webview/lib/main/aw_main_delegate.cc:58: cl->AppendSwitch(switches::kEnableZeroCopy); We can remove this switch if we don't have the function table https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right): https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:68: !gles2::MailboxSynchronizer::GetInstance()) { On 2014/07/24 19:21:26, sievers wrote: > On 2014/07/24 17:00:59, boliu wrote: > > On 2014/07/24 16:59:39, sievers wrote: > > > On 2014/07/24 01:29:07, boliu wrote: > > > > What does the rest of this change have to do with MailboxSynchronizer? > > That's > > > > initialized in aw_browser_main_parts.cc > > > > > > This async upload impl does not work with mailbox sync because it creates > EGL > > > images for the textures also. > > > Disabling it here makes it fall back to the idle uploader. > > > > I don't see how the functor table not being set leads to MailboxSynchronizer > not > > being initialized though. > > It has nothing to do with the draw functor table. > The draw functor table not being set falls back to async uploads. > MailboxSync enabled disallows AsyncPixelTransferManagerEGL. Oh...that wasn't obvious
https://codereview.chromium.org/413103002/diff/1/android_webview/lib/main/aw_... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/413103002/diff/1/android_webview/lib/main/aw_... android_webview/lib/main/aw_main_delegate.cc:58: cl->AppendSwitch(switches::kEnableZeroCopy); On 2014/07/24 20:50:24, boliu wrote: > We can remove this switch if we don't have the function table Done.
On 2014/07/24 20:50:24, boliu wrote: > lgtm % one change > > https://codereview.chromium.org/413103002/diff/1/android_webview/lib/main/aw_... > File android_webview/lib/main/aw_main_delegate.cc (right): > > https://codereview.chromium.org/413103002/diff/1/android_webview/lib/main/aw_... > android_webview/lib/main/aw_main_delegate.cc:58: > cl->AppendSwitch(switches::kEnableZeroCopy); > We can remove this switch if we don't have the function table > > https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... > File gpu/command_buffer/service/async_pixel_transfer_manager_android.cc (right): > > https://codereview.chromium.org/413103002/diff/1/gpu/command_buffer/service/a... > gpu/command_buffer/service/async_pixel_transfer_manager_android.cc:68: > !gles2::MailboxSynchronizer::GetInstance()) { > On 2014/07/24 19:21:26, sievers wrote: > > On 2014/07/24 17:00:59, boliu wrote: > > > On 2014/07/24 16:59:39, sievers wrote: > > > > On 2014/07/24 01:29:07, boliu wrote: > > > > > What does the rest of this change have to do with MailboxSynchronizer? > > > That's > > > > > initialized in aw_browser_main_parts.cc > > > > > > > > This async upload impl does not work with mailbox sync because it creates > > EGL > > > > images for the textures also. > > > > Disabling it here makes it fall back to the idle uploader. > > > > > > I don't see how the functor table not being set leads to MailboxSynchronizer > > not > > > being initialized though. > > > > It has nothing to do with the draw functor table. > > The draw functor table not being set falls back to async uploads. > > MailboxSync enabled disallows AsyncPixelTransferManagerEGL. > > Oh...that wasn't obvious Yea, I updated the description to explain it better.
The CQ bit was checked by sievers@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/413103002/20001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...) android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/21...) win_gpu_triggered_tests on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
https://codereview.chromium.org/413103002/diff/20001/android_webview/lib/main... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/413103002/diff/20001/android_webview/lib/main... android_webview/lib/main/aw_main_delegate.cc:51: if (gpu_memory_buffer_factory_.get()->Initialize()) { Amazingly, the function tables are set very late in the init process, so this will fail initialize even inside android :/ Maybe just add a switch to aw_switches?
https://codereview.chromium.org/413103002/diff/20001/android_webview/lib/main... File android_webview/lib/main/aw_main_delegate.cc (right): https://codereview.chromium.org/413103002/diff/20001/android_webview/lib/main... android_webview/lib/main/aw_main_delegate.cc:51: if (gpu_memory_buffer_factory_.get()->Initialize()) { On 2014/07/26 00:29:14, boliu wrote: > Amazingly, the function tables are set very late in the init process, so this > will fail initialize even inside android :/ > > Maybe just add a switch to aw_switches? Maybe we can switch the call order? Probably better to do the static initialization before running all the content main runner stuff, esp. "BasicStartupComplete".
The CQ bit was checked by sievers@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sievers@chromium.org/413103002/40001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)
The CQ bit was unchecked by sievers@chromium.org
Looks like testGetVisitedHistoryCallbackAfterDestroy() has been flaky. Committing manually.
Message was sent while issue was closed.
Committed patchset #3 manually as r286501 (presubmit successful). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
