Add task queue to Call.

- Add task queue to Call with the intent of replacing the use of one of the process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on the new task queue.

- BitrateAllocator is now created on libjingle's worker thread but always used on the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantics.

- The encoder thread is moved from VideoSendStream to ViEEncoder. Frames are forwarded directly to ViEEncoder which is responsible for timestamping ? and encoding the frames.

BUG=webrtc:5687
Committed: https://crrev.com/cc168360f41322332860cb075edeb1cde21aa473
Cr-Commit-Position: refs/heads/master@{#13767}

[perkj_webrtc, 2016-06-14 15:15:23]

Description was changed from

==========
Add task queue to Cal.
Split VideoSendStream in two. VideoSendStreamInternal is created and used on the
task queue.
BUG=
==========

to

==========
- Add task queue to Call with the intend to replace the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingles worker thread but always used on
the new task queue instead of both encoder threads and the process thread.

BUG=
==========

[perkj_webrtc, 2016-06-14 15:15:23]

perkj@webrtc.org changed reviewers:
+ tommi@webrtc.org

[tommi, 2016-06-17 07:59:02]

haven't taken a thorough look yet, but have at least one comment :)

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/bitrate_alloc...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/bitrate_alloc...
webrtc/call/bitrate_allocator.cc:118: }
run git cl format

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:138: rtc::TaskQueue worker_queu_;
worker_queue_

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

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:373:
ConstructionTask(std::unique_ptr<VideoSendStreamInternal>* send_stream,
todo(tommi): see if there's a more elegant way to do this

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:538: return send_stream_->GetStats();
what thread are we on here?

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:560: return send_stream_->DeliverRtcp(packet,
length);
and here?

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:62: rtc::TaskQueue* worker_queu,
worker_queue

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:93: rtc::TaskQueue* const worker_queu_;
worker_queue_

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:104: rtc::TaskQueue* worker_queu,
worker_queue :)

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:113: const std::map<uint32_t, RtpState>&
suspended_ssrcs);
should any of these more expensive parameters be passed via std::move?

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:128: // webrtc::VideoSendStream implementation.
are the below methods overrides?

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:177: rtc::TaskQueue* const worker_queu_;
ditto

[perkj_webrtc, 2016-06-17 13:18:19]

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

[perkj_webrtc, 2016-06-17 13:18:20]

Adding mflodman. 
I have not yet addressed the comments. 
only fyi yet.

[perkj_webrtc, 2016-06-27 14:33:54]

Description was changed from

==========
- Add task queue to Call with the intend to replace the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingles worker thread but always used on
the new task queue instead of both encoder threads and the process thread.

BUG=
==========

to

==========
- Add task queue to Call with the intend to replace the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingles worker thread but always used on
the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantic.

BUG=5687
==========

[perkj_webrtc, 2016-06-27 14:34:35]

Hi
Can you please review?

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/bitrate_alloc...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/bitrate_alloc...
webrtc/call/bitrate_allocator.cc:118: }
On 2016/06/17 07:59:01, tommi-webrtc wrote:
> run git cl format

Done.

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/call.cc#newco...
webrtc/call/call.cc:138: rtc::TaskQueue worker_queu_;
On 2016/06/17 07:59:01, tommi-webrtc wrote:
> worker_queue_

Done.

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

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:373:
ConstructionTask(std::unique_ptr<VideoSendStreamInternal>* send_stream,
On 2016/06/17 07:59:01, tommi-webrtc wrote:
> todo(tommi): see if there's a more elegant way to do this

Done.

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:538: return send_stream_->GetStats();
On 2016/06/17 07:59:01, tommi-webrtc wrote:
> what thread are we on here?
Added comments.

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.cc:560: return send_stream_->DeliverRtcp(packet,
length);
On 2016/06/17 07:59:01, tommi-webrtc wrote:
> and here?

added comment.

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:62: rtc::TaskQueue* worker_queu,
On 2016/06/17 07:59:01, tommi-webrtc wrote:
> worker_queue

Done.

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:93: rtc::TaskQueue* const worker_queu_;
On 2016/06/17 07:59:01, tommi-webrtc wrote:
> worker_queue_

Done.

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:104: rtc::TaskQueue* worker_queu,
On 2016/06/17 07:59:02, tommi-webrtc wrote:
> worker_queue :)

Done.

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:113: const std::map<uint32_t, RtpState>&
suspended_ssrcs);
On 2016/06/17 07:59:01, tommi-webrtc wrote:
> should any of these more expensive parameters be passed via std::move?

Done.

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:128: // webrtc::VideoSendStream implementation.
On 2016/06/17 07:59:01, tommi-webrtc wrote:
> are the below methods overrides?

No, not after splitting VideoSendStream in two. VideoSendStream delegates to
VideoSendStreamInternal.

https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
webrtc/video/video_send_stream.h:177: rtc::TaskQueue* const worker_queu_;
On 2016/06/17 07:59:02, tommi-webrtc wrote:
> ditto

Done.

[perkj_webrtc, 2016-06-27 15:14:11]

