Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(277)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 196433002: Cast: Log sender side packet events. (Closed)

Created:
6 years, 9 months ago by imcheng
Modified:
4 years, 11 months ago
CC:
chromium-reviews, hclam+watch_chromium.org, mikhal+watch_chromium.org, hguihot+watch_chromium.org, jasonroberts+watch_google.com, pwestin+watch_google.com, feature-media-reviews_chromium.org, jshin+watch_chromium.org, miu+watch_chromium.org, hubbe+watch_chromium.org, imcheng+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Visibility:
Public.

Description

Cast: Log sender side packet events. Split kPacketSentToPacer, kPacketSentToNetwork, kPacketRetransmitted into audio/video events. Pass LoggingImpl to PacedSender / RtpPacketizer so that they can insert packet events. RtpPacketizer - k{Audio,Video}PacketSentToPacer PacedSender - k{Audio,Video}SentToNetwork, k{A,V}PacketRetransmitted Per https://codereview.chromium.org/178073004/ these events will be sent back to the renderer process via IPC. BUG=343992 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257361

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : Added tests #

Patch Set 4 : Rebase #

Total comments: 4
Unified diffs Side-by-side diffs Delta from patch set Stats (+389 lines, -110 lines) Patch
M media/cast/cast_receiver_impl.cc View 1 chunk +1 line, -0 lines 0 comments Download
M media/cast/logging/encoding_event_subscriber_unittest.cc View 6 chunks +54 lines, -27 lines 0 comments Download
M media/cast/logging/logging_defines.h View 1 chunk +6 lines, -3 lines 0 comments Download
M media/cast/logging/logging_defines.cc View 4 chunks +12 lines, -7 lines 0 comments Download
M media/cast/logging/logging_impl.h View 1 chunk +4 lines, -0 lines 0 comments Download
M media/cast/logging/logging_impl.cc View 1 2 1 chunk +30 lines, -16 lines 0 comments Download
M media/cast/logging/proto/proto_utils.cc View 1 chunk +6 lines, -3 lines 0 comments Download
M media/cast/logging/proto/raw_events.proto View 1 chunk +10 lines, -7 lines 0 comments Download
M media/cast/logging/serialize_deserialize_test.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M media/cast/transport/cast_transport_sender_impl.h View 1 chunk +1 line, -2 lines 0 comments Download
M media/cast/transport/cast_transport_sender_impl.cc View 1 2 3 chunks +6 lines, -3 lines 0 comments Download
M media/cast/transport/pacing/paced_sender.h View 1 2 3 chunks +17 lines, -4 lines 0 comments Download
M media/cast/transport/pacing/paced_sender.cc View 1 2 7 chunks +70 lines, -4 lines 2 comments Download
M media/cast/transport/pacing/paced_sender_unittest.cc View 1 2 11 chunks +99 lines, -20 lines 2 comments Download
M media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer.h View 3 chunks +13 lines, -1 line 0 comments Download
M media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer.cc View 2 chunks +10 lines, -1 line 0 comments Download
M media/cast/transport/rtp_sender/rtp_packetizer/rtp_packetizer_unittest.cc View 1 2 5 chunks +26 lines, -6 lines 0 comments Download
M media/cast/transport/rtp_sender/rtp_sender.h View 3 chunks +5 lines, -0 lines 0 comments Download
M media/cast/transport/rtp_sender/rtp_sender.cc View 3 chunks +6 lines, -2 lines 0 comments Download
M media/cast/transport/transport_audio_sender.h View 2 chunks +4 lines, -0 lines 0 comments Download
M media/cast/transport/transport_audio_sender.cc View 1 chunk +2 lines, -1 line 0 comments Download
M media/cast/transport/transport_video_sender.h View 2 chunks +3 lines, -0 lines 0 comments Download
M media/cast/transport/transport_video_sender.cc View 1 chunk +2 lines, -1 line 0 comments Download

Messages

