Untangle ViEChannel and ViEEncoder.

Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

Also adding DCHECKs to document what's only used by the
sender/receiver side.

BUG=webrtc:5494
R=stefan@webrtc.org

Committed: https://crrev.com/1d04ac6f29f235e593fb04896ad229b63562717c
Cr-Commit-Position: refs/heads/master@{#11500}

[pbos-webrtc, 2016-02-01 18:08:30]

PTAL

[pbos-webrtc, 2016-02-01 18:17:17]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-01 18:17:25]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1654913002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1654913002/1

[pbos-webrtc, 2016-02-01 18:18:43]

I need to move the VCM outside of ViEEncoder for this to be shared, bummer.

[pbos-webrtc, 2016-02-01 18:21:31]

On 2016/02/01 18:18:43, pbos-webrtc wrote:
> I need to move the VCM outside of ViEEncoder for this to be shared, bummer.

Or destroy vie_channel_ before vie_encoder_, maybe that's fine for now.

[pbos-webrtc, 2016-02-01 18:22:05]

destroy ViEEncoder after ViEChannel

[pbos-webrtc, 2016-02-01 18:24:17]

deregister protection callback before destroying vie_channel_

[pbos-webrtc, 2016-02-01 18:24:29]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-01 18:24:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1654913002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1654913002/40001

[pbos-webrtc, 2016-02-01 18:33:05]

WDYT? Should I extract video processing and vcm to have it /obviously/ outlive
both objects or is this fine?

[stefan-webrtc, 2016-02-01 19:04:44]

https://codereview.webrtc.org/1654913002/diff/40001/webrtc/video/video_receiv...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/1654913002/diff/40001/webrtc/video/video_receiv...
webrtc/video/video_receive_stream.h:89: PayloadRouter payload_router_;
The receiver shouldn't have a payload router, only the sender.

https://codereview.webrtc.org/1654913002/diff/40001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/1654913002/diff/40001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:154: &payload_router_, vcm_,
encoder_feedback_->GetRtcpIntraFrameObserver(),
Is there any way we can make things a bit less complex by not passing a vcm_ to
ViEChannel for the send stream? Is this even the same behavior as before where
we created a separate vcm in ViEChannel?

[commit-bot: I haz the power, 2016-02-01 19:36:21]

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

[commit-bot: I haz the power, 2016-02-01 19:36:21]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2016-02-02 14:17:32]

remove receiver payload router

[pbos-webrtc, 2016-02-02 14:32:33]

remove sender VCM from ViEChannel

[pbos-webrtc, 2016-02-02 14:34:09]

PTAL, I'd still like to keep the vcm_ member as an input parameter so things can
be moved into VideoReceiveStream as appropriate after this CL. Also added a lot
of DCHECKs to mark what's receiver-only.

[pbos-webrtc, 2016-02-02 14:34:14]

The CQ bit was checked by pbos@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-02 14:36:31]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1654913002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1654913002/80001

[commit-bot: I haz the power, 2016-02-02 15:37:45]

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

[commit-bot: I haz the power, 2016-02-02 15:37:46]

Dry run: This issue passed the CQ dry run.

[pbos-webrtc, 2016-02-02 15:44:28]

Description was changed from

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects and shares VCM between
them (although currently still initialized inside ViEEncoder).

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

BUG=
R=stefan@webrtc.org
==========

to

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from the sender.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

BUG=
R=stefan@webrtc.org
==========

[pbos-webrtc, 2016-02-02 15:44:40]

Description was changed from

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from the sender.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

BUG=
R=stefan@webrtc.org
==========

to

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

BUG=
R=stefan@webrtc.org
==========

[stefan-webrtc, 2016-02-04 10:46:14]

https://codereview.webrtc.org/1654913002/diff/80001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1654913002/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:515: vcm_->RegisterPacketRequestCallback(nullptr);
Canw e move this down to line 522?

https://codereview.webrtc.org/1654913002/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:516: if (paced_sender_ == nullptr) {
Should always be nullptr for a receiver, right? Should we make sure it is in
this CL too?

https://codereview.webrtc.org/1654913002/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:887: rtp_rtcp->SetSendingMediaStatus(true);
Should we set this to true only for the senders?

[pbos-webrtc, 2016-02-04 14:03:49]

stefan@ feedback

[pbos-webrtc, 2016-02-04 14:04:21]

PTAL woo

https://codereview.webrtc.org/1654913002/diff/80001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1654913002/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:515: vcm_->RegisterPacketRequestCallback(nullptr);
On 2016/02/04 10:46:14, stefan-webrtc (holmer) wrote:
> Canw e move this down to line 522?

