Use SkJpegEncoder in WebKit platform

Use SkJpegEncoder in WebKit platform

This also makes CanvasAsyncBlobCreator use SkPngEncoder, since it is
natural to land jpeg and png together here.

The motivations for using Skia encoders are:
***Support for embedding ICC profiles
***Support for linear unpremultiplication and blending
***Support for multiple color types (including F16)
***Support for multiple alpha types (possible follow-up optimization)

As a long term refactoring goal, we should be able to delete
the WebKit image encoders and the ui/gfx image encoders, thus
leaving one set of image encoders to maintain.

Incidentally, I think this CL is also a nice simplication for
CanvasAsyncBlobCreator.

BUG=713862
Review-Url: https://codereview.chromium.org/2878333004
Cr-Commit-Position: refs/heads/master@{#472781}
Committed: https://chromium.googlesource.com/chromium/src/+/86074971b9640dbf59869071b4a18bec51c61424

[msarett1, 2017-05-15 18:51:28]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 18:52:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msarett1, 2017-05-15 18:58:32]

msarett@chromium.org changed reviewers:
+ fmalita@chromium.org, junov@chromium.org, xlai@chromium.org,
zakerinasab@chromium.org

[commit-bot: I haz the power, 2017-05-15 19:36:11]

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

[commit-bot: I haz the power, 2017-05-15 19:36:12]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[f(malita), 2017-05-15 19:53:26]

https://codereview.chromium.org/2878333004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2878333004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:201: dst_
= WTF::WrapUnique(new VectorWStream(encoded_image_.get()));
This is in part pre-existing, but it's unclear to me why encoded_image_ and dst_
have to be heap-allocated (as opposed to inlined in CanvasAsyncBlobCreator). 
I'm fine with leaving alone for now, we can look at it separately.

https://codereview.chromium.org/2878333004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:425: if
(kMimeTypeJpeg == mime_type_) {
nit: I don't think Yoda conditions are favored in Chromium/Blink.

https://codereview.chromium.org/2878333004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/2878333004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:102:
std::unique_ptr<SkEncoder> encoder_;
For encapsulation, I wonder if it'd make sense to have a thin wrapper for
SkEncoder + VectorWStream.  How about something like

  class ImageEncoder {
    ImageEncoder(...);

    encodeRows(...);

    encode(...);
  }

Then I think we could hide VectorWStream and some of the initialization
boilerplate.

WDYT?

[msarett1, 2017-05-15 23:47:46]

Patchset #2 (id:20001) has been deleted

[msarett1, 2017-05-15 23:49:08]

https://codereview.chromium.org/2878333004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2878333004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:201: dst_
= WTF::WrapUnique(new VectorWStream(encoded_image_.get()));
On 2017/05/15 19:53:25, f(malita) wrote:
> This is in part pre-existing, but it's unclear to me why encoded_image_ and
dst_
> have to be heap-allocated (as opposed to inlined in CanvasAsyncBlobCreator). 
> I'm fine with leaving alone for now, we can look at it separately.

I thought this was odd too...  I went ahead and changed it.  Seems fine.

https://codereview.chromium.org/2878333004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:425: if
(kMimeTypeJpeg == mime_type_) {
On 2017/05/15 19:53:26, f(malita) wrote:
> nit: I don't think Yoda conditions are favored in Chromium/Blink.

Done.

https://codereview.chromium.org/2878333004/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/2878333004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:102:
std::unique_ptr<SkEncoder> encoder_;
On 2017/05/15 19:53:26, f(malita) wrote:
> For encapsulation, I wonder if it'd make sense to have a thin wrapper for
> SkEncoder + VectorWStream.  How about something like
> 
>   class ImageEncoder {
>     ImageEncoder(...);
> 
>     encodeRows(...);
> 
>     encode(...);
>   }
> 
> Then I think we could hide VectorWStream and some of the initialization
> boilerplate.
> 
> WDYT?

I wrote a wrapper class, let me know if it is along the lines you were thinking.

I think it is nice to encapsulate VectorWStream.

I intentionally didn't encapsulate SkImageInfo or the Options.  The flexibility
provided by the SkImageInfo (and some of the Options) is part of the motivation
for this change.

Though I could see an argument to encapulate some of the Options that I am just
filling in with the old Chrome defaults...

https://codereview.chromium.org/2878333004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-encoders/ImageEncoder.h (right):

https://codereview.chromium.org/2878333004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-encoders/ImageEncoder.h:58:
VectorWStream dst_;
If I were to make this a ptr, I think I could move the VectorWStream class
definition into the cpp file...

[msarett1, 2017-05-15 23:49:16]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 23:50:45]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-16 01:46:01]

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

[commit-bot: I haz the power, 2017-05-16 01:46:02]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[msarett1, 2017-05-16 16:08:04]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-16 16:08:34]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msarett1, 2017-05-16 20:33:43]

Justin, would you be the right person to take a look for Olivia and Reza?  It
looks like they are both OOO.

[commit-bot: I haz the power, 2017-05-16 21:05:37]

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

[commit-bot: I haz the power, 2017-05-16 21:05:38]

Dry run: This issue passed the CQ dry run.

[f(malita), 2017-05-16 21:09:46]

