Don't reinit encoder when rotation changes.

Don't reinit encoder when rotation changes.

TESTED=By rotating phone in AppRTCMobile.
BUG=webrtc:7535
Review-Url: https://codereview.webrtc.org/2853463004
Cr-Commit-Position: refs/heads/master@{#17985}
Committed: https://chromium.googlesource.com/external/webrtc/+/70e39e159eb350cb1acbf81dbe0d84d51c77b664

[brandtr, 2017-04-28 10:46:38]

Description was changed from

==========
Don't reinit encoder when rotation changes.

BUG=webrtc:7535
==========

to

==========
Don't reinit encoder when rotation changes.

TESTED=By rotating phone in AppRTCMobile.
BUG=webrtc:7535
==========

[brandtr, 2017-04-28 10:55:33]

brandtr@webrtc.org changed reviewers:
+ asapersson@webrtc.org, magjed@webrtc.org

[brandtr, 2017-04-28 10:55:34]

magjed, asapersson: Please take a look. As far as I can tell, there are no
hidden assumptions in our wrappers that the encoder should be reinited before a
frame with a different rotation is sent for encode. Do you know of otherwise?

[brandtr, 2017-04-28 11:19:45]

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

[brandtr, 2017-04-28 11:19:46]

+perkj, who added this code in https://codereview.webrtc.org/2386573002

[perkj_webrtc, 2017-04-28 11:27:10]

There used to be. 
Please test again in apprtcdemo again chrome or something and disable the CVO
header extension. With this extension, the frames are always sent with the
camera mount rotation and the rotation info is sent separately. 
Without the extension, we rotated the image to have "heads up" before encoding.

[brandtr, 2017-04-28 14:51:28]

On 2017/04/28 11:27:10, perkj_webrtc wrote:
> There used to be. 
> Please test again in apprtcdemo again chrome or something and disable the CVO
> header extension. With this extension, the frames are always sent with the
> camera mount rotation and the rotation info is sent separately. 
> Without the extension, we rotated the image to have "heads up" before
encoding.

Yep, I'll test again with and without CVO.

When CVO is not negotiated, I guess the pre-rotation that we do should lead to
changes in width/height, meaning that the encoder will still be reinited even
after this CL. I will verify this.

[perkj_webrtc, 2017-04-28 14:58:08]

On 2017/04/28 14:51:28, brandtr wrote:
> On 2017/04/28 11:27:10, perkj_webrtc wrote:
> > There used to be. 
> > Please test again in apprtcdemo again chrome or something and disable the
CVO
> > header extension. With this extension, the frames are always sent with the
> > camera mount rotation and the rotation info is sent separately. 
> > Without the extension, we rotated the image to have "heads up" before
> encoding.
> 
> Yep, I'll test again with and without CVO.
> 
> When CVO is not negotiated, I guess the pre-rotation that we do should lead to
> changes in width/height, meaning that the encoder will still be reinited even
> after this CL. I will verify this.

Right, we probably moved where we do the rotation to before ViEEncoder and thus
this is no longer needed.... Sorry for my bad memory.

[brandtr_google, 2017-04-28 15:04:49]

On 2017/04/28 14:58:08, perkj_webrtc wrote:
> On 2017/04/28 14:51:28, brandtr wrote:
> > On 2017/04/28 11:27:10, perkj_webrtc wrote:
> > > There used to be. 
> > > Please test again in apprtcdemo again chrome or something and disable the
> CVO
> > > header extension. With this extension, the frames are always sent with the
> > > camera mount rotation and the rotation info is sent separately. 
> > > Without the extension, we rotated the image to have "heads up" before
> > encoding.
> > 
> > Yep, I'll test again with and without CVO.
> > 
> > When CVO is not negotiated, I guess the pre-rotation that we do should lead
to
> > changes in width/height, meaning that the encoder will still be reinited
even
> > after this CL. I will verify this.
> 
> Right, we probably moved where we do the rotation to before ViEEncoder and
thus
> this is no longer needed.... Sorry for my bad memory.

No problem, thanks for the input! I don't want to introduce any bugs here... I
will update here when I have tested with/without CVO.

When testing this CL in an app, there is a small, but noticeable, gain in the
smoothness when rotating the phone.

[magjed_webrtc, 2017-04-30 14:57:14]

lgtm

[åsapersson, 2017-05-02 08:30:24]

lgtm

[brandtr, 2017-05-02 09:30:56]

Rebase.

[brandtr, 2017-05-02 11:01:04]

On 2017/04/28 14:58:08, perkj_webrtc wrote:
> On 2017/04/28 14:51:28, brandtr wrote:
> > On 2017/04/28 11:27:10, perkj_webrtc wrote:
> > > There used to be. 
> > > Please test again in apprtcdemo again chrome or something and disable the
> CVO
> > > header extension. With this extension, the frames are always sent with the
> > > camera mount rotation and the rotation info is sent separately. 
> > > Without the extension, we rotated the image to have "heads up" before
> > encoding.
> > 
> > Yep, I'll test again with and without CVO.
> > 
> > When CVO is not negotiated, I guess the pre-rotation that we do should lead
to
> > changes in width/height, meaning that the encoder will still be reinited
even
> > after this CL. I will verify this.
> 
> Right, we probably moved where we do the rotation to before ViEEncoder and
thus
> this is no longer needed.... Sorry for my bad memory.

I've tested again with AppRTC. I tried different combinations of codecs,
mobile/desktop, and CVO negotiated/not negotiated. No problems seen.

As expected, with CVO negotiated, this CL reduces the number of encoder reinits.
Without CVO negotiated, this CL has no effect.

[brandtr, 2017-05-03 07:49:56]

Added some simple unit tests, just to show what we expect from the encoder.

sprang@: Now I need you :)

