Removed the legacy behavior of stopping playout when setting new receive codecs.

Removed the legacy behavior of stopping playout when setting new receive codecs.

BUG=webrtc:4690
Committed: https://crrev.com/917d4e1e7131f35764cff932a8793151585e8179
Cr-Commit-Position: refs/heads/master@{#14610}

[the sun, 2016-10-10 12:55:37]

solenberg@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[the sun, 2016-10-10 12:55:38]

Can you think of any reason why playout would need to be stopped when updating
the set of supported codecs?

[kwiberg-webrtc, 2016-10-10 21:48:41]

Description was changed from

==========
Removed the legacy behavior of stopping playout when setting new receive codecs.

BUG=webrtc:4690
==========

to

==========
Removed the legacy behavior of stopping playout when setting new receive codecs.

BUG=webrtc:4690
==========

[kwiberg-webrtc, 2016-10-10 21:48:41]

kwiberg@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[kwiberg-webrtc, 2016-10-10 21:53:06]

On 2016/10/10 12:55:38, the sun wrote:
> Can you think of any reason why playout would need to be stopped when updating
> the set of supported codecs?

Maybe... I seem to remember hlundin@ talking about NetEq needing to toss
queued-up incoming packets. Looping in the man, the myth, the legend himself so
that he can comment.

That said, I see no reason in principle. If lower levels need to do housekeeping
in response to a change like this, it seems like it's their job.

[hlundin-webrtc, 2016-10-11 11:59:50]

On 2016/10/10 21:53:06, kwiberg-webrtc wrote:
> On 2016/10/10 12:55:38, the sun wrote:
> > Can you think of any reason why playout would need to be stopped when
updating
> > the set of supported codecs?
> 
> Maybe... I seem to remember hlundin@ talking about NetEq needing to toss
> queued-up incoming packets. Looping in the man, the myth, the legend himself
so
> that he can comment.
> 
> That said, I see no reason in principle. If lower levels need to do
housekeeping
> in response to a change like this, it seems like it's their job.

When removing a decoder, NetEq will discard any queued-up packets with the
payload type that is removed. NetEq has one big-ol' lock protecting all of it,
so I cannot see any reason for concurrency problems either.

[hlundin-webrtc, 2016-10-11 12:03:03]

henrik.lundin@webrtc.org changed reviewers:
- henrik.lundin@webrtc.org

[hlundin-webrtc, 2016-10-11 12:03:03]

Removing self from reviewers. The UI is vile, defaulting "add as reviewer" just
because you reply to a question.

[kwiberg-webrtc, 2016-10-12 09:03:51]

lgtm

[the sun, 2016-10-12 09:07:03]

The CQ bit was checked by solenberg@webrtc.org

[commit-bot: I haz the power, 2016-10-12 09:07:17]

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, 2016-10-12 10:20:31]

Description was changed from

==========
Removed the legacy behavior of stopping playout when setting new receive codecs.

BUG=webrtc:4690
==========

to

==========
Removed the legacy behavior of stopping playout when setting new receive codecs.

BUG=webrtc:4690
==========

[commit-bot: I haz the power, 2016-10-12 10:20:32]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-10-12 10:20:41]

Description was changed from

==========
Removed the legacy behavior of stopping playout when setting new receive codecs.

BUG=webrtc:4690
==========

to

==========
Removed the legacy behavior of stopping playout when setting new receive codecs.

BUG=webrtc:4690

Committed: https://crrev.com/917d4e1e7131f35764cff932a8793151585e8179
Cr-Commit-Position: refs/heads/master@{#14610}
==========

[commit-bot: I haz the power, 2016-10-12 10:20:42]

Patchset 1 (id:??) landed as
https://crrev.com/917d4e1e7131f35764cff932a8793151585e8179
Cr-Commit-Position: refs/heads/master@{#14610}

[skvlad, 2016-11-02 21:22:29]

This change breaks a scenario that is unfortunately not covered by unit tests,
but can easily happen in a real call.

The scenario that is broken by the change is this:
1. A sends an offer to B, with a set of codecs C_a (which is a subset of C_b,
the codecs supported by B)
2. B responds with an answer, and sets the receive codecs to C_a.
3. At a later time, B generates a new offer which by default includes all codecs
in C_b.
4. B calls SetLocalDescription() with this offer, that adds new receive codecs.
5. Adding the new codecs fails, because of the check at
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel.....
This causes SetLocalDescription() itself to fail. The net effect is that B
cannot set a local description it just generated.

Before the CL mentioned above, we'd stop playout before changing the codecs, and
the operation would succeed.

[kwiberg-webrtc, 2016-11-03 09:17:19]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.webrtc.org/2478433003/ by kwiberg@webrtc.org.

The reason for reverting is: Reverting because of the reasons given in comment
#16:

"This change breaks a scenario that is unfortunately not covered by unit tests,
but can easily happen in a real call.

The scenario that is broken by the change is this:
1. A sends an offer to B, with a set of codecs C_a (which is a subset of C_b,
the codecs supported by B)
2. B responds with an answer, and sets the receive codecs to C_a.
3. At a later time, B generates a new offer which by default includes all codecs
in C_b.
4. B calls SetLocalDescription() with this offer, that adds new receive codecs.
5. Adding the new codecs fails, because of the check at
https://cs.chromium.org/chromium/src/third_party/webrtc/voice_engine/channel.....
This causes SetLocalDescription() itself to fail. The net effect is that B
cannot set a local description it just generated.

Before the CL mentioned above, we'd stop playout before changing the codecs, and
the operation would succeed.".