Add Cast PLI support on sender side.

Add Cast PLI support on sender side.
Main changes when receive a PLI message:
1. Request encoding a Key frame (for now keep the same resolution and bitrate).
2. Clear sending remaining frames before sending out the key frame.

BUG=589934
Committed: https://crrev.com/b9733f71a04374f7ec237ed01be627a9b47950dc
Cr-Commit-Position: refs/heads/master@{#381546}

[xjz, 2016-02-25 18:11:24]

Patchset #1 (id:1) has been deleted

[xjz, 2016-02-25 19:58:59]

Description was changed from

==========
Add Cast PLI support on sender side.

BUG=
==========

to

==========
Add Cast PLI support on sender side.
Main changes when receive a PLI message:
1. Request encoding a Key frame (for now keep the same resolution and bitrate).
2. Clear sending remaining frames before sending out the key frame.

BUG=589934
==========

[xjz, 2016-02-25 19:59:16]

Patchset #1 (id:20001) has been deleted

[xjz, 2016-02-25 21:28:07]

xjz@chromium.org changed reviewers:
+ isheriff@chromium.org, miu@chromium.org

[xjz, 2016-02-25 21:28:07]

PTAL. Add cast PLI message support and some unit tests.

[miu, 2016-02-26 23:36:07]

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rec...
File media/cast/net/rtcp/receiver_rtcp_session.cc (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rec...
media/cast/net/rtcp/receiver_rtcp_session.cc:101: const RtcpCastMessage*
cast_message,
Instead of including |picture_loss_indicator| in |cast_message|, please specify
it as a separate argument here.

In the case where you want to send a PLI but not an RRTR, you should modify this
method to handle an optionally null |cast_message|.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rec...
media/cast/net/rtcp/receiver_rtcp_session.cc:137: (cast_message &&
cast_message->picture_loss_indicator) ? nullptr
Separate argument for PLI here too.

Also, it seems like you can send the RRTR along with the PLI, if |cast_message|
was not null.  See comments in rtcp_builder.cc.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_builder.cc (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_builder.cc:167: const RtcpCastMessage* cast_message,
Add PLI argument here.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_builder.cc:241: return;
Remove this, since you'll want to call AddPli() from BuildRtcpFromReceiver(), as
noted above.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_defines.h (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_defines.h:54: bool picture_loss_indicator;
This isn't part of the Cast Feedback message.  I'd suggest removing this and
instead and adding some simple plumbing in the receiver code (see comment in
receiver_rtcp_session.cc).

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_utility.cc (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_utility.cc:61: // Check for receiving PLI.
Please move this to the case kPacketTypePayloadSpecific block below.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_utility.cc:62: if ((header.IC == 1) && (header.P == 0)
&&
OOC, why do we check header.P?  Doesn't seem to matter.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_utility.cc:206: // Ignore this  Rtcp if the sender ssrc
does not match.
nit: extra space

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_utility.h (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_utility.h:70: bool has_received_pli() const { return
received_picture_loss_indicator_; }
naming consistency: has_picture_loss_indicator() and has_picture_loss_indicator_

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/sen...
File media/cast/net/rtcp/sender_rtcp_session.cc (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/sen...
media/cast/net/rtcp/sender_rtcp_session.cc:121: return true;
Don't return early.  The parser may have other things...

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/frame...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:95: // Receiving PLI (picture loss indicator)
from the receiver.
s/Receiving/Called when receiving a/

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:172: bool picture_lost_at_receiver_;
Need comment.  Explain when it is set and when it gets cleared.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/video...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/video...
media/cast/sender/video_sender.cc:47: const base::TimeDelta
kMinKeyFrameRequestDuration =
Static initializer foo: In Chromium code, we can only use POD types for
constants.  So, this needs to become (with naming change, based on later
comments):

  const int kMinPliResolveIntervalMs = 500;

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/video...
media/cast/sender/video_sender.cc:165: if (picture_lost_at_receiver_) {
The client may call InsertRawVideoFrame() several times before
|last_encoded_key_frame_| is updated in OnEncodedVideoFrame().  That means we
would be making several redundant requests for key frames in the meantime.

Instead, what if the |last_encoded_key_frame_| timestamp became a
|last_attempt_to_resolve_picture_loss_| timestamp?  Then, the logic here would
look like:

  if (picture_lost_at_receiver_) {
    if (last_attempt_to_resolve_picture_loss_.is_null() ||
        ((reference_time -
last_attempt_to_resolve_picture_loss_).InMilliseconds() >
kMinPliResolveIntervalMs)) {
      video_encoder_->GenerateKeyFrame();
      last_attempt_to_resolve_picture_loss_ = reference_time;
    }
    // Note: |picture_lost_at_receiver_| is cleared in FrameSender
when...blahblahblah...
  }

Also, let's move all this down to line 210.  That ensures the reference_time is
valid (monotonically increasing), which is important for this logic to work
correctly.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/video...
File media/cast/sender/video_sender.h (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/video...
media/cast/sender/video_sender.h:107: base::TimeTicks last_encoded_key_frame_;
Need comment.  What it is, and why we're tracking it.

[xjz, 2016-02-27 05:53:32]

PTAL

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rec...
File media/cast/net/rtcp/receiver_rtcp_session.cc (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rec...
media/cast/net/rtcp/receiver_rtcp_session.cc:101: const RtcpCastMessage*
cast_message,
On 2016/02/26 23:36:06, miu wrote:
> Instead of including |picture_loss_indicator| in |cast_message|, please
specify
> it as a separate argument here.
> 
> In the case where you want to send a PLI but not an RRTR, you should modify
this
> method to handle an optionally null |cast_message|.

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rec...
media/cast/net/rtcp/receiver_rtcp_session.cc:137: (cast_message &&
cast_message->picture_loss_indicator) ? nullptr
On 2016/02/26 23:36:06, miu wrote:
> Separate argument for PLI here too.
> 
> Also, it seems like you can send the RRTR along with the PLI, if
|cast_message|
> was not null.  See comments in rtcp_builder.cc.

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_builder.cc (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_builder.cc:167: const RtcpCastMessage* cast_message,
On 2016/02/26 23:36:06, miu wrote:
> Add PLI argument here.

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_builder.cc:241: return;
On 2016/02/26 23:36:06, miu wrote:
> Remove this, since you'll want to call AddPli() from BuildRtcpFromReceiver(),
as
> noted above.

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_defines.h (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_defines.h:54: bool picture_loss_indicator;
On 2016/02/26 23:36:06, miu wrote:
> This isn't part of the Cast Feedback message.  I'd suggest removing this and
> instead and adding some simple plumbing in the receiver code (see comment in
> receiver_rtcp_session.cc).

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_utility.cc (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_utility.cc:61: // Check for receiving PLI.
On 2016/02/26 23:36:07, miu wrote:
> Please move this to the case kPacketTypePayloadSpecific block below.

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_utility.cc:62: if ((header.IC == 1) && (header.P == 0)
&&
On 2016/02/26 23:36:07, miu wrote:
> OOC, why do we check header.P?  Doesn't seem to matter.

Removed.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_utility.cc:206: // Ignore this  Rtcp if the sender ssrc
does not match.
On 2016/02/26 23:36:07, miu wrote:
> nit: extra space

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_utility.h (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_utility.h:70: bool has_received_pli() const { return
received_picture_loss_indicator_; }
On 2016/02/26 23:36:07, miu wrote:
> naming consistency: has_picture_loss_indicator() and
has_picture_loss_indicator_

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/sen...
File media/cast/net/rtcp/sender_rtcp_session.cc (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/net/rtcp/sen...
media/cast/net/rtcp/sender_rtcp_session.cc:121: return true;
On 2016/02/26 23:36:07, miu wrote:
> Don't return early.  The parser may have other things...

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/frame...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:95: // Receiving PLI (picture loss indicator)
from the receiver.
On 2016/02/26 23:36:07, miu wrote:
> s/Receiving/Called when receiving a/

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:172: bool picture_lost_at_receiver_;
On 2016/02/26 23:36:07, miu wrote:
> Need comment.  Explain when it is set and when it gets cleared.

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/video...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/video...
media/cast/sender/video_sender.cc:47: const base::TimeDelta
kMinKeyFrameRequestDuration =
On 2016/02/26 23:36:07, miu wrote:
> Static initializer foo: In Chromium code, we can only use POD types for
> constants.  So, this needs to become (with naming change, based on later
> comments):
> 
>   const int kMinPliResolveIntervalMs = 500;

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/video...
media/cast/sender/video_sender.cc:165: if (picture_lost_at_receiver_) {
On 2016/02/26 23:36:07, miu wrote:
> The client may call InsertRawVideoFrame() several times before
> |last_encoded_key_frame_| is updated in OnEncodedVideoFrame().  That means we
> would be making several redundant requests for key frames in the meantime.
> 
> Instead, what if the |last_encoded_key_frame_| timestamp became a
> |last_attempt_to_resolve_picture_loss_| timestamp?  Then, the logic here would
> look like:
> 
>   if (picture_lost_at_receiver_) {
>     if (last_attempt_to_resolve_picture_loss_.is_null() ||
>         ((reference_time -
> last_attempt_to_resolve_picture_loss_).InMilliseconds() >
> kMinPliResolveIntervalMs)) {
>       video_encoder_->GenerateKeyFrame();
>       last_attempt_to_resolve_picture_loss_ = reference_time;
>     }
>     // Note: |picture_lost_at_receiver_| is cleared in FrameSender
> when...blahblahblah...
>   }
> 
> Also, let's move all this down to line 210.  That ensures the reference_time
is
> valid (monotonically increasing), which is important for this logic to work
> correctly.

Done.

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/video...
File media/cast/sender/video_sender.h (right):

https://codereview.chromium.org/1709863002/diff/40001/media/cast/sender/video...
media/cast/sender/video_sender.h:107: base::TimeTicks last_encoded_key_frame_;
On 2016/02/26 23:36:07, miu wrote:
> Need comment.  What it is, and why we're tracking it.

Done.

[Irfan, 2016-02-29 16:01:24]

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_utility_unittest.cc (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_utility_unittest.cc:348: RtcpParser
parser1(kSourceSsrc, kSenderSsrc);
I find kSourceSssrc and kSenderSsrc unclear. Should they be renamed kLocalSsrc
kRemoteSsrc ?

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/sen...
File media/cast/net/rtcp/sender_rtcp_session.cc (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/sen...
media/cast/net/rtcp/sender_rtcp_session.cc:120: OnReceivedPli();
For readability, perhaps a few of these OnReceived functions can be eliminated
and code inlined here since it is a callback invocation

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/tes...
File media/cast/net/rtcp/test_rtcp_packet_builder.cc (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/tes...
media/cast/net/rtcp/test_rtcp_packet_builder.cc:186: void
TestRtcpPacketBuilder::AddPli(uint32_t sender_ssrc, uint32_t media_ssrc) {
Same as previous comment. I find "sender" and "media" on SSRC unclear, does
local and remote make sense here as well ?

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/frame...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:174: bool picture_lost_at_receiver_;
may be: This flag is set to true when a PLI message is received. It is cleared
once the FrameSender scheduled an encoded key frame to be sent.

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
media/cast/sender/video_sender.cc:44: // This is the minimum duration in
milliseconds that the sender will response
will respond

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
media/cast/sender/video_sender.cc:46: const int kMinPliResolveIntervalMs = 500;
may be: kMinPliResponseIntervaleMs

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
File media/cast/sender/video_sender.h (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
media/cast/sender/video_sender.h:110: base::TimeTicks
last_attempt_to_resolve_picture_loss_;
think name needs to indicate it tracks time. do not have good suggestions. may
be: last_pli_resolution_time ?

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
File media/cast/sender/video_sender_unittest.cc (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
media/cast/sender/video_sender_unittest.cc:628: 
do we need an EXPECT_EQ on how many RTP packets we expect before they get
cancelled ?

[xjz, 2016-02-29 19:48:59]

PTAL

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/rtc...
File media/cast/net/rtcp/rtcp_utility_unittest.cc (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/rtc...
media/cast/net/rtcp/rtcp_utility_unittest.cc:348: RtcpParser
parser1(kSourceSsrc, kSenderSsrc);
On 2016/02/29 16:01:23, Irfan wrote:
> I find kSourceSssrc and kSenderSsrc unclear. Should they be renamed kLocalSsrc
> kRemoteSsrc ?

Done.

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/sen...
File media/cast/net/rtcp/sender_rtcp_session.cc (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/sen...
media/cast/net/rtcp/sender_rtcp_session.cc:120: OnReceivedPli();
On 2016/02/29 16:01:23, Irfan wrote:
> For readability, perhaps a few of these OnReceived functions can be eliminated
> and code inlined here since it is a callback invocation

Done.

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/tes...
File media/cast/net/rtcp/test_rtcp_packet_builder.cc (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/net/rtcp/tes...
media/cast/net/rtcp/test_rtcp_packet_builder.cc:186: void
TestRtcpPacketBuilder::AddPli(uint32_t sender_ssrc, uint32_t media_ssrc) {
On 2016/02/29 16:01:23, Irfan wrote:
> Same as previous comment. I find "sender" and "media" on SSRC unclear, does
> local and remote make sense here as well ?

Done.

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/frame...
File media/cast/sender/frame_sender.h (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/frame...
media/cast/sender/frame_sender.h:174: bool picture_lost_at_receiver_;
On 2016/02/29 16:01:23, Irfan wrote:
> may be: This flag is set to true when a PLI message is received. It is cleared
> once the FrameSender scheduled an encoded key frame to be sent.

Done.

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
media/cast/sender/video_sender.cc:44: // This is the minimum duration in
milliseconds that the sender will response
On 2016/02/29 16:01:23, Irfan wrote:
> will respond

Done.

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
media/cast/sender/video_sender.cc:46: const int kMinPliResolveIntervalMs = 500;
On 2016/02/29 16:01:23, Irfan wrote:
> may be: kMinPliResponseIntervaleMs

Done.

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
File media/cast/sender/video_sender.h (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
media/cast/sender/video_sender.h:110: base::TimeTicks
last_attempt_to_resolve_picture_loss_;
On 2016/02/29 16:01:23, Irfan wrote:
> think name needs to indicate it tracks time. do not have good suggestions. may
> be: last_pli_resolution_time ?

Done. Renamed it to |last_time_attempted_to_resolve_pli_|.

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
File media/cast/sender/video_sender_unittest.cc (right):

https://codereview.chromium.org/1709863002/diff/60001/media/cast/sender/video...
media/cast/sender/video_sender_unittest.cc:628: 
On 2016/02/29 16:01:23, Irfan wrote:
> do we need an EXPECT_EQ on how many RTP packets we expect before they get
> cancelled ?

This check was done on Line 619 before paused the transport. Moved it here to
make it more obvious.

[miu, 2016-02-29 23:00:45]

https://codereview.chromium.org/1709863002/diff/80001/media/cast/sender/video...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1709863002/diff/80001/media/cast/sender/video...
media/cast/sender/video_sender.cc:44: // This is the minimum duration in
milliseconds that the sender will respond to
Hmm...This comment and var name still aren't quite right.  This is really the
"attempt interval."  The sender will respond immediately (without waiting) to
PLI's.  This variable just controls how often, after each attempt, the sender
will wait before trying to force another key frame while the PLI condition is
still present.

https://codereview.chromium.org/1709863002/diff/80001/media/cast/sender/video...
media/cast/sender/video_sender.cc:205: .InMilliseconds() >
kMinPliResponseIntervalMs)) {
BTW--Thinking about this a bit more: Is 500 ms enough time?  What if, for
example, the target playout delay is set to 1000 ms?  Perhaps, instead of this
constant, we should make the interval between attempts some multiple of the
target playout delay.  OTOH:

  const int64_t min_attempt_interval_ms =
      std::max<int64_t>(500,
                        6 * target_playout_delay_.InMilliseconds());

Feel free to argue this point!  :)  I'm not exactly sure what the right answer
is here.  PLI will occur when there is lots of network congestion (packets are
being dropped).  So, I'm thinking we want to make sure sending large KFs too
frequently isn't going to make the problem even worse.

https://codereview.chromium.org/1709863002/diff/80001/media/cast/test/end2end...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/1709863002/diff/80001/media/cast/test/end2end...
media/cast/test/end2end_unittest.cc:1290: TEST_F(End2EndTest,
TestPliGeneratingKeyFrames) {
IMO, we don't need this extra test here.  These end-to-end tests are more meant
to be sender<-->receiver integration tests.  What I mean is, if the Chromium
receiver actually used the PLI feature, the relevant end-to-end test to write
here would be something like End2EndTest.EvilNetwork, but in a low-latency mode
where we've allowed decoding to proceed even with missing packets.

[xjz, 2016-03-01 01:03:41]

Patchset #4 (id:100001) has been deleted

[xjz, 2016-03-01 01:14:28]

PTAL

https://codereview.chromium.org/1709863002/diff/80001/media/cast/sender/video...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1709863002/diff/80001/media/cast/sender/video...
media/cast/sender/video_sender.cc:44: // This is the minimum duration in
milliseconds that the sender will respond to
On 2016/02/29 23:00:45, miu wrote:
> Hmm...This comment and var name still aren't quite right.  This is really the
> "attempt interval."  The sender will respond immediately (without waiting) to
> PLI's.  This variable just controls how often, after each attempt, the sender
> will wait before trying to force another key frame while the PLI condition is
> still present.

Renamed to |kMinKeyFrameRequestOnPliIntervalMs|.

https://codereview.chromium.org/1709863002/diff/80001/media/cast/sender/video...
media/cast/sender/video_sender.cc:205: .InMilliseconds() >
kMinPliResponseIntervalMs)) {
On 2016/02/29 23:00:45, miu wrote:
> BTW--Thinking about this a bit more: Is 500 ms enough time?  What if, for
> example, the target playout delay is set to 1000 ms?  Perhaps, instead of this
> constant, we should make the interval between attempts some multiple of the
> target playout delay.  OTOH:
> 
>   const int64_t min_attempt_interval_ms =
>       std::max<int64_t>(500,
>                         6 * target_playout_delay_.InMilliseconds());
> 
> Feel free to argue this point!  :)  I'm not exactly sure what the right answer
> is here.  PLI will occur when there is lots of network congestion (packets are
> being dropped).  So, I'm thinking we want to make sure sending large KFs too
> frequently isn't going to make the problem even worse.

I agree. Frequent key frames also reduce the video quality. Took the suggestion.

https://codereview.chromium.org/1709863002/diff/80001/media/cast/test/end2end...
File media/cast/test/end2end_unittest.cc (right):

https://codereview.chromium.org/1709863002/diff/80001/media/cast/test/end2end...
media/cast/test/end2end_unittest.cc:1290: TEST_F(End2EndTest,
TestPliGeneratingKeyFrames) {
On 2016/02/29 23:00:45, miu wrote:
> IMO, we don't need this extra test here.  These end-to-end tests are more
meant
> to be sender<-->receiver integration tests.  What I mean is, if the Chromium
> receiver actually used the PLI feature, the relevant end-to-end test to write
> here would be something like End2EndTest.EvilNetwork, but in a low-latency
mode
> where we've allowed decoding to proceed even with missing packets.

Removed this test.

[miu, 2016-03-01 09:50:25]

lgtm

[xjz, 2016-03-01 17:34:36]

The CQ bit was checked by xjz@chromium.org

[commit-bot: I haz the power, 2016-03-01 17:35:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1709863002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1709863002/120001

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

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-01 17:46:23]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[xjz, 2016-03-01 19:00:54]

xjz@chromium.org changed reviewers:
+ dcheng@chromium.org

[xjz, 2016-03-01 19:00:55]

dcheng: PTAL. Need Owner's approval on chrome/common/cast_messages.h.

[dcheng, 2016-03-01 23:44:13]

https://codereview.chromium.org/1709863002/diff/120001/chrome/common/cast_mes...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/1709863002/diff/120001/chrome/common/cast_mes...
chrome/common/cast_messages.h:153: IPC_MESSAGE_CONTROL2(CastMsg_Pli, int32_t /*
channel_id */, uint32_t /* ssrc */)
These messages need more documentation in general. What's a ssrc, for example?

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:125:
params.pli_message.reset(
Maybe the parameters shouldn't be const?

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:141: // serializes it and
remember the serialized form instead.
... so who frees this memory?

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
File media/cast/net/rtcp/rtcp_builder.cc (right):

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_builder.cc:233: void RtcpBuilder::AddPli(const
RtcpPliMessage* pli_message) {
How about const RtcpPliMessage&? This should never be null right?

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
File media/cast/net/rtcp/rtcp_defines.h (right):

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_defines.h:65: RtcpPliMessage();
Is the default ctor necessary?

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_defines.h:66: ~RtcpPliMessage();
Don't need an explicit dtor: this is a trivial class, so the dtor won't actually
do anything.

https://codereview.chromium.org/1709863002/diff/120001/media/cast/sender/vide...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1709863002/diff/120001/media/cast/sender/vide...
media/cast/sender/video_sender.cc:206:
std::max<int64_t>(kMinKeyFrameRequestOnPliIntervalMs,
Just make the constant a int64_t: then I think you won't need to explicitly
specify a template param.

[xjz, 2016-03-02 04:24:17]

PTAL

https://codereview.chromium.org/1709863002/diff/120001/chrome/common/cast_mes...
File chrome/common/cast_messages.h (right):

https://codereview.chromium.org/1709863002/diff/120001/chrome/common/cast_mes...
chrome/common/cast_messages.h:153: IPC_MESSAGE_CONTROL2(CastMsg_Pli, int32_t /*
channel_id */, uint32_t /* ssrc */)
On 2016/03/01 23:44:12, dcheng wrote:
> These messages need more documentation in general. What's a ssrc, for example?

Add comment.

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:125:
params.pli_message.reset(
On 2016/03/01 23:44:12, dcheng wrote:
> Maybe the parameters shouldn't be const?

The const_cast is used here only for the purpose to set the scoped ptrs in
|params|. The content will not be modified.

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:141: // serializes it and
remember the serialized form instead.
On 2016/03/01 23:44:12, dcheng wrote:
> ... so who frees this memory?

It will be released below (line 146).

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
File media/cast/net/rtcp/rtcp_builder.cc (right):

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_builder.cc:233: void RtcpBuilder::AddPli(const
RtcpPliMessage* pli_message) {
On 2016/03/01 23:44:12, dcheng wrote:
> How about const RtcpPliMessage&? This should never be null right?

Done.

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
File media/cast/net/rtcp/rtcp_defines.h (right):

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_defines.h:65: RtcpPliMessage();
On 2016/03/01 23:44:13, dcheng wrote:
> Is the default ctor necessary?

Removed.

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_defines.h:66: ~RtcpPliMessage();
On 2016/03/01 23:44:12, dcheng wrote:
> Don't need an explicit dtor: this is a trivial class, so the dtor won't
actually
> do anything.

Done. Removed.

https://codereview.chromium.org/1709863002/diff/120001/media/cast/sender/vide...
File media/cast/sender/video_sender.cc (right):

https://codereview.chromium.org/1709863002/diff/120001/media/cast/sender/vide...
media/cast/sender/video_sender.cc:206:
std::max<int64_t>(kMinKeyFrameRequestOnPliIntervalMs,
On 2016/03/01 23:44:13, dcheng wrote:
> Just make the constant a int64_t: then I think you won't need to explicitly
> specify a template param.

Done.

[dcheng, 2016-03-02 07:41:24]

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:125:
params.pli_message.reset(
On 2016/03/02 at 04:24:17, xjz wrote:
> On 2016/03/01 23:44:12, dcheng wrote:
> > Maybe the parameters shouldn't be const?
> 
> The const_cast is used here only for the purpose to set the scoped ptrs in
|params|. The content will not be modified.

But what's the point of making it const? Either the scoped_ptr<> itself should
contain a const pointer (rare) or the parameters should just be non-const.
const_cast<> is usually a sign to the reader that they have to pay more
attention because something tricky is happening. Using it like this just adds
noise to the signal.

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:141: // serializes it and
remember the serialized form instead.
On 2016/03/02 at 04:24:17, xjz wrote:
> On 2016/03/01 23:44:12, dcheng wrote:
> > ... so who frees this memory?
> 
> It will be released below (line 146).

Reading the code, I understand how it works now: on the sending side, they are
unowned pointers but we stick them into a scoped_ptr anyway… and on the
receiving side, they need to be owned. But this is really weird and not a good
way to use scoped_ptr.

However, this is complex and hard to understand. Instead of having one uber
message with optional fields, can we just split this up into different IPC
messages?

In addition, once it's split up into different messages, there's no need to
worry about copies: IPCs bind to the arguments as const T&, and it makes no copy
before serializing arguments into the wire format.

[xjz, 2016-03-02 18:35:08]

On 2016/03/02 07:41:24, dcheng wrote:
>
https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
> File chrome/renderer/media/cast_transport_sender_ipc.cc (right):
> 
>
https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
> chrome/renderer/media/cast_transport_sender_ipc.cc:125:
> params.pli_message.reset(
> On 2016/03/02 at 04:24:17, xjz wrote:
> > On 2016/03/01 23:44:12, dcheng wrote:
> > > Maybe the parameters shouldn't be const?
> > 
> > The const_cast is used here only for the purpose to set the scoped ptrs in
> |params|. The content will not be modified.
> 
> But what's the point of making it const? Either the scoped_ptr<> itself should
> contain a const pointer (rare) or the parameters should just be non-const.
> const_cast<> is usually a sign to the reader that they have to pay more
> attention because something tricky is happening. Using it like this just adds
> noise to the signal.
> 
This is actually caused by this interface of the RTCP session with the
CastTransportSender 
IPC. It can be resolved after refactoring this interface. Please see comment
below.

>
https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
> chrome/renderer/media/cast_transport_sender_ipc.cc:141: // serializes it and
> remember the serialized form instead.
> On 2016/03/02 at 04:24:17, xjz wrote:
> > On 2016/03/01 23:44:12, dcheng wrote:
> > > ... so who frees this memory?
> > 
> > It will be released below (line 146).
> 
> Reading the code, I understand how it works now: on the sending side, they are
> unowned pointers but we stick them into a scoped_ptr anyway… and on the
> receiving side, they need to be owned. But this is really weird and not a good
> way to use scoped_ptr.
> 
> However, this is complex and hard to understand. Instead of having one uber
> message with optional fields, can we just split this up into different IPC
> messages?
> 
> In addition, once it's split up into different messages, there's no need to
> worry about copies: IPCs bind to the arguments as const T&, and it makes no
copy
> before serializing arguments into the wire format.

I agree with you that this is confusing and need to be cleaned up. The
const_cast 
can be removed after that. In fact this is already reported in this issue: 
https://crbug.com/569259. However, the refactor is not simple and small as it
might 
relate to a bunch of interface change. Can we put this issue in a separate CL?

[xjz, 2016-03-02 22:05:17]

On 2016/03/02 18:35:08, xjz wrote:
> On 2016/03/02 07:41:24, dcheng wrote:
> >
>
https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
> > File chrome/renderer/media/cast_transport_sender_ipc.cc (right):
> > 
> >
>
https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
> > chrome/renderer/media/cast_transport_sender_ipc.cc:125:
> > params.pli_message.reset(
> > On 2016/03/02 at 04:24:17, xjz wrote:
> > > On 2016/03/01 23:44:12, dcheng wrote:
> > > > Maybe the parameters shouldn't be const?
> > > 
> > > The const_cast is used here only for the purpose to set the scoped ptrs in
> > |params|. The content will not be modified.
> > 
> > But what's the point of making it const? Either the scoped_ptr<> itself
should
> > contain a const pointer (rare) or the parameters should just be non-const.
> > const_cast<> is usually a sign to the reader that they have to pay more
> > attention because something tricky is happening. Using it like this just
adds
> > noise to the signal.
> > 
> This is actually caused by this interface of the RTCP session with the
> CastTransportSender 
> IPC. It can be resolved after refactoring this interface. Please see comment
> below.
> 
> >
>
https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
> > chrome/renderer/media/cast_transport_sender_ipc.cc:141: // serializes it and
> > remember the serialized form instead.
> > On 2016/03/02 at 04:24:17, xjz wrote:
> > > On 2016/03/01 23:44:12, dcheng wrote:
> > > > ... so who frees this memory?
> > > 
> > > It will be released below (line 146).
> > 
> > Reading the code, I understand how it works now: on the sending side, they
are
> > unowned pointers but we stick them into a scoped_ptr anyway… and on the
> > receiving side, they need to be owned. But this is really weird and not a
good
> > way to use scoped_ptr.
> > 
> > However, this is complex and hard to understand. Instead of having one uber
> > message with optional fields, can we just split this up into different IPC
> > messages?
> > 
> > In addition, once it's split up into different messages, there's no need to
> > worry about copies: IPCs bind to the arguments as const T&, and it makes no
> copy
> > before serializing arguments into the wire format.
> 
> I agree with you that this is confusing and need to be cleaned up. The
> const_cast 
> can be removed after that. In fact this is already reported in this issue: 
> https://crbug.com/569259. However, the refactor is not simple and small as it
> might 
> relate to a bunch of interface change. Can we put this issue in a separate CL?

dcheng: After discussed with miu, we realized that a bunch of cast messages need

to be cleaned up. Please hold off the review of this CL for now. Thanks.

[xjz, 2016-03-15 23:49:48]

Patchset #6 (id:160001) has been deleted

[xjz, 2016-03-16 00:25:50]

PTAL
dcheng: The previous two concerns are not applicable after the refactor in
another CL. Rebased and added a new message for PLI. PTAL.

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:125:
params.pli_message.reset(
On 2016/03/02 07:41:24, dcheng wrote:
> On 2016/03/02 at 04:24:17, xjz wrote:
> > On 2016/03/01 23:44:12, dcheng wrote:
> > > Maybe the parameters shouldn't be const?
> > 
> > The const_cast is used here only for the purpose to set the scoped ptrs in
> |params|. The content will not be modified.
> 
> But what's the point of making it const? Either the scoped_ptr<> itself should
> contain a const pointer (rare) or the parameters should just be non-const.
> const_cast<> is usually a sign to the reader that they have to pay more
> attention because something tricky is happening. Using it like this just adds
> noise to the signal.

Not applicable.

https://codereview.chromium.org/1709863002/diff/120001/chrome/renderer/media/...
chrome/renderer/media/cast_transport_sender_ipc.cc:141: // serializes it and
remember the serialized form instead.
On 2016/03/02 07:41:24, dcheng wrote:
> On 2016/03/02 at 04:24:17, xjz wrote:
> > On 2016/03/01 23:44:12, dcheng wrote:
> > > ... so who frees this memory?
> > 
> > It will be released below (line 146).
> 
> Reading the code, I understand how it works now: on the sending side, they are
> unowned pointers but we stick them into a scoped_ptr anyway… and on the
> receiving side, they need to be owned. But this is really weird and not a good
> way to use scoped_ptr.
> 
> However, this is complex and hard to understand. Instead of having one uber
> message with optional fields, can we just split this up into different IPC
> messages?
> 
> In addition, once it's split up into different messages, there's no need to
> worry about copies: IPCs bind to the arguments as const T&, and it makes no
copy
> before serializing arguments into the wire format.

Not applicable after refactor.

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
File media/cast/net/rtcp/rtcp_defines.h (right):

https://codereview.chromium.org/1709863002/diff/120001/media/cast/net/rtcp/rt...
media/cast/net/rtcp/rtcp_defines.h:65: RtcpPliMessage();
On 2016/03/02 04:24:17, xjz wrote:
> On 2016/03/01 23:44:13, dcheng wrote:
> > Is the default ctor necessary?
> 
> Removed.

Re-add the default ctor as it is needed by IPC_MESSAGE_CONTROL.

[dcheng, 2016-03-16 01:39:24]

lgtm

https://codereview.chromium.org/1709863002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/1709863002/diff/180001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:305: const
::media::cast::RtcpPliMessage& pli_message) {
Nit: no :: for local consistency

https://codereview.chromium.org/1709863002/diff/180001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/1709863002/diff/180001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:112: void AddPli(const
RtcpPliMessage& pli_message) final;
Maybe just mark the entire class final?

[xjz, 2016-03-16 17:12:13]

Addressed comments.

https://codereview.chromium.org/1709863002/diff/180001/chrome/browser/media/c...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/1709863002/diff/180001/chrome/browser/media/c...
chrome/browser/media/cast_transport_host_filter.cc:305: const
::media::cast::RtcpPliMessage& pli_message) {
On 2016/03/16 01:39:24, dcheng wrote:
> Nit: no :: for local consistency

Done.

https://codereview.chromium.org/1709863002/diff/180001/media/cast/net/cast_tr...
File media/cast/net/cast_transport_sender_impl.h (right):

https://codereview.chromium.org/1709863002/diff/180001/media/cast/net/cast_tr...
media/cast/net/cast_transport_sender_impl.h:112: void AddPli(const
RtcpPliMessage& pli_message) final;
On 2016/03/16 01:39:24, dcheng wrote:
> Maybe just mark the entire class final?

Marked the class final. Code style still requires marking overriding method with
"override" or "final", which ensures that the function is virtual.

[xjz, 2016-03-16 20:30:24]

The CQ bit was checked by xjz@chromium.org

[xjz, 2016-03-16 20:30:25]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org,
dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1709863002/#ps200001
(title: "Address dcheng's comments.")

[commit-bot: I haz the power, 2016-03-16 20:31:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1709863002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1709863002/200001

[commit-bot: I haz the power, 2016-03-16 21:17:34]

Description was changed from

==========
Add Cast PLI support on sender side.
Main changes when receive a PLI message:
1. Request encoding a Key frame (for now keep the same resolution and bitrate).
2. Clear sending remaining frames before sending out the key frame.

BUG=589934
==========

to

==========
Add Cast PLI support on sender side.
Main changes when receive a PLI message:
1. Request encoding a Key frame (for now keep the same resolution and bitrate).
2. Clear sending remaining frames before sending out the key frame.

BUG=589934
==========

[commit-bot: I haz the power, 2016-03-16 21:17:36]

Committed patchset #7 (id:200001)

[commit-bot: I haz the power, 2016-03-16 21:18:41]

Description was changed from

==========
Add Cast PLI support on sender side.
Main changes when receive a PLI message:
1. Request encoding a Key frame (for now keep the same resolution and bitrate).
2. Clear sending remaining frames before sending out the key frame.

BUG=589934
==========

to

==========
Add Cast PLI support on sender side.
Main changes when receive a PLI message:
1. Request encoding a Key frame (for now keep the same resolution and bitrate).
2. Clear sending remaining frames before sending out the key frame.

BUG=589934

Committed: https://crrev.com/b9733f71a04374f7ec237ed01be627a9b47950dc
Cr-Commit-Position: refs/heads/master@{#381546}
==========

[commit-bot: I haz the power, 2016-03-16 21:18:43]

Patchset 7 (id:??) landed as
https://crrev.com/b9733f71a04374f7ec237ed01be627a9b47950dc
Cr-Commit-Position: refs/heads/master@{#381546}