Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2746903004:
Add traces for some video events. (Closed)
|
Created:
3 years, 9 months ago by ehmaldonado_webrtc Modified:
3 years, 9 months ago CC:
webrtc-reviews_webrtc.org, video-team_agora.io, yujie_mao (webrtc), zhengzhonghou_agora.io, stefan-webrtc, tterriberry_mozilla.com, mflodman Target Ref:
refs/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdd traces for some video events.
This CL adds traces to compute the following metrics that getStats()
captures for video:
- googFrameRateSent
- googFrameRateInput
- googFirsReceived
- googNacksReceived
- googFrameWidthSent
- googFrameHeightSent
- googAvgEncodeMs
- googEncodeUsagePercent
BUG=chromium:653087
Review-Url: https://codereview.webrtc.org/2746903004
Cr-Commit-Position: refs/heads/master@{#17260}
Committed: https://chromium.googlesource.com/external/webrtc/+/f7f7fb9cf8706aee5cb2e5ebf5335622f6193eba
Patch Set 1 #Patch Set 2 : Enable traces by default. #Patch Set 3 : Use the webrtc_stats category instead. #
Total comments: 2
Patch Set 4 : Rename traces to WebRTC.Video.* #
Total comments: 3
Patch Set 5 : Addressed comments. #Messages
Total messages: 23 (8 generated)
Description was changed from ========== Add traces for some video events. This CL adds traces to compute the metrics that getStats() captures for video. BUG=chromium:653087 ========== to ========== Add traces for some video events. This CL adds traces to compute the following metrics that getStats() captures for video: - googFrameRateSent - googFrameRateInput - googFirsReceived - googNacksReceived - googFrameWidthSent - googFrameHeightSent - googAvgEncodeMs - googEncodeUsagePercent BUG=chromium:653087 ==========
ehmaldonado@webrtc.org changed reviewers: + asapersson@webrtc.org, kjellander@webrtc.org, tommi@webrtc.org
An initial attempt to add traces to WebRTC. Please take a look :) I think this is a good place to trace those metrics, but I'm somewhat worried that we might be tracing too often. Am I tracing the right values? Should we trace some other values too, for this to be useful?
An initial attempt to add traces to WebRTC. Please take a look :) I think this is a good place to trace those metrics, but I'm somewhat worried that we might be tracing too often. Am I tracing the right values? Should we trace some other values too, for this to be useful?
On 2017/03/13 17:37:59, ehmaldonado_webrtc wrote: > An initial attempt to add traces to WebRTC. > Please take a look :) > > I think this is a good place to trace those metrics, but I'm somewhat worried > that we might be tracing too often. > > Am I tracing the right values? Should we trace some other values too, for this > to be useful? Can you try this in chrome and share the trace.json here as an example for how this will look? (how frequent, how costly etc)
On 2017/03/13 17:46:10, tommi (webrtc) wrote: > On 2017/03/13 17:37:59, ehmaldonado_webrtc wrote: > > An initial attempt to add traces to WebRTC. > > Please take a look :) > > > > I think this is a good place to trace those metrics, but I'm somewhat worried > > that we might be tracing too often. > > > > Am I tracing the right values? Should we trace some other values too, for this > > to be useful? > > Can you try this in chrome and share the trace.json here as an example for how > this will look? (how frequent, how costly etc) https://drive.google.com/file/d/0B99J5olOIugpZmZmZ3A3TXBkcUU/view?usp=sharing
Here is the trace: https://drive.google.com/file/d/0B99J5olOIugpZmZmZ3A3TXBkcUU/view?usp=sharing It doesn't look like it affects performance too much, but I don't know how to tell. Can you take a look, please?
Here is the trace with the traces enabled by default: https://drive.google.com/file/d/0B99J5olOIugpcVdDZFFoYzdkVVU/view?usp=sharing
ehmaldonado@webrtc.org changed reviewers: + nednguyen@google.com
+Ned in case you have any comments or concerns.
On 2017/03/15 11:00:21, ehmaldonado_webrtc wrote: > +Ned in case you have any comments or concerns. I will hold off on reviewing this until we finished to webrtc migration project.
lgtm % one suggestion for naming. https://codereview.webrtc.org/2746903004/diff/40001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/2746903004/diff/40001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:430: TRACE_EVENT_INSTANT2("webrtc_stats", "WebRTC::Video::FrameRateSent", nit: since WebRTC::Video::FrameRateSent does not map to C++ code, I would rather name this something like "WebRTC.Video.FrameRateSent". Even the "WebRTC." part might be too much since it's already in the category. Same for other names.
Patchset #4 (id:60001) has been deleted
Åsa: Do you have any comments? https://codereview.webrtc.org/2746903004/diff/40001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/2746903004/diff/40001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:430: TRACE_EVENT_INSTANT2("webrtc_stats", "WebRTC::Video::FrameRateSent", On 2017/03/15 13:49:08, tommi (webrtc) wrote: > nit: since WebRTC::Video::FrameRateSent does not map to C++ code, I would rather > name this something like "WebRTC.Video.FrameRateSent". Even the "WebRTC." part > might be too much since it's already in the category. > > Same for other names. Done. I decided to leave the WebRTC in so it has the same name as the histograms.
https://codereview.webrtc.org/2746903004/diff/80001/webrtc/video/send_statist... File webrtc/video/send_statistics_proxy.cc (right): https://codereview.webrtc.org/2746903004/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:657: "frame_height", uma_container_->max_sent_height_per_timestamp_, "frame_height" -> "frame_width" https://codereview.webrtc.org/2746903004/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:657: "frame_height", uma_container_->max_sent_height_per_timestamp_, I think this should be encoded_image._encodedWidth instead of max (and same below). uma_container_->max_sent_height_per_timestamp_ holds the max height for a timestamp. https://codereview.webrtc.org/2746903004/diff/80001/webrtc/video/send_statist... webrtc/video/send_statistics_proxy.cc:660: "frame_width", uma_container_->max_sent_width_per_timestamp_, "frame_height" -> "frame_width"
Done, thank you :)
lgtm
The CQ bit was checked by ehmaldonado@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from tommi@webrtc.org Link to the patchset: https://codereview.webrtc.org/2746903004/#ps100001 (title: "Addressed comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489598454171860,
"parent_rev": "8a8e57af86bc7754bd7d786ecc1b2798e4d6a5bd", "commit_rev":
"f7f7fb9cf8706aee5cb2e5ebf5335622f6193eba"}
Message was sent while issue was closed.
Description was changed from ========== Add traces for some video events. This CL adds traces to compute the following metrics that getStats() captures for video: - googFrameRateSent - googFrameRateInput - googFirsReceived - googNacksReceived - googFrameWidthSent - googFrameHeightSent - googAvgEncodeMs - googEncodeUsagePercent BUG=chromium:653087 ========== to ========== Add traces for some video events. This CL adds traces to compute the following metrics that getStats() captures for video: - googFrameRateSent - googFrameRateInput - googFirsReceived - googNacksReceived - googFrameWidthSent - googFrameHeightSent - googAvgEncodeMs - googEncodeUsagePercent BUG=chromium:653087 Review-Url: https://codereview.webrtc.org/2746903004 Cr-Commit-Position: refs/heads/master@{#17260} Committed: https://chromium.googlesource.com/external/webrtc/+/f7f7fb9cf8706aee5cb2e5ebf... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001) as https://chromium.googlesource.com/external/webrtc/+/f7f7fb9cf8706aee5cb2e5ebf... |
