Use SkPicture::ExtractBitmap callback in pdf too.

Use SkPicture::ExtractBitmap callback in pdf too, there is no need for a specialized function pointer for pdf only only to pass a rectangle, when we can use subseted bitmaps.

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

[reed1, 2013-09-27 14:50:20]

Will there need to be any changes on the Chrome side when we do the next DEPS
roll?

[edisonn, 2013-09-27 15:13:39]

On 2013/09/27 14:50:20, reed1 wrote:
> Will there need to be any changes on the Chrome side when we do the next DEPS
> roll?

Actually, no. VectorPlatformDeviceSkia does not set the callback to compress to
bitmaps JPEG yet. It is something which needs to be enabled...

[vandebo (ex-Chrome), 2013-09-27 16:06:13]

Also, please fix up the issue description.

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
src/pdf/SkPDFImage.cpp:487: // Extract subset
It's unfortunate that we have to do this without giving the caller a chance to
make a decision first.

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
src/pdf/SkPDFImage.cpp:488: if (!fBitmap.extractSubset(&subset, fSrcRect)) {
extractSubset doesn't have any special cases for fSrcRect beign the same size as
the image, so you might want to do that here.

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
src/pdf/SkPDFImage.cpp:490: if (!fBitmap.copyTo(&subset, fBitmap.config())) {
Don't you still need to subset the image after this?

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
src/pdf/SkPDFImage.cpp:496: if (data.get() && data->size() <
get_uncompressed_size(fBitmap, fSrcRect)) {
This file is basically 80-column clean, so please keep it that way (so says the
style guide).

[reed1, 2013-09-27 16:15:29]

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
src/pdf/SkPDFImage.cpp:488: if (!fBitmap.extractSubset(&subset, fSrcRect)) {
On 2013/09/27 16:06:13, vandebo wrote:
> extractSubset doesn't have any special cases for fSrcRect beign the same size
as
> the image, so you might want to do that here.

What is the concern with always calling extractSubset()?

[vandebo (ex-Chrome), 2013-09-27 16:16:36]

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
src/pdf/SkPDFImage.cpp:488: if (!fBitmap.extractSubset(&subset, fSrcRect)) {
On 2013/09/27 16:15:29, reed1 wrote:
> On 2013/09/27 16:06:13, vandebo wrote:
> > extractSubset doesn't have any special cases for fSrcRect beign the same
size
> as
> > the image, so you might want to do that here.
> 
> What is the concern with always calling extractSubset()?

Just that it's unnecessary computational overhead when the subset == the full
image.

[reed1, 2013-09-27 16:19:18]

On 2013/09/27 16:16:36, vandebo wrote:
> https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp
> File src/pdf/SkPDFImage.cpp (right):
> 
>
https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
> src/pdf/SkPDFImage.cpp:488: if (!fBitmap.extractSubset(&subset, fSrcRect)) {
> On 2013/09/27 16:15:29, reed1 wrote:
> > On 2013/09/27 16:06:13, vandebo wrote:
> > > extractSubset doesn't have any special cases for fSrcRect beign the same
> size
> > as
> > > the image, so you might want to do that here.
> > 
> > What is the concern with always calling extractSubset()?
> 
> Just that it's unnecessary computational overhead when the subset == the full
> image.

Hmmm, not sure that performing the check outside is much less overhead, and it
does add to the complexity of reading the code. I would suggest keeping things
simple until some measurement tells us otherwise.

[vandebo (ex-Chrome), 2013-09-27 16:23:35]

On 2013/09/27 16:19:18, reed1 wrote:
> On 2013/09/27 16:16:36, vandebo wrote:
> > https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp
> > File src/pdf/SkPDFImage.cpp (right):
> > 
> >
>
https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
> > src/pdf/SkPDFImage.cpp:488: if (!fBitmap.extractSubset(&subset, fSrcRect)) {
> > On 2013/09/27 16:15:29, reed1 wrote:
> > > On 2013/09/27 16:06:13, vandebo wrote:
> > > > extractSubset doesn't have any special cases for fSrcRect beign the same
> > size
> > > as
> > > > the image, so you might want to do that here.
> > > 
> > > What is the concern with always calling extractSubset()?
> > 
> > Just that it's unnecessary computational overhead when the subset == the
full
> > image.
> 
> Hmmm, not sure that performing the check outside is much less overhead, and it
> does add to the complexity of reading the code. I would suggest keeping things
> simple until some measurement tells us otherwise.

extractSubset may do a deep copy, in that case the savings will be substantial.
Maybe extractSubset should be changed to detect the subset == original case
though?

[reed1, 2013-09-27 16:26:37]

On 2013/09/27 16:23:35, vandebo wrote:
> On 2013/09/27 16:19:18, reed1 wrote:
> > On 2013/09/27 16:16:36, vandebo wrote:
> > > https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp
> > > File src/pdf/SkPDFImage.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
> > > src/pdf/SkPDFImage.cpp:488: if (!fBitmap.extractSubset(&subset, fSrcRect))
{
> > > On 2013/09/27 16:15:29, reed1 wrote:
> > > > On 2013/09/27 16:06:13, vandebo wrote:
> > > > > extractSubset doesn't have any special cases for fSrcRect beign the
same
> > > size
> > > > as
> > > > > the image, so you might want to do that here.
> > > > 
> > > > What is the concern with always calling extractSubset()?
> > > 
> > > Just that it's unnecessary computational overhead when the subset == the
> full
> > > image.
> > 
> > Hmmm, not sure that performing the check outside is much less overhead, and
it
> > does add to the complexity of reading the code. I would suggest keeping
things
> > simple until some measurement tells us otherwise.
> 
> extractSubset may do a deep copy, in that case the savings will be
substantial.
> Maybe extractSubset should be changed to detect the subset == original case
> though?

Good point, if the bitmap is on the gpu, we'll need a deep-copy (which we'll
need anyway if we're going to embed it). I suggest we mark that as a
fix/optimization for SkBItmap, and leave this code as is.

[edisonn, 2013-09-27 17:57:19]

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp
File src/pdf/SkPDFImage.cpp (right):

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
src/pdf/SkPDFImage.cpp:490: if (!fBitmap.copyTo(&subset, fBitmap.config())) {
On 2013/09/27 16:06:13, vandebo wrote:
> Don't you still need to subset the image after this?

removed. extractSubset will not fork for A1, if that is a mojor missing feature,
We will implement it later.

https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
src/pdf/SkPDFImage.cpp:496: if (data.get() && data->size() <
get_uncompressed_size(fBitmap, fSrcRect)) {
On 2013/09/27 16:06:13, vandebo wrote:
> This file is basically 80-column clean, so please keep it that way (so says
the
> style guide).

Done.

[edisonn, 2013-10-03 15:23:07]

On 2013/09/27 17:57:19, edisonn wrote:
> https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp
> File src/pdf/SkPDFImage.cpp (right):
> 
>
https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
> src/pdf/SkPDFImage.cpp:490: if (!fBitmap.copyTo(&subset, fBitmap.config())) {
> On 2013/09/27 16:06:13, vandebo wrote:
> > Don't you still need to subset the image after this?
> 
> removed. extractSubset will not fork for A1, if that is a mojor missing
feature,
> We will implement it later.
> 
>
https://codereview.chromium.org/25054002/diff/1/src/pdf/SkPDFImage.cpp#newcod...
> src/pdf/SkPDFImage.cpp:496: if (data.get() && data->size() <
> get_uncompressed_size(fBitmap, fSrcRect)) {
> On 2013/09/27 16:06:13, vandebo wrote:
> > This file is basically 80-column clean, so please keep it that way (so says
> the
> > style guide).
> 
> Done.

lgtm? failures have nothing to do with this cl

[vandebo (ex-Chrome), 2013-10-03 15:28:19]

Please fix the change description, then LGTM.

- no need to list the files changed
- remove the unused BUG= line
- title says what is changed, please also say why it is changed.

[scroggo, 2013-10-03 15:29:29]

lgtm.

[commit-bot: I haz the power, 2013-10-03 18:27:37]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/edisonn@google.com/25054002/36001

[commit-bot: I haz the power, 2013-10-03 18:27:41]

Presubmit check for 25054002-36001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Since the CL is editing public API, you must have an LGTM from one of:
('reed@chromium.org', 'reed@google.com', 'bsalomon@chromium.org',
'bsalomon@google.com')

Presubmit checks took 1.0s to calculate.

[edisonn, 2013-10-03 19:07:48]

Mike or Brian needs to ok the cl for public api changes

[bsalomon, 2013-10-03 19:15:50]

On 2013/10/03 19:07:48, edisonn wrote:
> Mike or Brian needs to ok the cl for public api changes

lgtm

[commit-bot: I haz the power, 2013-10-03 19:20:07]

CQ is trying da patch. Follow status at
https://skia-tree-status.appspot.com/cq/edisonn@google.com/25054002/36001

[commit-bot: I haz the power, 2013-10-03 19:29:23]

Change committed as 11591