cast: Wire upp logging to be sent over RTCP between receiver and sender.

cast: Wire upp logging to be sent over RTCP between receiver and sender.

Added lists to keep track of the unsent events for audio and video
to be sent via RTCP. This is needed to prevent log messages to be lost
when the log system pulls the messages to be processed by the backend.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246219

[pwestin, 2014-01-13 20:14:19]

Please review

[mikhal1, 2014-01-14 19:57:17]

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_defines.cc (right):

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.cc:12:
CastLoggingConfig::CastLoggingConfig(bool sender)
rename to is_sender for consistency

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_defines.h (right):

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
media/cast/logging/logging_defines.h:40: kAckReceived,
Shouldn't this be divided as well?

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_raw.cc (right):

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
media/cast/logging/logging_raw.cc:116: // TOOD audio or video
nit:todo format

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
media/cast/logging/logging_raw.cc:162: // We don't this frame (RTP timestamp) in
our map.
rephrase

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
media/cast/logging/logging_raw.cc:169: return;  // First event in chain can not
be late by definition.
But can two frames have the same event? How can this trigger?

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/test/end2e...
File media/cast/test/end2end_unittest.cc (right):

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/test/end2e...
media/cast/test/end2end_unittest.cc:1011: kVideoFrameEncoded);
check for not end

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/video_send...
File media/cast/video_sender/video_sender.cc (right):

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/video_send...
media/cast/video_sender/video_sender.cc:254: NOTREACHED();
break;

[pwestin, 2014-01-17 23:46:17]

PTAL

https://codereview.chromium.org/136903003/diff/1/media/cast/logging/logging_d...
File media/cast/logging/logging_defines.cc (right):

https://codereview.chromium.org/136903003/diff/1/media/cast/logging/logging_d...
media/cast/logging/logging_defines.cc:12:
CastLoggingConfig::CastLoggingConfig(bool sender)
On 2014/01/14 19:57:18, mikhal1 wrote:
> rename to is_sender for consistency

that wont compile

https://codereview.chromium.org/136903003/diff/1/media/cast/logging/logging_d...
File media/cast/logging/logging_defines.h (right):

https://codereview.chromium.org/136903003/diff/1/media/cast/logging/logging_d...
media/cast/logging/logging_defines.h:40: kAckReceived,
On 2014/01/14 19:57:18, mikhal1 wrote:
> Shouldn't this be divided as well?

In theory yes but we don't log audio acks since we dont care about them;
renaming to video

https://codereview.chromium.org/136903003/diff/1/media/cast/logging/logging_r...
File media/cast/logging/logging_raw.cc (right):

https://codereview.chromium.org/136903003/diff/1/media/cast/logging/logging_r...
media/cast/logging/logging_raw.cc:116: // TOOD audio or video
On 2014/01/14 19:57:18, mikhal1 wrote:
> nit:todo format

That todo was done

https://codereview.chromium.org/136903003/diff/1/media/cast/logging/logging_r...
media/cast/logging/logging_raw.cc:162: // We don't this frame (RTP timestamp) in
our map.
On 2014/01/14 19:57:18, mikhal1 wrote:
> rephrase

Done.

https://codereview.chromium.org/136903003/diff/1/media/cast/logging/logging_r...
media/cast/logging/logging_raw.cc:169: return;  // First event in chain can not
be late by definition.
On 2014/01/14 19:57:18, mikhal1 wrote:
> But can two frames have the same event? How can this trigger?

Correct but events could be reordered in theory.

https://codereview.chromium.org/136903003/diff/1/media/cast/test/end2end_unit...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/136903003/diff/1/media/cast/test/end2end_unit...
media/cast/test/end2end_unittest.cc:1011: kVideoFrameEncoded);
On 2014/01/14 19:57:18, mikhal1 wrote:
> check for not end

Done.

https://codereview.chromium.org/136903003/diff/1/media/cast/video_sender/vide...
File media/cast/video_sender/video_sender.cc (right):

https://codereview.chromium.org/136903003/diff/1/media/cast/video_sender/vide...
media/cast/video_sender/video_sender.cc:254: NOTREACHED();
On 2014/01/14 19:57:18, mikhal1 wrote:
> break;

Done.

[mikhal1, 2014-01-17 23:48:13]

lgtm + nit

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_raw.cc (right):

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
media/cast/logging/logging_raw.cc:169: return;  // First event in chain can not
be late by definition.
Could you add a comment pls?
On 2014/01/17 23:46:17, pwestin wrote:
> On 2014/01/14 19:57:18, mikhal1 wrote:
> > But can two frames have the same event? How can this trigger?
> 
> Correct but events could be reordered in theory.

[pwestin, 2014-01-17 23:57:22]

fixed nit

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
File media/cast/logging/logging_raw.cc (right):

https://chromiumcodereview.appspot.com/136903003/diff/1/media/cast/logging/lo...
media/cast/logging/logging_raw.cc:169: return;  // First event in chain can not
be late by definition.
On 2014/01/17 23:48:13, mikhal1 wrote:
> Could you add a comment pls?
> On 2014/01/17 23:46:17, pwestin wrote:
> > On 2014/01/14 19:57:18, mikhal1 wrote:
> > > But can two frames have the same event? How can this trigger?
> > 
> > Correct but events could be reordered in theory. 
> 

Done.