As an aside, if/when we remove VCMGenericDecoder, we should probably just pass
the rotation information through the decoder, as is currently done in the
encoder. This means updating all our decoder wrappers :(

[brandtr, 2017-05-03 07:50:17]

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

[brandtr, 2017-05-03 07:50:17]

Actually add sprang@, this time...

[sprang_webrtc, 2017-05-03 10:00:33]

lgtm

[brandtr, 2017-05-03 10:01:10]

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

[commit-bot: I haz the power, 2017-05-03 10:01:21]

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-05-03 10:18:06]

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

[commit-bot: I haz the power, 2017-05-03 10:18:08]

Dry run: This issue passed the CQ dry run.

[brandtr, 2017-05-03 10:38:42]

The CQ bit was checked by brandtr@webrtc.org

[brandtr, 2017-05-03 10:38:42]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
asapersson@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2853463004/#ps40001 (title:
"Add trivial unit tests.")

[commit-bot: I haz the power, 2017-05-03 10:38:47]

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-05-03 10:40:59]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493807922173290,
"parent_rev": "080830c513023df6dbf11b42e92c9df59fc8c049", "commit_rev":
"70e39e159eb350cb1acbf81dbe0d84d51c77b664"}

[commit-bot: I haz the power, 2017-05-03 10:41:02]

Description was changed from

==========
Don't reinit encoder when rotation changes.

TESTED=By rotating phone in AppRTCMobile.
BUG=webrtc:7535
==========

to

==========
Don't reinit encoder when rotation changes.

TESTED=By rotating phone in AppRTCMobile.
BUG=webrtc:7535

Review-Url: https://codereview.webrtc.org/2853463004
Cr-Commit-Position: refs/heads/master@{#17985}
Committed:
https://chromium.googlesource.com/external/webrtc/+/70e39e159eb350cb1acbf81db...
==========

[commit-bot: I haz the power, 2017-05-03 10:41:03]

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

[brandtr, 2017-05-03 10:53:03]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.webrtc.org/2861633003/ by brandtr@webrtc.org.

The reason for reverting is: Breaks compile on buildbots..

[kjellander_webrtc, 2017-05-03 11:03:43]

On 2017/05/03 10:53:03, brandtr wrote:
> A revert of this CL (patchset #3 id:40001) has been created in
> https://codereview.webrtc.org/2861633003/ by mailto:brandtr@webrtc.org.
> 
> The reason for reverting is: Breaks compile on buildbots..

Why did it break? Since all trybots are green. Is it because
https://chromium.googlesource.com/external/webrtc/+/080830c513023df6dbf11b42e...
was landed in between the trybot runs and the landing?

[kjellander_webrtc, 2017-05-03 11:04:03]

Description was changed from

==========
Don't reinit encoder when rotation changes.

TESTED=By rotating phone in AppRTCMobile.
BUG=webrtc:7535

Review-Url: https://codereview.webrtc.org/2853463004
Cr-Commit-Position: refs/heads/master@{#17985}
Committed:
https://chromium.googlesource.com/external/webrtc/+/70e39e159eb350cb1acbf81db...
==========

to

==========
Don't reinit encoder when rotation changes.

TESTED=By rotating phone in AppRTCMobile.
BUG=webrtc:7535

Review-Url: https://codereview.webrtc.org/2853463004
Cr-Commit-Position: refs/heads/master@{#17985}
Committed:
https://chromium.googlesource.com/external/webrtc/+/70e39e159eb350cb1acbf81db...
==========

[kjellander_webrtc, 2017-05-03 11:04:04]

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

[brandtr, 2017-05-03 11:06:16]

On 2017/05/03 11:03:43, kjellander_webrtc wrote:
> On 2017/05/03 10:53:03, brandtr wrote:
> > A revert of this CL (patchset #3 id:40001) has been created in
> > https://codereview.webrtc.org/2861633003/ by mailto:brandtr@webrtc.org.
> > 
> > The reason for reverting is: Breaks compile on buildbots..
> 
> Why did it break? Since all trybots are green. Is it because
>
https://chromium.googlesource.com/external/webrtc/+/080830c513023df6dbf11b42e...
> was landed in between the trybot runs and the landing?

Yep, that's the reason.

I ran the trybots in parallel, and then landed in parallel.