On 2016/06/27 14:34:35, perkj_webrtc_ooo_june20 wrote:
> Hi
> Can you please review?
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/bitrate_alloc...
> File webrtc/call/bitrate_allocator.cc (right):
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/bitrate_alloc...
> webrtc/call/bitrate_allocator.cc:118: }
> On 2016/06/17 07:59:01, tommi-webrtc wrote:
> > run git cl format
> 
> Done.
> 
> https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/call.cc
> File webrtc/call/call.cc (right):
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/call/call.cc#newco...
> webrtc/call/call.cc:138: rtc::TaskQueue worker_queu_;
> On 2016/06/17 07:59:01, tommi-webrtc wrote:
> > worker_queue_
> 
> Done.
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> File webrtc/video/video_send_stream.cc (right):
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> webrtc/video/video_send_stream.cc:373:
> ConstructionTask(std::unique_ptr<VideoSendStreamInternal>* send_stream,
> On 2016/06/17 07:59:01, tommi-webrtc wrote:
> > todo(tommi): see if there's a more elegant way to do this
> 
> Done.
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> webrtc/video/video_send_stream.cc:538: return send_stream_->GetStats();
> On 2016/06/17 07:59:01, tommi-webrtc wrote:
> > what thread are we on here?
> Added comments.
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> webrtc/video/video_send_stream.cc:560: return
send_stream_->DeliverRtcp(packet,
> length);
> On 2016/06/17 07:59:01, tommi-webrtc wrote:
> > and here?
> 
> added comment.
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> File webrtc/video/video_send_stream.h (right):
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> webrtc/video/video_send_stream.h:62: rtc::TaskQueue* worker_queu,
> On 2016/06/17 07:59:01, tommi-webrtc wrote:
> > worker_queue
> 
> Done.
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> webrtc/video/video_send_stream.h:93: rtc::TaskQueue* const worker_queu_;
> On 2016/06/17 07:59:01, tommi-webrtc wrote:
> > worker_queue_
> 
> Done.
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> webrtc/video/video_send_stream.h:104: rtc::TaskQueue* worker_queu,
> On 2016/06/17 07:59:02, tommi-webrtc wrote:
> > worker_queue :)
> 
> Done.
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> webrtc/video/video_send_stream.h:113: const std::map<uint32_t, RtpState>&
> suspended_ssrcs);
> On 2016/06/17 07:59:01, tommi-webrtc wrote:
> > should any of these more expensive parameters be passed via std::move?
> 
> Done.
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> webrtc/video/video_send_stream.h:128: // webrtc::VideoSendStream
implementation.
> On 2016/06/17 07:59:01, tommi-webrtc wrote:
> > are the below methods overrides?
> 
> No, not after splitting VideoSendStream in two. VideoSendStream delegates to
> VideoSendStreamInternal.
> 
>
https://codereview.webrtc.org/2060403002/diff/80001/webrtc/video/video_send_s...
> webrtc/video/video_send_stream.h:177: rtc::TaskQueue* const worker_queu_;
> On 2016/06/17 07:59:02, tommi-webrtc wrote:
> > ditto
> 
> Done.

Darn - from the failing tests I realized that
VideoSendStreamInternal::ReconfigureVideoEncoder must be done on the encoder
thread. We need to reconfigure the encoder to happen before a frame with a new
size is encoded.

[tommi, 2016-06-29 14:49:41]

https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.cc (right):

https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:101: for (const auto& config :
bitrate_observer_configs_) {
run git cl format

https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.cc:117: }
please :)

[tommi, 2016-06-29 14:53:08]

On 2016/06/29 14:49:41, tommi-webrtc wrote:
>
https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
> File webrtc/call/bitrate_allocator.cc (right):
> 
>
https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
> webrtc/call/bitrate_allocator.cc:101: for (const auto& config :
> bitrate_observer_configs_) {
> run git cl format
> 
>
https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
> webrtc/call/bitrate_allocator.cc:117: }
> please :)

bah, please ignore. I thought I was looking at a different cr.

[perkj_webrtc, 2016-06-30 08:16:18]

On 2016/06/29 14:53:08, tommi-webrtc wrote:
> On 2016/06/29 14:49:41, tommi-webrtc wrote:
> >
>
https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
> > File webrtc/call/bitrate_allocator.cc (right):
> > 
> >
>
https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
> > webrtc/call/bitrate_allocator.cc:101: for (const auto& config :
> > bitrate_observer_configs_) {
> > run git cl format
> > 
> >
>
https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
> > webrtc/call/bitrate_allocator.cc:117: }
> > please :)
> 
> bah, please ignore. I thought I was looking at a different cr.

this, needs a bit more cleaning before its done...

[perkj_webrtc, 2016-06-30 08:20:14]

On 2016/06/30 08:16:18, perkj_webrtc_ooo_june20 wrote:
> On 2016/06/29 14:53:08, tommi-webrtc wrote:
> > On 2016/06/29 14:49:41, tommi-webrtc wrote:
> > >
> >
>
https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
> > > File webrtc/call/bitrate_allocator.cc (right):
> > > 
> > >
> >
>
https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
> > > webrtc/call/bitrate_allocator.cc:101: for (const auto& config :
> > > bitrate_observer_configs_) {
> > > run git cl format
> > > 
> > >
> >
>
https://codereview.webrtc.org/2060403002/diff/140001/webrtc/call/bitrate_allo...
> > > webrtc/call/bitrate_allocator.cc:117: }
> > > please :)
> > 
> > bah, please ignore. I thought I was looking at a different cr.
> 
> this, needs a bit more cleaning before its done...

btw, tommi- can you look at why compile fails on GN ? 
In file included from ../../webrtc/video/vie_encoder.h:20:0,
                 from ../../webrtc/video/send_statistics_proxy.h:28,
                 from ../../webrtc/video/video_send_stream_tests.cc:38:
../../webrtc/base/task_queue.h:269:2: error: #error not supported.
 #error not supported.

[perkj_webrtc, 2016-07-04 07:23:41]

Description was changed from

==========
- Add task queue to Call with the intend to replace the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingles worker thread but always used on
the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantic.

BUG=5687
==========

to

==========
- Add task queue to Call with the intend to replace the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingles worker thread but always used on
the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantic.

BUG=webrtc::5687
==========

[perkj_webrtc, 2016-07-05 09:09:14]

Description was changed from

==========
- Add task queue to Call with the intend to replace the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingles worker thread but always used on
the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantic.

BUG=webrtc::5687
==========

to

==========
- Add task queue to Call with the intend to replace the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingles worker thread but always used on
the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantic.

BUG=webrtc:5687
==========

[perkj_webrtc, 2016-07-05 14:25:05]

Description was changed from

==========
- Add task queue to Call with the intend to replace the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingles worker thread but always used on
the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantic.

BUG=webrtc:5687
==========

to

==========
- Add task queue to Call with the intend to replace the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingles worker thread but always used on
the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantic.

- The encoder thread is moved from VideoSendStream to ViEEncoder. Frames are
forwarded directly to ViEEncoder which is responsible for timestamping ? and
encoding the frames.

BUG=webrtc:5687
==========

[perkj_webrtc, 2016-07-06 08:20:10]

perkj@webrtc.org changed reviewers:
+ sprang@webrtc.org, stefan@webrtc.org

[perkj_webrtc, 2016-07-06 08:20:12]

Hi, Can you please review and we can start a discussion about  how I should get
this landed? It is not that easy to break up much further but I could land the
changes for making the configurations movable separately.

[tommi, 2016-07-06 10:53:34]

Description was changed from

==========
- Add task queue to Call with the intend to replace the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingles worker thread but always used on
the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantic.

- The encoder thread is moved from VideoSendStream to ViEEncoder. Frames are
forwarded directly to ViEEncoder which is responsible for timestamping ? and
encoding the frames.

BUG=webrtc:5687
==========

