Use Offscreen Surface for Split Screen

Use Offscreen Surface for Split Screen

A dashed line is also added to split the screen. Currently, drawing
images with sRGB seems to still have bugs.

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

Committed: https://skia.googlesource.com/skia/+/74959a1471cd3b20e2dd79fc0c685e7205a25cd9

[liyuqian, 2016-06-14 16:16:38]

Description was changed from

==========
Use Offscreen Surface for Split Screen

A dashed line is also added to split the screen. Currently, drawing
images with sRGB seems to still have bugs.

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

to

==========
Use Offscreen Surface for Split Screen

A dashed line is also added to split the screen. Currently, drawing
images with sRGB seems to still have bugs.

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

[liyuqian, 2016-06-14 16:18:22]

Description was changed from

==========
Use Offscreen Surface for Split Screen

A dashed line is also added to split the screen. Currently, drawing
images with sRGB seems to still have bugs.

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

to

==========
Use Offscreen Surface for Split Screen

A dashed line is also added to split the screen. Currently, drawing
images with sRGB seems to still have bugs.

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

[liyuqian, 2016-06-14 16:18:23]

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

[liyuqian, 2016-06-14 16:23:47]

This implementation allows sRGB split screen for all slides (GM, SKP, JPG, and
future Sample). However, there seems to be problems rendering sRGB images: sRGB
ones are much brighter than the non-sRGB ones.

[liyuqian, 2016-06-15 13:27:18]

On 2016/06/14 16:23:47, liyuqian wrote:
> This implementation allows sRGB split screen for all slides (GM, SKP, JPG, and
> future Sample). However, there seems to be problems rendering sRGB images:
sRGB
> ones are much brighter than the non-sRGB ones.

Hi everyone, can you please check if this CL correctly implements the general
sRGB split screen function that you want?

Thanks,
Yuqian

[Brian Osman, 2016-06-15 13:41:58]

brianosman@google.com changed reviewers:
+ brianosman@google.com

[Brian Osman, 2016-06-15 13:41:59]

Yup, this looks like the right idea to me. (With one important fix that will
only affect GPU).

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
File tools/viewer/sk_app/WindowContext.cpp (right):

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.cpp:27: (GrPixelConfigIsSRGB(fPixelConfig) ?
SkSurfaceProps::kGammaCorrect_Flag : 0);
Need to also set this flag if sRGB is true:

((GrPixelConfigIsSRGB(fPixelConfig) || sRGB) ?
skSurfaceProps::kGammaCorrect_Flag : 0);

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
File tools/viewer/sk_app/WindowContext.h (right):

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.h:48: sk_sp<GrRenderTarget>, int colorBits,
bool offscreen=false, bool sRGB=false);
Nit: spaces around '='

[msarett, 2016-06-15 13:48:10]

"Currently, drawing images with sRGB seems to still have bugs."

Sorry I haven't had a chance to look at this yet.  If you want to poke around,
I'm guessing that we're dropping the sRGB flag from the SkImageInfo somewhere.

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
File tools/viewer/sk_app/WindowContext.cpp (right):

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.cpp:39: // kUnknown_SkAlphaType won't pass
Sorry I don't understand what you're saying here?  But I don't typically use
kUnknown anyway.  If you're sure it's opaque, kOpaque makes sense.  Otherwise,
it's probably best to use kPremul.

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.cpp:42: sRGB ? kSRGB_SkColorProfileType
Please use a SkColorSpace object here instead of the SkColorProfileType.

Ex:

(sRGB || fDisplayParams.fProfileType == kSRGB) ? SkColorSpace::NewSRGB() :
nullptr.

We currently try to keep the SkColorSpace and SkColorProfileType in sync, with
the eventual goal being to delete the SkProfileType.

[liyuqian, 2016-06-15 14:09:14]

liyuqian@google.com changed reviewers:
+ jvanverth@google.com

[liyuqian, 2016-06-15 14:09:15]

Thanks Brian and Matt! Please see my response below.

I also improved (in my opinion) the API a little bit in this new patch.

I'm adding Jim to reviewers for reviewing the API change and how to replace
SkProfileType with SkColorSpace.

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
File tools/viewer/sk_app/WindowContext.cpp (right):

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.cpp:27: (GrPixelConfigIsSRGB(fPixelConfig) ?
SkSurfaceProps::kGammaCorrect_Flag : 0);
On 2016/06/15 13:41:59, Brian Osman wrote:
> Need to also set this flag if sRGB is true:
> 
> ((GrPixelConfigIsSRGB(fPixelConfig) || sRGB) ?
> skSurfaceProps::kGammaCorrect_Flag : 0);

