Display JPGs in Viewer with Split Screen and ColorSpaceXform

Display JPGs in Viewer with Split Screen and ColorSpaceXform

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2058753002

Committed: https://skia.googlesource.com/skia/+/6f163d29438cefaaa10c5cd68e3b77c3cb9a6b79

[liyuqian, 2016-06-10 13:44:34]

Description was changed from

==========
Display JPGs in Viewer App

BUG=skia:
==========

to

==========
Display JPGs in Viewer App

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2058753002
==========

[liyuqian, 2016-06-13 12:58:59]

Description was changed from

==========
Display JPGs in Viewer App

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2058753002
==========

to

==========
Display JPGs in Viewer App with Split Screen and ColorSpaceXfor

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2058753002
==========

[liyuqian, 2016-06-13 14:24:07]

Description was changed from

==========
Display JPGs in Viewer App with Split Screen and ColorSpaceXfor

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2058753002
==========

to

==========
Display JPGs in Viewer with Split Screen and ColorSpaceXform

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2058753002
==========

[liyuqian, 2016-06-13 14:24:07]

liyuqian@google.com changed reviewers:
+ djsollen@google.com, msarett@google.com

[liyuqian, 2016-06-13 14:26:52]

Let's review and land this CL first before I filing some bugs and adding more
features.

[msarett, 2016-06-13 14:56:26]

