Add cropping to VIEEncoder to match simulcast streams resolution

Add cropping to VIEEncoder to match simulcast streams resolution

Detect when simulcaststreamfactory adjust resolution and remeber cropping
parameters in VIEEncoder.
Expose EncoderStreamFactory in webrtcvideoengine2.

BUG=webrtc:7375, webrtc:6958
Review-Url: https://codereview.webrtc.org/2936393002
Cr-Commit-Position: refs/heads/master@{#18632}
Committed: https://chromium.googlesource.com/external/webrtc/+/6b826ef66dbdfd1688d0b64b751c7c1ec8fb85d5

[ilnik, 2017-06-15 12:39:08]

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

[commit-bot: I haz the power, 2017-06-15 12:39:12]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-15 13:06:04]

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

[commit-bot: I haz the power, 2017-06-15 13:06:05]

Dry run: This issue passed the CQ dry run.

[ilnik, 2017-06-15 13:06:08]

Description was changed from

==========
Add cropping to VIEEncoder to match simulcast streams resolution

Detect when simulcaststreamfactory adjust resolution and remeber cropping
parameters in VIEEncoder.
Change *_loopback tools to work with the same factories as videoengine2.
Expose EncoderStreamFactory in webrtcvideoengine2.

BUG=webrtc:7375, webrtc:6958
==========

to

==========
Add cropping to VIEEncoder to match simulcast streams resolution

Detect when simulcaststreamfactory adjust resolution and remeber cropping
parameters in VIEEncoder.
Expose EncoderStreamFactory in webrtcvideoengine2.

BUG=webrtc:7375, webrtc:6958
==========

[ilnik, 2017-06-15 13:06:25]

ilnik@webrtc.org changed reviewers:
+ sprang@webrtc.org

[ilnik, 2017-06-15 13:11:04]

ilnik@webrtc.org changed reviewers:
+ magjed@webrtc.org

[ilnik, 2017-06-15 13:11:05]

+magjed for changes to webrtcvideoengine.

[magjed_webrtc, 2017-06-15 19:32:10]

Has anything changed in webrtcvideoengine, or have you just moved code? In that
case, why?

[ilnik, 2017-06-15 19:59:16]

On 2017/06/15 19:32:10, magjed_webrtc wrote:
> Has anything changed in webrtcvideoengine, or have you just moved code? In
that
> case, why?

I've moved declaration to header to expose this factory to be used outside of
webrtcvideoengine.cc in full stack tests.

[sprang_webrtc, 2017-06-16 08:57:23]

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:222: void SetSource(test::VideoCapturer*
video_capturer, bool respectSinkWants) {
nit: no camelcase

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:928:
Clock::GetRealTimeClock()->CurrentNtpInMilliseconds());
Add a member for the clock instance instead of calling the getter all the time.

also, nit: add brackets

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.h (right):

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.h:54: std::string clip_name;  // "Generator" to
generate frames instead.
Can we expose the generator feature more explicitly?
Make this an optional string or something?

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:587: crop_height_ = last_frame_info_->height -
highest_stream_height;
Hm, I'm not always cropping here is a good idea.. 

Normalizing to the closest resolution divisible by 4 is only one of the changes
the video stream factory can request. It may for instance also choose to create
streams of lower resolution if the number of assigned ssrcs isn't high enough.
Though I guess that case won't really work today either?

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:588: 
The full stack test is great, but would probably be good if you also added a
unit test on vie_encoder level, using a test factory design to trigger this
scenario.

[magjed_webrtc, 2017-06-16 09:36:28]

lgtm, assuming the code in webrtcvideoengine.cc hasn't changed.

[ilnik, 2017-06-16 11:22:49]

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.cc (right):

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:222: void SetSource(test::VideoCapturer*
video_capturer, bool respectSinkWants) {
On 2017/06/16 08:57:23, sprang_webrtc wrote:
> nit: no camelcase

Done.

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.cc:928:
Clock::GetRealTimeClock()->CurrentNtpInMilliseconds());
On 2017/06/16 08:57:23, sprang_webrtc wrote:
> Add a member for the clock instance instead of calling the getter all the
time.
> 
> also, nit: add brackets
> 

