Mask orientation and layout correctly.

Mask orientation and layout correctly.

Committed: http://code.google.com/p/skia/source/detail?r=9022

[bungeman-skia, 2013-05-06 14:48:38]

Fixes issue introduced with https://codereview.appspot.com/6499101/ .

[clefru, 2013-05-06 15:14:19]

On 2013/05/06 14:48:38, bungeman1 wrote:
> Fixes issue introduced with https://codereview.appspot.com/6499101/ .

Please add unit tests, then you find that getOrientation should return
kRGB_Layout when layout is unknown and doesn't do so. Returns 0x0 instead.

[bungeman-skia, 2013-05-06 16:03:11]

Adds a gm to test vertical bgr so we don't regress in the future.

>getOrientation should return kRGB_Layout when layout is unknown

getOrientation must return kUnknown if the layout is unknown. In this case we
must use A8 and cannot use subpixel rendering as we have no information about
the layout of the subpixels.

[bungeman-skia, 2013-05-06 17:34:37]

Correct our mac port to devolve to A8 instead of using the wrong lcd order until
we can properly support it.

[reed1, 2013-05-06 18:02:12]

https://codereview.chromium.org/14884010/diff/6001/src/ports/SkFontHost_mac.cpp
File src/ports/SkFontHost_mac.cpp (right):

https://codereview.chromium.org/14884010/diff/6001/src/ports/SkFontHost_mac.c...
src/ports/SkFontHost_mac.cpp:1839: rec->setHinting(SkPaint::kNormal_Hinting);
why do we explicitly set hinting in this case?

[bungeman-skia, 2013-05-06 18:16:10]

Added comment.

https://codereview.chromium.org/14884010/diff/6001/src/ports/SkFontHost_mac.cpp
File src/ports/SkFontHost_mac.cpp (right):

https://codereview.chromium.org/14884010/diff/6001/src/ports/SkFontHost_mac.c...
src/ports/SkFontHost_mac.cpp:1839: rec->setHinting(SkPaint::kNormal_Hinting);
On 2013/05/06 18:02:12, reed1 wrote:
> why do we explicitly set hinting in this case?

Added comment. The issue is that LCD no-hinting means LCD with-dilation, whereas
A8 no-hinting means A8 no-dilation. Setting the hinting to 'normal' here means
that the A8 generated will be A8 with-dilation, the closest possible match to
the LCD we would have otherwise drawn.

[reed1, 2013-05-06 18:19:00]

lgtm

[commit-bot: I haz the power, 2013-05-06 22:00:56]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/bungeman@google.com/14884010/15001

[commit-bot: I haz the power, 2013-05-06 22:23:11]

Change committed as 9022

[clefru, 2013-05-07 13:32:07]

On 2013/05/06 16:03:11, bungeman1 wrote:
> Adds a gm to test vertical bgr so we don't regress in the future.
> 
> >getOrientation should return kRGB_Layout when layout is unknown
> 
> getOrientation must return kUnknown if the layout is unknown. In this case we
> must use A8 and cannot use subpixel rendering as we have no information about
> the layout of the subpixels.

Thanks for the test case!

I added some remarks on the enum. I assumed that we would fallback on the
defaults in the case of unknown. If we don't, I would remove the comments.

[bungeman-skia, 2013-05-07 14:06:50]

GM images added with revision 9032.

[clefru, 2013-05-07 14:07:57]

https://codereview.chromium.org/14884010/diff/15001/include/core/SkDeviceProp...
File include/core/SkDeviceProperties.h (right):

https://codereview.chromium.org/14884010/diff/15001/include/core/SkDeviceProp...
include/core/SkDeviceProperties.h:27: kHorizontal_Orientation   = 0x2,  //!<
this is the default
How is this "default", when we don't default to it in the case of an unknown
layout?

https://codereview.chromium.org/14884010/diff/15001/include/core/SkDeviceProp...
include/core/SkDeviceProperties.h:42: kRGB_Layout       = 0x8,  //!< this is the
default
Same

