Cast: Fix rtcp event dedup logic in rtcp_receiver.

Cast: Fix rtcp event dedup logic in rtcp_receiver.

Previously the dedup logic is based on a hash of rtp timestamp, event
time, and event type. This is not sufficient for packet events since
two of them can have the same fields except for packet id. The hash
needs to also take account into packet id for packet events.

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

[Alpha Left Google, 2014-05-06 20:55:51]

https://codereview.chromium.org/266373008/diff/1/media/cast/rtcp/rtcp_receive...
File media/cast/rtcp/rtcp_receiver.cc (right):

https://codereview.chromium.org/266373008/diff/1/media/cast/rtcp/rtcp_receive...
media/cast/rtcp/rtcp_receiver.cc:43: size_t HashReceiverEvent(uint32
frame_rtp_timestamp,
I suggest we change the hash set to use a key of std::pair<uint64, uint64>. I
thought size_t was enough. This means the return type of this method should be:

typedef std::pair<uint64, uint64> ReceiverEventKey;

// <Describe how to combine multiple fields here..>
ReceiverEventKey ComputeReceiverEventKey(...);

[imcheng, 2014-05-06 21:19:47]

https://codereview.chromium.org/266373008/diff/1/media/cast/rtcp/rtcp_receive...
File media/cast/rtcp/rtcp_receiver.cc (right):

https://codereview.chromium.org/266373008/diff/1/media/cast/rtcp/rtcp_receive...
media/cast/rtcp/rtcp_receiver.cc:43: size_t HashReceiverEvent(uint32
frame_rtp_timestamp,
Are you suggesting that we get rid of the base::HashInts64 call below and just
let the hash_set take care of the hashing? Because reading this again, I do not
understand why is it necessary to call base::HashInts64.

On 2014/05/06 20:55:52, Alpha wrote:
> I suggest we change the hash set to use a key of std::pair<uint64, uint64>. I
> thought size_t was enough. This means the return type of this method should
be:
> 
> typedef std::pair<uint64, uint64> ReceiverEventKey;
> 
> // <Describe how to combine multiple fields here..>
> ReceiverEventKey ComputeReceiverEventKey(...);

[Alpha Left Google, 2014-05-06 21:21:45]

https://codereview.chromium.org/266373008/diff/1/media/cast/rtcp/rtcp_receive...
File media/cast/rtcp/rtcp_receiver.cc (right):

https://codereview.chromium.org/266373008/diff/1/media/cast/rtcp/rtcp_receive...
media/cast/rtcp/rtcp_receiver.cc:43: size_t HashReceiverEvent(uint32
frame_rtp_timestamp,
On 2014/05/06 21:19:47, imcheng1 wrote:
> Are you suggesting that we get rid of the base::HashInts64 call below and just
> let the hash_set take care of the hashing? Because reading this again, I do
not
> understand why is it necessary to call base::HashInts64.
> 
> On 2014/05/06 20:55:52, Alpha wrote:
> > I suggest we change the hash set to use a key of std::pair<uint64, uint64>.
I
> > thought size_t was enough. This means the return type of this method should
> be:
> > 
> > typedef std::pair<uint64, uint64> ReceiverEventKey;
> > 
> > // <Describe how to combine multiple fields here..>
> > ReceiverEventKey ComputeReceiverEventKey(...);
> 

That's right. I used it because base::hash_map might not support std::pair<> on
some platform and size_t was easier. But for correctness (that this patch is
trying to fix) using std::pair<> is the right thing to do.

[imcheng, 2014-05-06 22:29:38]

https://codereview.chromium.org/266373008/diff/1/media/cast/rtcp/rtcp_receive...
File media/cast/rtcp/rtcp_receiver.cc (right):

https://codereview.chromium.org/266373008/diff/1/media/cast/rtcp/rtcp_receive...
media/cast/rtcp/rtcp_receiver.cc:43: size_t HashReceiverEvent(uint32
frame_rtp_timestamp,
On 2014/05/06 21:21:45, Alpha wrote:
> On 2014/05/06 21:19:47, imcheng1 wrote:
> > Are you suggesting that we get rid of the base::HashInts64 call below and
just
> > let the hash_set take care of the hashing? Because reading this again, I do
> not
> > understand why is it necessary to call base::HashInts64.
> > 
> > On 2014/05/06 20:55:52, Alpha wrote:
> > > I suggest we change the hash set to use a key of std::pair<uint64,
uint64>.
> I
> > > thought size_t was enough. This means the return type of this method
should
> > be:
> > > 
> > > typedef std::pair<uint64, uint64> ReceiverEventKey;
> > > 
> > > // <Describe how to combine multiple fields here..>
> > > ReceiverEventKey ComputeReceiverEventKey(...);
> > 
> 
> That's right. I used it because base::hash_map might not support std::pair<>
on
> some platform and size_t was easier. But for correctness (that this patch is
> trying to fix) using std::pair<> is the right thing to do.

Done.

[Alpha Left Google, 2014-05-06 22:32:51]

lgtm

[imcheng, 2014-05-06 22:40:51]

The CQ bit was checked by imcheng@chromium.org

[commit-bot: I haz the power, 2014-05-06 22:42:45]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/imcheng@chromium.org/266373008/20001

[commit-bot: I haz the power, 2014-05-07 15:51:58]

Change committed as 268791