Remove JPEG encoder SkBitmap API

Remove JPEG encoder SkBitmap API

SkBitmap is being removed from Chrome. There are no longer any callers
of JPEGImageEncoder::encode(SkBitmap,...) so we can remove it and also
the support code it required.

Note: The libjpeg-turbo specialization was the fastest encoder, but it
required premultiplied data (SkBitmap). Thus some performance was lost
when callers were switched over to use the ImageDataBuffer encoder (in
the SkImage-replaces-SkBitmap endeavor).

No change in behavior, no new tests.

BUG=449197
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=201367

[Noel Gordon, 2015-08-27 14:16:48]

noel@chromium.org changed reviewers:
+ fmalita@chromium.org

[Noel Gordon, 2015-08-27 14:19:23]

https://codereview.chromium.org/1314763008/diff/1/Source/platform/image-encod...
File Source/platform/image-encoders/skia/JPEGImageEncoder.cpp (right):

https://codereview.chromium.org/1314763008/diff/1/Source/platform/image-encod...
Source/platform/image-encoders/skia/JPEGImageEncoder.cpp:34: // #include
"SkBitmap.h"
Will remove this after the trys all pass.

[Noel Gordon, 2015-08-27 14:36:38]

canvas and fast{image,canvas} layout tests passing locally for me:  patch for
landing, PTAL.

[reed1, 2015-08-27 14:51:34]

reed@google.com changed reviewers:
+ reed@google.com, scroggo@chromium.org

[f(malita), 2015-08-27 14:57:16]

LGTM.

To your perf comment: we could add an encode(SkImage) variant, and then read the
SkImage pixels in whatever format is required for libjpeg-turbo.  Would that
help?

[reed1, 2015-08-27 15:01:38]

SkImage->encode(kJPEG) exists, but hasn't yet been plumbed thru to blink's
codecs (the decode variant has!) so at some point, at least for SkImages, we
should call that directly (it will still land in blink's copy of libjpeg-turbo)

[urvang, 2015-08-27 16:59:14]

urvang@chromium.org changed reviewers:
+ urvang@chromium.org

[urvang, 2015-08-27 16:59:14]

Good riddance!
One question: why not remove the same from PNG and WebP encoders too?

[Noel Gordon, 2015-08-28 03:07:22]

On 2015/08/27 16:59:14, urvang wrote:
> Good riddance!
> One question: why not remove the same from PNG and WebP encoders too?

For PNG, there are a few hangers-on per
https://code.google.com/p/chromium/codesearch#search/&q=PNGImageEncoder::enco...

   WebKit/Source/platform/graphics/LoggingCanvas.cpp
   WebKit/Source/platform/graphics/PictureSnapshot.cpp

@f(maita): could you file a bug and kill those cases (convert them to
ImageDataBuffer)?

@urvang: For WEBP, I will do webp in a separate patch.  The current patch was to
test the we-dont-use-skbitmap-anymore waters.

[Noel Gordon, 2015-08-28 03:22:20]

On 2015/08/27 14:57:16, f(malita) wrote:
> LGTM.
> 
> To your perf comment: we could add an encode(SkImage) variant, and then read
the
> SkImage pixels in whatever format is required for libjpeg-turbo.  Would that
> help?

Yeah maybe, please file about it so we don't forget.

Re: the ImageDataBuffer interface: it was added because of webgl, the 3d canvas
case, perhaps because it has the unpremult pixels in it's backing storage, and
maybe the accelerated canvas 2d case is the same these days (dunno).

For a software (non-accelerated) 2d canvas, they were backed by a SkBitmap back
in the day, and had premultiplied pixels, and so used the SkBitmap interface to
the encoders.  These were switched over to use ImageDataBuffer and that changed
perf.  Not sure if the switchover change mentioned that.

[Noel Gordon, 2015-08-28 03:25:23]

On 2015/08/27 15:01:38, reed1 wrote:
> SkImage->encode(kJPEG) exists, but hasn't yet been plumbed thru to blink's
> codecs (the decode variant has!) so at some point, at least for SkImages, we
> should call that directly (it will still land in blink's copy of
libjpeg-turbo)

I've mostly spoken to Leon about decoders and related matters. But agree, what
to do about the encoders should also needs discussion.

[Noel Gordon, 2015-08-28 03:25:46]

Test passing, landing this change.

[Noel Gordon, 2015-08-28 03:25:51]

The CQ bit was checked by noel@chromium.org

[commit-bot: I haz the power, 2015-08-28 03:26:04]

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

[Noel Gordon, 2015-08-28 03:38:52]

Aside: developers do in fact care about toDataURL perf,
https://code.google.com/p/chromium/issues/detail?id=112966 for example [wacky
use-case, perhaps better served by <canvas>.toBlob()... but whatever, we should
be careful with toDataURL perf].

[commit-bot: I haz the power, 2015-08-28 04:22:39]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201367