Use the new function signature for EncodeBitmap.

Use the new function signature for EncodeBitmap.

Recent changes to Skia (https://codereview.chromium.org/15489004/)
modified the signature of SkPicture::EncodeBitmap. Modify chromium's
callers to use the new signature.

In gpu_benchmarking_extension's implementation of EncodeBitmap,
give priority to refEncodedData, which was previously the job
of SkOrderedWriteBuffer::writeBitmap, but no longer is.

NOTRY=true

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202089

[vmpstr, 2013-05-21 18:18:30]

https://codereview.chromium.org/15496006/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/15496006/diff/1/cc/resources/picture.cc#newco...
cc/resources/picture.cc:52: return false;
return NULL

[scroggo, 2013-05-21 18:36:35]

https://codereview.chromium.org/15496006/diff/1/cc/resources/picture.cc
File cc/resources/picture.cc (right):

https://codereview.chromium.org/15496006/diff/1/cc/resources/picture.cc#newco...
cc/resources/picture.cc:52: return false;
On 2013/05/21 18:18:31, vmpstr wrote:
> return NULL

Done.

[vmpstr, 2013-05-21 18:48:27]

https://codereview.chromium.org/15496006/diff/5001/content/renderer/gpu/gpu_b...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/15496006/diff/5001/content/renderer/gpu/gpu_b...
content/renderer/gpu/gpu_benchmarking_extension.cc:44: SkPixelRef* pr =
bm.pixelRef();
I was under the impression that SkOrderedWriteBuffer::writeBitmap already did
this check, and the encoder would only be called if refEncodedData returned
null. Is that not the case anymore?

I'm just trying to understand the difference between this EncodeBitmap and the
one in picture.cc

[scroggo, 2013-05-21 18:58:35]

https://codereview.chromium.org/15496006/diff/5001/content/renderer/gpu/gpu_b...
File content/renderer/gpu/gpu_benchmarking_extension.cc (right):

https://codereview.chromium.org/15496006/diff/5001/content/renderer/gpu/gpu_b...
content/renderer/gpu/gpu_benchmarking_extension.cc:44: SkPixelRef* pr =
bm.pixelRef();
On 2013/05/21 18:48:27, vmpstr wrote:
> I was under the impression that SkOrderedWriteBuffer::writeBitmap already did
> this check, and the encoder would only be called if refEncodedData returned
> null. Is that not the case anymore?
> 
> I'm just trying to understand the difference between this EncodeBitmap and the
> one in picture.cc

You are correct; writeBitmap DID do that check, but
https://codereview.chromium.org/15489004/ removes the check.

If we do the check in picture.cc, that encoded picture will contain the original
encoded data (when refEncodedData returns non-NULL). This has a couple of
downsides (which could be fixed):

1. Some bitmaps cannot be decoded by DecodeBitmap, so the result will be a red
bitmap.
 - This could be fixed by adding more decoders.
 - It could also be fixed by checking to see if the result of refEncodedData can
be decoded, and if not, re-encoding to a format that can be decoded.
2. Although the original encoded data means the result will be exactly like the
original, it won't perform the same lazy decoding (although maybe that's okay).

Should I add the check for refEncodedData in picture.cc?

[vmpstr, 2013-05-21 19:13:10]

On 2013/05/21 18:58:35, scroggo wrote:
>
https://codereview.chromium.org/15496006/diff/5001/content/renderer/gpu/gpu_b...
> File content/renderer/gpu/gpu_benchmarking_extension.cc (right):
> 
>
https://codereview.chromium.org/15496006/diff/5001/content/renderer/gpu/gpu_b...
> content/renderer/gpu/gpu_benchmarking_extension.cc:44: SkPixelRef* pr =
> bm.pixelRef();
> On 2013/05/21 18:48:27, vmpstr wrote:
> > I was under the impression that SkOrderedWriteBuffer::writeBitmap already
did
> > this check, and the encoder would only be called if refEncodedData returned
> > null. Is that not the case anymore?
> > 
> > I'm just trying to understand the difference between this EncodeBitmap and
the
> > one in picture.cc
> 
> You are correct; writeBitmap DID do that check, but
> https://codereview.chromium.org/15489004/ removes the check.
> 
> If we do the check in picture.cc, that encoded picture will contain the
original
> encoded data (when refEncodedData returns non-NULL). This has a couple of
> downsides (which could be fixed):
> 
> 1. Some bitmaps cannot be decoded by DecodeBitmap, so the result will be a red
> bitmap.
>  - This could be fixed by adding more decoders.
>  - It could also be fixed by checking to see if the result of refEncodedData
can
> be decoded, and if not, re-encoding to a format that can be decoded.
> 2. Although the original encoded data means the result will be exactly like
the
> original, it won't perform the same lazy decoding (although maybe that's
okay).
> 
> Should I add the check for refEncodedData in picture.cc?

Oh I see. Thanks for the explanation. I think we should keep the picture as is,
since that will guarantee that we can decode each picture and we can change the
format if we want. If we decide to use the refEncodedData, then that can always
be a follow-up.

lgtm from my side.

[alokp, 2013-05-21 19:15:37]

lgtm

[scroggo, 2013-05-22 14:19:33]

Adding OWNERS

[Ken Russell (switch to Gerrit), 2013-05-22 17:20:05]

content/renderer/gpu/ LGTM

[commit-bot: I haz the power, 2013-05-23 13:11:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scroggo@google.com/15496006/5001

[commit-bot: I haz the power, 2013-05-23 13:36:31]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-23 20:22:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scroggo@google.com/15496006/26001

[commit-bot: I haz the power, 2013-05-24 02:00:40]

Retried try job too often on win_rel for step(s) check_deps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-05-24 15:19:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/scroggo@google.com/15496006/26001

[commit-bot: I haz the power, 2013-05-24 15:19:45]

Change committed as 202089