Done.

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.cpp:39: // kUnknown_SkAlphaType won't pass
On 2016/06/15 13:48:10, msarett wrote:
> Sorry I don't understand what you're saying here?  But I don't typically use
> kUnknown anyway.  If you're sure it's opaque, kOpaque makes sense.  Otherwise,
> it's probably best to use kPremul.

I was referring to the previous implementation before my CL. I think that the
previous implementation has never executed this line because MakeRenderTarget
with kUnknown_SkAlphaType will cause crash by CheckAlphaAndGetFlags.

I was using kOpaque in the Raster backend when I create the Surface. Should I
also use kPremul there?

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.cpp:42: sRGB ? kSRGB_SkColorProfileType
On 2016/06/15 13:48:10, msarett wrote:
> Please use a SkColorSpace object here instead of the SkColorProfileType.
> 
> Ex:
> 
> (sRGB || fDisplayParams.fProfileType == kSRGB) ? SkColorSpace::NewSRGB() :
> nullptr.
> 
> We currently try to keep the SkColorSpace and SkColorProfileType in sync, with
> the eventual goal being to delete the SkProfileType.

The difficulty here is that DisplayParams maintains a SkProfileType which shall
be used if sRGB is not set to true. I'll add Jim to reviewers to let him decide
how to change all SkColorProfileType to SkColorSpace.

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
File tools/viewer/sk_app/WindowContext.h (right):

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.h:48: sk_sp<GrRenderTarget>, int colorBits,
bool offscreen=false, bool sRGB=false);
On 2016/06/15 13:41:59, Brian Osman wrote:
> Nit: spaces around '='

Done.

[djsollen, 2016-06-15 14:14:29]

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
File tools/viewer/sk_app/Window.cpp (right):

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/Window.cpp:10: #include "GrRenderTarget.h"
why do you need this include?

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
File tools/viewer/sk_app/Window.h (right):

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/Window.h:165: sk_sp<SkSurface> getOffscreenSurface(bool
sRGB);
why not pass SkSurfaceProps as an argument so that it is consistent with our
other APIs?

[msarett, 2016-06-15 14:20:40]

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
File tools/viewer/sk_app/WindowContext.cpp (right):

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.cpp:39: // kUnknown_SkAlphaType won't pass
On 2016/06/15 14:09:15, liyuqian wrote:
> On 2016/06/15 13:48:10, msarett wrote:
> > Sorry I don't understand what you're saying here?  But I don't typically use
> > kUnknown anyway.  If you're sure it's opaque, kOpaque makes sense. 
Otherwise,
> > it's probably best to use kPremul.
> 
> I was referring to the previous implementation before my CL. I think that the
> previous implementation has never executed this line because MakeRenderTarget
> with kUnknown_SkAlphaType will cause crash by CheckAlphaAndGetFlags.
> 
> I was using kOpaque in the Raster backend when I create the Surface. Should I
> also use kPremul there?

Depends on whether what you are drawing is opaque or premul.  Typically, you
should use premul if you're not sure.  If it's relevant, jpegs are opaque.

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.cpp:42: sRGB ? kSRGB_SkColorProfileType
On 2016/06/15 14:09:15, liyuqian wrote:
> On 2016/06/15 13:48:10, msarett wrote:
> > Please use a SkColorSpace object here instead of the SkColorProfileType.
> > 
> > Ex:
> > 
> > (sRGB || fDisplayParams.fProfileType == kSRGB) ? SkColorSpace::NewSRGB() :
> > nullptr.
> > 
> > We currently try to keep the SkColorSpace and SkColorProfileType in sync,
with
> > the eventual goal being to delete the SkProfileType.
> 
> The difficulty here is that DisplayParams maintains a SkProfileType which
shall
> be used if sRGB is not set to true. I'll add Jim to reviewers to let him
decide
> how to change all SkColorProfileType to SkColorSpace.

SGTM.

I still think it's possible to use SkColorSpace here without an API change, you
would just need to check the sRGB bool and the fDisplayParams.
(sRGB || fDisplayParams.fProfileType == kSRGB) ? SkColorSpace::NewSRGB() :
nullptr

But I'm ok if you want to avoid this clutter and stick with the current API.

[Brian Osman, 2016-06-15 15:32:04]

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
File tools/viewer/sk_app/WindowContext.cpp (right):

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/WindowContext.cpp:42: kOpaque_SkAlphaType,
kOpaque is probably okay, although kPremul is probably better. (I think that our
GMs generally don't rely on destination alpha, but there's no hard-and-fast
rule).

[liyuqian, 2016-06-15 16:43:12]

Please see me fixes and comments below.

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
File tools/viewer/sk_app/WindowContext.cpp (right):

