Client-side changes to support requesting lossless encode & color.

Client-side changes to support requesting lossless encode & color.

This adds setLosslessEncode() and setLosslessColor() calls to the
ClientPlugin API, which future updates to the app can use to request
lossless frames if desired.

BUG=260879, 134202
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273995

[Wez, 2014-05-28 21:43:23]

PTAL

This is the client-side part of crrev.com/304653002, which adds lossy/lossless
encode and color space controls to the VideoControl message.

[Jamie, 2014-05-28 22:21:18]

LGTM with nits.

https://codereview.chromium.org/307463005/diff/20001/remoting/client/plugin/c...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/307463005/diff/20001/remoting/client/plugin/c...
remoting/client/plugin/chromoting_instance.cc:891: if
(data.GetBoolean("lossless_encode", &lossless_encode)) {
JS uses camelCase for variables. I'm not sure how consistent we are about doing
the same for the JSON exchanged with the client plugin, but unless this naming
convention is the norm, I'd prefer that we use it here too (it's used for the
method names, for example).

https://codereview.chromium.org/307463005/diff/20001/remoting/webapp/client_p...
File remoting/webapp/client_plugin.js (right):

https://codereview.chromium.org/307463005/diff/20001/remoting/webapp/client_p...
remoting/webapp/client_plugin.js:578: * @param {boolean} want_lossless True to
request lossless encoding.
Nit: camelCase :)

https://codereview.chromium.org/307463005/diff/20001/remoting/webapp/client_p...
remoting/webapp/client_plugin.js:586: { method: 'videoControl', data: {
lossless_encode: want_lossless }}));
Nit: Quotes around dictionary keys, for consistency.

[Wez, 2014-05-28 22:40:50]

The CQ bit was checked by wez@chromium.org

[Wez, 2014-05-28 22:40:58]

The CQ bit was unchecked by wez@chromium.org

[Wez, 2014-05-28 22:41:07]

https://codereview.chromium.org/307463005/diff/20001/remoting/client/plugin/c...
File remoting/client/plugin/chromoting_instance.cc (right):

https://codereview.chromium.org/307463005/diff/20001/remoting/client/plugin/c...
remoting/client/plugin/chromoting_instance.cc:891: if
(data.GetBoolean("lossless_encode", &lossless_encode)) {
On 2014/05/28 22:21:19, Jamie wrote:
> JS uses camelCase for variables. I'm not sure how consistent we are about
doing
> the same for the JSON exchanged with the client plugin, but unless this naming
> convention is the norm, I'd prefer that we use it here too (it's used for the
> method names, for example).

Makes sense. Done!

https://codereview.chromium.org/307463005/diff/20001/remoting/webapp/client_p...
File remoting/webapp/client_plugin.js (right):

https://codereview.chromium.org/307463005/diff/20001/remoting/webapp/client_p...
remoting/webapp/client_plugin.js:578: * @param {boolean} want_lossless True to
request lossless encoding.
On 2014/05/28 22:21:19, Jamie wrote:
> Nit: camelCase :)

Done.

https://codereview.chromium.org/307463005/diff/20001/remoting/webapp/client_p...
remoting/webapp/client_plugin.js:586: { method: 'videoControl', data: {
lossless_encode: want_lossless }}));
On 2014/05/28 22:21:19, Jamie wrote:
> Nit: Quotes around dictionary keys, for consistency.

Not sure what you mean; this file consistently _doesn't_ put quotes around
dictionary keys (see pauseVideo, pauseAudio, etc).

[Jamie, 2014-05-28 22:53:55]

lgtm

https://codereview.chromium.org/307463005/diff/20001/remoting/webapp/client_p...
File remoting/webapp/client_plugin.js (right):

https://codereview.chromium.org/307463005/diff/20001/remoting/webapp/client_p...
remoting/webapp/client_plugin.js:586: { method: 'videoControl', data: {
lossless_encode: want_lossless }}));
On 2014/05/28 22:41:07, Wez wrote:
> On 2014/05/28 22:21:19, Jamie wrote:
> > Nit: Quotes around dictionary keys, for consistency.
> 
> Not sure what you mean; this file consistently _doesn't_ put quotes around
> dictionary keys (see pauseVideo, pauseAudio, etc).

My mistake. I was reading 'videoControl' as a key rather than a value.

[commit-bot: I haz the power, 2014-05-28 22:55:22]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wez@chromium.org/307463005/40001

[Wez, 2014-05-28 23:08:07]

The CQ bit was unchecked by wez@chromium.org

[Wez, 2014-05-30 17:06:29]

The CQ bit was checked by wez@chromium.org

[commit-bot: I haz the power, 2014-05-30 17:09:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wez@chromium.org/307463005/60001

[commit-bot: I haz the power, 2014-05-30 23:08:29]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)

[commit-bot: I haz the power, 2014-05-31 03:44:07]

Change committed as 273995