Done.

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/video_quality_te...
File webrtc/video/video_quality_test.h (right):

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/video_quality_te...
webrtc/video/video_quality_test.h:54: std::string clip_name;  // "Generator" to
generate frames instead.
On 2017/06/16 08:57:23, sprang_webrtc wrote:
> Can we expose the generator feature more explicitly?
> Make this an optional string or something?

I don't want to over-complicate this already huge config struct. This clip_name
was already implicit, as empty string leads to camera captured feed.

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:587: crop_height_ = last_frame_info_->height -
highest_stream_height;
On 2017/06/16 08:57:23, sprang_webrtc wrote:
> Hm, I'm not always cropping here is a good idea.. 
> 
> Normalizing to the closest resolution divisible by 4 is only one of the
changes
> the video stream factory can request. It may for instance also choose to
create
> streams of lower resolution if the number of assigned ssrcs isn't high enough.
> Though I guess that case won't really work today either?

Before, that case wouldn't work in the same way, dropping all the frames: codec
resolutions are set as a maximum from all the streams in
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
. So, if the upper stream is not matching the input frame resolution, frame will
be dropped.

Now, it will work by cropping the image, instead of scaling. Should I add
additional logic to make it scale if, e.g crop area is more than some percentage
of image area?

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:588: 
On 2017/06/16 08:57:23, sprang_webrtc wrote:
> The full stack test is great, but would probably be good if you also added a
> unit test on vie_encoder level, using a test factory design to trigger this
> scenario.

Done.

[sprang_webrtc, 2017-06-16 11:58:47]

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:587: crop_height_ = last_frame_info_->height -
highest_stream_height;
On 2017/06/16 11:22:49, ilnik wrote:
> On 2017/06/16 08:57:23, sprang_webrtc wrote:
> > Hm, I'm not always cropping here is a good idea.. 
> > 
> > Normalizing to the closest resolution divisible by 4 is only one of the
> changes
> > the video stream factory can request. It may for instance also choose to
> create
> > streams of lower resolution if the number of assigned ssrcs isn't high
enough.
> > Though I guess that case won't really work today either?
> 
> Before, that case wouldn't work in the same way, dropping all the frames:
codec
> resolutions are set as a maximum from all the streams in
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
> . So, if the upper stream is not matching the input frame resolution, frame
will
> be dropped.
> 
> Now, it will work by cropping the image, instead of scaling. Should I add
> additional logic to make it scale if, e.g crop area is more than some
percentage
> of image area?

Yeah, maybe that's a working stopgap solution. Crop if delta < 4 pixels,
otherwise scale?
But add a TODO about using VideoSinkWants signaling instead.

https://codereview.webrtc.org/2936393002/diff/20001/webrtc/video/vie_encoder_...
File webrtc/video/vie_encoder_unittest.cc (right):

https://codereview.webrtc.org/2936393002/diff/20001/webrtc/video/vie_encoder_...
webrtc/video/vie_encoder_unittest.cc:198: 
This class is currently test specific, so put it inside the test using it.

[ilnik, 2017-06-16 12:53:30]

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2936393002/diff/1/webrtc/video/vie_encoder.cc#n...
webrtc/video/vie_encoder.cc:587: crop_height_ = last_frame_info_->height -
highest_stream_height;
On 2017/06/16 11:58:47, sprang_webrtc wrote:
> On 2017/06/16 11:22:49, ilnik wrote:
> > On 2017/06/16 08:57:23, sprang_webrtc wrote:
> > > Hm, I'm not always cropping here is a good idea.. 
> > > 
> > > Normalizing to the closest resolution divisible by 4 is only one of the
> > changes
> > > the video stream factory can request. It may for instance also choose to
> > create
> > > streams of lower resolution if the number of assigned ssrcs isn't high
> enough.
> > > Though I guess that case won't really work today either?
> > 
> > Before, that case wouldn't work in the same way, dropping all the frames:
> codec
> > resolutions are set as a maximum from all the streams in
> >
>
https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/...
> > . So, if the upper stream is not matching the input frame resolution, frame
> will
> > be dropped.
> > 
> > Now, it will work by cropping the image, instead of scaling. Should I add
> > additional logic to make it scale if, e.g crop area is more than some
> percentage
> > of image area?
> 
> Yeah, maybe that's a working stopgap solution. Crop if delta < 4 pixels,
> otherwise scale?
> But add a TODO about using VideoSinkWants signaling instead.

