Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
541033003:
Added coordinates information to InputLatency (Closed)
DescriptionAdded coordinates information to InputLatency
The coordinates information will be used in trace-viewer to display input coordinates within the chrome compositor view.
BUG=
Patch Set 1 #
Total comments: 25
Patch Set 2 : Addressed Comments #Patch Set 3 : Fixed a few remaining coding style issues #
Total comments: 2
Patch Set 4 : Addresses code style comments #
Total comments: 2
Messages
Total messages: 22 (3 generated)
skyostil@chromium.org changed reviewers: + skyostil@chromium.org
https://codereview.chromium.org/541033003/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/541033003/diff/1/cc/trees/layer_tree_impl.cc#... cc/trees/layer_tree_impl.cc:853: state->BeginArray("traced_input_latency_ids"); swap_promise_trace_ids https://codereview.chromium.org/541033003/diff/1/cc/trees/layer_tree_impl.cc#... cc/trees/layer_tree_impl.cc:855: state->AppendDouble(swap_promise_list_[i]->TraceId()); indent++ https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:961: for (size_t i = 0; i < touch_event.touches_length; i++) Add {} https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:981: for (size_t i = 0; i < touch_event.touches_length; i++) Add {} https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:1067: ui::LatencyInfo::LatencyScreenCoordinates coordinates; Add a comment saying key events have no coordinates. https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:1115: ui::LatencyInfo::LatencyScreenCoordinates coordinates) { Make this parameter a const ref: const ui::LatencyInfo::LatencyScreenCoordinates& https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:1132: info.TraceEventType(WebInputEventTraits::GetName(type)); Add coordinates here instead. https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.cc File ui/events/latency_info.cc (right): https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.cc#ne... ui/events/latency_info.cc:121: scoped_ptr<base::ListValue> values(new base::ListValue()); |coordinates| https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.cc#ne... ui/events/latency_info.cc:125: base::DictionaryValue* coordinatePair = new base::DictionaryValue(); indent++ https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.cc#ne... ui/events/latency_info.cc:125: base::DictionaryValue* coordinatePair = new base::DictionaryValue(); scoped_ptr<base::DictionaryValue> coordinate_pair(make_scoped_ptr(new ...)); ... values->Append(coordinate_pair.release()); https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.h#new... ui/events/latency_info.h:18: const size_t kMaxLatencyCoordinates = 12; Delete. https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.h#new... ui/events/latency_info.h:100: typedef std::vector<std::pair<int32, int32> > LatencyScreenCoordinates; float InputCoordinates gfx::PointF instead of pair. https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.h#new... ui/events/latency_info.h:161: LatencyScreenCoordinates coordinates; InputCoordinates input_coordinates, add a comment saying these are screen coordinates.
https://codereview.chromium.org/541033003/diff/1/cc/trees/layer_tree_impl.cc File cc/trees/layer_tree_impl.cc (right): https://codereview.chromium.org/541033003/diff/1/cc/trees/layer_tree_impl.cc#... cc/trees/layer_tree_impl.cc:853: state->BeginArray("traced_input_latency_ids"); On 2014/09/04 19:10:43, Sami wrote: > swap_promise_trace_ids Done. https://codereview.chromium.org/541033003/diff/1/cc/trees/layer_tree_impl.cc#... cc/trees/layer_tree_impl.cc:855: state->AppendDouble(swap_promise_list_[i]->TraceId()); On 2014/09/04 19:10:43, Sami wrote: > indent++ Done. https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:961: for (size_t i = 0; i < touch_event.touches_length; i++) On 2014/09/04 19:10:43, Sami wrote: > Add {} Done. https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:981: for (size_t i = 0; i < touch_event.touches_length; i++) On 2014/09/04 19:10:43, Sami wrote: > Add {} Done. https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:1067: ui::LatencyInfo::LatencyScreenCoordinates coordinates; On 2014/09/04 19:10:43, Sami wrote: > Add a comment saying key events have no coordinates. Done. https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:1115: ui::LatencyInfo::LatencyScreenCoordinates coordinates) { On 2014/09/04 19:10:43, Sami wrote: > Make this parameter a const ref: > > const ui::LatencyInfo::LatencyScreenCoordinates& Done. https://codereview.chromium.org/541033003/diff/1/content/browser/renderer_hos... content/browser/renderer_host/render_widget_host_impl.cc:1132: info.TraceEventType(WebInputEventTraits::GetName(type)); On 2014/09/04 19:10:43, Sami wrote: > Add coordinates here instead. Done. https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.cc File ui/events/latency_info.cc (right): https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.cc#ne... ui/events/latency_info.cc:121: scoped_ptr<base::ListValue> values(new base::ListValue()); On 2014/09/04 19:10:43, Sami wrote: > |coordinates| Done. https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.cc#ne... ui/events/latency_info.cc:125: base::DictionaryValue* coordinatePair = new base::DictionaryValue(); On 2014/09/04 19:10:43, Sami wrote: > scoped_ptr<base::DictionaryValue> coordinate_pair(make_scoped_ptr(new ...)); > ... > values->Append(coordinate_pair.release()); Done. https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.h#new... ui/events/latency_info.h:18: const size_t kMaxLatencyCoordinates = 12; On 2014/09/04 19:10:43, Sami wrote: > Delete. Done. https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.h#new... ui/events/latency_info.h:100: typedef std::vector<std::pair<int32, int32> > LatencyScreenCoordinates; On 2014/09/04 19:10:43, Sami wrote: > float > > InputCoordinates > > gfx::PointF instead of pair. Done. https://codereview.chromium.org/541033003/diff/1/ui/events/latency_info.h#new... ui/events/latency_info.h:161: LatencyScreenCoordinates coordinates; On 2014/09/04 19:10:43, Sami wrote: > InputCoordinates input_coordinates, add a comment saying these are screen > coordinates. Done.
https://codereview.chromium.org/541033003/diff/40001/content/browser/renderer... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/541033003/diff/40001/content/browser/renderer... content/browser/renderer_host/render_widget_host_impl.cc:1140: info.coordinates = physical_coordinates; Clear & append this directly. https://codereview.chromium.org/541033003/diff/40001/ui/events/latency_info.cc File ui/events/latency_info.cc (right): https://codereview.chromium.org/541033003/diff/40001/ui/events/latency_info.c... ui/events/latency_info.cc:123: scoped_ptr<base::DictionaryValue> coordinate_pair( Indent++
cvicentiu@chromium.org changed reviewers: + miletus@chromium.org
miletus@chromium.org: Is this a good way to do this?
On 2014/09/05 18:09:06, cvicentiu wrote: > mailto:miletus@chromium.org: Is this a good way to do this? Can you explain why you need to use LatencyInfo to carry the coordinates ? Renderer is already getting input event data from browser. Saving the input coordinates in the LatencyInfo seems a duplicate to me. Can you just extract the input coordinate from the input event renderer receives, cache them in the tree and dump them to the trace at proper time ?
On 2014/09/05 18:32:59, Yufeng Shen wrote: > On 2014/09/05 18:09:06, cvicentiu wrote: > > mailto:miletus@chromium.org: Is this a good way to do this? > > Can you explain why you need to use LatencyInfo to carry the coordinates ? > Renderer is already getting input event data from browser. Saving the input > coordinates in the LatencyInfo seems a duplicate to me. Can you just extract > the input coordinate from the input event renderer receives, cache them in > the tree and dump them to the trace at proper time ? The advantage of doing this through LatencyInfo is that we already have the bookkeeping in place to find out which layer tree contains which input. Adding separate code to track the flow of input event data in a similar way seems a little redundant. Doing what you suggest would probably work for input that is handled by the compositor, but doing the same for input that flows through the main thread is a lot trickier.
On 2014/09/05 18:59:29, Sami wrote: > On 2014/09/05 18:32:59, Yufeng Shen wrote: > > On 2014/09/05 18:09:06, cvicentiu wrote: > > > mailto:miletus@chromium.org: Is this a good way to do this? > > > > Can you explain why you need to use LatencyInfo to carry the coordinates ? > > Renderer is already getting input event data from browser. Saving the input > > coordinates in the LatencyInfo seems a duplicate to me. Can you just extract > > the input coordinate from the input event renderer receives, cache them in > > the tree and dump them to the trace at proper time ? > > The advantage of doing this through LatencyInfo is that we already have the > bookkeeping in place to find out which layer tree contains which input. Adding > separate code to track the flow of input event data in a similar way seems a > little redundant. Doing what you suggest would probably work for input that is > handled by the compositor, but doing the same for input that flows through the > main thread is a lot trickier. OK. One think to keep in mind is that the LatencyInfo turns into a SwapPromise only when the input event is causing rendering. So piggy-backing LatencyInfo/SwapPromise won't give you the input coordinates int the case the input is not causing rendering or our input->renderering causality tracking is not working; (e.g. tap on a blank area or input event causing rAF).
https://codereview.chromium.org/541033003/diff/60001/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/541033003/diff/60001/ui/events/latency_info.h... ui/events/latency_info.h:99: typedef std::vector<gfx::PointF> InputCoordinates; I know Jared has concern about dynamic memory allocation in LatnecyInfo. Maybe just gfx::PointF[12] and a separate size variable ? Jared, wdyt ?
On 2014/09/08 18:28:52, Yufeng Shen wrote: > OK. One think to keep in mind is that the LatencyInfo turns into a SwapPromise > only when the input event is causing rendering. So piggy-backing > LatencyInfo/SwapPromise > won't give you the input coordinates int the case the input is not causing > rendering or > our input->renderering causality tracking is not working; (e.g. tap on a blank > area or > input event causing rAF). That's a good point, although I don't think it will be a problem in our use case because if the input doesn't trigger rendering, then we won't have a corresponding layer tree to which to attach that input either.
On 2014/09/08 18:32:09, Sami wrote: > On 2014/09/08 18:28:52, Yufeng Shen wrote: > > OK. One think to keep in mind is that the LatencyInfo turns into a SwapPromise > > only when the input event is causing rendering. So piggy-backing > > LatencyInfo/SwapPromise > > won't give you the input coordinates int the case the input is not causing > > rendering or > > our input->renderering causality tracking is not working; (e.g. tap on a blank > > area or > > input event causing rAF). > > That's a good point, although I don't think it will be a problem in our use case > because if the input doesn't trigger rendering, then we won't have a > corresponding layer tree to which to attach that input either. only if all of our frame production is purely input driven :)
miletus@chromium.org changed reviewers: + jdduke@chromium.org, sadrul@chromium.org
+ Jared and Sadrul.
On 2014/09/08 18:44:16, Yufeng Shen wrote: > + Jared and Sadrul. Can this not be achieved any other way? I'm curious what purpose having the coordinates visible in the trace viewer would serve? Is it more for completeness, or is there a genuine need or use-case you had in mind here? Regardless, ui::LatencyInfo should not be made any heavier than it already is. Until we have a way to prevent LatencyInfo from doing work that it is unnecessary in the common, untraced case, I'm not happy with making it any bulkier.
https://codereview.chromium.org/541033003/diff/60001/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/541033003/diff/60001/ui/events/latency_info.h... ui/events/latency_info.h:99: typedef std::vector<gfx::PointF> InputCoordinates; On 2014/09/08 18:29:01, Yufeng Shen wrote: > I know Jared has concern about dynamic memory allocation in LatnecyInfo. Maybe > just gfx::PointF[12] and a separate size > variable ? > > Jared, wdyt ? Yeah, a vector is a definite no-no here, and an array of 12 also seems excessive when the 95%+ case is using a single pointer. Why not just include the primary pointer (or two, handling 99.9% of the use cases with pinch) if you really need it here?
https://codereview.chromium.org/541033003/diff/60001/ui/events/latency_info.h File ui/events/latency_info.h (right): https://codereview.chromium.org/541033003/diff/60001/ui/events/latency_info.h... ui/events/latency_info.h:99: typedef std::vector<gfx::PointF> InputCoordinates; On 2014/09/08 18:29:01, Yufeng Shen wrote: > I know Jared has concern about dynamic memory allocation in LatnecyInfo. Maybe > just gfx::PointF[12] and a separate size > variable ? > > Jared, wdyt ? Yeah, a vector is a definite no-no here, and an array of 12 also seems excessive when the 95%+ case is using a single pointer. Why not just include the primary pointer (or two, handling 99.9% of the use cases with pinch) if you really need it here?
On 2014/09/08 20:12:20, jdduke wrote: > https://codereview.chromium.org/541033003/diff/60001/ui/events/latency_info.h > File ui/events/latency_info.h (right): > > https://codereview.chromium.org/541033003/diff/60001/ui/events/latency_info.h... > ui/events/latency_info.h:99: typedef std::vector<gfx::PointF> InputCoordinates; > On 2014/09/08 18:29:01, Yufeng Shen wrote: > > I know Jared has concern about dynamic memory allocation in LatnecyInfo. Maybe > > just gfx::PointF[12] and a separate size > > variable ? > > > > Jared, wdyt ? > > Yeah, a vector is a definite no-no here, and an array of 12 also seems excessive > when the 95%+ case is using a single pointer. Why not just include the primary > pointer (or two, handling 99.9% of the use cases with pinch) if you really need > it here? Right, a pair of coordinates would probably be enough for most cases. > Can this not be achieved any other way? I'm curious what purpose having the > coordinates visible in the trace viewer would serve? Is it more for > completeness, or is there a genuine need or use-case you had in mind here? The immediate motivation is improving trace viewer's input visualization with the hopes that it will speed up finding root causes behind perf issues. We only played around with this prototype briefly last week and it was pretty enlightening to see several frames worth of input collapse into a single layer tree, which means we've got some scheduling issues. Of course you can work the same thing out from following the flow arrows, but that's not as intuitive. Longer term I was thinking this might be useful for doing more input prediction in the browser. If we added raw and filtered coordinates plus any more data we can get from the sensor here, we could draw some real pretty diagrams in trace viewer. (For background, Vicentiu started this patch but unfortunately his internship ran out so I'll pick this up at some point.)
On Tue, Sep 9, 2014 at 7:09 AM, <skyostil@chromium.org> wrote: > On 2014/09/08 20:12:20, jdduke wrote: > >> https://codereview.chromium.org/541033003/diff/60001/ui/ >> events/latency_info.h >> File ui/events/latency_info.h (right): >> > > > https://codereview.chromium.org/541033003/diff/60001/ui/ > events/latency_info.h#newcode99 > >> ui/events/latency_info.h:99: typedef std::vector<gfx::PointF> >> > InputCoordinates; > >> On 2014/09/08 18:29:01, Yufeng Shen wrote: >> > I know Jared has concern about dynamic memory allocation in LatnecyInfo. >> > Maybe > >> > just gfx::PointF[12] and a separate size >> > variable ? >> > >> > Jared, wdyt ? >> > > Yeah, a vector is a definite no-no here, and an array of 12 also seems >> > excessive > >> when the 95%+ case is using a single pointer. Why not just include the >> primary >> pointer (or two, handling 99.9% of the use cases with pinch) if you really >> > need > >> it here? >> > > Right, a pair of coordinates would probably be enough for most cases. > > Can this not be achieved any other way? I'm curious what purpose having >> the >> coordinates visible in the trace viewer would serve? Is it more for >> completeness, or is there a genuine need or use-case you had in mind here? >> > > The immediate motivation is improving trace viewer's input visualization > with > the hopes that it will speed up finding root causes behind perf issues. We > only > played around with this prototype briefly last week and it was pretty > enlightening to see several frames worth of input collapse into a single > layer > tree, which means we've got some scheduling issues. Of course you can work > the > same thing out from following the flow arrows, but that's not as intuitive. > > If you see multiple input latency trace event ending at the same time, it also means they went into the same frame, which is easier to spot. > Longer term I was thinking this might be useful for doing more input > prediction > in the browser. If we added raw and filtered coordinates plus any more > data we > can get from the sensor here, we could draw some real pretty diagrams in > trace > viewer. > > (For background, Vicentiu started this patch but unfortunately his > internship > ran out so I'll pick this up at some point.) > > https://codereview.chromium.org/541033003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/09/09 11:09:09, Sami wrote: > The immediate motivation is improving trace viewer's input visualization with > the hopes that it will speed up finding root causes behind perf issues. We only > played around with this prototype briefly last week and it was pretty > enlightening to see several frames worth of input collapse into a single layer > tree, which means we've got some scheduling issues. Of course you can work the > same thing out from following the flow arrows, but that's not as intuitive. > > Longer term I was thinking this might be useful for doing more input prediction > in the browser. If we added raw and filtered coordinates plus any more data we > can get from the sensor here, we could draw some real pretty diagrams in trace > viewer. > > (For background, Vicentiu started this patch but unfortunately his internship > ran out so I'll pick this up at some point.) I see, thanks for the explanation. Yeah, let's stick with a pair of points if/when we go forward with this.
I took another look at this and ran into a NaCl linking issue because we're not building the IPC marshaling code for NaCl: ../../out_linux/Debug/gen/tc_irt/lib64/liblatency_info_nacl.a(latency_info_param_traits_31340469.o): error: undefined reference to 'IPC::ParamTraits<gfx::PointF>::Write(IPC::Message*, gfx::PointF const&)' Yufeng, since you added liblatency_info_nacl.a, do you have a feeling if it would be better to bring the necessary gfx types to NaCl or just use an array of floats to side-step the issue? I don't know much about working with NaCl.
On 2014/09/12 16:14:40, Sami wrote: > I took another look at this and ran into a NaCl linking issue because we're not > building the IPC marshaling code for NaCl: > > ../../out_linux/Debug/gen/tc_irt/lib64/liblatency_info_nacl.a(latency_info_param_traits_31340469.o): > error: undefined reference to > 'IPC::ParamTraits<gfx::PointF>::Write(IPC::Message*, gfx::PointF const&)' > > Yufeng, since you added liblatency_info_nacl.a, do you have a feeling if it > would be better to bring the necessary gfx types to NaCl or just use an array of > floats to side-step the issue? I don't know much about working with NaCl. Just using floats is easier. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