[bungeman-skia, 2013-05-07 14:24:31]

To state this again, 'unknown' means 'unknown', not 'do something incorrect'.

https://codereview.chromium.org/14884010/diff/15001/include/core/SkDeviceProp...
File include/core/SkDeviceProperties.h (right):

https://codereview.chromium.org/14884010/diff/15001/include/core/SkDeviceProp...
include/core/SkDeviceProperties.h:27: kHorizontal_Orientation   = 0x2,  //!<
this is the default
On 2013/05/07 14:07:57, clefru wrote:
> How is this "default", when we don't default to it in the case of an unknown
> layout?

When you 'MakeDefault' this is the value you get. If you 'Make' one with
'kUnknown' there is nothing to default, the user stated that the geometry is
unknown (or at least not one of the others). Otherwise we wouldn't call this
'kUnknown' we'd call it 'kDefault'. But it makes no sense to have such a value,
so it isn't provided.

[reed1, 2013-05-07 14:56:54]

On 2013/05/07 14:24:31, bungeman1 wrote:
> To state this again, 'unknown' means 'unknown', not 'do something incorrect'.
> 
>
https://codereview.chromium.org/14884010/diff/15001/include/core/SkDeviceProp...
> File include/core/SkDeviceProperties.h (right):
> 
>
https://codereview.chromium.org/14884010/diff/15001/include/core/SkDeviceProp...
> include/core/SkDeviceProperties.h:27: kHorizontal_Orientation   = 0x2,  //!<
> this is the default
> On 2013/05/07 14:07:57, clefru wrote:
> > How is this "default", when we don't default to it in the case of an unknown
> > layout?
> 
> When you 'MakeDefault' this is the value you get. If you 'Make' one with
> 'kUnknown' there is nothing to default, the user stated that the geometry is
> unknown (or at least not one of the others). Otherwise we wouldn't call this
> 'kUnknown' we'd call it 'kDefault'. But it makes no sense to have such a
value,
> so it isn't provided.

To quote Yoda: "all CL questions might be answered w/ more dox/comments in the
code"

[clefru, 2013-05-29 13:33:29]

Requires more work.
(Also just for the sake of the etiquette, multiple reviewers mean multiple LGTMs
before submit.)

On 2013/05/07 14:24:31, bungeman1 wrote:
> To state this again, 'unknown' means 'unknown', not 'do something incorrect'.
> 
>
https://codereview.chromium.org/14884010/diff/15001/include/core/SkDeviceProp...
> File include/core/SkDeviceProperties.h (right):
> 
>
https://codereview.chromium.org/14884010/diff/15001/include/core/SkDeviceProp...
> include/core/SkDeviceProperties.h:27: kHorizontal_Orientation   = 0x2,  //!<
> this is the default
> On 2013/05/07 14:07:57, clefru wrote:
> > How is this "default", when we don't default to it in the case of an unknown
> > layout?
> 
> When you 'MakeDefault' this is the value you get. 

I can't see how this is true. MakeDefault calls out to
GetSubpixelOrientation/Order() wrapped in fromOldLayout/Orientation which both
end as default: kUnknown_*. 

> If you 'Make' one with
> 'kUnknown' there is nothing to default, the user stated that the geometry is
> unknown (or at least not one of the others). Otherwise we wouldn't call this
> 'kUnknown' we'd call it 'kDefault'. But it makes no sense to have such a
value,
> so it isn't provided.

Let me approach this from a different angle. How is the SkDeviceProperties
intended to behave from a caller perspective:

Is it 3-state, unknown/rgb/bgr?
Or is it 2x2 state, unknown/known, rgb/bgr?

I personally would advocate a latter design, where simple client can just query
the rgb/bgr and not care where it came from. Smarter clients could query whether
this value was set explicitly or a default. 

If you want something that explicitly turns off subpixel rendering you should
consider:
unknown/known, rgb/bgr/off.

Please add unit tests for SkDeviceProperties to document your semantics.