Cast: Make receiver use cast_transport

Cast: Make receiver use cast_transport

Also, fix rtcp_builder to not be responsible for logging event redundancy.
Instead, we make the rtcp_event_subscriber do that.

Committed: https://crrev.com/8029778b0e060e4a22c59fa40ca95e12c437c1f7
Cr-Commit-Position: refs/heads/master@{#307814}

[hubbe, 2014-12-03 20:31:29]

hubbe@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2014-12-04 04:18:55]

https://codereview.chromium.org/765643006/diff/1/chrome/browser/media/cast_tr...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/765643006/diff/1/chrome/browser/media/cast_tr...
chrome/browser/media/cast_transport_host_filter.cc:242:
id_map_.Lookup(channel_id);
I think this statement fits on one line.

https://codereview.chromium.org/765643006/diff/1/chrome/browser/media/cast_tr...
chrome/browser/media/cast_transport_host_filter.cc:256:
id_map_.Lookup(channel_id);
ditto (one line)

https://codereview.chromium.org/765643006/diff/1/chrome/renderer/media/cast_t...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/765643006/diff/1/chrome/renderer/media/cast_t...
chrome/renderer/media/cast_transport_sender_ipc.cc:118: const_cast<
Line 118 and 119 can be one LOC.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/cast_transpor...
File media/cast/net/cast_transport_config.h (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/cast_transpor...
media/cast/net/cast_transport_config.h:131: PacketReceiverCallbackWithStatus;
Did you intend to keep this name change?  I don't see the "with status" part.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/cast_transpor...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/cast_transpor...
media/cast/net/cast_transport_sender.h:109: 
Were you going to put these methods in a separate CastTransportReceiver
interface?  I think it is important to enforce, for example, that receiver code
doesn't call methods like InitializeAudio/Video(), which are sender-specific.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
File media/cast/net/rtcp/receiver_rtcp_event_subscriber.cc (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
media/cast/net/rtcp/receiver_rtcp_event_subscriber.cc:19: for (size_t i = 0; i <
kNumResends; i++) {
nit: memset(send_ptrs_, 0, sizeof(send_ptrs_));

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
media/cast/net/rtcp/receiver_rtcp_event_subscriber.cc:51:
DCHECK(rtcp_events_.size() <= max_size_to_retain_);
This DCHECK should probably be moved to the end of the TruncateMapIfNeeded()
method.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
media/cast/net/rtcp/receiver_rtcp_event_subscriber.cc:71:
DCHECK(rtcp_events_.size() <= max_size_to_retain_);
ditto

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
File media/cast/net/rtcp/receiver_rtcp_event_subscriber.h (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
media/cast/net/rtcp/receiver_rtcp_event_subscriber.h:76: uint64 popped_events_;
Needs a comment to describe what these three members are and how they are used.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp.h
File media/cast/net/rtcp/rtcp.h (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp.h#n...
media/cast/net/rtcp/rtcp.h:65: RtcpTimeData ConvertToNTPAndSave(base::TimeTicks
now);
Please provide a short comment to explain how the caller uses this method.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp.h#n...
media/cast/net/rtcp/rtcp.h:67: // |cast_message| and |rtcp_events| is optional;
if |cast_message| is
and |rtp_receiver_statistics| is also optional.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_bui...
File media/cast/net/rtcp/rtcp_builder.h (left):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder.h:82: std::deque<RtcpReceiverLogMessage>
rtcp_events_history_;
Nice clean-up of the RtcpEventsMultiMap foo, BTW!  :)

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_bui...
File media/cast/net/rtcp/rtcp_builder_unittest.cc (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder_unittest.cc:56: if ((*golden_packet)[x] !=
packet->data[x])
nit: braces around then-clause

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_def...
File media/cast/net/rtcp/rtcp_defines.h (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_def...
media/cast/net/rtcp/rtcp_defines.h:115: uint8 fraction_lost;
IMO, now's a good time to replace these data members with more appropriate data
types.  Meaning, we shouldn't force the whole implementation to know of the RTCP
wire format.

For example, fraction_lost takes a value from 0 to 255, but that's not intuitive
at all.  Should it be a float, or remain an int that contains a percentage
value?

Also, it would be nice if each of the members of this struct had a short comment
for documentation purposes.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_def...
media/cast/net/rtcp/rtcp_defines.h:125: base::TimeTicks t;
naming: Can we do a little better than |t|, like |timestamp| or |report_time|?

https://codereview.chromium.org/765643006/diff/1/media/cast/net/udp_transport.cc
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/udp_transport...
media/cast/net/udp_transport.cc:122: // Loop while UdpSocket is delivering data
synchronously.  When it responds
Looks like ReceiveNextPacket() could be called from a task that was posted
before StopReceiving() was called.  I recommend adding an early return here:

  if (packet_receiver_.is_null())
    return;

https://codereview.chromium.org/765643006/diff/1/media/cast/net/udp_transport...
media/cast/net/udp_transport.cc:169: continue;
Don't need the continue-statement anymore.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/udp_transport...
media/cast/net/udp_transport.cc:173: length_or_status = net::ERR_IO_PENDING;
Since length_or_status is assigned the same from all three then/else blocks,
consider just doing it once after the if-elseif-else statement.

https://codereview.chromium.org/765643006/diff/1/media/cast/test/simulator.cc
File media/cast/test/simulator.cc (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/test/simulator.cc...
media/cast/test/simulator.cc:393: if (receiver)
receiver->ReceivePacket(packet.Pass());
style nit: Need newline before start of then-clause.

[hubbe, 2014-12-05 23:57:21]

https://codereview.chromium.org/765643006/diff/1/chrome/browser/media/cast_tr...
File chrome/browser/media/cast_transport_host_filter.cc (right):

https://codereview.chromium.org/765643006/diff/1/chrome/browser/media/cast_tr...
chrome/browser/media/cast_transport_host_filter.cc:242:
id_map_.Lookup(channel_id);
On 2014/12/04 04:18:53, miu wrote:
> I think this statement fits on one line.

Fixed all over this file.

https://codereview.chromium.org/765643006/diff/1/chrome/browser/media/cast_tr...
chrome/browser/media/cast_transport_host_filter.cc:256:
id_map_.Lookup(channel_id);
On 2014/12/04 04:18:53, miu wrote:
> ditto (one line)

Done.

https://codereview.chromium.org/765643006/diff/1/chrome/renderer/media/cast_t...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/765643006/diff/1/chrome/renderer/media/cast_t...
chrome/renderer/media/cast_transport_sender_ipc.cc:118: const_cast<
On 2014/12/04 04:18:53, miu wrote:
> Line 118 and 119 can be one LOC.

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/cast_transpor...
File media/cast/net/cast_transport_config.h (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/cast_transpor...
media/cast/net/cast_transport_config.h:131: PacketReceiverCallbackWithStatus;
On 2014/12/04 04:18:53, miu wrote:
> Did you intend to keep this name change?  I don't see the "with status" part.

It returns a bool, that's the status.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/cast_transpor...
File media/cast/net/cast_transport_sender.h (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/cast_transpor...
media/cast/net/cast_transport_sender.h:109: 
On 2014/12/04 04:18:53, miu wrote:
> Were you going to put these methods in a separate CastTransportReceiver
> interface?  I think it is important to enforce, for example, that receiver
code
> doesn't call methods like InitializeAudio/Video(), which are sender-specific.

Originally I was going to, but it seemed to complicate the RPC code quite a bit
which led me to conclude that a common interface was needed.

It's possible to write some interfaces on top of this one that enforces what you
describe, but it would be purely for our own peace of mind.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
File media/cast/net/rtcp/receiver_rtcp_event_subscriber.cc (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
media/cast/net/rtcp/receiver_rtcp_event_subscriber.cc:19: for (size_t i = 0; i <
kNumResends; i++) {
On 2014/12/04 04:18:53, miu wrote:
> nit: memset(send_ptrs_, 0, sizeof(send_ptrs_));

Not sure that is better.
The compiler should be much better at analyzing and optimizing the loop.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
media/cast/net/rtcp/receiver_rtcp_event_subscriber.cc:51:
DCHECK(rtcp_events_.size() <= max_size_to_retain_);
On 2014/12/04 04:18:53, miu wrote:
> This DCHECK should probably be moved to the end of the TruncateMapIfNeeded()
> method.

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
media/cast/net/rtcp/receiver_rtcp_event_subscriber.cc:71:
DCHECK(rtcp_events_.size() <= max_size_to_retain_);
On 2014/12/04 04:18:53, miu wrote:
> ditto

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
File media/cast/net/rtcp/receiver_rtcp_event_subscriber.h (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/receiver...
media/cast/net/rtcp/receiver_rtcp_event_subscriber.h:76: uint64 popped_events_;
On 2014/12/04 04:18:53, miu wrote:
> Needs a comment to describe what these three members are and how they are
used.

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp.h
File media/cast/net/rtcp/rtcp.h (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp.h#n...
media/cast/net/rtcp/rtcp.h:65: RtcpTimeData ConvertToNTPAndSave(base::TimeTicks
now);
On 2014/12/04 04:18:54, miu wrote:
> Please provide a short comment to explain how the caller uses this method.

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp.h#n...
media/cast/net/rtcp/rtcp.h:67: // |cast_message| and |rtcp_events| is optional;
if |cast_message| is
On 2014/12/04 04:18:54, miu wrote:
> and |rtp_receiver_statistics| is also optional.

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_bui...
File media/cast/net/rtcp/rtcp_builder.h (left):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder.h:82: std::deque<RtcpReceiverLogMessage>
rtcp_events_history_;
On 2014/12/04 04:18:54, miu wrote:
> Nice clean-up of the RtcpEventsMultiMap foo, BTW!  :)

Acknowledged.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_bui...
File media/cast/net/rtcp/rtcp_builder_unittest.cc (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_bui...
media/cast/net/rtcp/rtcp_builder_unittest.cc:56: if ((*golden_packet)[x] !=
packet->data[x])
On 2014/12/04 04:18:54, miu wrote:
> nit: braces around then-clause

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_def...
File media/cast/net/rtcp/rtcp_defines.h (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_def...
media/cast/net/rtcp/rtcp_defines.h:115: uint8 fraction_lost;
On 2014/12/04 04:18:54, miu wrote:
> IMO, now's a good time to replace these data members with more appropriate
data
> types.  Meaning, we shouldn't force the whole implementation to know of the
RTCP
> wire format.
> 
> For example, fraction_lost takes a value from 0 to 255, but that's not
intuitive
> at all.  Should it be a float, or remain an int that contains a percentage
> value?
> 
> Also, it would be nice if each of the members of this struct had a short
comment
> for documentation purposes.

I'm just moving this struct, I'd prefer not to change it.
However, I'll add a TODO for documenting the members.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/rtcp/rtcp_def...
media/cast/net/rtcp/rtcp_defines.h:125: base::TimeTicks t;
On 2014/12/04 04:18:54, miu wrote:
> naming: Can we do a little better than |t|, like |timestamp| or |report_time|?

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/udp_transport.cc
File media/cast/net/udp_transport.cc (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/net/udp_transport...
media/cast/net/udp_transport.cc:122: // Loop while UdpSocket is delivering data
synchronously.  When it responds
On 2014/12/04 04:18:54, miu wrote:
> Looks like ReceiveNextPacket() could be called from a task that was posted
> before StopReceiving() was called.  I recommend adding an early return here:
> 
>   if (packet_receiver_.is_null())
>     return;

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/udp_transport...
media/cast/net/udp_transport.cc:169: continue;
On 2014/12/04 04:18:55, miu wrote:
> Don't need the continue-statement anymore.

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/net/udp_transport...
media/cast/net/udp_transport.cc:173: length_or_status = net::ERR_IO_PENDING;
On 2014/12/04 04:18:55, miu wrote:
> Since length_or_status is assigned the same from all three then/else blocks,
> consider just doing it once after the if-elseif-else statement.

Done.

https://codereview.chromium.org/765643006/diff/1/media/cast/test/simulator.cc
File media/cast/test/simulator.cc (right):

https://codereview.chromium.org/765643006/diff/1/media/cast/test/simulator.cc...
media/cast/test/simulator.cc:393: if (receiver)
receiver->ReceivePacket(packet.Pass());
On 2014/12/04 04:18:55, miu wrote:
> style nit: Need newline before start of then-clause.

Done.

[miu, 2014-12-06 03:55:36]

lgtm, but CastStreamingApiTestWithPixelOutput.EndToEnd needs attention before
commit.

[miu, 2014-12-06 03:56:31]

...and a compile failure on the linux_chromium_rel_ng trybot.

[hubbe, 2014-12-08 19:25:10]

hubbe@chromium.org changed reviewers:
+ palmer@chromium.org

[hubbe, 2014-12-08 19:25:11]

+palmer for cast_messages.h

Still working on making sure all building and testing works.

[hubbe, 2014-12-09 23:40:08]

hubbe@chromium.org changed reviewers:
+ tsepez@chromium.org
- palmer@chromium.org

[hubbe, 2014-12-09 23:40:09]

-palmer, +tsepez

[Tom Sepez, 2014-12-10 23:52:57]

Messages LGTM

https://codereview.chromium.org/765643006/diff/90001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/765643006/diff/90001/chrome/renderer/media/ca...
chrome/renderer/media/cast_transport_sender_ipc.cc:94: 
nit: extra blank line.

https://codereview.chromium.org/765643006/diff/90001/chrome/renderer/media/ca...
chrome/renderer/media/cast_transport_sender_ipc.cc:104: // To avoid copies, we
put pointers to objects we don't really
Sounds dangerous, can we avoid this?

[hubbe, 2014-12-11 00:10:32]

https://codereview.chromium.org/765643006/diff/90001/chrome/renderer/media/ca...
File chrome/renderer/media/cast_transport_sender_ipc.cc (right):

https://codereview.chromium.org/765643006/diff/90001/chrome/renderer/media/ca...
chrome/renderer/media/cast_transport_sender_ipc.cc:94: 
On 2014/12/10 23:52:57, Tom Sepez wrote:
> nit: extra blank line.

Done.

https://codereview.chromium.org/765643006/diff/90001/chrome/renderer/media/ca...
chrome/renderer/media/cast_transport_sender_ipc.cc:104: // To avoid copies, we
put pointers to objects we don't really
On 2014/12/10 23:52:57, Tom Sepez wrote:
> Sounds dangerous, can we avoid this?

The only way I see to avoid it is to actually copy the data, which is not
entirely fast since the data can be ~1kb or so.

While it's certainly not the safest thing in the world to do, it's fairly well
guarded against outside breakage; since |params| contain scoped_ptrs, it cannot
be copied, which means that when we pass params to the message below, it *must*
take a reference. (And by google style guide standards, it must take a const
reference.)

I've discussed this with people on my team, and we think it's reasonable, so I'm
going to submit. If you (and/or other people) disagree strongly, I will clean it
up in a followup cl.

[hubbe, 2014-12-11 00:10:38]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2014-12-11 00:12:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/765643006/90001

[commit-bot: I haz the power, 2014-12-11 01:06:08]

Committed patchset #6 (id:90001)

[commit-bot: I haz the power, 2014-12-11 01:06:54]

Patchset 6 (id:??) landed as
https://crrev.com/8029778b0e060e4a22c59fa40ca95e12c437c1f7
Cr-Commit-Position: refs/heads/master@{#307814}