Done.

https://codereview.webrtc.org/2936393002/diff/20001/webrtc/video/vie_encoder_...
File webrtc/video/vie_encoder_unittest.cc (right):

https://codereview.webrtc.org/2936393002/diff/20001/webrtc/video/vie_encoder_...
webrtc/video/vie_encoder_unittest.cc:198: 
On 2017/06/16 11:58:47, sprang_webrtc wrote:
> This class is currently test specific, so put it inside the test using it.

Done.

[sprang_webrtc, 2017-06-16 12:58:25]

lgtm

https://codereview.webrtc.org/2936393002/diff/40001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2936393002/diff/40001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:796: // happen after SinkWants signaled correctly
from ReconfigreEncoder.
nit: Reconfigure

[ilnik, 2017-06-16 13:02:10]

https://codereview.webrtc.org/2936393002/diff/40001/webrtc/video/vie_encoder.cc
File webrtc/video/vie_encoder.cc (right):

https://codereview.webrtc.org/2936393002/diff/40001/webrtc/video/vie_encoder....
webrtc/video/vie_encoder.cc:796: // happen after SinkWants signaled correctly
from ReconfigreEncoder.
On 2017/06/16 12:58:25, sprang_webrtc wrote:
> nit: Reconfigure

Done.

[ilnik, 2017-06-16 13:02:27]

The CQ bit was checked by ilnik@webrtc.org

[ilnik, 2017-06-16 13:02:27]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
sprang@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2936393002/#ps60001 (title:
"Fix typo")

[commit-bot: I haz the power, 2017-06-16 13:02:34]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-16 13:53:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497618147329930,
"parent_rev": "f79dbadc09255c91b9163449dfa8898a8be29744", "commit_rev":
"6b826ef66dbdfd1688d0b64b751c7c1ec8fb85d5"}

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

Description was changed from

==========
Add cropping to VIEEncoder to match simulcast streams resolution

Detect when simulcaststreamfactory adjust resolution and remeber cropping
parameters in VIEEncoder.
Expose EncoderStreamFactory in webrtcvideoengine2.

BUG=webrtc:7375, webrtc:6958
==========

to

==========
Add cropping to VIEEncoder to match simulcast streams resolution

Detect when simulcaststreamfactory adjust resolution and remeber cropping
parameters in VIEEncoder.
Expose EncoderStreamFactory in webrtcvideoengine2.

BUG=webrtc:7375, webrtc:6958

Review-Url: https://codereview.webrtc.org/2936393002
Cr-Commit-Position: refs/heads/master@{#18632}
Committed:
https://chromium.googlesource.com/external/webrtc/+/6b826ef66dbdfd1688d0b64b7...
==========

[commit-bot: I haz the power, 2017-06-16 13:53:55]

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

[kjellander_webrtc, 2017-06-25 19:31:47]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.webrtc.org/2960573002/ by kjellander@webrtc.org.

The reason for reverting is: Makes webrtc_perf_tests fail+timeout on Mac:
https://build.chromium.org/p/client.webrtc.perf/builders/Mac%2010.11/builds/2896

[ RUN      ] FullStackTest.SimulcastFullHdOveruse
- Uh, I'm-I'm not quite dead, sir.
- Uh, I-I think uh, I could pull through, sir.
../../webrtc/video/video_quality_test.cc:419: Failure
Expected: (frames_processed) > (last_frames_processed), actual: 591 vs 591
Analyzer stalled while waiting for test to finish..