lgtm % some nits

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:440:
encoder_ = ImageEncoder::Make(&encoded_image_, src_data_, options);
We can pull this out of both blocks, looks identical.

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:443:
return encoder_.get();
Is .get() required? I think uniqueptr converts to bool fine.

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-encoders/ImageEncoder.h (right):

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-encoders/ImageEncoder.h:37: static bool
Encode(Vector<unsigned char>* dst,
I was actually thinking encode() would be non-static, like encodeRows().  Then
you don't need two version and can avoid passing the options here.  You'd have
to create a ImageEncoder on stack, but that's not a big deal.

[...]

Ah, scratch that - I see SkEncoder doesn't have an encode() virtual.

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-encoders/ImageEncoder.h:45: static
std::unique_ptr<ImageEncoder> Make(Vector<unsigned char>* dst,
Nit: "Create" is the common factory name in Blink.

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-encoders/ImageEncoder.h:49: static
std::unique_ptr<ImageEncoder> Make(Vector<unsigned char>* dst,
Ditto.

[msarett1, 2017-05-17 02:06:46]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 02:08:38]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msarett1, 2017-05-17 02:26:59]

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:440:
encoder_ = ImageEncoder::Make(&encoded_image_, src_data_, options);
On 2017/05/16 21:09:45, f(malita) wrote:
> We can pull this out of both blocks, looks identical.

Unfortunately I can't.  One takes SkJpegEncoder::Options and the other takes
SkPngEncoder::Options.

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:443:
return encoder_.get();
On 2017/05/16 21:09:46, f(malita) wrote:
> Is .get() required? I think uniqueptr converts to bool fine.

I thought it did too.  For some reason, removing the .get() breaks my local
compile.

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/image-encoders/ImageEncoder.h (right):

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-encoders/ImageEncoder.h:37: static bool
Encode(Vector<unsigned char>* dst,
On 2017/05/16 21:09:46, f(malita) wrote:
> I was actually thinking encode() would be non-static, like encodeRows().  Then
> you don't need two version and can avoid passing the options here.  You'd have
> to create a ImageEncoder on stack, but that's not a big deal.
> 
> [...]
> 
> Ah, scratch that - I see SkEncoder doesn't have an encode() virtual.

Acknowledged.

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-encoders/ImageEncoder.h:45: static
std::unique_ptr<ImageEncoder> Make(Vector<unsigned char>* dst,
On 2017/05/16 21:09:46, f(malita) wrote:
> Nit: "Create" is the common factory name in Blink.

Done.

https://codereview.chromium.org/2878333004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/image-encoders/ImageEncoder.h:49: static
std::unique_ptr<ImageEncoder> Make(Vector<unsigned char>* dst,
On 2017/05/16 21:09:46, f(malita) wrote:
> Ditto.

Done.

[commit-bot: I haz the power, 2017-05-17 04:00:13]

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

[commit-bot: I haz the power, 2017-05-17 04:00:14]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msarett1, 2017-05-17 12:39:09]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 12:39:25]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 14:36:33]

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

[commit-bot: I haz the power, 2017-05-17 14:36:34]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msarett1, 2017-05-17 14:58:11]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 14:58:52]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 17:05:01]

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

[commit-bot: I haz the power, 2017-05-17 17:05:02]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msarett1, 2017-05-17 17:13:15]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 17:14:17]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 19:41:43]

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

[commit-bot: I haz the power, 2017-05-17 19:41:44]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msarett1, 2017-05-17 19:46:22]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 19:48:02]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-17 21:47:06]

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

[commit-bot: I haz the power, 2017-05-17 21:47:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[msarett1, 2017-05-17 22:43:04]

The CQ bit was checked by msarett@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-17 22:44:58]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 04:26:03]

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

[commit-bot: I haz the power, 2017-05-18 04:26:04]

Dry run: This issue passed the CQ dry run.

[msarett1, 2017-05-18 12:19:06]

The CQ bit was checked by msarett@chromium.org

[msarett1, 2017-05-18 12:19:07]

The patchset sent to the CQ was uploaded after l-g-t-m from fmalita@chromium.org
Link to the patchset: https://codereview.chromium.org/2878333004/#ps80001
(title: "Create instead of Make")

[commit-bot: I haz the power, 2017-05-18 12:19:40]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-18 12:25:31]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1495109946980930,
"parent_rev": "8b69ce5fc7a7e267f2f893df6954c3787d5f2d2f", "commit_rev":
"86074971b9640dbf59869071b4a18bec51c61424"}

[commit-bot: I haz the power, 2017-05-18 12:25:41]

Description was changed from

==========
Use SkJpegEncoder in WebKit platform

This also makes CanvasAsyncBlobCreator use SkPngEncoder, since it is
natural to land jpeg and png together here.

The motivations for using Skia encoders are:
***Support for embedding ICC profiles
***Support for linear unpremultiplication and blending
***Support for multiple color types (including F16)
***Support for multiple alpha types (possible follow-up optimization)

As a long term refactoring goal, we should be able to delete
the WebKit image encoders and the ui/gfx image encoders, thus
leaving one set of image encoders to maintain.

Incidentally, I think this CL is also a nice simplication for
CanvasAsyncBlobCreator.

BUG=713862
==========

to

==========
Use SkJpegEncoder in WebKit platform

This also makes CanvasAsyncBlobCreator use SkPngEncoder, since it is
natural to land jpeg and png together here.

The motivations for using Skia encoders are:
***Support for embedding ICC profiles
***Support for linear unpremultiplication and blending
***Support for multiple color types (including F16)
***Support for multiple alpha types (possible follow-up optimization)

As a long term refactoring goal, we should be able to delete
the WebKit image encoders and the ui/gfx image encoders, thus
leaving one set of image encoders to maintain.

Incidentally, I think this CL is also a nice simplication for
CanvasAsyncBlobCreator.

BUG=713862

Review-Url: https://codereview.chromium.org/2878333004
Cr-Commit-Position: refs/heads/master@{#472781}
Committed:
https://chromium.googlesource.com/chromium/src/+/86074971b9640dbf59869071b4a1...
==========

[commit-bot: I haz the power, 2017-05-18 12:25:49]

Committed patchset #4 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/86074971b9640dbf59869071b4a1...