Fix for Issue 331895: Make gesturenav screenshot greyscale

Using grayscale screenshots for GestureNav so that the user can clearly tell when screenshot is shown vs. the live content.

BUG=331895
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249431

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=249782

[mfomitchev, 2014-01-15 16:03:02]

Hey guys,
This is not a full fix, but I would appreciate some quick feedback on the
general approach here. See the review desc for details.

Thanks,
Mikhail

[tdanderson, 2014-01-15 21:39:33]

+danakj for her thoughts since this really isn't my area of expertise.

> I've created a bunch of new Convert* methods in png_code.cc, but the only one
I really need is |ConvertSkiatoGray|. Is this a good approach? ...

I believe it is preferable to keep dead code out of the codebase, so
implementing all of the unused functions would not be the best thing to do. 

A possible exception would be if these new functions are deemed useful, either
because they can/will be used in the near future, or if they can be useful in
refactoring existing code. I have no idea if that is the case here.

[danakj, 2014-01-15 21:40:39]

+pkasting

[sadrul, 2014-01-15 21:43:06]

I am not familiar with the actual encoding code, so looked for nits/readability
only.

https://codereview.chromium.org/136453009/diff/1/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/136453009/diff/1/ui/gfx/codec/png_codec.cc#ne...
ui/gfx/codec/png_codec.cc:24: static const uint32_t RED_TO_GRAY_COEF = 6969;    
  // .212671 * 32768
two spaces before EOL // comments

https://codereview.chromium.org/136453009/diff/1/ui/gfx/codec/png_codec.cc#ne...
ui/gfx/codec/png_codec.cc:835: }
I wonder if this could somehow be simplified, e.g.

  int input_color_component_map[] = {
    [FORMAT_RGB] = 3,
    [FORMAT_RGBA] = 4,
    ...
  };

  int output_color_component_map[] = {
    [PNG_COLOR_TYPE_RGB] = 3,
    [PNG_COLOR_TYPE_RGB_ALPHA] = 4,
    ...
  };

  FormatConverter converters[][FORMAT_SkBitmap + 1] = {
    [PNG_COLOR_TYPE_RGB] = { [FORMAT_RGBA] = ConvertRGBAtoRGB },
    [PNG_COLOR_TYPE_RGB_ALPHA] = { [FORMAT_BGRA] = ConvertBetweenBGRAandRGBA },
    ...
  };

then, the switch-case is used to figure out png_output_color_type, and the maps
above are used to get |input_color_components|, |output_color_components|, and
|converter|. Perhaps that makes the code even less readable.

https://codereview.chromium.org/136453009/diff/1/ui/gfx/codec/png_codec.h
File ui/gfx/codec/png_codec.h (right):

https://codereview.chromium.org/136453009/diff/1/ui/gfx/codec/png_codec.h#new...
ui/gfx/codec/png_codec.h:80: bool discard_color = false);
default parameter values are not allowed in chromium style guide

[Peter Kasting, 2014-01-16 05:51:35]

On 2014/01/15 21:40:39, danakj wrote:
> +pkasting

You should probably say what you want my input on :)

The CL description makes it sound as if this adds a bunch of new methods that
aren't used, just for symmetry.  If that's true, don't do that; only add what's
actually being used.

My instinct is that having a method that converts Skia images to greyscale
separately from the encode process -- and basically leaving png_codec.cc
untouched -- is a better way of doing this.  We already have color_utils.h that
could perhaps be a home for something like this, or maybe there's a good spot in
the Skia files.  Maybe ask a Skia owner.

[mfomitchev, 2014-01-16 19:49:56]

> My instinct is that having a method that converts Skia images to greyscale
separately from the encode process -- and basically leaving png_codec.cc
untouched -- is a better way of doing this.

Then we'd still be spending 3 bytes per pixel instead of 1 byte per pixel to
store the screenshot data. Probably best avoid that? 

Terry suggested having EncodeWithCompressionLevel fall through to "default:" in
every "new" case except the one we need (PNGCodec::FORMAT_SkBitmap,
discard_color == true, discard_transparency == true). Then we could get rid of
all the unused Convert* methods and the unsupported encoding combinations would
just fail. What do you think about this approach?