Merged below things with this one.

https://codereview.webrtc.org/1654913002/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:516: if (paced_sender_ == nullptr) {
On 2016/02/04 10:46:14, stefan-webrtc (holmer) wrote:
> Should always be nullptr for a receiver, right? Should we make sure it is in
> this CL too?

Merged with above !sender_ check.

https://codereview.webrtc.org/1654913002/diff/80001/webrtc/video/vie_channel....
webrtc/video/vie_channel.cc:887: rtp_rtcp->SetSendingMediaStatus(true);
On 2016/02/04 10:46:14, stefan-webrtc (holmer) wrote:
> Should we set this to true only for the senders?

Done.

[pbos-webrtc, 2016-02-04 14:05:06]

Description was changed from

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

BUG=
R=stefan@webrtc.org
==========

to

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

Also adding DCHECKs to document what's used by the
sender/receiver side.

BUG=
R=stefan@webrtc.org
==========

[pbos-webrtc, 2016-02-04 14:05:22]

Description was changed from

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

Also adding DCHECKs to document what's used by the
sender/receiver side.

BUG=
R=stefan@webrtc.org
==========

to

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

Also adding DCHECKs to document what's only used by the
sender/receiver side.

BUG=
R=stefan@webrtc.org
==========

[stefan-webrtc, 2016-02-05 07:55:03]

lg, just a few comments!

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel...
webrtc/video/vie_channel.cc:93: bool sender)
We could get rid of the sender flag and have send_payload_router signal that
this is a sending channel?

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel...
webrtc/video/vie_channel.cc:485: vcm_->SetVideoProtection(protection_method,
true);
Should this really not be called on the send-side too? It looks to me like this
actually was doing something on both sides.

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel...
webrtc/video/vie_channel.cc:522: vie_receiver_.SetNackStatus(false,
max_nack_reordering_threshold_);
Actually, shouldn't we move this inside !sender_ too? Now it looks a bit too
weird to me, where we explicitly call vie_receiver_ on a sending channel.

[stefan-webrtc, 2016-02-05 08:16:49]

Maybe assign a bug for this as you already have indicated that you will do
several follow-ups refactoring vie channel etc.

[pbos-webrtc, 2016-02-05 09:52:36]

Description was changed from

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

Also adding DCHECKs to document what's only used by the
sender/receiver side.

BUG=
R=stefan@webrtc.org
==========

to

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

Also adding DCHECKs to document what's only used by the
sender/receiver side.

BUG=webrtc:5494
R=stefan@webrtc.org
==========

[pbos-webrtc, 2016-02-05 09:54:37]

PTAL

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel...
webrtc/video/vie_channel.cc:93: bool sender)
On 2016/02/05 07:55:02, stefan-webrtc (holmer) wrote:
> We could get rid of the sender flag and have send_payload_router signal that
> this is a sending channel?

I prefer having sender_ since it makes it easier to read. I am aiming to get rid
of this completely though, but sender_ and !sender_ makes it easier to read for
me while refactoring.

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel...
webrtc/video/vie_channel.cc:485: vcm_->SetVideoProtection(protection_method,
true);
On 2016/02/05 07:55:02, stefan-webrtc (holmer) wrote:
> Should this really not be called on the send-side too? It looks to me like
this
> actually was doing something on both sides.

The vcm_ on the send side before was bogus (the one in use is in ViEEncoder).

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel...
webrtc/video/vie_channel.cc:522: vie_receiver_.SetNackStatus(false,
max_nack_reordering_threshold_);
On 2016/02/05 07:55:02, stefan-webrtc (holmer) wrote:
> Actually, shouldn't we move this inside !sender_ too? Now it looks a bit too
> weird to me, where we explicitly call vie_receiver_ on a sending channel.

ViEReceiver is actually used for both sender/receiver, the sender also has a
receiving part.

[pbos-webrtc, 2016-02-05 09:55:22]

On 2016/02/05 08:16:49, stefan-webrtc (holmer) wrote:
> Maybe assign a bug for this as you already have indicated that you will do
> several follow-ups refactoring vie channel etc.

Done!

[stefan-webrtc, 2016-02-05 10:07:27]

lgtm

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel.cc
File webrtc/video/vie_channel.cc (right):

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel...
webrtc/video/vie_channel.cc:93: bool sender)
On 2016/02/05 09:54:37, pbos-webrtc wrote:
> On 2016/02/05 07:55:02, stefan-webrtc (holmer) wrote:
> > We could get rid of the sender flag and have send_payload_router signal that
> > this is a sending channel?
> 
> I prefer having sender_ since it makes it easier to read. I am aiming to get
rid
> of this completely though, but sender_ and !sender_ makes it easier to read
for
> me while refactoring.

