Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2174203002:
OnDrawHardware() implementation with async messages (Closed)
DescriptionAdds alternative OnDrawHardware() implementation that uses async IPC messages
The purpose of this CL is to remove unnecessary blocking on UI thread which
happens when BrowserViewRenderer sends a synchronous IPC message to get the
next hardware frame. This new implementation instead sends an async message
and receives the frame with another async message from render thread.
This change alone is not sufficient as it causes scroll latency.
The async implementation is turned on by command line param.
Committed: https://crrev.com/5339299e1c78677caaf1c3b434ac8f154f0468ec
Cr-Commit-Position: refs/heads/master@{#416404}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Further changes. #
Total comments: 3
Patch Set 3 : OnDrawHardware now fully async #Patch Set 4 : Code cleanup. #Patch Set 5 : Added boolean switch for old/new implementation #Patch Set 6 : Added flag to branch old and new implementation #
Total comments: 28
Patch Set 7 : Added command line param #Patch Set 8 : Implemented suggestions from previous code review #
Total comments: 18
Patch Set 9 : Implemented CR suggestions #
Total comments: 10
Patch Set 10 : Implemented code review #
Total comments: 13
Patch Set 11 : Code review #
Total comments: 1
Patch Set 12 : Code review #Patch Set 13 : Code review #Patch Set 14 : Added DemandDrawHwAsync to TestSynchronousCompositor #
Total comments: 1
Patch Set 15 : Removed unnecessary return statement #Messages
Total messages: 58 (22 generated)
ojars@google.com changed reviewers: + boliu@chromium.org
I have some questions. Would be good to talk on Monday.
some high level comments https://codereview.chromium.org/2174203002/diff/1/android_webview/browser/bro... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/1/android_webview/browser/bro... android_webview/browser/browser_view_renderer.cc:267: void BrowserViewRenderer::OnDrawHardwareProcessFrame( You want to add this method to SynchronousCompositorClient https://codereview.chromium.org/2174203002/diff/1/content/common/android/sync... File content/common/android/sync_compositor_messages.h (right): https://codereview.chromium.org/2174203002/diff/1/content/common/android/sync... content/common/android/sync_compositor_messages.h:173: content::SyncCompositorCommonRendererParams, Don't need this. CommonRendererParams are sent asynchronously already https://codereview.chromium.org/2174203002/diff/1/content/renderer/android/sy... File content/renderer/android/synchronous_compositor_proxy.cc (right): https://codereview.chromium.org/2174203002/diff/1/content/renderer/android/sy... content/renderer/android/synchronous_compositor_proxy.cc:177: SyncCompositorCommonRendererParams params; this change should be done in SendDemandDrawHwReply
hush@chromium.org changed reviewers: + hush@chromium.org
I'm not sure what will break by making OnDrawHardware async. What kind of frame do we get after calling BrowserVewRenderer::OnDrawHardware?
On 2016/07/26 20:50:50, hush wrote: > I'm not sure what will break by making OnDrawHardware async. That's what this CL is going to find out, hopefully. > What kind of frame > do we get after calling BrowserVewRenderer::OnDrawHardware? nothing, it won't have a return value I think
Description was changed from ========== Changing OnDrawHardware() to async (incomplete) ========== to ========== Changing OnDrawHardware() to async (incomplete) CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ==========
OnDrawHardware() in BVR is now async except the first 60 calls. Currently it
crashes when starting the async calls.
Here are two stack logs of different crashes:
signal 11 (SIGSEGV), code 1, fault addr 0x0 in tid 17440 (RenderThread)
pid: 17369, tid: 17440, name: RenderThread >>> com.google.android.gm <<<
signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x0
r0 166489d1 r1 9fc7a184 r2 00008001 r3 00000001
r4 9fc7a184 r5 93ce489b r6 93ce489b r7 00000000
r8 9fc7a46c r9 00000000 sl f8ca8a4c fp b369c580
ip 8c52b8cc sp 9fc7a130 lr 8c15b927 pc 8fe55984
Stack Trace:
RELADDR FUNCTION
FILE:LINE
v------> begin
/usr/local/google/home/ojars/chromium/src/third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libcxx/include/vector:1472
00103984 ReturnResources
/usr/local/google/home/ojars/chromium/src/cc/resources/transferable_resource.cc:36
00088923 ReturnResourcesInChildFrame
/usr/local/google/home/ojars/chromium/src/android_webview/browser/hardware_renderer.cc:183
000889db CommitFrame
/usr/local/google/home/ojars/chromium/src/android_webview/browser/hardware_renderer.cc:67
000923db DrawGL
/usr/local/google/home/ojars/chromium/src/android_webview/browser/render_thread_manager.cc:262
0000144f <unknown>
/system/lib/libwebviewchromium_plat_support.so
00050517 <unknown>
/system/lib/libhwui.so
00051b69 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051f71 <unknown>
/system/lib/libhwui.so
00051b77 <unknown>
/system/lib/libhwui.so
00051e61
android::uirenderer::RenderNode::prepareTree(android::uirenderer::TreeInfo&)+60
/system/lib/libhwui.so
0007f1d1 <unknown>
/system/lib/libandroid_runtime.so
0001b2dd <unknown>
/system/lib/libhwui.so
0001c271 <unknown>
/system/lib/libhwui.so
0001c39b <unknown>
/system/lib/libhwui.so
0001f405 android::uirenderer::renderthread::RenderThread::threadLoop()+80
/system/lib/libhwui.so
0001006d android::Thread::_threadLoop(void*)+112
/system/lib/libutils.so
0005ee23 android::AndroidRuntime::javaThreadShell(void*)+70
/system/lib/libandroid_runtime.so
0003f45f __pthread_start(void*)+30
/system/lib/libc.so
00019b43 __start_thread+6
/system/lib/libc.so
-----------------------------------------------------
signal 11 (SIGSEGV), code 1, fault addr 0x10 in tid 18800 (RenderThread)
pid: 18751, tid: 18800, name: RenderThread >>> com.google.android.gm <<<
signal 11 (SIGSEGV), code 1 (SEGV_MAPERR), fault addr 0x10
r0 00000000 r1 00000000 r2 89032020 r3 00000002
r4 89032000 r5 b3aaad90 r6 9fcfa424 r7 00000000
r8 9fcfa3f8 r9 86e10000 sl 43700000 fp 44870000
ip 8d99df70 sp 9fcf9ff8 lr 8d996e73 pc 8ba45c28
Stack Trace:
RELADDR FUNCTION
FILE:LINE
v------> back
/usr/local/google/home/ojars/chromium/src/third_party/android_tools/ndk/sources/cxx-stl/llvm-libc++/libcxx/include/vector:686
00088c28 DrawGL
/usr/local/google/home/ojars/chromium/src/android_webview/browser/hardware_renderer.cc:108
00092509 DrawGL
/usr/local/google/home/ojars/chromium/src/android_webview/browser/render_thread_manager.cc:325
0000144f <unknown>
/system/lib/libwebviewchromium_plat_support.so
000197a1 <unknown>
/system/lib/libhwui.so
0003fe71 <unknown>
/system/lib/libhwui.so
0002a623 <unknown>
/system/lib/libhwui.so
0002770b <unknown>
/system/lib/libhwui.so
000289ff <unknown>
/system/lib/libhwui.so
0003ebe5 <unknown>
/system/lib/libhwui.so
0001a67b <unknown>
/system/lib/libhwui.so
0001c417 <unknown>
/system/lib/libhwui.so
0001f405 android::uirenderer::renderthread::RenderThread::threadLoop()+80
/system/lib/libhwui.so
0001006d android::Thread::_threadLoop(void*)+112
/system/lib/libutils.so
0005ee23 android::AndroidRuntime::javaThreadShell(void*)+70
/system/lib/libandroid_runtime.so
0003f45f __pthread_start(void*)+30
/system/lib/libc.so
00019b43 __start_thread+6
/system/lib/libc.so
https://codereview.chromium.org/2174203002/diff/1/android_webview/browser/bro...
File android_webview/browser/browser_view_renderer.cc (right):
https://codereview.chromium.org/2174203002/diff/1/android_webview/browser/bro...
android_webview/browser/browser_view_renderer.cc:267: void
BrowserViewRenderer::OnDrawHardwareProcessFrame(
On 2016/07/24 17:33:54, boliu wrote:
> You want to add this method to SynchronousCompositorClient
Done.
https://codereview.chromium.org/2174203002/diff/1/content/common/android/sync...
File content/common/android/sync_compositor_messages.h (right):
https://codereview.chromium.org/2174203002/diff/1/content/common/android/sync...
content/common/android/sync_compositor_messages.h:173:
content::SyncCompositorCommonRendererParams,
On 2016/07/24 17:33:54, boliu wrote:
> Don't need this. CommonRendererParams are sent asynchronously already
Done.
https://codereview.chromium.org/2174203002/diff/1/content/renderer/android/sy...
File content/renderer/android/synchronous_compositor_proxy.cc (right):
https://codereview.chromium.org/2174203002/diff/1/content/renderer/android/sy...
content/renderer/android/synchronous_compositor_proxy.cc:177:
SyncCompositorCommonRendererParams params;
On 2016/07/24 17:33:54, boliu wrote:
> this change should be done in SendDemandDrawHwReply
Done.
https://codereview.chromium.org/2174203002/diff/20001/android_webview/browser... File android_webview/browser/hardware_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/20001/android_webview/browser... android_webview/browser/hardware_renderer.cc:185: DCHECK(child_frame_->frame->delegated_frame_data); crash looks like this DCHECK is failing the two DCHECKs above are redundant because of the if statement https://codereview.chromium.org/2174203002/diff/20001/content/browser/android... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/20001/content/browser/android... content/browser/android/synchronous_compositor_host.cc:147: frame.frame.reset(new cc::CompositorFrame); you are creating a CompositorFrame here that's empty, ie without delegated_frame_data, then in BVR, you are passing this empty frame into ChildFrame, then into frame consumer etc I think you should either make this method return void, or just remove all that stuff in BVR after calling DemandDrawHw
OnDrawHardware now works asynchronously. https://codereview.chromium.org/2174203002/diff/20001/android_webview/browser... File android_webview/browser/hardware_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/20001/android_webview/browser... android_webview/browser/hardware_renderer.cc:185: DCHECK(child_frame_->frame->delegated_frame_data); On 2016/07/29 00:48:21, boliu wrote: > crash looks like this DCHECK is failing > > the two DCHECKs above are redundant because of the if statement Done.
Code cleanup.
Two changes here: 1. boolean switch for old/new implementation 2. common renderer params now sent synchronously as a first step to solve the frame latency problem.
Added flag to branch old and new implementation.
https://codereview.chromium.org/2174203002/diff/100001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:240: bool AFLAG = false; Here is an example of an actual command line switch, although the CL is for removing it https://codereview.chromium.org/1913723002 basically you add a string to android_webview/common/aw_switches.cc/h, then check for it using base::CommandLine::ForCurrentProcess only check once though, not every frame https://codereview.chromium.org/2174203002/diff/100001/android_webview/browse... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/2174203002/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer.h:146: void OnDrawHardwareProcessFrame( move this up, next to DidOverscroll, where all the other SynchronousCompositorClient are declared https://codereview.chromium.org/2174203002/diff/100001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2174203002/diff/100001/content/app/content_ma... content/app/content_main_runner.cc:759: VLOG(1) << "gl::init::InitializeGLOneOff failed"; let's pull out the binding init into a separate CL https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:128: if (!frame.frame->delegated_frame_data) { this code is duplicated actually, let's do this, remove the return value from DemandDrawHw as well, and just call DemandDrawHwReceiveFrame instead, this avoids code duplication in BVR as well, although you'll need to keep a bit more state there https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... File content/browser/android/synchronous_compositor_host.h (right): https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... content/browser/android/synchronous_compositor_host.h:61: SynchronousCompositor::Frame DemandDrawHw_Async( this should not have a return value https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... content/browser/android/synchronous_compositor_host.h:77: void DemandDrawHwReceiveFrame(uint32_t output_surface_id, doesn't need to be public https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... content/browser/android/synchronous_compositor_host.h:78: const cc::CompositorFrame&); variable name required, by style guide https://codereview.chromium.org/2174203002/diff/100001/content/common/android... File content/common/android/sync_compositor_messages.h (right): https://codereview.chromium.org/2174203002/diff/100001/content/common/android... content/common/android/sync_compositor_messages.h:128: IPC_MESSAGE_ROUTED1(CompositorMsg_DemandDrawHw, Use the same naming pattern, so SyncCompositorMsg_DemandDrawHwAsync or something like that https://codereview.chromium.org/2174203002/diff/100001/content/common/android... content/common/android/sync_compositor_messages.h:172: IPC_MESSAGE_ROUTED2(CompositorHostMsg_Frame, SyncCompositorHostMsg_ReturnFrame? https://codereview.chromium.org/2174203002/diff/100001/content/public/browser... File content/public/browser/android/synchronous_compositor.h (right): https://codereview.chromium.org/2174203002/diff/100001/content/public/browser... content/public/browser/android/synchronous_compositor.h:71: virtual Frame DemandDrawHw_Async( add some documentation https://codereview.chromium.org/2174203002/diff/100001/content/public/browser... content/public/browser/android/synchronous_compositor.h:74: const gfx::Transform& transform_for_tile_priority) = 0; blank line below https://codereview.chromium.org/2174203002/diff/100001/content/renderer/andro... File content/renderer/android/synchronous_compositor_proxy.cc (right): https://codereview.chromium.org/2174203002/diff/100001/content/renderer/andro... content/renderer/android/synchronous_compositor_proxy.cc:153: void SynchronousCompositorProxy::DemandDrawHw_Async( this is mostly identical to DemandDrawHw so should merge them, ie both should just call a private helper method that does the work https://codereview.chromium.org/2174203002/diff/100001/content/renderer/andro... content/renderer/android/synchronous_compositor_proxy.cc:348: software_draw_reply_)); the DCHECKs were for exactly one of these things being set, this second one doesn't do that anymore, in particular allows both the hardware draw ones to be set at the same time https://codereview.chromium.org/2174203002/diff/100001/content/renderer/andro... File content/renderer/android/synchronous_compositor_proxy.h (right): https://codereview.chromium.org/2174203002/diff/100001/content/renderer/andro... content/renderer/android/synchronous_compositor_proxy.h:76: void DemandDrawHw_Async(const SyncCompositorDemandDrawHwParams& params); drop the underscore on all these new methods
Added check for command line arg https://codereview.chromium.org/2174203002/diff/100001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:240: bool AFLAG = false; On 2016/08/11 19:23:39, boliu wrote: > Here is an example of an actual command line switch, although the CL is for > removing it > > https://codereview.chromium.org/1913723002 > > basically you add a string to android_webview/common/aw_switches.cc/h, then > check for it using base::CommandLine::ForCurrentProcess > > only check once though, not every frame Done.
Implemented suggestions from last code review. I diffed this upload against origin/master, hope that's fine. https://codereview.chromium.org/2174203002/diff/100001/android_webview/browse... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/2174203002/diff/100001/android_webview/browse... android_webview/browser/browser_view_renderer.h:146: void OnDrawHardwareProcessFrame( On 2016/08/11 19:23:39, boliu wrote: > move this up, next to DidOverscroll, where all the other > SynchronousCompositorClient are declared Done. https://codereview.chromium.org/2174203002/diff/100001/content/app/content_ma... File content/app/content_main_runner.cc (right): https://codereview.chromium.org/2174203002/diff/100001/content/app/content_ma... content/app/content_main_runner.cc:759: VLOG(1) << "gl::init::InitializeGLOneOff failed"; On 2016/08/11 19:23:39, boliu wrote: > let's pull out the binding init into a separate CL Done. https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:128: if (!frame.frame->delegated_frame_data) { On 2016/08/11 19:23:39, boliu wrote: > this code is duplicated > > actually, let's do this, remove the return value from DemandDrawHw as well, and > just call DemandDrawHwReceiveFrame instead, this avoids code duplication in BVR > as well, although you'll need to keep a bit more state there Done. https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... File content/browser/android/synchronous_compositor_host.h (right): https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... content/browser/android/synchronous_compositor_host.h:61: SynchronousCompositor::Frame DemandDrawHw_Async( On 2016/08/11 19:23:39, boliu wrote: > this should not have a return value Done. https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... content/browser/android/synchronous_compositor_host.h:77: void DemandDrawHwReceiveFrame(uint32_t output_surface_id, On 2016/08/11 19:23:39, boliu wrote: > doesn't need to be public Done. https://codereview.chromium.org/2174203002/diff/100001/content/browser/androi... content/browser/android/synchronous_compositor_host.h:78: const cc::CompositorFrame&); On 2016/08/11 19:23:39, boliu wrote: > variable name required, by style guide Done. https://codereview.chromium.org/2174203002/diff/100001/content/common/android... File content/common/android/sync_compositor_messages.h (right): https://codereview.chromium.org/2174203002/diff/100001/content/common/android... content/common/android/sync_compositor_messages.h:128: IPC_MESSAGE_ROUTED1(CompositorMsg_DemandDrawHw, On 2016/08/11 19:23:40, boliu wrote: > Use the same naming pattern, so SyncCompositorMsg_DemandDrawHwAsync or something > like that Done. https://codereview.chromium.org/2174203002/diff/100001/content/common/android... content/common/android/sync_compositor_messages.h:172: IPC_MESSAGE_ROUTED2(CompositorHostMsg_Frame, On 2016/08/11 19:23:40, boliu wrote: > SyncCompositorHostMsg_ReturnFrame? Done. https://codereview.chromium.org/2174203002/diff/100001/content/public/browser... File content/public/browser/android/synchronous_compositor.h (right): https://codereview.chromium.org/2174203002/diff/100001/content/public/browser... content/public/browser/android/synchronous_compositor.h:71: virtual Frame DemandDrawHw_Async( On 2016/08/11 19:23:40, boliu wrote: > add some documentation Done. https://codereview.chromium.org/2174203002/diff/100001/content/public/browser... content/public/browser/android/synchronous_compositor.h:74: const gfx::Transform& transform_for_tile_priority) = 0; On 2016/08/11 19:23:40, boliu wrote: > blank line below Done. https://codereview.chromium.org/2174203002/diff/100001/content/renderer/andro... File content/renderer/android/synchronous_compositor_proxy.cc (right): https://codereview.chromium.org/2174203002/diff/100001/content/renderer/andro... content/renderer/android/synchronous_compositor_proxy.cc:153: void SynchronousCompositorProxy::DemandDrawHw_Async( On 2016/08/11 19:23:40, boliu wrote: > this is mostly identical to DemandDrawHw so should merge them, ie both should > just call a private helper method that does the work Done. https://codereview.chromium.org/2174203002/diff/100001/content/renderer/andro... content/renderer/android/synchronous_compositor_proxy.cc:348: software_draw_reply_)); On 2016/08/11 19:23:40, boliu wrote: > the DCHECKs were for exactly one of these things being set, this second one > doesn't do that anymore, in particular allows both the hardware draw ones to be > set at the same time Done. https://codereview.chromium.org/2174203002/diff/100001/content/renderer/andro... File content/renderer/android/synchronous_compositor_proxy.h (right): https://codereview.chromium.org/2174203002/diff/100001/content/renderer/andro... content/renderer/android/synchronous_compositor_proxy.h:76: void DemandDrawHw_Async(const SyncCompositorDemandDrawHwParams& params); On 2016/08/11 19:23:40, boliu wrote: > drop the underscore on all these new methods Done.
https://codereview.chromium.org/2174203002/diff/140001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:246: frame_produced_ = false; you don't need this, just check the frame before calling OnDrawHardwareProcessFrame https://codereview.chromium.org/2174203002/diff/140001/android_webview/browse... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/2174203002/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.h:228: gfx::Rect viewport_rect_for_tile_priority_; there is no need to save these two because they are already saved, transform is in external_draw_constraints, and viewport can be recomputed fwiw, remember this code will be deleted again once the futures thing works. and this will be passed to render through the futures object https://codereview.chromium.org/2174203002/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.h:230: bool async_frame_messages_; const, and move it up next to ui_task_runner_ ie the other const variables maybe rename this async_on_draw_hardware_? "message" doesn't make sense here https://codereview.chromium.org/2174203002/diff/140001/android_webview/common... File android_webview/common/aw_switches.h (right): https://codereview.chromium.org/2174203002/diff/140001/android_webview/common... android_webview/common/aw_switches.h:10: extern const char kBVRAsyncFrameMessages[]; kAsyncOnDrawHardware maybe? https://codereview.chromium.org/2174203002/diff/140001/content/browser/androi... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/140001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:131: DemandDrawHwHelper(output_surface_id, compositor_frame); just directly return from this https://codereview.chromium.org/2174203002/diff/140001/content/browser/androi... File content/browser/android/synchronous_compositor_host.h (right): https://codereview.chromium.org/2174203002/diff/140001/content/browser/androi... content/browser/android/synchronous_compositor_host.h:63: const gfx::Transform& transform_for_tile_priority); override surprised compiler didn't warn about this https://codereview.chromium.org/2174203002/diff/140001/content/browser/androi... content/browser/android/synchronous_compositor_host.h:64: SynchronousCompositor::Frame DemandDrawHwHelper( helper can be private? https://codereview.chromium.org/2174203002/diff/140001/content/public/browser... File content/public/browser/android/synchronous_compositor.h (right): https://codereview.chromium.org/2174203002/diff/140001/content/public/browser... content/public/browser/android/synchronous_compositor.h:71: // Same as DemandDrawHw, but uses asynchronous IPC messages. Calls SynchronousCompositorClient::OnDrawHardwareProcessFrame to return the frame. https://codereview.chromium.org/2174203002/diff/140001/content/renderer/andro... File content/renderer/android/synchronous_compositor_proxy.cc (right): https://codereview.chromium.org/2174203002/diff/140001/content/renderer/andro... content/renderer/android/synchronous_compositor_proxy.cc:155: DemandDrawHwHelper(params, nullptr, true); how about.. DoDemandDrawHw?
Patchset #9 (id:160001) has been deleted
Implemented suggestions https://codereview.chromium.org/2174203002/diff/140001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:246: frame_produced_ = false; On 2016/08/23 02:39:52, boliu wrote: > you don't need this, just check the frame before calling > OnDrawHardwareProcessFrame Done. https://codereview.chromium.org/2174203002/diff/140001/android_webview/browse... File android_webview/browser/browser_view_renderer.h (right): https://codereview.chromium.org/2174203002/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.h:228: gfx::Rect viewport_rect_for_tile_priority_; On 2016/08/23 02:39:52, boliu wrote: > there is no need to save these two because they are already saved, transform is > in external_draw_constraints, and viewport can be recomputed > > fwiw, remember this code will be deleted again once the futures thing works. and > this will be passed to render through the futures object Done. https://codereview.chromium.org/2174203002/diff/140001/android_webview/browse... android_webview/browser/browser_view_renderer.h:230: bool async_frame_messages_; On 2016/08/23 02:39:52, boliu wrote: > const, and move it up next to ui_task_runner_ ie the other const variables > > maybe rename this async_on_draw_hardware_? "message" doesn't make sense here Done. https://codereview.chromium.org/2174203002/diff/140001/android_webview/common... File android_webview/common/aw_switches.h (right): https://codereview.chromium.org/2174203002/diff/140001/android_webview/common... android_webview/common/aw_switches.h:10: extern const char kBVRAsyncFrameMessages[]; On 2016/08/23 02:39:52, boliu wrote: > kAsyncOnDrawHardware maybe? Done. https://codereview.chromium.org/2174203002/diff/140001/content/browser/androi... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/140001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:131: DemandDrawHwHelper(output_surface_id, compositor_frame); On 2016/08/23 02:39:52, boliu wrote: > just directly return from this Done. https://codereview.chromium.org/2174203002/diff/140001/content/browser/androi... File content/browser/android/synchronous_compositor_host.h (right): https://codereview.chromium.org/2174203002/diff/140001/content/browser/androi... content/browser/android/synchronous_compositor_host.h:63: const gfx::Transform& transform_for_tile_priority); On 2016/08/23 02:39:52, boliu wrote: > override > > surprised compiler didn't warn about this Done. https://codereview.chromium.org/2174203002/diff/140001/content/browser/androi... content/browser/android/synchronous_compositor_host.h:64: SynchronousCompositor::Frame DemandDrawHwHelper( On 2016/08/23 02:39:52, boliu wrote: > helper can be private? Done. https://codereview.chromium.org/2174203002/diff/140001/content/public/browser... File content/public/browser/android/synchronous_compositor.h (right): https://codereview.chromium.org/2174203002/diff/140001/content/public/browser... content/public/browser/android/synchronous_compositor.h:71: // Same as DemandDrawHw, but uses asynchronous IPC messages. On 2016/08/23 02:39:52, boliu wrote: > Calls SynchronousCompositorClient::OnDrawHardwareProcessFrame to return the > frame. Done. https://codereview.chromium.org/2174203002/diff/140001/content/renderer/andro... File content/renderer/android/synchronous_compositor_proxy.cc (right): https://codereview.chromium.org/2174203002/diff/140001/content/renderer/andro... content/renderer/android/synchronous_compositor_proxy.cc:155: DemandDrawHwHelper(params, nullptr, true); On 2016/08/23 02:39:53, boliu wrote: > how about.. DoDemandDrawHw? Done.
write a better CL description, first a one line summary, then a blank line, then free form each line should not exceed 80 chars https://codereview.chromium.org/2174203002/diff/180001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/180001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:252: return current_compositor_frame_consumer_->HasFrameOnUI(); there used to be an TRACE_EVENT_INSTANT0 here, don't drop it https://codereview.chromium.org/2174203002/diff/180001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:270: viewport_rect_for_tile_priority = last_on_draw_global_visible_rect_; factor this duplicate code into a helper method, like ComputeViewportRectForTilePriority https://codereview.chromium.org/2174203002/diff/180001/content/browser/androi... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/180001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:138: client_->OnDrawHardwareProcessFrame(std::move(frame)); you could do this all on one line too https://codereview.chromium.org/2174203002/diff/180001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:141: SynchronousCompositor::Frame SynchronousCompositorHost::DemandDrawHwHelper( rename to ProcessHardwareFrame? https://codereview.chromium.org/2174203002/diff/180001/content/renderer/andro... File content/renderer/android/synchronous_compositor_proxy.cc (right): https://codereview.chromium.org/2174203002/diff/180001/content/renderer/andro... content/renderer/android/synchronous_compositor_proxy.cc:168: bool async) { you don't need this param, it's exactly equivalent to !reply_message
Description was changed from ========== Changing OnDrawHardware() to async (incomplete) CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel ========== to ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it breaks things e.g. scroll offset has latency. The async implementation is turned on by command line param. ==========
Description was changed from ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it breaks things e.g. scroll offset has latency. The async implementation is turned on by command line param. ========== to ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it breaks things e.g. scroll offset has latency. The async implementation is turned on by command line param. ==========
Description was changed from ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it breaks things e.g. scroll offset has latency. The async implementation is turned on by command line param. ========== to ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it breaks things e.g. scroll offset has latency. The async implementation is turned on by command line param. ==========
Description was changed from ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it breaks things e.g. scroll offset has latency. The async implementation is turned on by command line param. ========== to ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it causes scroll latency. The async implementation is turned on by command line param. ==========
Also updated the CL description https://codereview.chromium.org/2174203002/diff/180001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/180001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:252: return current_compositor_frame_consumer_->HasFrameOnUI(); On 2016/08/23 22:08:01, boliu wrote: > there used to be an TRACE_EVENT_INSTANT0 here, don't drop it Done. https://codereview.chromium.org/2174203002/diff/180001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:270: viewport_rect_for_tile_priority = last_on_draw_global_visible_rect_; On 2016/08/23 22:08:01, boliu wrote: > factor this duplicate code into a helper method, like > ComputeViewportRectForTilePriority Done. https://codereview.chromium.org/2174203002/diff/180001/content/browser/androi... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/180001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:138: client_->OnDrawHardwareProcessFrame(std::move(frame)); On 2016/08/23 22:08:02, boliu wrote: > you could do this all on one line too Done. https://codereview.chromium.org/2174203002/diff/180001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:141: SynchronousCompositor::Frame SynchronousCompositorHost::DemandDrawHwHelper( On 2016/08/23 22:08:02, boliu wrote: > rename to ProcessHardwareFrame? Done. https://codereview.chromium.org/2174203002/diff/180001/content/renderer/andro... File content/renderer/android/synchronous_compositor_proxy.cc (right): https://codereview.chromium.org/2174203002/diff/180001/content/renderer/andro... content/renderer/android/synchronous_compositor_proxy.cc:168: bool async) { On 2016/08/23 22:08:02, boliu wrote: > you don't need this param, it's exactly equivalent to !reply_message Done.
boliu@chromium.org changed reviewers: + dcheng@chromium.org, sievers@chromium.org
lgtm +sievers for content/public +dcheng for ipc Note the async path is behind a switch, and can't ship in its current state. We'll need to add the blocking behavior back in a future CL, just not on the UI thread. https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:232: // If the WebView is on a layer, WebView does not know what transform is move this comment to the helper method https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:247: remove empty line
On 2016/08/29 22:02:10, boliu wrote: > lgtm > > +sievers for content/public > lgtm
https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:262: if (!frame.frame.get()) Nit: I think .get() can probably be omitted here. https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:269: std::unique_ptr<ChildFrame> child_frame = base::WrapUnique(new ChildFrame( Nit: prefer base::MakeUnique<ChildFrame>(); https://codereview.chromium.org/2174203002/diff/200001/content/browser/androi... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/200001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:146: *frame.frame = std::move(const_cast<cc::CompositorFrame&>(compositor_frame)); Hmm... I wonder if we can try to make this better, this seems pretty sketchy. Can we use some type traits in the IPC code to determine when to pass by const ref / copy vs move?
https://codereview.chromium.org/2174203002/diff/200001/content/browser/androi... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/200001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:146: *frame.frame = std::move(const_cast<cc::CompositorFrame&>(compositor_frame)); On 2016/08/31 18:06:44, dcheng wrote: > Hmm... > > I wonder if we can try to make this better, this seems pretty sketchy. > > Can we use some type traits in the IPC code to determine when to pass by const > ref / copy vs move? So uhh, for this, I think we'll just copy what chrome does with RenderWidgetHostImpl::OnSwapCompositorFrame. It just reads the message. But I am curious what would it take to teach IPC about movable types. So some questions that's more for me learning this.. I imagine it would be something like ParamTrain::IsMovable that's then specialized for the type? And then ipc would do a std::move when it calls handlers. The type in the handler method would simply be cc::CompositorFrame (ie by value)?
https://codereview.chromium.org/2174203002/diff/200001/content/browser/androi... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/200001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:146: *frame.frame = std::move(const_cast<cc::CompositorFrame&>(compositor_frame)); On 2016/08/31 18:23:53, boliu wrote: > On 2016/08/31 18:06:44, dcheng wrote: > > Hmm... > > > > I wonder if we can try to make this better, this seems pretty sketchy. > > > > Can we use some type traits in the IPC code to determine when to pass by const > > ref / copy vs move? > > So uhh, for this, I think we'll just copy what chrome does with > RenderWidgetHostImpl::OnSwapCompositorFrame. It just reads the message. > > > But I am curious what would it take to teach IPC about movable types. So some > questions that's more for me learning this.. > > I imagine it would be something like ParamTrain::IsMovable that's then > specialized for the type? Does that involve updating all ParamTraits though? > And then ipc would do a std::move when it calls > handlers. The type in the handler method would simply be cc::CompositorFrame (ie > by value)?
https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:232: // If the WebView is on a layer, WebView does not know what transform is On 2016/08/29 22:02:10, boliu wrote: > move this comment to the helper method Done. https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:247: On 2016/08/29 22:02:10, boliu wrote: > remove empty line Done. https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:262: if (!frame.frame.get()) On 2016/08/31 18:06:44, dcheng wrote: > Nit: I think .get() can probably be omitted here. Done. https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:269: std::unique_ptr<ChildFrame> child_frame = base::WrapUnique(new ChildFrame( On 2016/08/31 18:06:44, dcheng wrote: > Nit: prefer base::MakeUnique<ChildFrame>(); Done. https://codereview.chromium.org/2174203002/diff/200001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:269: std::unique_ptr<ChildFrame> child_frame = base::WrapUnique(new ChildFrame( On 2016/08/31 18:06:44, dcheng wrote: > Nit: prefer base::MakeUnique<ChildFrame>(); Done. https://codereview.chromium.org/2174203002/diff/200001/content/browser/androi... File content/browser/android/synchronous_compositor_host.cc (right): https://codereview.chromium.org/2174203002/diff/200001/content/browser/androi... content/browser/android/synchronous_compositor_host.cc:146: *frame.frame = std::move(const_cast<cc::CompositorFrame&>(compositor_frame)); On 2016/08/31 18:23:53, boliu wrote: > On 2016/08/31 18:06:44, dcheng wrote: > > Hmm... > > > > I wonder if we can try to make this better, this seems pretty sketchy. > > > > Can we use some type traits in the IPC code to determine when to pass by const > > ref / copy vs move? > > So uhh, for this, I think we'll just copy what chrome does with > RenderWidgetHostImpl::OnSwapCompositorFrame. It just reads the message. > > > But I am curious what would it take to teach IPC about movable types. So some > questions that's more for me learning this.. > > I imagine it would be something like ParamTrain::IsMovable that's then > specialized for the type? And then ipc would do a std::move when it calls > handlers. The type in the handler method would simply be cc::CompositorFrame (ie > by value)? Done.
https://codereview.chromium.org/2174203002/diff/220001/android_webview/browse... File android_webview/browser/browser_view_renderer.cc (right): https://codereview.chromium.org/2174203002/diff/220001/android_webview/browse... android_webview/browser/browser_view_renderer.cc:243: if (!frame.frame.get()) { remove .get here too
ipc lgtm. I hate to introduce more use of IPC_MESSAGE_HANDLER_GENERIC, but this seems strictly better than const casting.
The CQ bit was checked by ojars@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, sievers@chromium.org, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2174203002/#ps240001 (title: "Code review")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
looks like needs a rebase
The CQ bit was checked by ojars@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, dcheng@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2174203002/#ps260001 (title: "Code review")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
On 2016/09/02 22:12:31, commit-bot: I haz the power wrote: > 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...) Oh yeah, make sure android_webview_unittests compile
The CQ bit was checked by ojars@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, dcheng@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2174203002/#ps280001 (title: "Added DemandDrawHwAsync to TestSynchronousCompositor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2174203002/diff/280001/content/public/test/te... File content/public/test/test_synchronous_compositor_android.cc (right): https://codereview.chromium.org/2174203002/diff/280001/content/public/test/te... content/public/test/test_synchronous_compositor_android.cc:40: return; just leave this empty, don't return in a void method
The CQ bit was unchecked by boliu@chromium.org
The CQ bit was checked by ojars@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, dcheng@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2174203002/#ps300001 (title: "Removed unnecessary return statement")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it causes scroll latency. The async implementation is turned on by command line param. ========== to ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it causes scroll latency. The async implementation is turned on by command line param. ==========
Message was sent while issue was closed.
Committed patchset #15 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it causes scroll latency. The async implementation is turned on by command line param. ========== to ========== Adds alternative OnDrawHardware() implementation that uses async IPC messages The purpose of this CL is to remove unnecessary blocking on UI thread which happens when BrowserViewRenderer sends a synchronous IPC message to get the next hardware frame. This new implementation instead sends an async message and receives the frame with another async message from render thread. This change alone is not sufficient as it causes scroll latency. The async implementation is turned on by command line param. Committed: https://crrev.com/5339299e1c78677caaf1c3b434ac8f154f0468ec Cr-Commit-Position: refs/heads/master@{#416404} ==========
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/5339299e1c78677caaf1c3b434ac8f154f0468ec Cr-Commit-Position: refs/heads/master@{#416404} |