to

==========
- Add task queue to Call with the intent of replacing the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingle's worker thread but always used
on the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantics.

- The encoder thread is moved from VideoSendStream to ViEEncoder. Frames are
forwarded directly to ViEEncoder which is responsible for timestamping ? and
encoding the frames.

BUG=webrtc:5687
==========

[tommi, 2016-07-06 11:15:02]

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:201: // deleted before any other members.
pending tasks may actually not run when the worker_queue_ goes out of scope. 
They will be cancelled and deleted.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:438: std::vector<uint32_t> ssrcs = config.rtp.ssrcs;
do we need this array?  It looks like we create a copy just to iterate over the
ssrcs but I might be missing something.  In any case, I don't think the variable
needs to be declared here.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/test/fake_encoder...
webrtc/test/fake_encoder.cc:103: encoded.ntp_time_ms_ =
input_image.ntp_time_ms();
was this missing before?  Would it make sense to move setting these properties
into the constructor of EncodedImage so we won't have to update the call sites?

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/test/fake_encoder...
webrtc/test/fake_encoder.cc:109: if (min_stream_bits > bits_available && i > 0)
seems like this check should be done before we initialize EncodedImage.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/test/fake_encoder...
webrtc/test/fake_encoder.cc:111: assert(callback_ != NULL);
can you change all assert()s to RTC_DCHECK in this file?

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:217: ~ConstructionTask() { done_event_->Set();
}
override

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:219: bool Run() override {
nit: make the implementation of Run() private

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:251: ~DestructAndGetRTPStateTask() {
override

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:256: bool Run() override {
private

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:109: std::map<uint32_t, RtpState>
suspended_ssrcs);
will be the map be passed in via move()?

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:126: VideoSendStream::RtpStateMap
GetRtpStates() const;
by-value on purpose?

[perkj_webrtc, 2016-07-06 13:08:54]

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:201: // deleted before any other members.
On 2016/07/06 11:15:01, tommi-webrtc wrote:
> pending tasks may actually not run when the worker_queue_ goes out of scope. 
> They will be cancelled and deleted.

updated comment. That is what I want. I don't care if a task does not complete.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:438: std::vector<uint32_t> ssrcs = config.rtp.ssrcs;
On 2016/07/06 11:15:01, tommi-webrtc wrote:
> do we need this array?  It looks like we create a copy just to iterate over
the
> ssrcs but I might be missing something.  In any case, I don't think the
variable
> needs to be declared here.

Done.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/test/fake_encoder...
webrtc/test/fake_encoder.cc:103: encoded.ntp_time_ms_ =
input_image.ntp_time_ms();
On 2016/07/06 11:15:01, tommi-webrtc wrote:
> was this missing before?  Would it make sense to move setting these properties
> into the constructor of EncodedImage so we won't have to update the call
sites?

EncodedImage is a horrible struct that is used in both Chrome and else where. So
updating ctor is not that easy. (pbos have a todo to clean this struct up but I
guess that todo will be left for the future).

I added it here to be able to test ViEEncoder, but it turns out ntp_timestamp is
not used by any real encoder implementations so I should probly not do that. It
is used by subclasses of EncodedFrame the receive side.
https://cs.chromium.org/chromium/src/third_party/webrtc/video_frame.h?sq=pack...

I refactored the test and is no longer dependent on any changes in this file.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/test/fake_encoder...
webrtc/test/fake_encoder.cc:109: if (min_stream_bits > bits_available && i > 0)
On 2016/07/06 11:15:02, tommi-webrtc wrote:
> seems like this check should be done before we initialize EncodedImage.

ok, should not matter much since its only pointer initialization.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/test/fake_encoder...
webrtc/test/fake_encoder.cc:111: assert(callback_ != NULL);
On 2016/07/06 11:15:01, tommi-webrtc wrote:
> can you change all assert()s to RTC_DCHECK in this file?

Done.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:217: ~ConstructionTask() { done_event_->Set();
}
On 2016/07/06 11:15:02, tommi-webrtc wrote:
> override

Done.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:219: bool Run() override {
On 2016/07/06 11:15:02, tommi-webrtc wrote:
> nit: make the implementation of Run() private

Done.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:251: ~DestructAndGetRTPStateTask() {
On 2016/07/06 11:15:02, tommi-webrtc wrote:
> override

Done.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:256: bool Run() override {
On 2016/07/06 11:15:02, tommi-webrtc wrote:
> private

Done.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:109: std::map<uint32_t, RtpState>
suspended_ssrcs);
On 2016/07/06 11:15:02, tommi-webrtc wrote:
> will be the map be passed in via move()?

yes

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:126: VideoSendStream::RtpStateMap
GetRtpStates() const;
On 2016/07/06 11:15:02, tommi-webrtc wrote:
> by-value on purpose?

Yes, same as before. See VideoSendStream::GetRtpStates before my change.

See new usage in VideoSendStream::DestructAndGetRTPStateTask- we talked about
this and if I understood you corretly, this will turn into a move in release
build.

[sprang_webrtc, 2016-07-06 15:02:52]

Overall looks good.
nit: run git cl format; think I spotted some bad indentation

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:275: static const int kEncoderTimeOutMs =
2000;
Maybe we can use PacedSender::kMaxQueueLengthMs, which is the maximum time
packets will spend in the pacer queue. This queue length is limiting input rate
to the encoder. Maybe even with some error margin added.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:380: LOG(LS_INFO) << "~VideoSendStream: ";
Is this useful? I see there was a log like this before as well, but that one
appended the config as to the log..

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:456: // Called on a network thread.
Do we wanna dcheck that?

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:605: LOG_F(LS_INFO) << "Start";
Was this left intentionally?

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:655: void
VideoSendStreamInternal::EncoderIsActive() {
Can we rename this SignalEncoderActive() or something? Sounds like a question
now :)

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:213: }
Can posted_frames_waiting_for_encode_ ever be 0 here?
Maybe only call EncodeVideoFrame() on posted_frames_waiting_for_encode_ == 1
Of course, I intend to totally muck this up later :D

[perkj_webrtc, 2016-07-07 08:37:12]

ptal

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:275: static const int kEncoderTimeOutMs =
2000;
On 2016/07/06 15:02:51, språng wrote:
> Maybe we can use PacedSender::kMaxQueueLengthMs, which is the maximum time
> packets will spend in the pacer queue. This queue length is limiting input
rate
> to the encoder. Maybe even with some error margin added.

But I don't think the pacer queue length is related to the encoder time out.
This is here to disable padding if the encoder is no longer producing output.
Apparently added to make sure that if a camera is unplugged, we stop sending any
video data at all after a while.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:380: LOG(LS_INFO) << "~VideoSendStream: ";
On 2016/07/06 15:02:51, språng wrote:
> Is this useful? I see there was a log like this before as well, but that one
> appended the config as to the log..

removed

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:456: // Called on a network thread.
On 2016/07/06 15:02:51, språng wrote:
> Do we wanna dcheck that?

It is not that easy. It seems like it the libjingle worker thread in protuction,
but another thread in tests. See comment in PacketReceiver::DeliveryStatus
Call::DeliverPacket.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:605: LOG_F(LS_INFO) << "Start";
On 2016/07/06 15:02:51, språng wrote:
> Was this left intentionally?

yes, but it should be LOG...

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:655: void
VideoSendStreamInternal::EncoderIsActive() {
On 2016/07/06 15:02:51, språng wrote:
> Can we rename this SignalEncoderActive() or something? Sounds like a question
> now :)

Done.

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:213: }
On 2016/07/06 15:02:51, språng wrote:
> Can posted_frames_waiting_for_encode_ ever be 0 here?
> Maybe only call EncodeVideoFrame() on posted_frames_waiting_for_encode_ == 1
> Of course, I intend to totally muck this up later :D

ptal

[sprang_webrtc, 2016-07-07 09:25:10]

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:275: static const int kEncoderTimeOutMs =
2000;
On 2016/07/07 08:37:12, perkj_webrtc wrote:
> On 2016/07/06 15:02:51, språng wrote:
> > Maybe we can use PacedSender::kMaxQueueLengthMs, which is the maximum time
> > packets will spend in the pacer queue. This queue length is limiting input
> rate
> > to the encoder. Maybe even with some error margin added.
> 
> But I don't think the pacer queue length is related to the encoder time out.
> This is here to disable padding if the encoder is no longer producing output.
> Apparently added to make sure that if a camera is unplugged, we stop sending
any
> video data at all after a while.

It is at least a lower bound. What I'm worried is that a large frame with low
bandwidth sits around in the pacer for two seconds, is borderline long enough to
disable the encoder. Likely, if the encoder overshot the bitrate target by a
lot, the next few frames will be below target to compensate, needing padding to
keep bwe up... But maybe if padding kicks in again as soon as any frame is
produce, this is a non issue.
wdyt?

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:456: // Called on a network thread.
On 2016/07/07 08:37:12, perkj_webrtc wrote:
> On 2016/07/06 15:02:51, språng wrote:
> > Do we wanna dcheck that?
> 
> It is not that easy. It seems like it the libjingle worker thread in
protuction,
> but another thread in tests. See comment in PacketReceiver::DeliveryStatus
> Call::DeliverPacket.

Acknowledged.

[perkj_webrtc, 2016-07-08 06:36:29]

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:275: static const int kEncoderTimeOutMs =
2000;
On 2016/07/07 09:25:10, språng wrote:
> On 2016/07/07 08:37:12, perkj_webrtc wrote:
> > On 2016/07/06 15:02:51, språng wrote:
> > > Maybe we can use PacedSender::kMaxQueueLengthMs, which is the maximum time
> > > packets will spend in the pacer queue. This queue length is limiting input
> > rate
> > > to the encoder. Maybe even with some error margin added.
> > 
> > But I don't think the pacer queue length is related to the encoder time out.
> > This is here to disable padding if the encoder is no longer producing
output.
> > Apparently added to make sure that if a camera is unplugged, we stop sending
> any
> > video data at all after a while.
> 
> It is at least a lower bound. What I'm worried is that a large frame with low
> bandwidth sits around in the pacer for two seconds, is borderline long enough
to
> disable the encoder. Likely, if the encoder overshot the bitrate target by a
> lot, the next few frames will be below target to compensate, needing padding
to
> keep bwe up... But maybe if padding kicks in again as soon as any frame is
> produce, this is a non issue.
> wdyt?

I see. The timeout happens to be the same as PacedSender::kMaxQueueLength at the
moment but if a new frame is produced, the send stream report that the encoder
is active again and padding resumes so I think this is fine as you said. If
possible I would like to avoid a direct dependency on the paced sender.

[sprang_webrtc, 2016-07-08 07:18:21]

lgtm

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/500001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:275: static const int kEncoderTimeOutMs =
2000;
On 2016/07/08 06:36:28, perkj_webrtc wrote:
> On 2016/07/07 09:25:10, språng wrote:
> > On 2016/07/07 08:37:12, perkj_webrtc wrote:
> > > On 2016/07/06 15:02:51, språng wrote:
> > > > Maybe we can use PacedSender::kMaxQueueLengthMs, which is the maximum
time
> > > > packets will spend in the pacer queue. This queue length is limiting
input
> > > rate
> > > > to the encoder. Maybe even with some error margin added.
> > > 
> > > But I don't think the pacer queue length is related to the encoder time
out.
> > > This is here to disable padding if the encoder is no longer producing
> output.
> > > Apparently added to make sure that if a camera is unplugged, we stop
sending
> > any
> > > video data at all after a while.
> > 
> > It is at least a lower bound. What I'm worried is that a large frame with
low
> > bandwidth sits around in the pacer for two seconds, is borderline long
enough
> to
> > disable the encoder. Likely, if the encoder overshot the bitrate target by a
> > lot, the next few frames will be below target to compensate, needing padding
> to
> > keep bwe up... But maybe if padding kicks in again as soon as any frame is
> > produce, this is a non issue.
> > wdyt?
> 
> I see. The timeout happens to be the same as PacedSender::kMaxQueueLength at
the
> moment but if a new frame is produced, the send stream report that the encoder
> is active again and padding resumes so I think this is fine as you said. If
> possible I would like to avoid a direct dependency on the paced sender. 

Acknowledged.

[tommi, 2016-07-08 11:46:02]

lgtm

[stefan-webrtc, 2016-07-08 15:56:43]

A first pass.

Overall I think it looks like a reasonable to use a task queue in the VSS as we
already are configuring the encoder in a similar fashion. I'm however not super
happy about the introduction of webrtc::internal::VideoSendStreamInternal. It
feels like the VSS/VSSI became more complicated.

I'd also appreciate if it was possible to split this up into smaller CLs. For
instance making the send stream config changes in one CL and moving the frame
timestamping in another. The fewer things we do in the CL that introduces the
task queue the better.

Adding pbos@ too as I will be away on training next week, so review might be
slow. It also makes sense to have another pair of eyes on this since it's a
pretty major change.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:78: VideoEncoderConfig encoder_config) override;
Should we change this for audio above too, and for receive streams?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/config.h#newcode127
webrtc/config.h:127: private:
Put private on the bottom instead

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/test/fake_encoder...
webrtc/test/fake_encoder.cc:37: RTC_DCHECK(max_kbps >= -1);  // max_kbps == -1
disables it.
RTC_DCHECK_GE, etc., here and below.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.cc (left):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:196: RTC_DCHECK(metrics_observer);
Why was this removed? Feels a bit random to change this file in this already
large CL.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:109: const VideoSendStream::Config::Rtp&
rtp_config,
Should this be changed to "const std::vector<uint32_t>& rtx_ssrcs" instead and
the method renamed to AccumulateRtxStats or something?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:190: class VideoSendStream::ConstructionTask :
public rtc::QueuedTask {
How much value does it add to construct/destruct the VSSI on the task queue
(i.e., what are the benefits)? I have a hard time assessing if it's worth doing
given how much code complexity it seems to add.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:217: ~ConstructionTask() override {
done_event_->Set(); }
Empty line above

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:242: class
VideoSendStream::DestructAndGetRTPStateTask : public rtc::QueuedTask {
Rtp

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:251: ~DestructAndGetRTPStateTask() override {
Empty line above

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:369: // can be registered.
I think a better comment would be:
"Wait for ConstructionTask so that the  send_stream_ has been constructed before
we try to use it."

Maybe we also should add a comment somewhere in this file describing the intent.
Are all methods of VSSI meant to be called via the worker queue, or are some
allowed to be called on other threads? Why?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:395: // frame is forwarded to the encoder, the
frame will be dropped.
Does this mean the tests should be changed, or is this something we actually
need to have deterministic behavior? In that case I wouldn't refer to the test
as it can make someone think there are things to fix here.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:423: config_.rtp.max_packet_size - 20);  // -
20 for RTP header size.
20 should be a named constant.

Also, I'm not sure if 20 is large enough if we, for instance, are retransmitting
a red packet and thereby have both red and rtx headers and at least one header
extension. At least we should describe how 20 was derived, or say that it's
chosen to be large enough to be on the safe side.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:571: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_) {
No {}

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:578: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_) {
Same here.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:614: // Add our self as bitrate observer.
I think you can remove this comment

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:653: // if a camera stop producing frames,
temporary or permanently during a call.
this should be "if a camera stops producing frames, temporarily or permanently
during a call."

And you can probably even remove "temporarily or permanently".

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:50: // VideoSendStreamInternal is created and
deleted on |worker_queue|.
Hm, so now we have webrtc::VideoSendStream, webrtc::internal::VideoSendStream
and webrtc::internal::VideoSendStreamInternal... Doesn't feel great. :/

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:95: class VideoSendStreamInternal : public
webrtc::BitrateAllocatorObserver,
Move this to the .cc file

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:32: VideoCodecType PayloadNameToCodecType(const
std::string& payload_name) {
I think a lot of the changes in this file should be moved to a separate CL to
reduce the complexity of this CL. It will be way easier to review in pieces...
Would be good if as much as possible of the stuff that doesn't directly involved
the task queue was done in isolated CLs. A huge CL might become difficult to
revert later if we find out it's causing some problem.

I'm mostly thinking of these methods and the moving of timestamping, which all
seem pretty isolated.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:298: RTC_DCHECK_RUN_ON(&encoder_queue_);
Is there a point in DCHECKing this? Seems like it's basically verifying that the
encoder queue works at all, which I guess is already verified by its unittests?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:320: void ViEEncoder::ConfigureEncoderInternal(const
VideoCodec& video_codec,
Most methods in this class don't have a corresponding "Internal" version. Maybe
you should merge this one with ConfigureEncoder above too?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:365: // timestamps in
https://bugs.chromium.org/p/webrtc/issues/detail?id=5740.
It's not clear what the TODO is. How do we determine that this is TODO is done
and can be removed?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
File webrtc/video/vie_encoder_unittest.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:136: return 1;
Why are you returning 1 here? 0 is what I'd expect.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:238: TEST_F(ViEEncoderTest,
DropsPendingFramesOnslowEncode) {
OnSlowEncode

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (left):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video_send_stream...
webrtc/video_send_stream.h:152: rtc::VideoSinkInterface<VideoFrame>*
local_renderer = nullptr;
Wouldn't it make sense to do this separately?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video_send_stream...
webrtc/video_send_stream.h:74: Config(const Config&) = default;
Perhaps put this last?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video_send_stream...
webrtc/video_send_stream.h:94: int payload_type = 0;
Why was this changed?

[stefan-webrtc, 2016-07-08 15:57:19]

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

[stefan-webrtc, 2016-07-08 15:57:20]

Now actually adding pbos... :)

[perkj_webrtc, 2016-07-11 11:41:09]

Lets talk a bit offline.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:78: VideoEncoderConfig encoder_config) override;
On 2016/07/08 15:56:41, stefan-webrtc (holmer) wrote:
> Should we change this for audio above too, and for receive streams?

This match CreateVideoReceiveStream now. I guess audio should change if there is
a gain to do it. 
https://cs.corp.google.com/piper///depot/google3/third_party/chromium_headles...

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/config.h
File webrtc/config.h (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/config.h#newcode127
webrtc/config.h:127: private:
On 2016/07/08 15:56:41, stefan-webrtc (holmer) wrote:
> Put private on the bottom instead

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/test/fake_encoder.cc
File webrtc/test/fake_encoder.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/test/fake_encoder...
webrtc/test/fake_encoder.cc:37: RTC_DCHECK(max_kbps >= -1);  // max_kbps == -1
disables it.
On 2016/07/08 15:56:41, stefan-webrtc (holmer) wrote:
> RTC_DCHECK_GE, etc., here and below.

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/overuse_fra...
File webrtc/video/overuse_frame_detector.cc (left):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/overuse_fra...
webrtc/video/overuse_frame_detector.cc:196: RTC_DCHECK(metrics_observer);
On 2016/07/08 15:56:41, stefan-webrtc (holmer) wrote:
> Why was this removed? Feels a bit random to change this file in this already
> large CL.


To not not have to instantiate a SendStatisticsProxy in a unittest
(VieKeyRequestTest) that mocks ViEEncoder.... There are still dependencies that
are not straight forward. I think this make more sense than to have to create a
SendStatisticsProxy for an unrelated test.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/send_statis...
File webrtc/video/send_statistics_proxy.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/send_statis...
webrtc/video/send_statistics_proxy.cc:109: const VideoSendStream::Config::Rtp&
rtp_config,
On 2016/07/08 15:56:41, stefan-webrtc (holmer) wrote:
> Should this be changed to "const std::vector<uint32_t>& rtx_ssrcs" instead and
> the method renamed to AccumulateRtxStats or something?

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:190: class VideoSendStream::ConstructionTask :
public rtc::QueuedTask {
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> How much value does it add to construct/destruct the VSSI on the task queue
> (i.e., what are the benefits)? I have a hard time assessing if it's worth
doing
> given how much code complexity it seems to add.

The ctor still calls a few constructors and other methods. 
So if we aim for makeing those objects also created and made single 
thread/sequenced on the worker queue it makes sense. 

But due to then need for VideSendStream::StopPermanentlyAndGetRtpStates to be
synchronous there is maybe not that much value in this cl.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:217: ~ConstructionTask() override {
done_event_->Set(); }
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> Empty line above

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:242: class
VideoSendStream::DestructAndGetRTPStateTask : public rtc::QueuedTask {
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> Rtp

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:251: ~DestructAndGetRTPStateTask() override {
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> Empty line above

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:369: // can be registered.
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> I think a better comment would be:
> "Wait for ConstructionTask so that the  send_stream_ has been constructed
before
> we try to use it."
> 
> Maybe we also should add a comment somewhere in this file describing the
intent.
> Are all methods of VSSI meant to be called via the worker queue, or are some
> allowed to be called on other threads? Why?

Added description to VideoSendStreamImpl.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:395: // frame is forwarded to the encoder, the
frame will be dropped.
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> Does this mean the tests should be changed, or is this something we actually
> need to have deterministic behavior? In that case I wouldn't refer to the test
> as it can make someone think there are things to fix here.

I guess we should decide. But it make sense that after start, a frame will not
be dropped and for testing this is the easiest. But in practice in production,
frame forwarding and   calling Start happen on separate threads and have nothing
to do  with each other.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:423: config_.rtp.max_packet_size - 20);  // -
20 for RTP header size.
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> 20 should be a named constant.
> 
> Also, I'm not sure if 20 is large enough if we, for instance, are
retransmitting
> a red packet and thereby have both red and rtx headers and at least one header
> extension. At least we should describe how 20 was derived, or say that it's
> chosen to be large enough to be on the safe side.

use static const size_t kRtpPacketSizeOverhead = 28; ? Should this also be used
for the protection calculator below?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:571: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_) {
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> No {}

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:578: for (RtpRtcp* rtp_rtcp :
rtp_rtcp_modules_) {
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> Same here.

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:614: // Add our self as bitrate observer.
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> I think you can remove this comment

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:653: // if a camera stop producing frames,
temporary or permanently during a call.
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> this should be "if a camera stops producing frames, temporarily or permanently
> during a call."
> 
> And you can probably even remove "temporarily or permanently".

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:50: // VideoSendStreamInternal is created and
deleted on |worker_queue|.
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> Hm, so now we have webrtc::VideoSendStream, webrtc::internal::VideoSendStream
> and webrtc::internal::VideoSendStreamInternal... Doesn't feel great. :/

VideoSendStreamImpl ?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:95: class VideoSendStreamInternal : public
webrtc::BitrateAllocatorObserver,
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> Move this to the .cc file

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:32: VideoCodecType PayloadNameToCodecType(const
std::string& payload_name) {
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> I think a lot of the changes in this file should be moved to a separate CL to
> reduce the complexity of this CL. It will be way easier to review in pieces...
> Would be good if as much as possible of the stuff that doesn't directly
involved
> the task queue was done in isolated CLs. A huge CL might become difficult to
> revert later if we find out it's causing some problem.
> 
> I'm mostly thinking of these methods and the moving of timestamping, which all
> seem pretty isolated.

I have spend a couple of weeks to try to land smaller changes but always found
one dependency that made a test break. In the end I gave up and did this huge cl
at once. If you feel strong about it I can try again with this part as you
suggest. I am afraid it will require a bit of changes in VideoSendStream that
would be discarded in the next cl. Before this cl, the encoder thread lived in
VideoSendStream.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:298: RTC_DCHECK_RUN_ON(&encoder_queue_);
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> Is there a point in DCHECKing this? Seems like it's basically verifying that
the
> encoder queue works at all, which I guess is already verified by its
unittests?

Needed to compile. Otherwise AccessOn thread annotation gets angry.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:320: void ViEEncoder::ConfigureEncoderInternal(const
VideoCodec& video_codec,
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> Most methods in this class don't have a corresponding "Internal" version.
Maybe
> you should merge this one with ConfigureEncoder above too?

I can't due to  VideoCodec video_codec = VideoEncoderConfigToVideoCodec(...

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:365: // timestamps in
https://bugs.chromium.org/p/webrtc/issues/detail?id=5740.
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> It's not clear what the TODO is. How do we determine that this is TODO is done
> and can be removed?
I removed the whole todo in that case. It was added here as a notice that this
code need some love. We have 3 timestamps in the video frame and the below logic
is not clear to me why it is done the way it is but I definetly don't want to
change it now.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
File webrtc/video/vie_encoder_unittest.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:136: return 1;
On 2016/07/08 15:56:43, stefan-webrtc (holmer) wrote:
> Why are you returning 1 here? 0 is what I'd expect.

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder_unittest.cc:238: TEST_F(ViEEncoderTest,
DropsPendingFramesOnslowEncode) {
On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> OnSlowEncode

Done.

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (left):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video_send_stream...
webrtc/video_send_stream.h:152: rtc::VideoSinkInterface<VideoFrame>*
local_renderer = nullptr;
On 2016/07/08 15:56:43, stefan-webrtc (holmer) wrote:
> Wouldn't it make sense to do this separately?

This is part of moving the input directly to ViEEncoder. How about I try to make
a separate cl of that?

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video_send_stream.h
File webrtc/video_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video_send_stream...
webrtc/video_send_stream.h:74: Config(const Config&) = default;
On 2016/07/08 15:56:43, stefan-webrtc (holmer) wrote:
> Perhaps put this last?

This currently match with what has been done for VideoReceiveStream. I guess the
idea is to point it out but it is against the style guide so I moved it.
https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video_send_stream...
webrtc/video_send_stream.h:94: int payload_type = 0;
On 2016/07/08 15:56:43, stefan-webrtc (holmer) wrote:
> Why was this changed?

good question... 
VieKeyRequestTest.CreateAndTriggerRequest fails due to that
_codecDataBase.DeregisterExternalEncoder(payloadType, &wasSendCodec) dchecks... 

Ok, probably should fix that test to use a valid config instead.

[perkj_webrtc, 2016-07-11 12:10:53]

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/580001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:423: config_.rtp.max_packet_size - 20);  // -
20 for RTP header size.
On 2016/07/11 11:41:08, perkj_webrtc wrote:
> On 2016/07/08 15:56:42, stefan-webrtc (holmer) wrote:
> > 20 should be a named constant.
> > 
> > Also, I'm not sure if 20 is large enough if we, for instance, are
> retransmitting
> > a red packet and thereby have both red and rtx headers and at least one
header
> > extension. At least we should describe how 20 was derived, or say that it's
> > chosen to be large enough to be on the safe side.
> 
> use static const size_t kRtpPacketSizeOverhead = 28; ? Should this also be
used
> for the protection calculator below?

Actually- I removed -20 all togheter. It turns out that the set mtu size is set
to 1200 in WebRtcVideoSendStream. Also- we add + 28 to this value below when we
create the rtp streams.

[pbos-webrtc, 2016-07-13 12:35:39]

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.h (left):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.h:165: ObserverConfigs bitrate_observer_configs_
GUARDED_BY(crit_sect_);
Can you have these GUARDED_BY the sequenced_checker_ instead?

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:244: worker_queue_("worker_queue") {
call_worker_queue or something tying it to call?

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/encoder_sta...
File webrtc/video/encoder_state_feedback_unittest.cc (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/encoder_sta...
webrtc/video/encoder_state_feedback_unittest.cc:41: : encoder_(),
pref removing from init-list

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:193: // An encoder may deliver frames through
the EncodedImageCallback| on an
zero or two |s

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:214: // |module_process_thread| where created
(libjingles worker thread).
s/where/were, and 's

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:235: int ProtectionRequest(const
FecProtectionParams* delta_params,
Why protected? Either public or private is fine right?

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:332: std::unique_ptr<VideoSendStreamImpl>*
send_stream_;
* const

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:333: rtc::Event* done_event_;
Event* const

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:356: send_stream_.reset();
Remove this from destructor, should be done as part of Run() ? Otherwise
motivate with a big comment, but I think we should always wait for Run to finish
before destroying the task/queue?

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:418: volatile int activity_;
This is always called on this encoder_task_checker_ right? No need for atomics
then even, switch to bool encoder_active_since_last_run_

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:753: LOG_F(LS_INFO) << "Encoder timed out.";
s/LOG_F/LOG

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:760: LOG_F(LS_INFO) << "Encoder is active.";
s/LOG_F/LOG

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:766: void
VideoSendStreamImpl::ReconfigureVideoEncoder(
This name is confusing because it doesn't actually reconfigure any VideoEncoder,
can you update the naming here?

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:46: class VideoSendStreamImpl;
Should this be named something related to RTP or otherwise less than a "full
VideoSendStream" ?

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:49: // Internally, it delegates all public
methods to VideoSendStreamImpl.
or ViEEncoder?

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:2015: << "Timed out while waiting for
encoder rate to be set.";
This doesn't check kIncreasedStartBitrateKbps anymore.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:489:
RTC_DCHECK(encoded_sequenced_checker_.CalledSequentially());
This can be called in parallel for simulcast encoders based on hardware
encoders.

I believe this DCHECK is invalid, though technically you could have one race
checker per simulcast layer (though not a sequential task checker).

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:42: //  Instantiate
.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:61: // |module_process_thread| where created
(libjingles worker thread).
were, libjingle's

[perkj_webrtc, 2016-07-14 10:11:02]

Patchset #39 (id:760001) has been deleted

[perkj_webrtc, 2016-07-14 10:11:28]

ptal

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/call/bitrate_allo...
File webrtc/call/bitrate_allocator.h (left):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/call/bitrate_allo...
webrtc/call/bitrate_allocator.h:165: ObserverConfigs bitrate_observer_configs_
GUARDED_BY(crit_sect_);
On 2016/07/13 12:35:38, pbos-webrtc wrote:
> Can you have these GUARDED_BY the sequenced_checker_ instead?

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/call/call.cc
File webrtc/call/call.cc (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/call/call.cc#newc...
webrtc/call/call.cc:244: worker_queue_("worker_queue") {
On 2016/07/13 12:35:38, pbos-webrtc wrote:
> call_worker_queue or something tying it to call?

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/encoder_sta...
File webrtc/video/encoder_state_feedback_unittest.cc (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/encoder_sta...
webrtc/video/encoder_state_feedback_unittest.cc:41: : encoder_(),
On 2016/07/13 12:35:38, pbos-webrtc wrote:
> pref removing from init-list

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.cc (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:193: // An encoder may deliver frames through
the EncodedImageCallback| on an
On 2016/07/13 12:35:38, pbos-webrtc wrote:
> zero or two |s

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:214: // |module_process_thread| where created
(libjingles worker thread).
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> s/where/were, and 's

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:235: int ProtectionRequest(const
FecProtectionParams* delta_params,
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> Why protected? Either public or private is fine right?

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:332: std::unique_ptr<VideoSendStreamImpl>*
send_stream_;
On 2016/07/13 12:35:38, pbos-webrtc wrote:
> * const

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:333: rtc::Event* done_event_;
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> Event* const

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:356: send_stream_.reset();
On 2016/07/13 12:35:38, pbos-webrtc wrote:
> Remove this from destructor, should be done as part of Run() ? Otherwise
> motivate with a big comment, but I think we should always wait for Run to
finish
> before destroying the task/queue?

Done.
fyi taskqueues does not guarantee that a task runs if the queue it self is
deleted. It do however guarantee that all tasks are cleaned up. 
Here- this is called from call that also waits from the task to complete and
thus it is guaranteed that is runs.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:418: volatile int activity_;
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> This is always called on this encoder_task_checker_ right? No need for atomics
> then even, switch to bool encoder_active_since_last_run_

See the updated comment.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:753: LOG_F(LS_INFO) << "Encoder timed out.";
On 2016/07/13 12:35:38, pbos-webrtc wrote:
> s/LOG_F/LOG

ok, agree, but you and stefan disagrees.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:760: LOG_F(LS_INFO) << "Encoder is active.";
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> s/LOG_F/LOG

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.cc:766: void
VideoSendStreamImpl::ReconfigureVideoEncoder(
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> This name is confusing because it doesn't actually reconfigure any
VideoEncoder,
> can you update the naming here?

SignalEncoderConfigurationChanged

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
File webrtc/video/video_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:46: class VideoSendStreamImpl;
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> Should this be named something related to RTP or otherwise less than a "full
> VideoSendStream" ?

I would like to wait until dust has settled. It is not clear where we do the
split yet.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream.h:49: // Internally, it delegates all public
methods to VideoSendStreamImpl.
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> or ViEEncoder?

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
File webrtc/video/video_send_stream_tests.cc (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/video_send_...
webrtc/video/video_send_stream_tests.cc:2015: << "Timed out while waiting for
encoder rate to be set.";
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> This doesn't check kIncreasedStartBitrateKbps anymore.

fixed

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.cc:489:
RTC_DCHECK(encoded_sequenced_checker_.CalledSequentially());
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> This can be called in parallel for simulcast encoders based on hardware
> encoders.
> 
> I believe this DCHECK is invalid, though technically you could have one race
> checker per simulcast layer (though not a sequential task checker).

ok, thanks. updated the comment.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/vie_encoder.h
File webrtc/video/vie_encoder.h (right):

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:42: //  Instantiate
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> .

Done.

https://codereview.webrtc.org/2060403002/diff/720001/webrtc/video/vie_encoder...
webrtc/video/vie_encoder.h:61: // |module_process_thread| where created
(libjingles worker thread).
On 2016/07/13 12:35:39, pbos-webrtc wrote:
> were, libjingle's

Done.

[perkj_webrtc, 2016-07-15 08:01:04]

pretty please!

[stefan-webrtc, 2016-07-15 14:50:26]

Recap of offline discussion:
- We will hold off with this CL until Per is back to avoid potential problems
with bug fixes which we'll have to merge upstream while he's away.
- Peter will land his CL which will make it possible to split this CL in at
least two parts.
- This CL will be thoroughly reviewed when Per is back, and hopefully it will be
a smooth enough process to get it in at that point.

[perkj_webrtc, 2016-08-15 09:09:50]

Hi
I am back, have updated the cl and would like to land this as is. The cl Stefan
refer to has not been landed by pbos yet and is not actually needed for this cl
to land. If it had been landed I could have tried to make this cl slighly
smaller by having the configuration changes moved to a separate cl.

mflodman, would you like to take a look?

[mflodman, 2016-08-16 07:16:43]

LGTM after talking to Per about this.

[perkj_webrtc, 2016-08-16 07:20:02]

The CQ bit was checked by perkj@webrtc.org

[perkj_webrtc, 2016-08-16 07:20:03]

The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org,
tommi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2060403002/#ps840001 (title:
"Fix audio thread check when adding audio to bitrateallocator.")

[commit-bot: I haz the power, 2016-08-16 07:20:06]

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

[stefan-webrtc, 2016-08-16 07:32:07]

lgtm

https://codereview.webrtc.org/2060403002/diff/840001/webrtc/audio/audio_send_...
File webrtc/audio/audio_send_stream.h (right):

https://codereview.webrtc.org/2060403002/diff/840001/webrtc/audio/audio_send_...
webrtc/audio/audio_send_stream.h:63: rtc::TaskQueue* worker_queue_;
rtc::TaskQueue* const

[commit-bot: I haz the power, 2016-08-16 07:38:49]

Description was changed from

==========
- Add task queue to Call with the intent of replacing the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingle's worker thread but always used
on the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantics.

- The encoder thread is moved from VideoSendStream to ViEEncoder. Frames are
forwarded directly to ViEEncoder which is responsible for timestamping ? and
encoding the frames.

BUG=webrtc:5687
==========

to

==========
- Add task queue to Call with the intent of replacing the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingle's worker thread but always used
on the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantics.

- The encoder thread is moved from VideoSendStream to ViEEncoder. Frames are
forwarded directly to ViEEncoder which is responsible for timestamping ? and
encoding the frames.

BUG=webrtc:5687
==========

[commit-bot: I haz the power, 2016-08-16 07:38:50]

Committed patchset #42 (id:840001)

[commit-bot: I haz the power, 2016-08-16 07:38:56]

Description was changed from

==========
- Add task queue to Call with the intent of replacing the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingle's worker thread but always used
on the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantics.

- The encoder thread is moved from VideoSendStream to ViEEncoder. Frames are
forwarded directly to ViEEncoder which is responsible for timestamping ? and
encoding the frames.

BUG=webrtc:5687
==========

to

==========
- Add task queue to Call with the intent of replacing the use of one of the
process threads.

- Split VideoSendStream in two. VideoSendStreamInternal is created and used on
the new task queue.

- BitrateAllocator is now created on libjingle's worker thread but always used
on the new task queue instead of both encoder threads and the process thread.

- VideoEncoderConfig and VideoSendStream::Config support move semantics.

- The encoder thread is moved from VideoSendStream to ViEEncoder. Frames are
forwarded directly to ViEEncoder which is responsible for timestamping ? and
encoding the frames.

BUG=webrtc:5687

Committed: https://crrev.com/cc168360f41322332860cb075edeb1cde21aa473
Cr-Commit-Position: refs/heads/master@{#13767}
==========

[commit-bot: I haz the power, 2016-08-16 07:38:57]

Patchset 42 (id:??) landed as
https://crrev.com/cc168360f41322332860cb075edeb1cde21aa473
Cr-Commit-Position: refs/heads/master@{#13767}

[perkj_webrtc, 2016-08-16 09:40:39]

A revert of this CL (patchset #42 id:840001) has been created in
https://codereview.webrtc.org/2248713003/ by perkj@webrtc.org.

The reason for reverting is: Failed on Win 10 Chrome FYI.


https://build.chromium.org/p/chromium.webrtc.fyi/builders/Win10%20Tester/buil...


#
# Fatal error in
e:\b\c\b\win_builder\src\third_party\webrtc\base\task_queue_win.cc, line 138
# last system error: 87
# Check failed: ((DWORD)0xFFFFFFFF) != result (4294967295 vs. 4294967295)
# 

WebRtcBrowserTest



#
.