Total messages: 16 (1 generated)
imcheng
6 years, 9 months ago (2014-03-12 09:35:42 UTC) #1
imcheng
Ping.
6 years, 9 months ago (2014-03-13 17:59:41 UTC) #2
Alpha Left Google
LGTM after one nit. https://codereview.chromium.org/196433002/diff/60001/media/cast/transport/pacing/paced_sender.cc File media/cast/transport/pacing/paced_sender.cc (right): https://codereview.chromium.org/196433002/diff/60001/media/cast/transport/pacing/paced_sender.cc#newcode205 media/cast/transport/pacing/paced_sender.cc:205: bool is_audio; This can be ...
6 years, 9 months ago (2014-03-13 19:56:14 UTC) #3
imcheng
https://codereview.chromium.org/196433002/diff/60001/media/cast/transport/pacing/paced_sender.cc File media/cast/transport/pacing/paced_sender.cc (right): https://codereview.chromium.org/196433002/diff/60001/media/cast/transport/pacing/paced_sender.cc#newcode205 media/cast/transport/pacing/paced_sender.cc:205: bool is_audio; On 2014/03/13 19:56:14, Alpha wrote: > This ...
6 years, 9 months ago (2014-03-13 21:48:58 UTC) #4
Alpha Left Google
On 2014/03/13 21:48:58, imcheng1 wrote: > https://codereview.chromium.org/196433002/diff/60001/media/cast/transport/pacing/paced_sender.cc > File media/cast/transport/pacing/paced_sender.cc (right): > > https://codereview.chromium.org/196433002/diff/60001/media/cast/transport/pacing/paced_sender.cc#newcode205 > ...
6 years, 9 months ago (2014-03-16 03:53:36 UTC) #5
Alpha Left Google
The CQ bit was checked by hclam@chromium.org
6 years, 9 months ago (2014-03-16 03:53:41 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/196433002/60001
6 years, 9 months ago (2014-03-16 03:53:46 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/196433002/60001
6 years, 9 months ago (2014-03-16 04:42:38 UTC) #8
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 9 months ago (2014-03-16 05:11:59 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: tryserver.chromium on linux_chromium_chromeos_rel
6 years, 9 months ago (2014-03-16 05:12:00 UTC) #10
Alpha Left Google
The CQ bit was checked by hclam@chromium.org
6 years, 9 months ago (2014-03-16 05:12:37 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/imcheng@chromium.org/196433002/60001
6 years, 9 months ago (2014-03-16 05:12:42 UTC) #12
commit-bot: I haz the power
Change committed as 257361
6 years, 9 months ago (2014-03-16 06:37:59 UTC) #13
Nico
https://codereview.chromium.org/196433002/diff/60001/media/cast/transport/pacing/paced_sender_unittest.cc File media/cast/transport/pacing/paced_sender_unittest.cc (right): https://codereview.chromium.org/196433002/diff/60001/media/cast/transport/pacing/paced_sender_unittest.cc#newcode98 media/cast/transport/pacing/paced_sender_unittest.cc:98: i++; Is this intentional? A new warning I'm testing ...
4 years, 11 months ago (2016-01-05 18:56:13 UTC) #15
imcheng
4 years, 11 months ago (2016-01-05 19:18:10 UTC) #16
Message was sent while issue was closed. 
https://codereview.chromium.org/196433002/diff/60001/media/cast/transport/pac...
File media/cast/transport/pacing/paced_sender_unittest.cc (right):

https://codereview.chromium.org/196433002/diff/60001/media/cast/transport/pac...
media/cast/transport/pacing/paced_sender_unittest.cc:98: i++;
On 2016/01/05 18:56:13, Nico wrote:
> Is this intentional? A new warning I'm testing says:
> 
> ../../media/cast/net/pacing/paced_sender_unittest.cc:121:7: error: variable
'i'
> is incremented both in the loop header and in the loop body
> [-Werror,-Wfor-loop-analysis]
>       i++;
>       ^

Looks like a mistake. It should be removed.

Powered by Google App Engine
This is Rietveld 408576698