Agree, was mostly thinking about the bool sender in the constructor. You could
change to sender_(send_payload_router != nullptr)

But feel free to skip

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel...
webrtc/video/vie_channel.cc:485: vcm_->SetVideoProtection(protection_method,
true);
On 2016/02/05 09:54:37, pbos-webrtc wrote:
> On 2016/02/05 07:55:02, stefan-webrtc (holmer) wrote:
> > Should this really not be called on the send-side too? It looks to me like
> this
> > actually was doing something on both sides.
> 
> The vcm_ on the send side before was bogus (the one in use is in ViEEncoder).

Acknowledged.

https://codereview.webrtc.org/1654913002/diff/100001/webrtc/video/vie_channel...
webrtc/video/vie_channel.cc:522: vie_receiver_.SetNackStatus(false,
max_nack_reordering_threshold_);
On 2016/02/05 09:54:37, pbos-webrtc wrote:
> On 2016/02/05 07:55:02, stefan-webrtc (holmer) wrote:
> > Actually, shouldn't we move this inside !sender_ too? Now it looks a bit too
> > weird to me, where we explicitly call vie_receiver_ on a sending channel.
> 
> ViEReceiver is actually used for both sender/receiver, the sender also has a
> receiving part.

Ah yes... :/

Should probably remove the option to disable this on the sender. Not now though.

[pbos-webrtc, 2016-02-05 10:22:50]

rebase

[pbos-webrtc, 2016-02-05 10:25:57]

Description was changed from

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

Also adding DCHECKs to document what's only used by the
sender/receiver side.

BUG=webrtc:5494
R=stefan@webrtc.org
==========

to

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

Also adding DCHECKs to document what's only used by the
sender/receiver side.

BUG=webrtc:5494
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1d04ac6f29f235e593fb04896...
==========

[pbos-webrtc, 2016-02-05 10:25:58]

Committed patchset #7 (id:120001) manually as
1d04ac6f29f235e593fb04896ad229b63562717c (presubmit successful).

[commit-bot: I haz the power, 2016-02-05 10:25:59]

Description was changed from

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

Also adding DCHECKs to document what's only used by the
sender/receiver side.

BUG=webrtc:5494
R=stefan@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/1d04ac6f29f235e593fb04896...
==========

to

==========
Untangle ViEChannel and ViEEncoder.

Extracts shared members outside the two objects, removing PayloadRouter
from receivers and the VCM for ViEChannel from senders.

Removes Start/StopThreadsAndSetSharedMembers that was used to set the
shared state between them.

Also adding DCHECKs to document what's only used by the
sender/receiver side.

BUG=webrtc:5494
R=stefan@webrtc.org

Committed: https://crrev.com/1d04ac6f29f235e593fb04896ad229b63562717c
Cr-Commit-Position: refs/heads/master@{#11500}
==========

[commit-bot: I haz the power, 2016-02-05 10:26:01]

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

[mflodman, 2016-02-05 13:01:27]

mflodman@webrtc.org changed reviewers:
+ mflodman@webrtc.org

[mflodman, 2016-02-05 13:01:29]

Two drive-by after the fact comments.

https://codereview.webrtc.org/1654913002/diff/120001/webrtc/video/video_recei...
File webrtc/video/video_receive_stream.h (right):

https://codereview.webrtc.org/1654913002/diff/120001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.h:24: #include "webrtc/video/payload_router.h"
This doesn't seem righ.

https://codereview.webrtc.org/1654913002/diff/120001/webrtc/video/video_recei...
webrtc/video/video_receive_stream.h:84: ProcessThread* const process_thread_;
Old, but we should forward declare ProcessThread in this header.

[pbos-webrtc, 2016-02-05 13:33:29]

On 2016/02/05 13:01:29, mflodman wrote:
> Two drive-by after the fact comments.
> 
>
https://codereview.webrtc.org/1654913002/diff/120001/webrtc/video/video_recei...
> File webrtc/video/video_receive_stream.h (right):
> 
>
https://codereview.webrtc.org/1654913002/diff/120001/webrtc/video/video_recei...
> webrtc/video/video_receive_stream.h:24: #include
"webrtc/video/payload_router.h"
> This doesn't seem righ.
> 
>
https://codereview.webrtc.org/1654913002/diff/120001/webrtc/video/video_recei...
> webrtc/video/video_receive_stream.h:84: ProcessThread* const process_thread_;
> Old, but we should forward declare ProcessThread in this header.

Done as patchset in https://codereview.webrtc.org/1671893002.