[commit-bot: I haz the power, 2014-01-17 23:58:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/136903003/270001

[commit-bot: I haz the power, 2014-01-18 00:23:38]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[pwestin, 2014-01-21 17:07:58]

Ami please review cast_session_delegate.cc

[Ami GONE FROM CHROMIUM, 2014-01-21 19:54:08]

cast_session_delegate.cc LGTM

[Ami GONE FROM CHROMIUM, 2014-01-21 19:54:26]

BTW feel free to TBR trivial changes.


On Tue, Jan 21, 2014 at 9:07 AM, <pwestin@google.com> wrote:

> Ami please review cast_session_delegate.cc
>
> https://chromiumcodereview.appspot.com/136903003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-01-21 20:04:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/136903003/540001

[commit-bot: I haz the power, 2014-01-21 20:04:37]

Failed to apply patch for media/cast/cast.gyp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file media/cast/cast.gyp
  Hunk #1 FAILED at 67.
  1 out of 1 hunk FAILED -- saving rejects to file media/cast/cast.gyp.rej

Patch:       media/cast/cast.gyp
Index: media/cast/cast.gyp
diff --git a/media/cast/cast.gyp b/media/cast/cast.gyp
index
db7d0c51fbb45d40b4aafb9fed1b9c5f47c0860c..3ecbb05210f508d06cb75c19cf1f6b23818b9948
100644
--- a/media/cast/cast.gyp
+++ b/media/cast/cast.gyp
@@ -67,6 +67,7 @@
             'framer/cast_message_builder_unittest.cc',
             'framer/frame_buffer_unittest.cc',
             'framer/framer_unittest.cc',
+            'logging/logging_unittest.cc',
             'rtcp/mock_rtcp_receiver_feedback.cc',
             'rtcp/mock_rtcp_receiver_feedback.h',
             'rtcp/mock_rtcp_sender_feedback.cc',

[Alpha Left Google, 2014-01-21 21:14:44]

Drive-by comments.

https://codereview.chromium.org/136903003/diff/540001/media/cast/logging/logg...
File media/cast/logging/logging_raw.cc (right):

https://codereview.chromium.org/136903003/diff/540001/media/cast/logging/logg...
media/cast/logging/logging_raw.cc:29: InsertRtcpFrameEvent(time_of_event, event,
rtp_timestamp, base::TimeDelta());
It's also not clear to me why we need to insert twice. It seems wasteful to
store the same event twice. Can you please give some comments?

https://codereview.chromium.org/136903003/diff/540001/media/cast/logging/logg...
media/cast/logging/logging_raw.cc:153: if (event != kVideoFrameCaptured &&
It does feel odd that we're doing this filtering internal in the logging
infrastructure. Shouldn't it be the client's responsibility to filter?

https://codereview.chromium.org/136903003/diff/540001/media/cast/logging/logg...
File media/cast/logging/logging_raw.h (right):

https://codereview.chromium.org/136903003/diff/540001/media/cast/logging/logg...
media/cast/logging/logging_raw.h:81: void InsertRtcpFrameEvent(const
base::TimeTicks& time_of_event,
I'm quite confused. What is the purpose of having two "events"? One for
BaseFrameEvent and RtcpFrameEvent? What are they used for and what are they? Can
you please add some comments.

[pwestin, 2014-01-21 22:00:27]

Without doing a massive re-factor this was the easiest way to allow logs to be
sent and received via RTCP to be added without duplicates and missing events.

https://codereview.chromium.org/136903003/diff/540001/media/cast/logging/logg...
File media/cast/logging/logging_raw.cc (right):

https://codereview.chromium.org/136903003/diff/540001/media/cast/logging/logg...
media/cast/logging/logging_raw.cc:29: InsertRtcpFrameEvent(time_of_event, event,
rtp_timestamp, base::TimeDelta());
On 2014/01/21 21:14:45, Alpha wrote:
> It's also not clear to me why we need to insert twice. It seems wasteful to
> store the same event twice. Can you please give some comments?

Due to timing; we dont want to send duplicates of the same event.

https://codereview.chromium.org/136903003/diff/540001/media/cast/logging/logg...
media/cast/logging/logging_raw.cc:153: if (event != kVideoFrameCaptured &&
On 2014/01/21 21:14:45, Alpha wrote:
> It does feel odd that we're doing this filtering internal in the logging
> infrastructure. Shouldn't it be the client's responsibility to filter?

we don't have a specific mechanism for events to be sent via RTCP; we dont want
the one calling log to call twice...

https://codereview.chromium.org/136903003/diff/540001/media/cast/logging/logg...
File media/cast/logging/logging_raw.h (right):

https://codereview.chromium.org/136903003/diff/540001/media/cast/logging/logg...
media/cast/logging/logging_raw.h:81: void InsertRtcpFrameEvent(const
base::TimeTicks& time_of_event,
On 2014/01/21 21:14:45, Alpha wrote:
> I'm quite confused. What is the purpose of having two "events"? One for
> BaseFrameEvent and RtcpFrameEvent? What are they used for and what are they?
Can
> you please add some comments.

They are driven by very different timers and we also don't want to create a
infinite loop between sender and receiver. If the log class could handle
multiple listeners that run in different pace this could be avoided.

[commit-bot: I haz the power, 2014-01-21 22:29:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/136903003/620002

[commit-bot: I haz the power, 2014-01-22 00:30:50]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2014-01-22 01:03:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pwestin@google.com/136903003/620002

[commit-bot: I haz the power, 2014-01-22 02:55:29]

Change committed as 246219