https://codereview.chromium.org/2069653002/diff/1/tools/viewer/sk_app/WindowC...
tools/viewer/sk_app/WindowContext.cpp:42: sRGB ? kSRGB_SkColorProfileType
On 2016/06/15 14:20:40, msarett wrote:
> On 2016/06/15 14:09:15, liyuqian wrote:
> > On 2016/06/15 13:48:10, msarett wrote:
> > > Please use a SkColorSpace object here instead of the SkColorProfileType.
> > > 
> > > Ex:
> > > 
> > > (sRGB || fDisplayParams.fProfileType == kSRGB) ? SkColorSpace::NewSRGB() :
> > > nullptr.
> > > 
> > > We currently try to keep the SkColorSpace and SkColorProfileType in sync,
> with
> > > the eventual goal being to delete the SkProfileType.
> > 
> > The difficulty here is that DisplayParams maintains a SkProfileType which
> shall
> > be used if sRGB is not set to true. I'll add Jim to reviewers to let him
> decide
> > how to change all SkColorProfileType to SkColorSpace.
> 
> SGTM.
> 
> I still think it's possible to use SkColorSpace here without an API change,
you
> would just need to check the sRGB bool and the fDisplayParams.
> (sRGB || fDisplayParams.fProfileType == kSRGB) ? SkColorSpace::NewSRGB() :
> nullptr
> 
> But I'm ok if you want to avoid this clutter and stick with the current API.

Done. I've adopted your approach.

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
File tools/viewer/sk_app/Window.cpp (right):

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/Window.cpp:10: #include "GrRenderTarget.h"
On 2016/06/15 14:14:29, djsollen wrote:
> why do you need this include?

Line 118. The first nullptr is GrRenderTarget* and the compiler is complaining
if I don't include it.

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
File tools/viewer/sk_app/Window.h (right):

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/Window.h:165: sk_sp<SkSurface> getOffscreenSurface(bool
sRGB);
On 2016/06/15 14:14:29, djsollen wrote:
> why not pass SkSurfaceProps as an argument so that it is consistent with our
> other APIs?

Because SkSurfaceProps is kind of complicated to construct. A lot of that work
is done in WindowContext::createRenderSurface. This simplified API reuses that
code. I'll add comment to this method to specify that this API is only for split
screen usage. Hopefully that will make things clear?

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
File tools/viewer/sk_app/WindowContext.cpp (right):

https://codereview.chromium.org/2069653002/diff/20001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/WindowContext.cpp:42: kOpaque_SkAlphaType,
On 2016/06/15 15:32:03, Brian Osman wrote:
> kOpaque is probably okay, although kPremul is probably better. (I think that
our
> GMs generally don't rely on destination alpha, but there's no hard-and-fast
> rule).

Done. Ok, I'll change both kOpaque to kPremul.

[liyuqian, 2016-06-16 14:47:08]

Any update (like lgtm :)?

[djsollen, 2016-06-16 16:55:06]

https://codereview.chromium.org/2069653002/diff/40001/tools/viewer/sk_app/Win...
File tools/viewer/sk_app/WindowContext.h (right):

https://codereview.chromium.org/2069653002/diff/40001/tools/viewer/sk_app/Win...
tools/viewer/sk_app/WindowContext.h:48: sk_sp<GrRenderTarget>, int colorBits,
bool offscreen = false, bool sRGB = false);
As a public API I don't like this and think it tries to combine 2 different
functions because their implementation is similar not because they are intended
to be used the same way.

I propose a new function something like

sk_sp<SkSurface> createOffscreenSurface(...)

This function can share a common private implementation with the existing
function.

[liyuqian, 2016-06-16 20:24:08]

Hi Derek, I've revised the API according to your suggestion.

[djsollen, 2016-06-16 20:25:26]

lgtm

[liyuqian, 2016-06-16 20:43:09]

The CQ bit was checked by liyuqian@google.com

[liyuqian, 2016-06-16 20:43:10]

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

[commit-bot: I haz the power, 2016-06-16 20:43:18]

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

[commit-bot: I haz the power, 2016-06-16 21:10:38]

Description was changed from

==========
Use Offscreen Surface for Split Screen

A dashed line is also added to split the screen. Currently, drawing
images with sRGB seems to still have bugs.

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

to

==========
Use Offscreen Surface for Split Screen

A dashed line is also added to split the screen. Currently, drawing
images with sRGB seems to still have bugs.

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

Committed:
https://skia.googlesource.com/skia/+/74959a1471cd3b20e2dd79fc0c685e7205a25cd9
==========

[commit-bot: I haz the power, 2016-06-16 21:10:40]

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