Add SkImageGeneratorWIC

Add SkImageGeneratorWIC

This will be a replacement for SkImageDecoder_WIC.

BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1785613010
CQ_EXTRA_TRYBOTS=client.skia.compile:Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot,Build-Mac-Clang-x86_64-Release-CMake-Trybot

Committed: https://skia.googlesource.com/skia/+/fc0b6d1053bb56698f2844bd5ca30bced7bb389d

[msarett, 2016-03-11 18:07:00]

Description was changed from

==========
Add SkImageGeneratorWIC

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

to

==========
Add SkImageGeneratorWIC

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-03-11 18:07:54]

Patchset #1 (id:1) has been deleted

[msarett, 2016-03-11 18:14:23]

Patchset #1 (id:20001) has been deleted

[msarett, 2016-03-11 18:15:07]

Description was changed from

==========
Add SkImageGeneratorWIC

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add SkImageGeneratorWIC

This will be a replacement for SkImageDecoder_WIC.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[msarett, 2016-03-11 18:17:06]

Patchset #1 (id:40001) has been deleted

[msarett, 2016-03-11 18:18:23]

msarett@google.com changed reviewers:
+ bungeman@gmail.com, corey.lucier@gmail.com, scroggo@google.com

[msarett, 2016-03-11 18:18:24]

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
File src/ports/SkImageGeneratorWIC.cpp (right):

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.cpp:82: if (GUID_WICPixelFormat16bppBGR555 ==
format ||
This is annoying.

I could limit this to the opaque types that are actually outputted by images
that we decode, but it feels safer to just check all opaque types.

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
File src/ports/SkImageGeneratorWIC.h (right):

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.h:17: * Requires that the client has initialized
COM on the calling thread.
This is different than SkImageDecoder_WIC.

[scroggo, 2016-03-11 19:10:48]

https://codereview.chromium.org/1785613010/diff/60001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1785613010/diff/60001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:800: return false;
It looks like this method should return an error string.

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
File src/ports/SkImageGeneratorWIC.cpp (right):

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.cpp:30: // Coreate an IStream.
Create*

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.cpp:82: if (GUID_WICPixelFormat16bppBGR555 ==
format ||
On 2016/03/11 18:18:24, msarett wrote:
> This is annoying.
> 
> I could limit this to the opaque types that are actually outputted by images
> that we decode, but it feels safer to just check all opaque types.

Yeah, it would be nice if there was an easier way to determine opacity/alpha
type.

On another note, this really feels like a switch statement to me, though perhaps
that's just personal preference.

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
File src/ports/SkImageGeneratorWIC.h (right):

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.h:17: * Requires that the client has initialized
COM on the calling thread.
On 2016/03/11 18:18:24, msarett wrote:
> This is different than SkImageDecoder_WIC.

Why? The only other use I see of SkAutoCoInitialize is in SkXPSDevice, which
does the initialization internally, like SkImageDecoder_WIC does. Is this for
efficiency? Or do you expect that the client would have already initialized it?

[msarett, 2016-03-11 19:20:15]

https://codereview.chromium.org/1785613010/diff/60001/dm/DMSrcSink.cpp
File dm/DMSrcSink.cpp (right):

https://codereview.chromium.org/1785613010/diff/60001/dm/DMSrcSink.cpp#newcod...
dm/DMSrcSink.cpp:800: return false;
On 2016/03/11 19:10:48, scroggo wrote:
> It looks like this method should return an error string.

Haha yeah.  Interesting that the compiler let me get away with this.

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
File src/ports/SkImageGeneratorWIC.cpp (right):

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.cpp:30: // Coreate an IStream.
On 2016/03/11 19:10:48, scroggo wrote:
> Create*

Done.

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.cpp:82: if (GUID_WICPixelFormat16bppBGR555 ==
format ||
On 2016/03/11 19:10:48, scroggo wrote:
> On 2016/03/11 18:18:24, msarett wrote:
> > This is annoying.
> > 
> > I could limit this to the opaque types that are actually outputted by images
> > that we decode, but it feels safer to just check all opaque types.
> 
> Yeah, it would be nice if there was an easier way to determine opacity/alpha
> type.
> 
> On another note, this really feels like a switch statement to me, though
perhaps
> that's just personal preference.

I actually tried to make it a switch.  For some reason, the compiler won't allow
me to treat WICPixelFormatGUID as if it's an int/enum.

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
File src/ports/SkImageGeneratorWIC.h (right):

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.h:17: * Requires that the client has initialized
COM on the calling thread.
On 2016/03/11 19:10:48, scroggo wrote:
> On 2016/03/11 18:18:24, msarett wrote:
> > This is different than SkImageDecoder_WIC.
> 
> Why? The only other use I see of SkAutoCoInitialize is in SkXPSDevice, which
> does the initialization internally, like SkImageDecoder_WIC does. Is this for
> efficiency? Or do you expect that the client would have already initialized
it?

It is also used in SkImageDecoder_WIC.  com is initialized (and uninitialized)
in the scope of the decode() function.

[scroggo, 2016-03-11 19:46:21]

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
File src/ports/SkImageGeneratorWIC.h (right):

https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.h:17: * Requires that the client has initialized
COM on the calling thread.
On 2016/03/11 19:20:14, msarett wrote:
> On 2016/03/11 19:10:48, scroggo wrote:
> > On 2016/03/11 18:18:24, msarett wrote:
> > > This is different than SkImageDecoder_WIC.
> > 
> > Why? The only other use I see of SkAutoCoInitialize is in SkXPSDevice, which
> > does the initialization internally, like SkImageDecoder_WIC does. Is this
for
> > efficiency? Or do you expect that the client would have already initialized
> it?
> 
> It is also used in SkImageDecoder_WIC.  com is initialized (and uninitialized)
> in the scope of the decode() function.

Right. Why not do it that way in SkImageGeneratorWIC?

[msarett, 2016-03-11 19:50:51]

On 2016/03/11 19:46:21, scroggo wrote:
>
https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
> File src/ports/SkImageGeneratorWIC.h (right):
> 
>
https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
> src/ports/SkImageGeneratorWIC.h:17: * Requires that the client has initialized
> COM on the calling thread.
> On 2016/03/11 19:20:14, msarett wrote:
> > On 2016/03/11 19:10:48, scroggo wrote:
> > > On 2016/03/11 18:18:24, msarett wrote:
> > > > This is different than SkImageDecoder_WIC.
> > > 
> > > Why? The only other use I see of SkAutoCoInitialize is in SkXPSDevice,
which
> > > does the initialization internally, like SkImageDecoder_WIC does. Is this
> for
> > > efficiency? Or do you expect that the client would have already
initialized
> > it?
> > 
> > It is also used in SkImageDecoder_WIC.  com is initialized (and
uninitialized)
> > in the scope of the decode() function.
> 
> Right. Why not do it that way in SkImageGeneratorWIC?

I talked to Ben about this a little bit.

I think it would be doable to initialize com in NewFromEncodedWIC() and then
pass ownership to the class when we reach the constructor.

However, it looks like most libraries that use COM require the client of the
library to have initialized it.  I think this is a "better" way to use COM.

Corey, do you have any thoughts on if/how this might affect your use of WIC?

[msarett, 2016-03-11 21:58:58]

msarett@google.com changed reviewers:
+ bungeman@google.com

[msarett, 2016-03-17 14:26:36]

On 2016/03/11 19:50:51, msarett wrote:
> On 2016/03/11 19:46:21, scroggo wrote:
> >
>
https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
> > File src/ports/SkImageGeneratorWIC.h (right):
> > 
> >
>
https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
> > src/ports/SkImageGeneratorWIC.h:17: * Requires that the client has
initialized
> > COM on the calling thread.
> > On 2016/03/11 19:20:14, msarett wrote:
> > > On 2016/03/11 19:10:48, scroggo wrote:
> > > > On 2016/03/11 18:18:24, msarett wrote:
> > > > > This is different than SkImageDecoder_WIC.
> > > > 
> > > > Why? The only other use I see of SkAutoCoInitialize is in SkXPSDevice,
> which
> > > > does the initialization internally, like SkImageDecoder_WIC does. Is
this
> > for
> > > > efficiency? Or do you expect that the client would have already
> initialized
> > > it?
> > > 
> > > It is also used in SkImageDecoder_WIC.  com is initialized (and
> uninitialized)
> > > in the scope of the decode() function.
> > 
> > Right. Why not do it that way in SkImageGeneratorWIC?
> 
> I talked to Ben about this a little bit.
> 
> I think it would be doable to initialize com in NewFromEncodedWIC() and then
> pass ownership to the class when we reach the constructor.
> 
> However, it looks like most libraries that use COM require the client of the
> library to have initialized it.  I think this is a "better" way to use COM.
> 
> Corey, do you have any thoughts on if/how this might affect your use of WIC?

Based on my read of COM documentation, I think it makes more to allow the
client of Skia to initialize COM.  It could be initialized once (instead
of on every decode) and it gives the client control over the threading
configuration.

On the other hand, it gives the client one more thing to do...

While I think this is best as is, though I don't feel too strongly.  The
good news is that I don't think this is too tough to change if we find that
we made the wrong decision.

[scroggo, 2016-03-17 17:05:59]

On 2016/03/17 14:26:36, msarett wrote:
> On 2016/03/11 19:50:51, msarett wrote:
> > On 2016/03/11 19:46:21, scroggo wrote:
> > >
> >
>
https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
> > > File src/ports/SkImageGeneratorWIC.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1785613010/diff/60001/src/ports/SkImageGenera...
> > > src/ports/SkImageGeneratorWIC.h:17: * Requires that the client has
> initialized
> > > COM on the calling thread.
> > > On 2016/03/11 19:20:14, msarett wrote:
> > > > On 2016/03/11 19:10:48, scroggo wrote:
> > > > > On 2016/03/11 18:18:24, msarett wrote:
> > > > > > This is different than SkImageDecoder_WIC.
> > > > > 
> > > > > Why? The only other use I see of SkAutoCoInitialize is in SkXPSDevice,
> > which
> > > > > does the initialization internally, like SkImageDecoder_WIC does. Is
> this
> > > for
> > > > > efficiency? Or do you expect that the client would have already
> > initialized
> > > > it?
> > > > 
> > > > It is also used in SkImageDecoder_WIC.  com is initialized (and
> > uninitialized)
> > > > in the scope of the decode() function.
> > > 
> > > Right. Why not do it that way in SkImageGeneratorWIC?
> > 
> > I talked to Ben about this a little bit.
> > 
> > I think it would be doable to initialize com in NewFromEncodedWIC() and then
> > pass ownership to the class when we reach the constructor.
> > 
> > However, it looks like most libraries that use COM require the client of the
> > library to have initialized it.  I think this is a "better" way to use COM.
> > 
> > Corey, do you have any thoughts on if/how this might affect your use of WIC?
> 
> Based on my read of COM documentation, I think it makes more to allow the
> client of Skia to initialize COM.  It could be initialized once (instead
> of on every decode) and it gives the client control over the threading
> configuration.
> 
> On the other hand, it gives the client one more thing to do...
> 
> While I think this is best as is, though I don't feel too strongly.  The
> good news is that I don't think this is too tough to change if we find that
> we made the wrong decision.

Sounds fine to me. I have an inline suggestion that maybe our docs should be
more explicit, but otherwise lgtm

https://codereview.chromium.org/1785613010/diff/80001/src/ports/SkImageGenera...
File src/ports/SkImageGeneratorWIC.h (right):

https://codereview.chromium.org/1785613010/diff/80001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.h:17: * Requires that the client has initialized
COM on the calling thread.
nit: So every thread that uses this needs to have initialized COM? e.g. Call
NewFromEncodedWIC on the main thread, then call getPixels from a thread spun up
for decoding. Both need to have initialized COM? Should this comment be more
specific about what it means by "the calling thread"?

[msarett, 2016-03-17 17:32:35]

https://codereview.chromium.org/1785613010/diff/80001/src/ports/SkImageGenera...
File src/ports/SkImageGeneratorWIC.h (right):

https://codereview.chromium.org/1785613010/diff/80001/src/ports/SkImageGenera...
src/ports/SkImageGeneratorWIC.h:17: * Requires that the client has initialized
COM on the calling thread.
On 2016/03/17 17:05:59, scroggo wrote:
> nit: So every thread that uses this needs to have initialized COM? e.g. Call
> NewFromEncodedWIC on the main thread, then call getPixels from a thread spun
up
> for decoding. Both need to have initialized COM? Should this comment be more
> specific about what it means by "the calling thread"?

Yes, I've tried to make this clearer.

[msarett, 2016-03-17 17:52:06]

Description was changed from

==========
Add SkImageGeneratorWIC

This will be a replacement for SkImageDecoder_WIC.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Add SkImageGeneratorWIC

This will be a replacement for SkImageDecoder_WIC.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia.compile:Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot,Build-Mac-Clang-x86_64-Release-CMake-Trybot
==========

[msarett, 2016-03-17 17:53:56]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-17 17:54:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1785613010/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1785613010/120001

[commit-bot: I haz the power, 2016-03-17 18:02:07]

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

[commit-bot: I haz the power, 2016-03-17 18:02:08]

Dry run: Try jobs failed on following builders:
  Build-Win-MSVC-x86_64-Debug-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Win-MSVC-x86_6...)

[msarett, 2016-03-17 18:05:53]

The CQ bit was checked by msarett@google.com to run a CQ dry run

[commit-bot: I haz the power, 2016-03-17 18:06:04]

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

[commit-bot: I haz the power, 2016-03-17 18:29:11]

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

[commit-bot: I haz the power, 2016-03-17 18:29:12]

Dry run: This issue passed the CQ dry run.

[scroggo, 2016-03-17 18:30:29]

lgtm

[msarett, 2016-03-17 18:52:40]

Patchset #6 (id:160001) has been deleted

[msarett, 2016-03-17 20:35:56]

The CQ bit was checked by msarett@google.com

[msarett, 2016-03-17 20:35:56]

The patchset sent to the CQ was uploaded after l-g-t-m from scroggo@google.com
Link to the patchset: https://codereview.chromium.org/1785613010/#ps180001
(title: "Blacklist bad images")

[commit-bot: I haz the power, 2016-03-17 20:36:00]

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

[commit-bot: I haz the power, 2016-03-17 20:50:20]

Description was changed from

==========
Add SkImageGeneratorWIC

This will be a replacement for SkImageDecoder_WIC.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia.compile:Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot,Build-Mac-Clang-x86_64-Release-CMake-Trybot
==========

to

==========
Add SkImageGeneratorWIC

This will be a replacement for SkImageDecoder_WIC.

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
CQ_EXTRA_TRYBOTS=client.skia.compile:Build-Ubuntu-GCC-x86_64-Release-CMake-Trybot,Build-Mac-Clang-x86_64-Release-CMake-Trybot

Committed:
https://skia.googlesource.com/skia/+/fc0b6d1053bb56698f2844bd5ca30bced7bb389d
==========

[commit-bot: I haz the power, 2016-03-17 20:50:22]

Committed patchset #6 (id:180001) as
https://skia.googlesource.com/skia/+/fc0b6d1053bb56698f2844bd5ca30bced7bb389d