[Peter Kasting, 2014-01-16 20:53:55]

On 2014/01/16 19:49:56, mfomitchev wrote:
> > My instinct is that having a method that converts Skia images to greyscale
> separately from the encode process -- and basically leaving png_codec.cc
> untouched -- is a better way of doing this.
> 
> Then we'd still be spending 3 bytes per pixel instead of 1 byte per pixel to
> store the screenshot data. Probably best avoid that?

Sending to whom?  I'm sorry, I have absolutely zero background on this issue.

Why can't EncodeOnWorker() convert the bitmap to greyscale and then encode it?

[mfomitchev, 2014-01-16 21:07:09]

On 2014/01/16 20:53:55, Peter Kasting wrote:
> On 2014/01/16 19:49:56, mfomitchev wrote:
> > > My instinct is that having a method that converts Skia images to greyscale
> > separately from the encode process -- and basically leaving png_codec.cc
> > untouched -- is a better way of doing this.
> > 
> > Then we'd still be spending 3 bytes per pixel instead of 1 byte per pixel to
> > store the screenshot data. Probably best avoid that?
> 
> Sending to whom?  I'm sorry, I have absolutely zero background on this issue.
> 
> Why can't EncodeOnWorker() convert the bitmap to greyscale and then encode it?

s_p_ending, not sending :) Unless I am missing something, if we convert to
grayscale and then encode as RGB, each pixel will be represented by 3 bytes, so
we will be wasting memory. We'd also be doing two paths on pixel data - one to
convert to grayscale, and another one to encode.

[Peter Kasting, 2014-01-16 21:18:56]

On 2014/01/16 21:07:09, mfomitchev wrote:
> s_p_ending, not sending :)

Oops!  Sorry, I should learn to read.

> Unless I am missing something, if we convert to
> grayscale and then encode as RGB, each pixel will be represented by 3 bytes,
so
> we will be wasting memory.

I guess the first question would be "does it matter"?  We're spending 3Bpp
before this patch anyway, so nothing is getting worse, right?  I don't know how
many of these images we have or how big they are, though.

Next question would be, can the PNG encoder be made to auto-detect that the
bitmap is greyscale and Just Do The Right Thing?  Maybe Skia images have (or can
add) a bit notifying consumers that they're all greyscale?

> We'd also be doing two paths on pixel data - one to
> convert to grayscale, and another one to encode.

Yes, and again, the first question I have is whether that matters.  If this
isn't in an inner loop, perhaps we don't care.

[mfomitchev, 2014-01-16 21:54:58]

