Added framer to cast.

Added framer to cast.

BUG=

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

[pwestin, 2013-08-27 22:02:17]

Please review

[Alpha Left Google, 2013-08-28 00:27:31]

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/cast_define...
media/cast/cast_defines.h:38: template <class Collection>
Is this being used? I can't find reference in this patch. There's also
base/stl_util.h which provides a useful STL related functions.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/cast_receiv...
File media/cast/cast_receiver.gyp (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/cast_receiv...
media/cast/cast_receiver.gyp:22: 'framer/framer.gyp:framer',
framer is too general and it will probably clash with other targets, suggest
cast_framer.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
File media/cast/framer/cast_message_builder.cc (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.cc:55: if (!UpdateAckMessage())  return;
nit: This looks strange, return can just move to next line.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.cc:88: if (last_acked_frame_id_ ==
frame_id)  return false;
nit: return false can move to next line or one space.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.cc:102: if (last_update_time_.is_null()
&& frame_id_map_->Empty())  return false;
nit: return can go to next line or 1 space.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.cc:111: if
(!UpdateCastMessageInternal(&message))  return;
nit: return can go to next line or 1 space.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.cc:152: if (frame_id_map_->Empty()) 
return;
nit: return can go to next line or 1 space.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.cc:161: std::map<uint8,
base::TimeTicks>::iterator it =
Use typdef'ed type here for readability.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.cc:172: std::set<uint16> missing;
This should use a typedef'ed type like PacketIdSet missing;

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.cc:180: std::pair<uint8, std::set<uint16>
>(
Use std::make_pair() instead so types are inferred.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
File media/cast/framer/cast_message_builder.h (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.h:5: // Handle NACK list and manage ACK.
nit: *Handles and *manages.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.h:25: uint32 media_ssrc,
nit: this can go to the previous line i think.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder.h:56: std::map<uint8, base::TimeTicks>
time_last_nacked_map_;
nit: Use a typedef here.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
File media/cast/framer/cast_message_builder_unittest.cc (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder_unittest.cc:37: std::map<uint8,
std::set<uint16> >::const_iterator frame_it =
This looks bulky is there a typedef you can use?

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder_unittest.cc:42:
missing_packets_.insert(std::pair<uint8, int>
you can just std::make_pair() which does the same thing for you and types are
inferred.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder_unittest.cc:68: uint8 last_frame_acked() 
{ return last_frame_acked_; }
nit: one space after ().

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder_unittest.cc:73: std::map<uint8, int>
missing_packets_;
Use a typedef here.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/cast...
media/cast/framer/cast_message_builder_unittest.cc:136: CastMessageBuilder*
cast_msg_builder_;
Use scoped_ptr<>.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
File media/cast/framer/frame_buffer.cc (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_buffer.cc:20: std::map<uint16, std::vector<uint8>
>::iterator it;
There's no need to do this, STL will cleanup.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_buffer.cc:51: std::copy(&payload_data[0],
&payload_data[payload_size], data.begin());
It's cleaner to be: std::copy(payload_data, payload_data + payload_size,
data.begin());

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_buffer.cc:52: packets_.insert(std::pair<uint16,
std::vector<uint8> >(
std::make_pair() would be perfect for this. This also means copying the entire
vector<>, I suggest you insert an empty vector<> and then insert the payload
data to the vector in the map.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_buffer.cc:65: if (!Complete())  return false;
nit: One space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_buffer.cc:85: if (!Complete())  return false;
nit: One space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_buffer.cc:97: std::map<uint16, std::vector<uint8>
>::const_iterator it;
Use the typedef. That should be PacketMap.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
File media/cast/framer/frame_id_map.cc (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:48: std::set<uint16>::const_iterator
it_after_last_received =
Should use PacketIdSet here.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:93: reference_frame_id,
nit: intent.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:97: std::pair<uint8, FrameInfo*>
insert_obj(frame_id, frame_info);
Use std::make_pair() here.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:123: frame_map_.clear();
This will leak because FrameInfo* is just a pointer. Use linked_ptr<> to contain
FrameInfo.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:154: if (it == frame_map_.end())  break;
nit: one space before break.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:155: if (!it->second->Complete())  break;
nit: one space before break.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:165: if (NextContinuousFrame(frame_id)) 
return true;
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:178: if (it_best_match == frame_map_.end()) 
return false;  // No complete frame.
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:200: if (it_best_match == frame_map_.end()) 
return false;
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:236: if (waiting_for_key_ &&
!frame->is_key_frame())  return false;
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:241: if (frame->is_key_frame())  return true;
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.cc:242: if (waiting_for_key_ &&
!frame->is_key_frame())  return false;
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
File media/cast/framer/frame_id_map.h (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.h:42: std::set<uint16> missing_packets_;
I suggest a typedef for this, e.g. PacketIdSet.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.h:47: typedef std::map<uint8, FrameInfo*>
FrameMap;
Use std::map<uint8, linked_ptr<FrameInfo> > instead of a raw pointer.

See description here:
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/linked...

It is used inside STL containers which is the perfect use here.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/frame_id_map.h:66: uint8_t LastContinuousFrame() const;
Should be uint8.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
File media/cast/framer/framer.cc (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.cc:48: std::pair<uint8,
FrameBuffer*>(rtp_header.frame_id, frame_buffer));
Use std::make_pair().

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.cc:72: if (WaitForNextFrame(timeout))  return false;
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.cc:82: if (it == frames_.end())  return false;
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.cc:98: if (WaitForNextFrame(timeout))  return false;
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.cc:101: if (!decoder_faster_than_max_frame_rate_) 
return false;
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.cc:111: if (it == frames_.end())  return false;
nit: one space before return.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.cc:117: base::TimeDelta wait_time = timeout -
clock_->NowTicks();
nit: this looks strange, return should better be moved to next line.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.cc:137: delete it->second;
Use linked_ptr<> and there's no need to delete manually.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
File media/cast/framer/framer.gyp (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.gyp:8: 'target_name': 'framer',
Use a more specific target name like cast_frame to avoid clash.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
File media/cast/framer/framer.h (right):

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.h:24: typedef std::map<uint8, FrameBuffer*> FrameList;
This should be linked_ptr<FrameBuffer> instead of FrameBuffer*. It acks like a a
scoped_ptr<> for STL containers.

https://chromiumcodereview.appspot.com/23467003/diff/1/media/cast/framer/fram...
media/cast/framer/framer.h:72: CastMessageBuilder* cast_msg_builder_;
Use scoped_ptr<>.

[pwestin, 2013-08-28 16:40:43]

PTAL

https://codereview.chromium.org/23467003/diff/1/media/cast/cast_defines.h
File media/cast/cast_defines.h (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/cast_defines.h#new...
media/cast/cast_defines.h:38: template <class Collection>
On 2013/08/28 00:27:31, Alpha wrote:
> Is this being used? I can't find reference in this patch. There's also
> base/stl_util.h which provides a useful STL related functions.

It was used; removed since we introduced the linked_ptr

https://codereview.chromium.org/23467003/diff/1/media/cast/cast_receiver.gyp
File media/cast/cast_receiver.gyp (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/cast_receiver.gyp#...
media/cast/cast_receiver.gyp:22: 'framer/framer.gyp:framer',
On 2013/08/28 00:27:31, Alpha wrote:
> framer is too general and it will probably clash with other targets, suggest
> cast_framer.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
File media/cast/framer/cast_message_builder.cc (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.cc:55: if (!UpdateAckMessage())  return;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: This looks strange, return can just move to next line.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.cc:88: if (last_acked_frame_id_ ==
frame_id)  return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: return false can move to next line or one space.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.cc:102: if (last_update_time_.is_null()
&& frame_id_map_->Empty())  return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: return can go to next line or 1 space.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.cc:111: if
(!UpdateCastMessageInternal(&message))  return;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: return can go to next line or 1 space.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.cc:152: if (frame_id_map_->Empty()) 
return;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: return can go to next line or 1 space.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.cc:161: std::map<uint8,
base::TimeTicks>::iterator it =
On 2013/08/28 00:27:31, Alpha wrote:
> Use typdef'ed type here for readability.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.cc:172: std::set<uint16> missing;
On 2013/08/28 00:27:31, Alpha wrote:
> This should use a typedef'ed type like PacketIdSet missing;

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.cc:180: std::pair<uint8, std::set<uint16>
>(
On 2013/08/28 00:27:31, Alpha wrote:
> Use std::make_pair() instead so types are inferred.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
File media/cast/framer/cast_message_builder.h (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.h:5: // Handle NACK list and manage ACK.
On 2013/08/28 00:27:31, Alpha wrote:
> nit: *Handles and *manages.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.h:25: uint32 media_ssrc,
On 2013/08/28 00:27:31, Alpha wrote:
> nit: this can go to the previous line i think.

yes, however I think this is much more readable; ok?

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder.h:56: std::map<uint8, base::TimeTicks>
time_last_nacked_map_;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: Use a typedef here.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
File media/cast/framer/cast_message_builder_unittest.cc (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder_unittest.cc:37: std::map<uint8,
std::set<uint16> >::const_iterator frame_it =
On 2013/08/28 00:27:31, Alpha wrote:
> This looks bulky is there a typedef you can use?

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder_unittest.cc:42:
missing_packets_.insert(std::pair<uint8, int>
On 2013/08/28 00:27:31, Alpha wrote:
> you can just std::make_pair() which does the same thing for you and types are
> inferred.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder_unittest.cc:68: uint8 last_frame_acked() 
{ return last_frame_acked_; }
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space after ().

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder_unittest.cc:73: std::map<uint8, int>
missing_packets_;
On 2013/08/28 00:27:31, Alpha wrote:
> Use a typedef here.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/cast_messag...
media/cast/framer/cast_message_builder_unittest.cc:136: CastMessageBuilder*
cast_msg_builder_;
On 2013/08/28 00:27:31, Alpha wrote:
> Use scoped_ptr<>.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_buffe...
File media/cast/framer/frame_buffer.cc (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_buffe...
media/cast/framer/frame_buffer.cc:20: std::map<uint16, std::vector<uint8>
>::iterator it;
On 2013/08/28 00:27:31, Alpha wrote:
> There's no need to do this, STL will cleanup.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_buffe...
media/cast/framer/frame_buffer.cc:51: std::copy(&payload_data[0],
&payload_data[payload_size], data.begin());
On 2013/08/28 00:27:31, Alpha wrote:
> It's cleaner to be: std::copy(payload_data, payload_data + payload_size,
> data.begin());

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_buffe...
media/cast/framer/frame_buffer.cc:52: packets_.insert(std::pair<uint16,
std::vector<uint8> >(
On 2013/08/28 00:27:31, Alpha wrote:
> std::make_pair() would be perfect for this. This also means copying the entire
> vector<>, I suggest you insert an empty vector<> and then insert the payload
> data to the vector in the map.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_buffe...
media/cast/framer/frame_buffer.cc:65: if (!Complete())  return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: One space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_buffe...
media/cast/framer/frame_buffer.cc:85: if (!Complete())  return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: One space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_buffe...
media/cast/framer/frame_buffer.cc:97: std::map<uint16, std::vector<uint8>
>::const_iterator it;
On 2013/08/28 00:27:31, Alpha wrote:
> Use the typedef. That should be PacketMap.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
File media/cast/framer/frame_id_map.cc (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:48: std::set<uint16>::const_iterator
it_after_last_received =
On 2013/08/28 00:27:31, Alpha wrote:
> Should use PacketIdSet here.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:93: reference_frame_id,
On 2013/08/28 00:27:31, Alpha wrote:
> nit: intent.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:97: std::pair<uint8, FrameInfo*>
insert_obj(frame_id, frame_info);
On 2013/08/28 00:27:31, Alpha wrote:
> Use std::make_pair() here.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:123: frame_map_.clear();
On 2013/08/28 00:27:31, Alpha wrote:
> This will leak because FrameInfo* is just a pointer. Use linked_ptr<> to
contain
> FrameInfo.
> 

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:154: if (it == frame_map_.end())  break;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before break.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:155: if (!it->second->Complete())  break;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before break.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:165: if (NextContinuousFrame(frame_id)) 
return true;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:178: if (it_best_match == frame_map_.end()) 
return false;  // No complete frame.
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:200: if (it_best_match == frame_map_.end()) 
return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:236: if (waiting_for_key_ &&
!frame->is_key_frame())  return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:241: if (frame->is_key_frame())  return true;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.cc:242: if (waiting_for_key_ &&
!frame->is_key_frame())  return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_map.h
File media/cast/framer/frame_id_map.h (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.h:42: std::set<uint16> missing_packets_;
On 2013/08/28 00:27:31, Alpha wrote:
> I suggest a typedef for this, e.g. PacketIdSet.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.h:47: typedef std::map<uint8, FrameInfo*>
FrameMap;
On 2013/08/28 00:27:31, Alpha wrote:
> Use std::map<uint8, linked_ptr<FrameInfo> > instead of a raw pointer.
> 
> See description here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/memory/linked...
> 
> It is used inside STL containers which is the perfect use here.
> 

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/frame_id_ma...
media/cast/framer/frame_id_map.h:66: uint8_t LastContinuousFrame() const;
On 2013/08/28 00:27:31, Alpha wrote:
> Should be uint8.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.cc
File media/cast/framer/framer.cc (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.cc#n...
media/cast/framer/framer.cc:48: std::pair<uint8,
FrameBuffer*>(rtp_header.frame_id, frame_buffer));
On 2013/08/28 00:27:31, Alpha wrote:
> Use std::make_pair().
> 

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.cc#n...
media/cast/framer/framer.cc:72: if (WaitForNextFrame(timeout))  return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.cc#n...
media/cast/framer/framer.cc:82: if (it == frames_.end())  return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.cc#n...
media/cast/framer/framer.cc:98: if (WaitForNextFrame(timeout))  return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.cc#n...
media/cast/framer/framer.cc:101: if (!decoder_faster_than_max_frame_rate_) 
return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.cc#n...
media/cast/framer/framer.cc:111: if (it == frames_.end())  return false;
On 2013/08/28 00:27:31, Alpha wrote:
> nit: one space before return.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.cc#n...
media/cast/framer/framer.cc:117: base::TimeDelta wait_time = timeout -
clock_->NowTicks();
On 2013/08/28 00:27:31, Alpha wrote:
> nit: this looks strange, return should better be moved to next line.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.cc#n...
media/cast/framer/framer.cc:137: delete it->second;
On 2013/08/28 00:27:31, Alpha wrote:
> Use linked_ptr<> and there's no need to delete manually.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.gyp
File media/cast/framer/framer.gyp (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.gyp#...
media/cast/framer/framer.gyp:8: 'target_name': 'framer',
On 2013/08/28 00:27:31, Alpha wrote:
> Use a more specific target name like cast_frame to avoid clash.

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.h
File media/cast/framer/framer.h (right):

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.h#ne...
media/cast/framer/framer.h:24: typedef std::map<uint8, FrameBuffer*> FrameList;
On 2013/08/28 00:27:31, Alpha wrote:
> This should be linked_ptr<FrameBuffer> instead of FrameBuffer*. It acks like a
a
> scoped_ptr<> for STL containers.
> 

Done.

https://codereview.chromium.org/23467003/diff/1/media/cast/framer/framer.h#ne...
media/cast/framer/framer.h:72: CastMessageBuilder* cast_msg_builder_;
On 2013/08/28 00:27:31, Alpha wrote:
> Use scoped_ptr<>.
> 

Done.

[Alpha Left Google, 2013-08-28 19:13:48]

LGTM after two comments.

https://codereview.chromium.org/23467003/diff/14001/media/cast/framer/frame_i...
File media/cast/framer/frame_id_map.cc (right):

https://codereview.chromium.org/23467003/diff/14001/media/cast/framer/frame_i...
media/cast/framer/frame_id_map.cc:92: FrameInfo* frame_info = new
FrameInfo(frame_id,
Should be linked_ptr<FrameInfo> frame_info = ...

https://codereview.chromium.org/23467003/diff/14001/media/cast/framer/framer.cc
File media/cast/framer/framer.cc (right):

https://codereview.chromium.org/23467003/diff/14001/media/cast/framer/framer....
media/cast/framer/framer.cc:38: FrameBuffer* frame_buffer = new FrameBuffer();
Should be linked_ptr<FrameBuffer> frame_buffer = ...

[pwestin, 2013-08-28 20:37:21]

Done

https://codereview.chromium.org/23467003/diff/14001/media/cast/framer/frame_i...
File media/cast/framer/frame_id_map.cc (right):

https://codereview.chromium.org/23467003/diff/14001/media/cast/framer/frame_i...
media/cast/framer/frame_id_map.cc:92: FrameInfo* frame_info = new
FrameInfo(frame_id,
On 2013/08/28 19:13:48, Alpha wrote:
> Should be linked_ptr<FrameInfo> frame_info = ...

Done.

https://codereview.chromium.org/23467003/diff/14001/media/cast/framer/framer.cc
File media/cast/framer/framer.cc (right):

https://codereview.chromium.org/23467003/diff/14001/media/cast/framer/framer....
media/cast/framer/framer.cc:38: FrameBuffer* frame_buffer = new FrameBuffer();
On 2013/08/28 19:13:48, Alpha wrote:
> Should be linked_ptr<FrameBuffer> frame_buffer = ...

Done.

[commit-bot: I haz the power, 2013-08-29 23:35:50]

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

[commit-bot: I haz the power, 2013-08-29 23:35:57]

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 52.
  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
dec8f53a37ef3f98b85c5d9eeae83a67c8a37fd0..991fdea3b21781b314110ef5eee95f3f47448639
100644
--- a/media/cast/cast.gyp
+++ b/media/cast/cast.gyp
@@ -52,9 +52,13 @@
           ],
           'include_dirs': [
             '<(DEPTH)/',
+            '<(DEPTH)/third_party/',
           ],
           'sources': [
             'congestion_control/congestion_control_unittest.cc',
+            'framer/cast_message_builder_unittest.cc',
+            'framer/frame_buffer_unittest.cc',
+            'framer/framer_unittest.cc',
             'pacing/paced_sender_unittest.cc',
             'rtcp/rtcp_receiver_unittest.cc',
             'rtcp/rtcp_sender_unittest.cc',

[Alpha Left Google, 2013-08-29 23:36:35]

looks like you need to merge.

[commit-bot: I haz the power, 2013-08-30 00:13:40]

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

[commit-bot: I haz the power, 2013-08-30 02:54:06]

Change committed as 220490