Mostly nits.  Seems like a cool feature!

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
File tools/viewer/ImageSlide.cpp (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
tools/viewer/ImageSlide.cpp:31: // canvas->drawImage(fImage.get(), 0, 0); //
drawImage is currently having bugs
I would say go ahead and file a bug.  That way the comment can say something
like:

// skbug.com/XXXX
// drawImage() and drawBitmap() behave differently in sRGB mode.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
tools/viewer/ImageSlide.cpp:37: sk_sp<SkData>
encoded(SkData::NewFromFileName(fPath.c_str()));
nit:

sk_sp<SkData> encoded = SkData::NewFromFileName(fPath.c_str());

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
tools/viewer/ImageSlide.cpp:45: fImage->asLegacyBitmap(&fXformedBitmap,
SkImage::kRW_LegacyBitmapMode);
nit: I believe that MakeFromEncoded() is lazy, meaning that it won't decode the
image (it will just hold the encoded data).  asLegacyBitmap() will need to
decode the image (since bitmaps always have pixels).  By calling
asLegacyBitmap() twice, we will actually decode the image twice, which is a bit
inefficient.

Could we instead create fXformedBitmap as a (deep) copy of fOriginalBitmap
before modifying the pixels?

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide.h
File tools/viewer/ImageSlide.h (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
tools/viewer/ImageSlide.h:30: SkBitmap               fOriginalBitmap,
fXformedBitmap;
nit: Skia style puts these fields on two separate lines.
nit: kLegacyBitmap (or something else) might be a better name than
kOriginalBitmap?

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:19: #include "SkMetaData.h"
nit: alphabetical order

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:239: SkString skpName;
nit: More appropriate variable name?

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:349: contentRect.fLeft += inSplitScreen ?
(contentRect.fRight - contentRect.fLeft) * 0.5 : 0;
What's going on here?  Are we upscaling?

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:366: drawSlide(canvas, true);
Is it ok/intentional to call this twice?

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.h
File tools/viewer/Viewer.h (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.h#n...
tools/viewer/Viewer.h:38: void drawSlide(SkCanvas* canvs, bool inSplitScreen =
false);
nit: canvs -> canvas

nit: I think Skia style avoids default parameters when it's reasonable/possible

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.h#n...
tools/viewer/Viewer.h:57: bool                   fSplitScreen = false;
nit: I think Skia style would initialize this in the constructor(s).

[liyuqian, 2016-06-13 16:10:41]

Thank you Matt! I've revised the code according to your helpful comments.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
File tools/viewer/ImageSlide.cpp (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
tools/viewer/ImageSlide.cpp:31: // canvas->drawImage(fImage.get(), 0, 0); //
drawImage is currently having bugs
On 2016/06/13 14:56:26, msarett wrote:
> I would say go ahead and file a bug.  That way the comment can say something
> like:
> 
> // skbug.com/XXXX
> // drawImage() and drawBitmap() behave differently in sRGB mode.

Done.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
tools/viewer/ImageSlide.cpp:37: sk_sp<SkData>
encoded(SkData::NewFromFileName(fPath.c_str()));
On 2016/06/13 14:56:26, msarett wrote:
> nit:
> 
> sk_sp<SkData> encoded = SkData::NewFromFileName(fPath.c_str());

no viable conversion from 'SkData *' to 'sk_sp<SkData>'

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
tools/viewer/ImageSlide.cpp:45: fImage->asLegacyBitmap(&fXformedBitmap,
SkImage::kRW_LegacyBitmapMode);
On 2016/06/13 14:56:26, msarett wrote:
> nit: I believe that MakeFromEncoded() is lazy, meaning that it won't decode
the
> image (it will just hold the encoded data).  asLegacyBitmap() will need to
> decode the image (since bitmaps always have pixels).  By calling
> asLegacyBitmap() twice, we will actually decode the image twice, which is a
bit
> inefficient.
> 
> Could we instead create fXformedBitmap as a (deep) copy of fOriginalBitmap
> before modifying the pixels?

Done.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide.h
File tools/viewer/ImageSlide.h (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
tools/viewer/ImageSlide.h:30: SkBitmap               fOriginalBitmap,
fXformedBitmap;
On 2016/06/13 14:56:26, msarett wrote:
> nit: Skia style puts these fields on two separate lines.
> nit: kLegacyBitmap (or something else) might be a better name than
> kOriginalBitmap?

Done.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:19: #include "SkMetaData.h"
On 2016/06/13 14:56:26, msarett wrote:
> nit: alphabetical order

Done.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:239: SkString skpName;
On 2016/06/13 14:56:26, msarett wrote:
> nit: More appropriate variable name?

Done.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:349: contentRect.fLeft += inSplitScreen ?
(contentRect.fRight - contentRect.fLeft) * 0.5 : 0;
On 2016/06/13 14:56:26, msarett wrote:
> What's going on here?  Are we upscaling?

We are translating the image half screen to the right. Thus we have two copies
of the image on each half of the screen.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:366: drawSlide(canvas, true);
On 2016/06/13 14:56:26, msarett wrote:
> Is it ok/intentional to call this twice?

Yes, this is used to draw the same slide twice. The second draw will draw the
slide on the right half screen so it looks like a split screen.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.h
File tools/viewer/Viewer.h (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.h#n...
tools/viewer/Viewer.h:38: void drawSlide(SkCanvas* canvs, bool inSplitScreen =
false);
On 2016/06/13 14:56:26, msarett wrote:
> nit: canvs -> canvas
> 
> nit: I think Skia style avoids default parameters when it's
reasonable/possible

Done.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.h#n...
tools/viewer/Viewer.h:57: bool                   fSplitScreen = false;
On 2016/06/13 14:56:26, msarett wrote:
> nit: I think Skia style would initialize this in the constructor(s).

Done.

[msarett, 2016-06-13 16:57:35]

lgtm + nits

I'd prefer we land the following first and rebase:
https://codereview.chromium.org/2061783002/

That might bring some clarity to some of the bugginess we are seeing.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
File tools/viewer/ImageSlide.cpp (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
tools/viewer/ImageSlide.cpp:37: sk_sp<SkData>
encoded(SkData::NewFromFileName(fPath.c_str()));
On 2016/06/13 16:10:40, liyuqian wrote:
> On 2016/06/13 14:56:26, msarett wrote:
> > nit:
> > 
> > sk_sp<SkData> encoded = SkData::NewFromFileName(fPath.c_str());
> 
> no viable conversion from 'SkData *' to 'sk_sp<SkData>'

Ahh sorry!  You should use SkData::MakeFromFileName().

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:349: contentRect.fLeft += inSplitScreen ?
(contentRect.fRight - contentRect.fLeft) * 0.5 : 0;
On 2016/06/13 16:10:40, liyuqian wrote:
> On 2016/06/13 14:56:26, msarett wrote:
> > What's going on here?  Are we upscaling?
> 
> We are translating the image half screen to the right. Thus we have two copies
> of the image on each half of the screen.

Can you add a comment here?

[liyuqian, 2016-06-13 19:02:30]

Thanks.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
File tools/viewer/ImageSlide.cpp (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/ImageSlide...
tools/viewer/ImageSlide.cpp:37: sk_sp<SkData>
encoded(SkData::NewFromFileName(fPath.c_str()));
On 2016/06/13 16:57:35, msarett wrote:
> On 2016/06/13 16:10:40, liyuqian wrote:
> > On 2016/06/13 14:56:26, msarett wrote:
> > > nit:
> > > 
> > > sk_sp<SkData> encoded = SkData::NewFromFileName(fPath.c_str());
> > 
> > no viable conversion from 'SkData *' to 'sk_sp<SkData>'
> 
> Ahh sorry!  You should use SkData::MakeFromFileName().

Done.

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp
File tools/viewer/Viewer.cpp (right):

https://codereview.chromium.org/2058753002/diff/40001/tools/viewer/Viewer.cpp...
tools/viewer/Viewer.cpp:349: contentRect.fLeft += inSplitScreen ?
(contentRect.fRight - contentRect.fLeft) * 0.5 : 0;
On 2016/06/13 16:57:35, msarett wrote:
> On 2016/06/13 16:10:40, liyuqian wrote:
> > On 2016/06/13 14:56:26, msarett wrote:
> > > What's going on here?  Are we upscaling?
> > 
> > We are translating the image half screen to the right. Thus we have two
copies
> > of the image on each half of the screen.
> 
> Can you add a comment here?

Done.

[liyuqian, 2016-06-13 19:02:36]

The CQ bit was checked by liyuqian@google.com

[liyuqian, 2016-06-13 19:02:36]

The patchset sent to the CQ was uploaded after l-g-t-m from msarett@google.com
Link to the patchset: https://codereview.chromium.org/2058753002/#ps120001
(title: "Unmerge")

[commit-bot: I haz the power, 2016-06-13 19:02:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2058753002/120001

[commit-bot: I haz the power, 2016-06-13 19:26:49]

Description was changed from

==========
Display JPGs in Viewer with Split Screen and ColorSpaceXform

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2058753002
==========

to

==========
Display JPGs in Viewer with Split Screen and ColorSpaceXform

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2058753002

Committed:
https://skia.googlesource.com/skia/+/6f163d29438cefaaa10c5cd68e3b77c3cb9a6b79
==========

[commit-bot: I haz the power, 2016-06-13 19:26:50]

Committed patchset #7 (id:120001) as
https://skia.googlesource.com/skia/+/6f163d29438cefaaa10c5cd68e3b77c3cb9a6b79

[commit-bot: I haz the power, 2016-06-13 19:26:52]

CQ bit was unchecked