Remove RtpDemuxer tweak for preventing multiple RSID inspections

Remove RtpDemuxer tweak for preventing multiple RSID inspections

We have a tweak preventing multiple deep-examinations of packets; packets with a given SSRC are only inspected deeply (RSID) once (only the first received packet). Once we move to many-to-one stream-to-sink associations, this becomes less useful, and is better removed.

BUG=webrtc:7135
Review-Url: https://codereview.webrtc.org/2955373002
Cr-Commit-Position: refs/heads/master@{#18859}
Committed: https://chromium.googlesource.com/external/webrtc/+/9addbebf427a8a49cd5a43cf4827c7c8b6e24c21

[eladalon, 2017-06-28 14:58:45]

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

[eladalon, 2017-06-28 14:58:46]

PTAL

[danilchap, 2017-06-28 16:42:39]

Description was changed from

==========
Remove RtpDemuxer tweak for preventing multiple RSID inspections

We have a tweak preventing multiple deep-examinations of packets; packets with a
given SSRC are only inspected deeply (RSID) once (only the first received
packet). Once we move to many-to-one stream-to-sink associations, this becomes
less useful, and is better removed.

BUG=BUG=webrtc:7135
==========

to

==========
Remove RtpDemuxer tweak for preventing multiple RSID inspections

We have a tweak preventing multiple deep-examinations of packets; packets with a
given SSRC are only inspected deeply (RSID) once (only the first received
packet). Once we move to many-to-one stream-to-sink associations, this becomes
less useful, and is better removed.

BUG=webrtc:7135
==========

[danilchap, 2017-06-28 16:47:43]

lgtm

[danilchap, 2017-06-28 16:51:35]

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

https://codereview.webrtc.org/2955373002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:65: ResolveRsidToSsrcAssociations(packet);
may be add if (!rsid_sinks_.empty()) as a fast check.
(may be consider it a preliminary optimization and do not)

[eladalon, 2017-06-29 08:10:00]

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

https://codereview.webrtc.org/2955373002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:65: ResolveRsidToSsrcAssociations(packet);
On 2017/06/28 16:51:34, danilchap OOO until July17 wrote:
> may be add if (!rsid_sinks_.empty()) as a fast check.
> (may be consider it a preliminary optimization and do not)

Would need to also check for no RSID resolution observers. Then again, as we've
discussed, we only expect such observers when rsid_sinks_ is not empty here... I
feel that this gives the class too many undocumented assumptions about the way
it's used, though. I'd like to postpone thinking about this until we've removed
many-to-many support, added MID/PlType demuxing, etc.

[eladalon, 2017-06-29 08:10:21]

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

[stefan-webrtc, 2017-06-29 14:50:41]

rs-lgtm, but you should probably have nisse's stamp as well before landing...

[nisse-webrtc, 2017-06-29 14:59:43]

lgtm

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

https://codereview.webrtc.org/2955373002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:102: NotifyObserversOfRsidResolution(rsid,
packet.Ssrc());
Does this mean we also get one callback to the observer(s) per packet?

Sounds excessive, but ok if this is going to be fixed soon when you change from
multimap to map.

[eladalon, 2017-06-29 17:59:07]

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

https://codereview.webrtc.org/2955373002/diff/1/webrtc/call/rtp_demuxer.cc#ne...
webrtc/call/rtp_demuxer.cc:102: NotifyObserversOfRsidResolution(rsid,
packet.Ssrc());
On 2017/06/29 14:59:43, nisse-webrtc (ooo August 14) wrote:
> Does this mean we also get one callback to the observer(s) per packet?
> 
> Sounds excessive, but ok if this is going to be fixed soon when you change
from
> multimap to map. 

Yes and yes.

[eladalon, 2017-06-29 17:59:17]

The CQ bit was checked by eladalon@webrtc.org

[commit-bot: I haz the power, 2017-06-29 17:59:20]

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-29 18:30:09]

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

[commit-bot: I haz the power, 2017-06-29 18:30:10]

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

[eladalon, 2017-06-29 20:16:21]

The CQ bit was checked by eladalon@webrtc.org

[commit-bot: I haz the power, 2017-06-29 20:16:30]

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-29 20:47:00]

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

[commit-bot: I haz the power, 2017-06-29 20:47:01]

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

[eladalon, 2017-06-30 10:35:37]

The CQ bit was checked by eladalon@webrtc.org

[eladalon, 2017-06-30 10:35:38]

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

[commit-bot: I haz the power, 2017-06-30 10:35:43]

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-30 10:41:09]

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

[commit-bot: I haz the power, 2017-06-30 10:41:11]

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

[eladalon, 2017-06-30 12:53:47]

The CQ bit was checked by eladalon@webrtc.org

[commit-bot: I haz the power, 2017-06-30 12:53:52]

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-30 13:26:55]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498827227762070,
"parent_rev": "49085ef280db84e47f41a06a247db56c70f6f037", "commit_rev":
"9addbebf427a8a49cd5a43cf4827c7c8b6e24c21"}

[commit-bot: I haz the power, 2017-06-30 13:27:08]

Description was changed from

==========
Remove RtpDemuxer tweak for preventing multiple RSID inspections

We have a tweak preventing multiple deep-examinations of packets; packets with a
given SSRC are only inspected deeply (RSID) once (only the first received
packet). Once we move to many-to-one stream-to-sink associations, this becomes
less useful, and is better removed.

BUG=webrtc:7135
==========

to

==========
Remove RtpDemuxer tweak for preventing multiple RSID inspections

We have a tweak preventing multiple deep-examinations of packets; packets with a
given SSRC are only inspected deeply (RSID) once (only the first received
packet). Once we move to many-to-one stream-to-sink associations, this becomes
less useful, and is better removed.

BUG=webrtc:7135

Review-Url: https://codereview.webrtc.org/2955373002
Cr-Commit-Position: refs/heads/master@{#18859}
Committed:
https://chromium.googlesource.com/external/webrtc/+/9addbebf427a8a49cd5a43cf4...
==========

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

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/9addbebf427a8a49cd5a43cf4...