Remove --tab-capture-upscale/downscale-quality.

Remove --tab-capture-upscale/downscale-quality.

Historically three scalers were designed for tab-capture upscaling
and downscaling: fast, good, best. Experiments show that "good" or
"best" down-scalers may cause downstream encoders to perform much
worse, as the detail they maintain adds excessive entropy to the
images.

This change removes the command-line switches and always uses
"fast" for video down-scaling and "best" for upscaling.

BUG=350135
Committed: https://crrev.com/9000b88d353b4929a21b4cc5fcb2b87abadd37a1
Cr-Commit-Position: refs/heads/master@{#385626}

[xjz, 2016-02-11 18:49:48]

Patchset #1 (id:1) has been deleted

[xjz, 2016-02-11 22:25:25]

xjz@chromium.org changed reviewers:
+ miu@chromium.org

[xjz, 2016-02-11 22:25:26]

PTAL

[miu, 2016-02-19 01:47:59]

https://codereview.chromium.org/1690013002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc
(right):

https://codereview.chromium.org/1690013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc:214:
#if defined(USE_AURA)
I think you'll want to remove all the code from here to the end of the file too.

But...perhaps you should run these on a few of the lab machines and your desktop
to determine relative performance first?  One thing that's been bothering me is
that we don't have numbers that say, "Good is X% slower than Fast" and "Best is
Y% slower than Fast."  And also, downscaling versus upscaling.

[xjz, 2016-02-19 04:03:41]

On 2016/02/19 01:47:59, miu wrote:
>
https://codereview.chromium.org/1690013002/diff/20001/chrome/browser/extensio...
> File chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc
> (right):
> 
>
https://codereview.chromium.org/1690013002/diff/20001/chrome/browser/extensio...
> chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc:214:
> #if defined(USE_AURA)
> I think you'll want to remove all the code from here to the end of the file
too.
> 
> But...perhaps you should run these on a few of the lab machines and your
desktop
> to determine relative performance first?  One thing that's been bothering me
is
> that we don't have numbers that say, "Good is X% slower than Fast" and "Best
is
> Y% slower than Fast."  And also, downscaling versus upscaling.

I may need to think it more. I just did normal mirroring on a few of the lab
machines 
with different scaler options. If we really want to get the speed difference
quantitively, 
we might need to test specifically on scaling. I will do more tests with some
fixed 
resolution changes as the performance varies with the scaling factor for sure.

[miu, 2016-02-19 04:46:30]

On 2016/02/19 04:03:41, xjz wrote:
> On 2016/02/19 01:47:59, miu wrote:
> >
>
https://codereview.chromium.org/1690013002/diff/20001/chrome/browser/extensio...
> > File
chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc
> > (right):
> > 
> >
>
https://codereview.chromium.org/1690013002/diff/20001/chrome/browser/extensio...
> >
chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc:214:
> > #if defined(USE_AURA)
> > I think you'll want to remove all the code from here to the end of the file
> too.
> > 
> > But...perhaps you should run these on a few of the lab machines and your
> desktop
> > to determine relative performance first?  One thing that's been bothering me
> is
> > that we don't have numbers that say, "Good is X% slower than Fast" and "Best
> is
> > Y% slower than Fast."  And also, downscaling versus upscaling.
> 
> I may need to think it more. I just did normal mirroring on a few of the lab
> machines 
> with different scaler options. If we really want to get the speed difference
> quantitively, 
> we might need to test specifically on scaling. I will do more tests with some
> fixed 
> resolution changes as the performance varies with the scaling factor for sure.

Oh, yes, definitely test the isolated performance.  We don't want to throw this
away just because we haven't figured out how to make it work with ZC.

[xjz, 2016-03-28 19:05:53]

Patchset #2 (id:40001) has been deleted

[xjz, 2016-03-28 19:52:15]

PTAL

https://codereview.chromium.org/1690013002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc
(right):

https://codereview.chromium.org/1690013002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/api/tab_capture/tab_capture_performancetest.cc:214:
#if defined(USE_AURA)
On 2016/02/19 01:47:59, miu wrote:
> I think you'll want to remove all the code from here to the end of the file
too.
> 
> But...perhaps you should run these on a few of the lab machines and your
desktop
> to determine relative performance first?  One thing that's been bothering me
is
> that we don't have numbers that say, "Good is X% slower than Fast" and "Best
is
> Y% slower than Fast."  And also, downscaling versus upscaling.

Done.

Did a bunch of tests on several lab machines and also on my own desktops. 
Results show that the speeds of the three scaling methods are quite close on
high end machines, but not on other machines. The "best" down-scaling could be
up to 96% slower than the "good" one, and the "good" one could be up to 250%
slower than the "fast" one. As we don't have criteria to determine the proper
scaling method in flight, and generally the down-scaling is more often to be
done on low end machines, we choose "fast" down-scaling method by default. The
two "upscaling" methods perform comparable speed on different machines, so we
choose "best" scaling method as the default method for upscaling. Also added the
comment in the code.

[miu, 2016-03-29 19:48:59]

Description was changed from

==========
Remove --tab-capture-upscale/downscale-quality.
Historically three scalers were designed for tap-capture upscaling
and downscaling: fast, good, best. Experiments show that "good" or
"best" down-scalers may cause zero configure dropping resolution as
they consume more resources than "fast" down-scaler. Therefore, we
remove the switches and stick to "fast" down-scaler and "best"
up-scaler.

BUG=350135
==========

to

==========
Remove --tab-capture-upscale/downscale-quality.

Historically three scalers were designed for tab-capture upscaling
and downscaling: fast, good, best. Experiments show that "good" or
"best" down-scalers may cause downstream encoders to perform much
worse, as the detail they maintain adds excessive entropy to the
images.

This change removes the command-line switches and always uses
"fast" for video down-scaling, "best" for single-frame snapshot
down-scaling, and "best" for upscaling in both use cases.

BUG=350135
==========

[miu, 2016-03-29 20:08:52]

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
File content/browser/compositor/gl_helper.cc (right):

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
content/browser/compositor/gl_helper.cc:1202: // Tests show that the two
up-scaling methods perform comparable speeds on the
This comment should provide a little more context about the use case, and the
numbers are probably too much detail.  Suggest you replace this comment with:

"The YUV readback pipeline is used by real-time video capture of
tabs/windows/desktops.  Therefore, the scaler chosen here is based on
performance measurements of full end-to-end systems.  When down-scaling, always
use the "fast" scaler because it performs well on both low- and high- end
machines, provides decent image quality, and doesn't overwhelm downstream video
encoders with too much entropy (which can drastically increase CPU utilization).
 When up-scaling, always use "best" because the quality improvement is huge with
insignificant performance penalty.  Note that this strategy differs from
single-frame snapshot capture (i.e., using the Skia/RGB code path), where "best"
is always preferred."

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
content/browser/compositor/gl_helper.cc:1211: GLHelper::ScalerQuality quality =
((src_size.width() < dst_size.width()) &&
Should this be:

  GLHelper::ScalerQuality quality = ((src_size.GetArea() < dst_size.GetArea())
      ? GLHelper::SCALER_QUALITY_BEST
      : GLHelper::SCALER_QUALITY_FAST;

[miu, 2016-03-29 20:10:57]

cc: +hubbe

hubbe FYI: xjz@ did some performance measurements in the lab, and after
discussing, we finally resolved the issue of when to use each scaler algorithm
in the GLHelper YUV readback path.  Feel free to comment if anything doesn't
look right.

[xjz, 2016-03-29 21:14:50]

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
File content/browser/compositor/gl_helper.cc (right):

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
content/browser/compositor/gl_helper.cc:1202: // Tests show that the two
up-scaling methods perform comparable speeds on the
On 2016/03/29 20:08:52, miu wrote:
> This comment should provide a little more context about the use case, and the
> numbers are probably too much detail.  Suggest you replace this comment with:
> 
> "The YUV readback pipeline is used by real-time video capture of
> tabs/windows/desktops.  Therefore, the scaler chosen here is based on
> performance measurements of full end-to-end systems.  When down-scaling,
always
> use the "fast" scaler because it performs well on both low- and high- end
> machines, provides decent image quality, and doesn't overwhelm downstream
video
> encoders with too much entropy (which can drastically increase CPU
utilization).
>  When up-scaling, always use "best" because the quality improvement is huge
with
> insignificant performance penalty.  Note that this strategy differs from
> single-frame snapshot capture (i.e., using the Skia/RGB code path), where
"best"
> is always preferred."

Yuri: Thanks for the suggestion. But do you want to change the default scaler
for single-frame snapshot capture path from "good" to "best"? It is "good" in
current code:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co....
As in the case of multiple parses down-scaling, "good" is almost as good as
"fast" in quality but is much faster, maybe just keep using "good" as the
default scaler for the single-frame capture path?

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
content/browser/compositor/gl_helper.cc:1211: GLHelper::ScalerQuality quality =
((src_size.width() < dst_size.width()) &&
On 2016/03/29 20:08:52, miu wrote:
> Should this be:
> 
>   GLHelper::ScalerQuality quality = ((src_size.GetArea() < dst_size.GetArea())
>       ? GLHelper::SCALER_QUALITY_BEST
>       : GLHelper::SCALER_QUALITY_FAST;

The scaling is done in two separate dimensions. My intention was to use "best"
only when we need do upscaling in both dimensions. If using this condition check
on area, we may use "best" when one dimension is down-scaling, and results in a
performance penalty.

[miu, 2016-03-31 21:44:06]

lgtm % fixing that one comment in gl_helper.cc.

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
File content/browser/compositor/gl_helper.cc (right):

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
content/browser/compositor/gl_helper.cc:1202: // Tests show that the two
up-scaling methods perform comparable speeds on the
On 2016/03/29 21:14:50, xjz wrote:
> On 2016/03/29 20:08:52, miu wrote:
> > This comment should provide a little more context about the use case, and
the
> > numbers are probably too much detail.  Suggest you replace this comment
with:
> > 
> > "The YUV readback pipeline is used by real-time video capture of
> > tabs/windows/desktops.  Therefore, the scaler chosen here is based on
> > performance measurements of full end-to-end systems.  When down-scaling,
> always
> > use the "fast" scaler because it performs well on both low- and high- end
> > machines, provides decent image quality, and doesn't overwhelm downstream
> video
> > encoders with too much entropy (which can drastically increase CPU
> utilization).
> >  When up-scaling, always use "best" because the quality improvement is huge
> with
> > insignificant performance penalty.  Note that this strategy differs from
> > single-frame snapshot capture (i.e., using the Skia/RGB code path), where
> "best"
> > is always preferred."
> 
> Yuri: Thanks for the suggestion. But do you want to change the default scaler
> for single-frame snapshot capture path from "good" to "best"? It is "good" in
> current code:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co....
> As in the case of multiple parses down-scaling, "good" is almost as good as
> "fast" in quality but is much faster, maybe just keep using "good" as the
> default scaler for the single-frame capture path?

Ah, no.  :)  Actually, we should just leave out that last part of the comment,
as discussed.

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
content/browser/compositor/gl_helper.cc:1211: GLHelper::ScalerQuality quality =
((src_size.width() < dst_size.width()) &&
On 2016/03/29 21:14:50, xjz wrote:
> On 2016/03/29 20:08:52, miu wrote:
> > Should this be:
> > 
> >   GLHelper::ScalerQuality quality = ((src_size.GetArea() <
dst_size.GetArea())
> >       ? GLHelper::SCALER_QUALITY_BEST
> >       : GLHelper::SCALER_QUALITY_FAST;
> 
> The scaling is done in two separate dimensions. My intention was to use "best"
> only when we need do upscaling in both dimensions. If using this condition
check
> on area, we may use "best" when one dimension is down-scaling, and results in
a
> performance penalty. 

Sounds good.

[xjz, 2016-03-31 23:03:39]

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
File content/browser/compositor/gl_helper.cc (right):

https://codereview.chromium.org/1690013002/diff/60001/content/browser/composi...
content/browser/compositor/gl_helper.cc:1202: // Tests show that the two
up-scaling methods perform comparable speeds on the
On 2016/03/31 21:44:05, miu wrote:
> On 2016/03/29 21:14:50, xjz wrote:
> > On 2016/03/29 20:08:52, miu wrote:
> > > This comment should provide a little more context about the use case, and
> the
> > > numbers are probably too much detail.  Suggest you replace this comment
> with:
> > > 
> > > "The YUV readback pipeline is used by real-time video capture of
> > > tabs/windows/desktops.  Therefore, the scaler chosen here is based on
> > > performance measurements of full end-to-end systems.  When down-scaling,
> > always
> > > use the "fast" scaler because it performs well on both low- and high- end
> > > machines, provides decent image quality, and doesn't overwhelm downstream
> > video
> > > encoders with too much entropy (which can drastically increase CPU
> > utilization).
> > >  When up-scaling, always use "best" because the quality improvement is
huge
> > with
> > > insignificant performance penalty.  Note that this strategy differs from
> > > single-frame snapshot capture (i.e., using the Skia/RGB code path), where
> > "best"
> > > is always preferred."
> > 
> > Yuri: Thanks for the suggestion. But do you want to change the default
scaler
> > for single-frame snapshot capture path from "good" to "best"? It is "good"
in
> > current code:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co....
> > As in the case of multiple parses down-scaling, "good" is almost as good as
> > "fast" in quality but is much faster, maybe just keep using "good" as the
> > default scaler for the single-frame capture path?
> 
> Ah, no.  :)  Actually, we should just leave out that last part of the comment,
> as discussed.

Done.

[xjz, 2016-03-31 23:03:51]

The CQ bit was checked by xjz@chromium.org

[xjz, 2016-03-31 23:03:51]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1690013002/#ps80001
(title: "Address Yuri's comments.")

[commit-bot: I haz the power, 2016-03-31 23:04:15]

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

[commit-bot: I haz the power, 2016-03-31 23:07:02]

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

[commit-bot: I haz the power, 2016-03-31 23:07:03]

Try jobs failed on following builders:
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[xjz, 2016-04-01 00:05:54]

Description was changed from

==========
Remove --tab-capture-upscale/downscale-quality.

Historically three scalers were designed for tab-capture upscaling
and downscaling: fast, good, best. Experiments show that "good" or
"best" down-scalers may cause downstream encoders to perform much
worse, as the detail they maintain adds excessive entropy to the
images.

This change removes the command-line switches and always uses
"fast" for video down-scaling, "best" for single-frame snapshot
down-scaling, and "best" for upscaling in both use cases.

BUG=350135
==========

to

==========
Remove --tab-capture-upscale/downscale-quality.

Historically three scalers were designed for tab-capture upscaling
and downscaling: fast, good, best. Experiments show that "good" or
"best" down-scalers may cause downstream encoders to perform much
worse, as the detail they maintain adds excessive entropy to the
images.

This change removes the command-line switches and always uses
"fast" for video down-scaling and "best" for upscaling.

BUG=350135
==========

[xjz, 2016-04-01 00:10:14]

xjz@chromium.org changed reviewers:
+ ccameron@chromium.org

[xjz, 2016-04-01 00:10:14]

ccameron: PTAL. Need owner's approval.

[xjz, 2016-04-06 18:25:41]

xjz@chromium.org changed reviewers:
+ piman@chromium.org

[xjz, 2016-04-06 18:25:42]

PTAL

ccameron: need owner's approval on content/browser/compositor/gl_helper.*
changes.

piman: need owner's approval on changes:
chrome/browser/chromeos/login/chrome_restart_request.cc
content/public/common/content_switches.*

[piman, 2016-04-06 18:32:47]

https://codereview.chromium.org/1690013002/diff/120001/content/browser/compos...
File content/browser/compositor/gl_helper.cc (right):

https://codereview.chromium.org/1690013002/diff/120001/content/browser/compos...
content/browser/compositor/gl_helper.cc:1215: : GLHelper::SCALER_QUALITY_FAST;
If SCALER_QUALITY_GOOD is not used any more, can you remove the enum and the
related code?

[xjz, 2016-04-06 18:44:05]

Thanks piman. Replied your comments:

https://codereview.chromium.org/1690013002/diff/120001/content/browser/compos...
File content/browser/compositor/gl_helper.cc (right):

https://codereview.chromium.org/1690013002/diff/120001/content/browser/compos...
content/browser/compositor/gl_helper.cc:1215: : GLHelper::SCALER_QUALITY_FAST;
On 2016/04/06 18:32:47, piman wrote:
> If SCALER_QUALITY_GOOD is not used any more, can you remove the enum and the
> related code?

SCALER_QUALITY_GOOD is used as the default scaler in single-frame snapshot
capture:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co....
It provides nearly as good quality as "best" down-scaler, but much faster in
general.

[piman, 2016-04-06 19:20:08]

https://codereview.chromium.org/1690013002/diff/120001/content/browser/compos...
File content/browser/compositor/gl_helper.cc (right):

https://codereview.chromium.org/1690013002/diff/120001/content/browser/compos...
content/browser/compositor/gl_helper.cc:1215: : GLHelper::SCALER_QUALITY_FAST;
On 2016/04/06 18:44:05, xjz wrote:
> On 2016/04/06 18:32:47, piman wrote:
> > If SCALER_QUALITY_GOOD is not used any more, can you remove the enum and the
> > related code?
> 
> SCALER_QUALITY_GOOD is used as the default scaler in single-frame snapshot
> capture:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co....
> It provides nearly as good quality as "best" down-scaler, but much faster in
> general.

If that the case, then can you leave the policy decision (this line) out of
GLHelper, and to the calling code instead? The only advantage of baking it in is
if it can remove code. Since it can't, let's leave it outside for consistency
with other readback API?

[xjz, 2016-04-06 23:03:37]

xjz@chromium.org changed reviewers:
- ccameron@chromium.org

[xjz, 2016-04-06 23:03:38]

Addressed piman's comments.

-ccameron@ as there is no change on gl_helper.* now.

https://codereview.chromium.org/1690013002/diff/120001/content/browser/compos...
File content/browser/compositor/gl_helper.cc (right):

https://codereview.chromium.org/1690013002/diff/120001/content/browser/compos...
content/browser/compositor/gl_helper.cc:1215: : GLHelper::SCALER_QUALITY_FAST;
On 2016/04/06 19:20:07, piman wrote:
> On 2016/04/06 18:44:05, xjz wrote:
> > On 2016/04/06 18:32:47, piman wrote:
> > > If SCALER_QUALITY_GOOD is not used any more, can you remove the enum and
the
> > > related code?
> > 
> > SCALER_QUALITY_GOOD is used as the default scaler in single-frame snapshot
> > capture:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/co....
> > It provides nearly as good quality as "best" down-scaler, but much faster in
> > general.
> 
> If that the case, then can you leave the policy decision (this line) out of
> GLHelper, and to the calling code instead? The only advantage of baking it in
is
> if it can remove code. Since it can't, let's leave it outside for consistency
> with other readback API?

Done.

[piman, 2016-04-06 23:10:45]

LGTM, thanks!

[xjz, 2016-04-06 23:11:48]

The CQ bit was checked by xjz@chromium.org

[xjz, 2016-04-06 23:11:48]

The patchset sent to the CQ was uploaded after l-g-t-m from miu@chromium.org
Link to the patchset: https://codereview.chromium.org/1690013002/#ps140001
(title: "Address piman's comments.")

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

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1690013002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1690013002/140001

[commit-bot: I haz the power, 2016-04-07 02:27:44]

Description was changed from

==========
Remove --tab-capture-upscale/downscale-quality.

Historically three scalers were designed for tab-capture upscaling
and downscaling: fast, good, best. Experiments show that "good" or
"best" down-scalers may cause downstream encoders to perform much
worse, as the detail they maintain adds excessive entropy to the
images.

This change removes the command-line switches and always uses
"fast" for video down-scaling and "best" for upscaling.

BUG=350135
==========

to

==========
Remove --tab-capture-upscale/downscale-quality.

Historically three scalers were designed for tab-capture upscaling
and downscaling: fast, good, best. Experiments show that "good" or
"best" down-scalers may cause downstream encoders to perform much
worse, as the detail they maintain adds excessive entropy to the
images.

This change removes the command-line switches and always uses
"fast" for video down-scaling and "best" for upscaling.

BUG=350135
==========

[commit-bot: I haz the power, 2016-04-07 02:27:45]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-04-07 02:28:59]

Description was changed from

==========
Remove --tab-capture-upscale/downscale-quality.

Historically three scalers were designed for tab-capture upscaling
and downscaling: fast, good, best. Experiments show that "good" or
"best" down-scalers may cause downstream encoders to perform much
worse, as the detail they maintain adds excessive entropy to the
images.

This change removes the command-line switches and always uses
"fast" for video down-scaling and "best" for upscaling.

BUG=350135
==========

to

==========
Remove --tab-capture-upscale/downscale-quality.

Historically three scalers were designed for tab-capture upscaling
and downscaling: fast, good, best. Experiments show that "good" or
"best" down-scalers may cause downstream encoders to perform much
worse, as the detail they maintain adds excessive entropy to the
images.

This change removes the command-line switches and always uses
"fast" for video down-scaling and "best" for upscaling.

BUG=350135

Committed: https://crrev.com/9000b88d353b4929a21b4cc5fcb2b87abadd37a1
Cr-Commit-Position: refs/heads/master@{#385626}
==========

[commit-bot: I haz the power, 2016-04-07 02:29:01]

Patchset 6 (id:??) landed as
https://crrev.com/9000b88d353b4929a21b4cc5fcb2b87abadd37a1
Cr-Commit-Position: refs/heads/master@{#385626}