Log an error in RtpDemuxer::FindSsrcAssociations() if kMaxProcessedSsrcs exceeded

Log an error in RtpDemuxer::FindSsrcAssociations() if kMaxProcessedSsrcs exceeded

BUG=None

Review-Url: https://codereview.webrtc.org/2941513002
Cr-Commit-Position: refs/heads/master@{#18569}
Committed: https://chromium.googlesource.com/external/webrtc/+/dea075c7a6bee96f319192f7beca03ef76fc0500

[eladalon, 2017-06-13 12:54:52]

eladalon@webrtc.org changed reviewers:
+ danilchap@webrtc.org, nisse@webrtc.org

[eladalon, 2017-06-13 12:54:52]

PTAL

[danilchap, 2017-06-13 12:59:01]

https://codereview.webrtc.org/2941513002/diff/1/webrtc/call/rtp_demuxer.cc
File webrtc/call/rtp_demuxer.cc (right):

https://codereview.webrtc.org/2941513002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:123: LOG(LS_WARNING) << "More than kMaxProcessedSsrcs
different SSRCs seen.";
if you have to log it, then likely you want to log it only once.
consider adding a variable to ensure you do not spam log file.

[danilchap, 2017-06-13 13:00:08]

https://codereview.webrtc.org/2941513002/diff/1/webrtc/call/rtp_demuxer.cc
File webrtc/call/rtp_demuxer.cc (right):

https://codereview.webrtc.org/2941513002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:123: LOG(LS_WARNING) << "More than kMaxProcessedSsrcs
different SSRCs seen.";
log value of the kMaxProcessedSsrcs: 
"More than " << kMaxProcessedSsrcs << " different"

[nisse-webrtc, 2017-06-13 13:09:26]

https://codereview.webrtc.org/2941513002/diff/1/webrtc/call/rtp_demuxer.cc
File webrtc/call/rtp_demuxer.cc (right):

https://codereview.webrtc.org/2941513002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:90: FindSsrcAssociations(packet);
Unrelated to this cl, but I wonder if it's really necessary to do this first?

I would have imagined that we could first lookup the ssrc in the |sinks_|
multimap, and only if the returned |it_range| is empty, attempt to process the
rsid. If we can get away with that, we don't need the |processed_ssrcs| mapping
at all.

Is there any valid use case where it's possible that |it_range| is non-empty but
the ssrc isn't present in the |processed_ssrcs_| map?

[danilchap, 2017-06-13 13:18:53]

lgtm

[eladalon, 2017-06-13 13:19:12]

https://codereview.webrtc.org/2941513002/diff/1/webrtc/call/rtp_demuxer.cc
File webrtc/call/rtp_demuxer.cc (right):

https://codereview.webrtc.org/2941513002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:90: FindSsrcAssociations(packet);
On 2017/06/13 13:09:26, nisse-webrtc wrote:
> Unrelated to this cl, but I wonder if it's really necessary to do this first?
> 
> I would have imagined that we could first lookup the ssrc in the |sinks_|
> multimap, and only if the returned |it_range| is empty, attempt to process the
> rsid. If we can get away with that, we don't need the |processed_ssrcs|
mapping
> at all.
> 
> Is there any valid use case where it's possible that |it_range| is non-empty
but
> the ssrc isn't present in the |processed_ssrcs_| map?

Yes. It could be that sink A is associated with SSRC 1, and sink B is associated
with with RSID "ELAD". If we now get an RTP message that has (SSRC=1,
RSID=ELAD), we don't want to avoid finding out sink B's association.

[eladalon, 2017-06-13 13:19:25]

The CQ bit was checked by eladalon@webrtc.org

[commit-bot: I haz the power, 2017-06-13 13:19:36]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-13 13:23:25]

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

[commit-bot: I haz the power, 2017-06-13 13:23:26]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18110)

[nisse-webrtc, 2017-06-13 13:28:54]

On 2017/06/13 13:19:12, eladalon wrote:
> Yes. It could be that sink A is associated with SSRC 1, and sink B is
associated
> with with RSID "ELAD". If we now get an RTP message that has (SSRC=1,
> RSID=ELAD), we don't want to avoid finding out sink B's association.

My question is: Is there any valid usecase where that state can happen occur?
Registering multiple sinks for the same ssrc is something we only do for
flexfec. And I don't know how we deal (or should deal) with rsid in flexfec. 

Would the SDP configure a media stream identified by rsid (unknown ssrc), and at
the same time, configure a flexfec stream protecting that media stream, using an
explicitly configured ssrc?

Anyway, lgtm for this logging change. But it would be a lot nicer if we could
get rid of the extra state.

[eladalon, 2017-06-13 14:06:13]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-06-13 14:06:14]

The patchset sent to the CQ was uploaded after l-g-t-m from nisse@webrtc.org,
danilchap@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2941513002/#ps60001 (title:
".")

[commit-bot: I haz the power, 2017-06-13 14:06:21]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-13 14:10:10]

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

[commit-bot: I haz the power, 2017-06-13 14:10:11]

Try jobs failed on following builders:
  presubmit on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/18113)

[eladalon, 2017-06-13 14:42:35]

eladalon@webrtc.org changed reviewers:
+ stefan@webrtc.org

[eladalon, 2017-06-13 14:42:36]

Owner added.

[stefan-webrtc, 2017-06-13 14:51:48]

Rubberstamp lgtm

[eladalon, 2017-06-13 14:53:09]

The CQ bit was checked by eladalon@webrtc.org

[commit-bot: I haz the power, 2017-06-13 14:53:22]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-06-13 14:57:32]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497365589764250,
"parent_rev": "7ed35f46433e6da91e7843eab20bad92a8f2b145", "commit_rev":
"dea075c7a6bee96f319192f7beca03ef76fc0500"}

[commit-bot: I haz the power, 2017-06-13 14:57:37]

Description was changed from

==========
Log an error in RtpDemuxer::FindSsrcAssociations() if kMaxProcessedSsrcs
exceeded

BUG=None
==========

to

==========
Log an error in RtpDemuxer::FindSsrcAssociations() if kMaxProcessedSsrcs
exceeded

BUG=None

Review-Url: https://codereview.webrtc.org/2941513002
Cr-Commit-Position: refs/heads/master@{#18569}
Committed:
https://chromium.googlesource.com/external/webrtc/+/dea075c7a6bee96f319192f7b...
==========

[commit-bot: I haz the power, 2017-06-13 14:57:38]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/dea075c7a6bee96f319192f7b...