There is one of these screenshots (weakly referenced?) for every navigation
entry (every page reachable by pressing back or forward repeatedly). I don't
know how much these things matter. I can investigate if needed. That said, can
you please comment on the approach which Terry suggested (I described it in
comment #6)? It doesn't leave any uncertainty re memory/runtime speed, so it
would be less work...

[sadrul, 2014-01-16 21:58:51]

> > Unless I am missing something, if we convert to
> > grayscale and then encode as RGB, each pixel will be represented by 3 bytes,
> so
> > we will be wasting memory.
> 
> I guess the first question would be "does it matter"?  We're spending 3Bpp
> before this patch anyway, so nothing is getting worse, right?  I don't know
how
> many of these images we have or how big they are, though.

We store approximately 10 screenshots for each tab. One of the reasons we want
to try grayscaling the screenshots is to save some memory so that they are
purged from memory less often.

[Peter Kasting, 2014-01-16 22:01:10]

On 2014/01/16 21:54:58, mfomitchev wrote:
> can
> you please comment on the approach which Terry suggested (I described it in
> comment #6)? It doesn't leave any uncertainty re memory/runtime speed, so it
> would be less work...

It certainly seems better than the more generalized version implemented in this
patch.

My primary concerns are twofold:

(1) By adding more params to the encoder API, we impose costs on everyone
calling the API.  If we converted to greyscale first and then the encoder
autodetermined that the source was greyscale, we'd still potentially pay more
cycles than if we did everything in one pass, but we would still achieve the
desired memory savings, and the API would be simpler and thus easier to use.

(2) If we only implement conversion to greyscale within this file, what about
other future code that wants to convert to greyscale for some other reason?  We
may end up over time with a binary that has multiple redundant
convert-to-greyscale bits in different object files.  That seems suboptimal.

[mfomitchev, 2014-01-17 17:22:30]

I've tried using png_set_rgb_to_gray_fixed() to do the RGB -> gray
transformation, it is excluded by the configuration we have for libpng. Looks
like this is done by |#define PNG_NO_READ_RGB_TO_GRAY| in
third_party/libpng/pngusr.h. Would it be appropriate to remove that?

[mfomitchev, 2014-01-17 21:50:37]

Ok, so I have spend most of Friday trying to figure out a way to have RGB
transformed to grayscale when we encode PNG data, and I couldn't find anything I
could use directly in pnglib. PNG_RGB_TO_GRAY transform bits (which I mentioned
above and which are respected when we read the PNG data) seem to be ignored when
we write the PNG data. Looking at png_do_write_transformations() in pngwtran.c,
I don't see anything I could use directly to transform RGB data to grayscale.

Peter, do you have any advice? Thanks!

[Peter Kasting, 2014-01-17 22:08:09]

On 2014/01/17 21:50:37, mfomitchev wrote:
> Ok, so I have spend most of Friday trying to figure out a way to have RGB
> transformed to grayscale when we encode PNG data, and I couldn't find anything
I
> could use directly in pnglib. PNG_RGB_TO_GRAY transform bits (which I
mentioned
> above and which are respected when we read the PNG data) seem to be ignored
when
> we write the PNG data. Looking at png_do_write_transformations() in
pngwtran.c,
> I don't see anything I could use directly to transform RGB data to grayscale.
> 
> Peter, do you have any advice? Thanks!

My thought had been to do something more like what your patch does, but instead
of triggering it via an added param, auto-detect that it should be triggered,
e.g. by scanning the input bytes or reading some signal value from somewhere.

It was a vague thought.  If it's not possible, it's not possible.

[mfomitchev, 2014-01-24 17:27:05]

I've re-implemented  the feature minimizing the impact on PNGCodec and moving
the grayscale conversion to NavigationEntryScreenshotManager. Peter, I hope this
addresses your concerns.

I've tested the performance running the chrome binary on my Ubuntu machine
comparing the original approach with the new approach.
ScreenshotData::EncodeOnWorker takes ~55ms with the old approach and ~65ms with
the new approach, so it doesn't seem too bad.

If this looks okay, it would be nice to create a test for
PNGCodec::EncodeA8SkBitmap. Where would be a good place to put it?

[Peter Kasting, 2014-01-25 03:29:57]

Awesome.  LGTM.  Don't know where the test should go, perhaps a ui/gfx/ OWNER
can speak to that.

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:683: // supported formats
Nit: Trailing period

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:692: //converter = NULL;
Nit: Remove this line.  If you need to explain that |converter| is explicitly
NULL, or say what that means, say that, otherwise just nuke it.

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:743: DCHECK_EQ(input.bytesPerPixel(), bpp);
If these are always equal, why pass |bpp| at all?  We should just read it from
|input|.  This would also eliminate the need for the next DCHECK, since Skia
will guarantee that.

If you really want, you can DCHECK the BPP value on the caller side.

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:794: std::vector<unsigned char>* output) {
Nit: Fix indenting

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec.h
File ui/gfx/codec/png_codec.h (right):

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.h:103: std::vector<unsigned char>* output);
Nit: Fix indenting

[mfomitchev, 2014-01-27 17:15:28]

+Nasko - Nasko, can you please review the changes to
navigation_entry_screenshot_manager.cc?

Peter, just one minor question below.

Thanks!

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:683: // supported formats
On 2014/01/25 03:29:58, Peter Kasting wrote:
> Nit: Trailing period

Done.

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:692: //converter = NULL;
On 2014/01/25 03:29:58, Peter Kasting wrote:
> Nit: Remove this line.  If you need to explain that |converter| is explicitly
> NULL, or say what that means, say that, otherwise just nuke it.

Done.

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:743: DCHECK_EQ(input.bytesPerPixel(), bpp);
Now EncodeA8SkBitmap() and EncodeBGRASkBitmap() are identical. Should  we just
replace them with one method EncodeSkBitmap() and clarify that only two formats
are supported in the comments? We'd can also rename FastEncodeBGRASkBitmap() to
FastEncodeSkBitmap().

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:743: DCHECK_EQ(input.bytesPerPixel(), bpp);
On 2014/01/25 03:29:58, Peter Kasting wrote:
> If these are always equal, why pass |bpp| at all?  We should just 
> read it from |input|.  This would also eliminate the need for the > next
DCHECK, since Skia will guarantee that.
> 
> If you really want, you can DCHECK the BPP value on the caller 
> side.

Now EncodeA8SkBitmap() and EncodeBGRASkBitmap() are identical. Should  we just
replace them with one method EncodeSkBitmap() and clarify that only two formats
are supported in the comments? We'd can also rename FastEncodeBGRASkBitmap() to
FastEncodeSkBitmap().

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:794: std::vector<unsigned char>* output) {
On 2014/01/25 03:29:58, Peter Kasting wrote:
> Nit: Fix indenting

Done.

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec.h
File ui/gfx/codec/png_codec.h (right):

https://codereview.chromium.org/136453009/diff/520001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.h:103: std::vector<unsigned char>* output);
On 2014/01/25 03:29:58, Peter Kasting wrote:
> Nit: Fix indenting

Done.

[nasko, 2014-01-27 17:26:14]

Rubberstamp LGTM

[danakj, 2014-01-27 18:40:58]

On 2014/01/25 03:29:57, Peter Kasting wrote:
> Awesome.  LGTM.  Don't know where the test should go, perhaps a ui/gfx/ OWNER
> can speak to that.

Offhand, ui/gfx/codec/png_codec_unittest.cc sounds like the place.

[mfomitchev, 2014-01-27 21:13:25]

Added the unit test

[commit-bot: I haz the power, 2014-01-30 18:25:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/650001

[commit-bot: I haz the power, 2014-01-30 19:02:14]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-01-30 19:02:23]

CQ bit was unchecked on CL. Ignoring.

[mfomitchev, 2014-02-03 19:44:40]

Hi Elliot,

Can you please review changes to PNGCodec? Thanks!

[Peter Kasting, 2014-02-03 19:53:03]

LGTM

https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec.cc
File ui/gfx/codec/png_codec.cc (right):

https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:743: DCHECK(bpp == 1 || bpp == 4);  // We support
kA8_Config and kARGB_8888_Config
Nit: Trailing period

https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:746: if (bpp == 1) {
Nit: You can simplify this:

  uint8_t* addr = (bpp == 1) ? input.getAddr8(0, 0) : input.getAddr32(0, 0);
  return EncodeWithCompressionLevel(reinterpret_cast<unsigned char*>(addr),
                                    PNGCodec::FORMAT_SkBitmap,
                                    Size(input.width(), input.height()),
                                    static_cast<int>(input.rowBytes()),
                                    discard_transparency,
                                    std::vector<PNGCodec::Comment>(),
                                    compression_level,
                                    output);

https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec....
ui/gfx/codec/png_codec.cc:801: bool PNGCodec::EncodeA8SkBitmap(const SkBitmap&
input,
Nit: As you noted, this function could now be combined with
EncodeBGRASkBitmap(), which I think would be an improvement.

You could even nuke all three wrappers around InternalEncodeSkBitmap(), rename
that to EncodeSkBitmap(), and have callers call that directly.  I don't know
that there is a ton of gain from having a separate FastEncodeSkBitmap() function
versus passing an int or bool to specify the compression level.

[Elliot Glaysher, 2014-02-03 23:22:02]

lgtm

[Peter Kasting, 2014-02-04 01:37:14]

Just looked at the unittest.

https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec_...
File ui/gfx/codec/png_codec_unittest.cc (right):

https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec_...
ui/gfx/codec/png_codec_unittest.cc:252: // Returns true if the BGRA 4-bit
SkColor is "close" to the Gray 8-bit SkColor.
Nit: Don't you mean 32-bit (or 4-byte)?

https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec_...
ui/gfx/codec/png_codec_unittest.cc:265: for (int i = 0; i < w * h; i++) {
Nit: {} unnecessary

https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec_...
ui/gfx/codec/png_codec_unittest.cc:275: for (int i = 0; i < w * h; i++) {
Nit: {} unnecessary

https://codereview.chromium.org/136453009/diff/650001/ui/gfx/codec/png_codec_...
ui/gfx/codec/png_codec_unittest.cc:1080:
EXPECT_TRUE(BGRAGrayColorsClose(decoded_pixel, original_pixel));
How come we can't just check for direct equality?  Shouldn't a lossless encode
of a greyscale image be perfectly representable in full color?

[mfomitchev, 2014-02-04 20:08:11]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-02-04 20:09:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/560002

[commit-bot: I haz the power, 2014-02-04 22:50:57]

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

[commit-bot: I haz the power, 2014-02-04 22:50:58]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, sync_integration_tests,
unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[mfomitchev, 2014-02-04 22:53:21]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-02-04 23:18:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/560002

[commit-bot: I haz the power, 2014-02-05 01:30:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/560002

[mfomitchev, 2014-02-05 16:41:06]

The CQ bit was unchecked by mfomitchev@chromium.org

[mfomitchev, 2014-02-05 16:41:07]

The CQ bit was checked by mfomitchev@chromium.org

[Paweł Hajdan Jr., 2014-02-05 19:48:33]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-05 19:52:47]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-06 09:24:54]

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

[commit-bot: I haz the power, 2014-02-06 09:24:55]

List of reviewers changed. phajdan.jr@chromium.org did a drive-by without
LGTM'ing!

[mfomitchev, 2014-02-06 15:17:03]

The CQ bit was checked by mfomitchev@chromium.org

[commit-bot: I haz the power, 2014-02-06 15:17:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/560002

[commit-bot: I haz the power, 2014-02-06 17:46:24]

Change committed as 249431

[bsalomon, 2014-02-06 20:09:13]

https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:64:
paint.setColorFilter(SkLumaColorFilter::Create());
The color filter is leaking here. One fix is:

paint.setColorFilter(SkLumaColorFilter::Create())->unref();

[tdanderson, 2014-02-07 19:02:55]

The CQ bit was checked by tdanderson@chromium.org

[commit-bot: I haz the power, 2014-02-07 19:04:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mfomitchev@chromium.org/136453009/120002

[mfomitchev, 2014-02-07 19:05:43]

https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_screenshot_manager.cc (right):

https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_h...
content/browser/frame_host/navigation_entry_screenshot_manager.cc:64:
paint.setColorFilter(SkLumaColorFilter::Create());
On 2014/02/06 20:09:14, bsalomon wrote:
> The color filter is leaking here. One fix is:
> 
> paint.setColorFilter(SkLumaColorFilter::Create())->unref();

Think we have to unref the filter, not the paint? I've submitted the patch.

[bsalomon, 2014-02-07 20:50:01]

On 2014/02/07 19:05:43, mfomitchev wrote:
>
https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_h...
> File content/browser/frame_host/navigation_entry_screenshot_manager.cc
(right):
> 
>
https://codereview.chromium.org/136453009/diff/560002/content/browser/frame_h...
> content/browser/frame_host/navigation_entry_screenshot_manager.cc:64:
> paint.setColorFilter(SkLumaColorFilter::Create());
> On 2014/02/06 20:09:14, bsalomon wrote:
> > The color filter is leaking here. One fix is:
> > 
> > paint.setColorFilter(SkLumaColorFilter::Create())->unref();
> 
> Think we have to unref the filter, not the paint? I've submitted the patch.

Right, but SkPaint::setColorFilter() returns a pointer to the filter not to the
paint. 

https://code.google.com/p/skia/source/browse/trunk/include/core/SkPaint.h#525

Anyway, what you wrote works, too.

[commit-bot: I haz the power, 2014-02-07 21:23:23]

Change committed as 249782