Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2145153002:
Event log visualization tool keeps a map from SSRC to parsed headers (Closed)
DescriptionKeep a map from SSRC to parsed headers in that stream
and use the preparsed headers to plot the network delay changes.
This is the first of several CLs that clean up the visualization
tool to make it easier to add new metrics.
Committed: https://crrev.com/88e64e5c6713bc63cbb21476a1d71153b8e70ac4
Cr-Commit-Position: refs/heads/master@{#13499}
Patch Set 1 #
Total comments: 30
Patch Set 2 : Comments from Philip. #Patch Set 3 : Use emplace_back #
Total comments: 2
Patch Set 4 : Nit #
Messages
Total messages: 29 (13 generated)
Description was changed from ========== Keep a map from SSRC to parsed headers in that stream and use the preparsed headers to plot the network delay changes. This is the first of several CLs that clean up the visualization tool to make it easier to add new metrics. ========== to ========== Keep a map from SSRC to parsed headers in that stream and use the preparsed headers to plot the network delay changes. This is the first of several CLs that clean up the visualization tool to make it easier to add new metrics. ==========
terelius@webrtc.org changed reviewers: + philipel@webrtc.org, stefan@webrtc.org
The CQ bit was checked by terelius@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Please take a look
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:101: size_t header_length, total_length; split to two lines https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:382: WrappingDifference(packet.header.extension.absoluteSendTime, You can include base/mod_ops.h and use the MinDiff function instead. MinDiff<uint64_t, 1 << 24>(packet.header.extension.absoluteSendTime, last_abs_send_time) https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:384: int64_t recv_time_diff = packet.timestamp - last_timestamp; Do we know that |last_timestamp| <= |packet.timestamp| at this point? Maybe add a DCHECK or use MinDiff if that is the intended diff we want to calculate. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:401: time_series.points.push_back(TimeSeriesPoint(x, y)); Change to 'time_series.points.emplace_back(x, y)'. The TimeSeriesPoint object will be constructed in place instead of being constructed then copied. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:421: for (auto& kv : rtp_packets_) { Can |kv| be changed to something more descriptive? Not super important since you define |stream_id| and |packet_stream| shortly after. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:434: double accumulated_delay = 0; accumulated_delay_ms https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:438: WrappingDifference(packet.header.extension.absoluteSendTime, MinDiff() https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:440: int64_t recv_time_diff = packet.timestamp - last_timestamp; Maybe DCHECK or MinDiff()? https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:457: time_series.points.push_back(TimeSeriesPoint(x, accumulated_delay)); emplace_back https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:45: class StreamId { Keep the implementation of StreamId in the .cc file.
https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:101: size_t header_length, total_length; On 2016/07/14 14:36:14, philipel wrote: > split to two lines Done. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:382: WrappingDifference(packet.header.extension.absoluteSendTime, On 2016/07/14 14:36:14, philipel wrote: > You can include base/mod_ops.h and use the MinDiff function instead. > > MinDiff<uint64_t, 1 << 24>(packet.header.extension.absoluteSendTime, > last_abs_send_time) That's not quite the same thing though. I want to compute a-b and choose a representative for the residue class with the smallest possible magnitude. MinDiff computes either a-b or b-a, and chooses the smallest positive representative for each residue class. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:384: int64_t recv_time_diff = packet.timestamp - last_timestamp; On 2016/07/14 14:36:14, philipel wrote: > Do we know that |last_timestamp| <= |packet.timestamp| at this point? Maybe add > a DCHECK or use MinDiff if that is the intended diff we want to calculate. The timestamp refers to the logging time. I think we can guarantee that |last_timestamp| <= |packet.timestamp|, but only because packets belonging to the same SSRC are logged on the same thread. If incoming packets were logged on different threads, there could be a race between reading the time and inserting the header into the log. If packets were slightly reordered in the log compared to their timestamps (because they are being delivered on multiple threads simultaneously), that would just mean that packets don't have a definite arrival order (in any part of WebRTC). We would then be comparing the network latency change relative to some nearby packet, rather than to *the* preceding packet. The distinction is not important in the graph. I don't think a DCHECK would add much value here, but let me know if you disagree. As mentioned before, MinDiff does not quite do what I want. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:401: time_series.points.push_back(TimeSeriesPoint(x, y)); On 2016/07/14 14:36:14, philipel wrote: > Change to 'time_series.points.emplace_back(x, y)'. The TimeSeriesPoint object > will be constructed in place instead of being constructed then copied. The chromium style guide is rather ambiguous about emplace: "When using emplacement for performance reasons, your type should probably be movable (since e.g. a vector of it might be resized); given a movable type, then, consider whether you really need to avoid the move done by push_back(). For readability concerns, treat like auto; sometimes the brevity over push_back() is a win, sometimes a loss." TimeSeriesPoint is a small POD structure so constructing it is cheap. Do you still prefer emplace? https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:421: for (auto& kv : rtp_packets_) { On 2016/07/14 14:36:14, philipel wrote: > Can |kv| be changed to something more descriptive? Not super important since you > define |stream_id| and |packet_stream| shortly after. I agree with you, but for (auto& kv : map) seems to be something of an informal convention for iterating over maps. It is used in ~80 other places in the code. Stefan, any input on this? https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:434: double accumulated_delay = 0; On 2016/07/14 14:36:14, philipel wrote: > accumulated_delay_ms Done. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:438: WrappingDifference(packet.header.extension.absoluteSendTime, On 2016/07/14 14:36:14, philipel wrote: > MinDiff() See above. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:440: int64_t recv_time_diff = packet.timestamp - last_timestamp; On 2016/07/14 14:36:14, philipel wrote: > Maybe DCHECK or MinDiff()? See above. I don't see what a DCHECK would accomplish. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:457: time_series.points.push_back(TimeSeriesPoint(x, accumulated_delay)); On 2016/07/14 14:36:14, philipel wrote: > emplace_back See above. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.h:45: class StreamId { On 2016/07/14 14:36:14, philipel wrote: > Keep the implementation of StreamId in the .cc file. Done.
https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:382: WrappingDifference(packet.header.extension.absoluteSendTime, On 2016/07/18 15:48:13, terelius wrote: > On 2016/07/14 14:36:14, philipel wrote: > > You can include base/mod_ops.h and use the MinDiff function instead. > > > > MinDiff<uint64_t, 1 << 24>(packet.header.extension.absoluteSendTime, > > last_abs_send_time) > > That's not quite the same thing though. I want to compute a-b and choose a > representative for the residue class with the smallest possible magnitude. > MinDiff computes either a-b or b-a, and chooses the smallest positive > representative for each residue class. Right, I see. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:384: int64_t recv_time_diff = packet.timestamp - last_timestamp; On 2016/07/18 15:48:13, terelius wrote: > On 2016/07/14 14:36:14, philipel wrote: > > Do we know that |last_timestamp| <= |packet.timestamp| at this point? Maybe > add > > a DCHECK or use MinDiff if that is the intended diff we want to calculate. > > The timestamp refers to the logging time. I think we can guarantee that > |last_timestamp| <= |packet.timestamp|, but only because packets belonging to > the same SSRC are logged on the same thread. If incoming packets were logged on > different threads, there could be a race between reading the time and inserting > the header into the log. > > If packets were slightly reordered in the log compared to their timestamps > (because they are being delivered on multiple threads simultaneously), that > would just mean that packets don't have a definite arrival order (in any part of > WebRTC). We would then be comparing the network latency change relative to some > nearby packet, rather than to *the* preceding packet. The distinction is not > important in the graph. > > I don't think a DCHECK would add much value here, but let me know if you > disagree. As mentioned before, MinDiff does not quite do what I want. Acknowledged. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:401: time_series.points.push_back(TimeSeriesPoint(x, y)); On 2016/07/18 15:48:13, terelius wrote: > On 2016/07/14 14:36:14, philipel wrote: > > Change to 'time_series.points.emplace_back(x, y)'. The TimeSeriesPoint object > > will be constructed in place instead of being constructed then copied. > > The chromium style guide is rather ambiguous about emplace: > > "When using emplacement for performance reasons, your type should probably be > movable (since e.g. a vector of it might be resized); given a movable type, > then, consider whether you really need to avoid the move done by push_back(). > For readability concerns, treat like auto; sometimes the brevity over > push_back() is a win, sometimes a loss." > > TimeSeriesPoint is a small POD structure so constructing it is cheap. Do you > still prefer emplace? Still prefer emplace_back, the reason is that you use two arguments (x and y) so it's not ambiguous whether an object is being created or not. If it was just one argument I would prefer push_back. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:421: for (auto& kv : rtp_packets_) { On 2016/07/18 15:48:13, terelius wrote: > On 2016/07/14 14:36:14, philipel wrote: > > Can |kv| be changed to something more descriptive? Not super important since > you > > define |stream_id| and |packet_stream| shortly after. > > I agree with you, but for (auto& kv : map) seems to be something of an informal > convention for iterating over maps. It is used in ~80 other places in the code. > > Stefan, any input on this? |kv| is fine. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:438: WrappingDifference(packet.header.extension.absoluteSendTime, On 2016/07/18 15:48:14, terelius wrote: > On 2016/07/14 14:36:14, philipel wrote: > > MinDiff() > > See above. Acknowledged. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:440: int64_t recv_time_diff = packet.timestamp - last_timestamp; On 2016/07/18 15:48:14, terelius wrote: > On 2016/07/14 14:36:14, philipel wrote: > > Maybe DCHECK or MinDiff()? > > See above. I don't see what a DCHECK would accomplish. Acknowledged. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:457: time_series.points.push_back(TimeSeriesPoint(x, accumulated_delay)); On 2016/07/18 15:48:14, terelius wrote: > On 2016/07/14 14:36:14, philipel wrote: > > emplace_back > > See above. See above :)
https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:401: time_series.points.push_back(TimeSeriesPoint(x, y)); On 2016/07/18 16:29:47, philipel wrote: > On 2016/07/18 15:48:13, terelius wrote: > > On 2016/07/14 14:36:14, philipel wrote: > > > Change to 'time_series.points.emplace_back(x, y)'. The TimeSeriesPoint > object > > > will be constructed in place instead of being constructed then copied. > > > > The chromium style guide is rather ambiguous about emplace: > > > > "When using emplacement for performance reasons, your type should probably be > > movable (since e.g. a vector of it might be resized); given a movable type, > > then, consider whether you really need to avoid the move done by push_back(). > > For readability concerns, treat like auto; sometimes the brevity over > > push_back() is a win, sometimes a loss." > > > > TimeSeriesPoint is a small POD structure so constructing it is cheap. Do you > > still prefer emplace? > > Still prefer emplace_back, the reason is that you use two arguments (x and y) so > it's not ambiguous whether an object is being created or not. If it was just one > argument I would prefer push_back. Done. https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:457: time_series.points.push_back(TimeSeriesPoint(x, accumulated_delay)); On 2016/07/18 16:29:47, philipel wrote: > On 2016/07/18 15:48:14, terelius wrote: > > On 2016/07/14 14:36:14, philipel wrote: > > > emplace_back > > > > See above. > > See above :) Done.
lgtm
https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... File webrtc/tools/event_log_visualizer/analyzer.cc (right): https://codereview.webrtc.org/2145153002/diff/1/webrtc/tools/event_log_visual... webrtc/tools/event_log_visualizer/analyzer.cc:421: for (auto& kv : rtp_packets_) { On 2016/07/18 15:48:13, terelius wrote: > On 2016/07/14 14:36:14, philipel wrote: > > Can |kv| be changed to something more descriptive? Not super important since > you > > define |stream_id| and |packet_stream| shortly after. > > I agree with you, but for (auto& kv : map) seems to be something of an informal > convention for iterating over maps. It is used in ~80 other places in the code. > > Stefan, any input on this? I think kv is ok the same way "it" is used for iterators. It doesn't add much value to call something rtp_packet_iterator or, e.g., id_packet_pair. Or maybe the later is useful... I've used kv a lot in the past.
lgtm with nit fixed. https://codereview.webrtc.org/2145153002/diff/40001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2145153002/diff/40001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.h:79: std::map<StreamId, std::vector<LoggedRtpPacket> > rtp_packets_; I think you can remove the space between > >
https://codereview.webrtc.org/2145153002/diff/40001/webrtc/tools/event_log_vi... File webrtc/tools/event_log_visualizer/analyzer.h (right): https://codereview.webrtc.org/2145153002/diff/40001/webrtc/tools/event_log_vi... webrtc/tools/event_log_visualizer/analyzer.h:79: std::map<StreamId, std::vector<LoggedRtpPacket> > rtp_packets_; On 2016/07/18 17:10:59, stefan-webrtc (holmer) wrote: > I think you can remove the space between > > Done. Old habit from pre C++11 era.
The CQ bit was checked by terelius@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from philipel@webrtc.org, stefan@webrtc.org Link to the patchset: https://codereview.webrtc.org/2145153002/#ps60001 (title: "Nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/6894)
terelius@webrtc.org changed reviewers: + phoglund@webrtc.org
rs lgtm
The CQ bit was checked by terelius@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Keep a map from SSRC to parsed headers in that stream and use the preparsed headers to plot the network delay changes. This is the first of several CLs that clean up the visualization tool to make it easier to add new metrics. ========== to ========== Keep a map from SSRC to parsed headers in that stream and use the preparsed headers to plot the network delay changes. This is the first of several CLs that clean up the visualization tool to make it easier to add new metrics. ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== Keep a map from SSRC to parsed headers in that stream and use the preparsed headers to plot the network delay changes. This is the first of several CLs that clean up the visualization tool to make it easier to add new metrics. ========== to ========== Keep a map from SSRC to parsed headers in that stream and use the preparsed headers to plot the network delay changes. This is the first of several CLs that clean up the visualization tool to make it easier to add new metrics. Committed: https://crrev.com/88e64e5c6713bc63cbb21476a1d71153b8e70ac4 Cr-Commit-Position: refs/heads/master@{#13499} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/88e64e5c6713bc63cbb21476a1d71153b8e70ac4 Cr-Commit-Position: refs/heads/master@{#13499} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
