Add PlatformPictureSkia and RecordingPlatformDeviceSkia.

Add PlatformPictureSkia and RecordingPlatformDeviceSkia.

PlatformPictureSkia can be used to record drawing comands while still
providing support for platform paint.

BUG=123392

[reveman, 2012-09-04 18:06:12]

Hi,

this allows us to support windowless NPAPI plugins when using SkPicture
recording and per-tile painting in the compositor.

The recording canvas returned from PlatformPictureSkia::BeginPlatformRecording()
allows platform painting by playing back all previous recorded commands to a
bitmap canvas when BeginPlatformPaint() is called and then inserting a drawing
command with this bitmap into the SkPicture when EndPlatformPaint() is called.

All needed on the compositor side to take advantage of this is
s/SkPicture/PlatformPictureSkia/ in SkPictureCanvasLayerTextureUpdater.h.

[alokp, 2012-09-04 19:51:15]

I do not like the fact that we need to inherit from SkPicture. reed@ may have be
the best person to enumerate all the issues with sub-classing. I had to deal
with plenty of issues when implementing accelerated painting because
PlatformCanvas inherited from SkCanvas. I would like to avoid that if possible.

Can't we use the same pattern used to draw native controls? Last time I looked
at it, the main issue with supporting windowless plugins was not how we will
record the plugin draw calls, but rather how to support blending for transparent
plugins.

[reed1, 2012-09-04 20:46:55]

Is this API just for chrome, or for the plugin itself?

In general, I think I would prefer private embedding of the SkPicture to
subclassing as well, but just from an API defensive stand-point, nothing
specific.

Taking a further step back: we are desperately trying to remove several patterns
in chrome, that are being repeated here:

1. subclass SkDevice (we want to make devices private to skia)
2. calling setDevice

Perhaps these are necessary here. I don't understand enough about the problems
faced by windowless plugins. Can you elaborate?

[reveman, 2012-09-04 21:43:59]

On 2012/09/04 20:46:55, reed1 wrote:
> Is this API just for chrome, or for the plugin itself?

Just for chrome.

> 
> In general, I think I would prefer private embedding of the SkPicture to
> subclassing as well, but just from an API defensive stand-point, nothing
> specific.

Sure, makes sense. I'll change this.

> 
> Taking a further step back: we are desperately trying to remove several
patterns
> in chrome, that are being repeated here:
> 
> 1. subclass SkDevice (we want to make devices private to skia)
> 2. calling setDevice
> 
> Perhaps these are necessary here. I don't understand enough about the problems
> faced by windowless plugins. Can you elaborate?

The problem with windowless NPAPI plugins is that they are given access to the
platform specific bitmap used for page rendering output.  They can do any kind
of blending and read backs from this bitmap to generate output.

When recording drawing commands to a SkPicture there's no platform specific
bitmap that can be given to the plugin. Giving the plugin a clear transparent
buffer and assuming they do standard porter-duff 'over' blending might work in
most cases but there's no guarantee. This CL instead plays back all recorded
commands to a temporary platform bitmap and then gives this bitmap to the
plugin. It's not efficient but it's guaranteed to not break any old NPAPI
plugins.

In regards to the subclassing of SkDevice and calling setDevice. I'm only doing
that as it's the current way chrome implements the
BeginPlatformPaint/EndPlatformPaint hooks. If we move away from using the
current SkDevice pattern for implementing this then the changes in this CL will
change with it.

[reveman, 2012-09-10 23:47:31]

Latest patch doesn't subclass SkPicture and adds the
skia::SupportsDirectPlatformPaint function to avoid using platform paint for
text and native controls.

I also created this doc:

https://docs.google.com/a/google.com/document/d/1cJX7gGcemHIsQ_brLxG1UxrmQ6AA...

In case any of you have questions about the approach or want to propose an
alternative solution to the problem this patch is trying to solve.

[alokp, 2012-09-17 16:29:30]

Minor comments. reed@ would be the best person to review skia usage.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_canvas.h...
skia/ext/platform_canvas.h:124: // given canvas without any indirect rendering
taking place.
I am not quite clear on the differences between these two functions. May be a
comment about usage would help.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_device.cc
File skia/ext/platform_device.cc (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_device.c...
skia/ext/platform_device.cc:79: bool PlatformDevice::IsNativeRenderingAllowed()
{
Can we get rid of IsNativeFontRenderingAllowed?

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
File skia/ext/platform_picture_skia.cc (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
skia/ext/platform_picture_skia.cc:17: record_->unref();
can we use SkAutoRef to do this automatically?

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
skia/ext/platform_picture_skia.cc:23: if (record_) {
I would defer to Mike's expertise here.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
File skia/ext/platform_picture_skia.h (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
skia/ext/platform_picture_skia.h:18: SK_API PlatformPictureSkia();
Don't we export the whole class?

[reed1, 2012-09-17 16:51:24]

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_canvas.h...
skia/ext/platform_canvas.h:124: // given canvas without any indirect rendering
taking place.
On 2012/09/17 16:29:30, Alok Priyadarshi wrote:
> I am not quite clear on the differences between these two functions. May be a
> comment about usage would help.

+1 -- not sure the diff between these two functions.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_device.h...
skia/ext/platform_device.h:127: // Returns if GDI is allowed to render to this
device.
Is this the same semantic as SupportsPlatformPaint? If so, do we really need two
different expressions for the same idea?

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
File skia/ext/recording_platform_device_skia.cc (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:51: // Copy picture and playback to
raster surface.
Why do you need to make a copy of the picture?

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:65: // recorded matrix operations.
If you're done with the raster after this call, it will be more efficient to
mark that bitmap has being "immutable" before it is recorded, otherwise the
picture-recording code will make a deep-copy of it.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:67: SkCanvas* canvas =
picture_->getRecordingCanvas();
Shouldn't we "reset" the recording at this point, since the bitmap, by
definition, contains all of the necessary data for the drawing up to this point?

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:77: // PlatformCanvas matches begin
and end calls.
1. not sure what that comment means.
2. who deletes the canvas that raster_canvas_ points to?

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:79: raster_canvas_ = NULL;
At this point, perhaps we should just covert over to a raster from now own. Why
have a raster + more picture opts, when we could just become a raster?

[reveman, 2012-09-17 21:22:16]

Thanks for looking at this! I'll update the patch shortly based on your
feedback.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_canvas.h...
skia/ext/platform_canvas.h:124: // given canvas without any indirect rendering
taking place.
On 2012/09/17 16:51:24, reed1 wrote:
> On 2012/09/17 16:29:30, Alok Priyadarshi wrote:
> > I am not quite clear on the differences between these two functions. May be
a
> > comment about usage would help.
> 
> +1 -- not sure the diff between these two functions.

On 2012/09/17 16:29:30, Alok Priyadarshi wrote:
> I am not quite clear on the differences between these two functions. May be a
> comment about usage would help.

Sure, does updating the comment to the following make it clear?

// Returns true if native platform routines can be used to draw on the          
// given canvas without any indirect rendering taking place. Indirect           
// rendering can be expensive so it's recommended to avoid using                
// platform paint if possible when direct platform paint isn't supported.       
// E.g. if there's a method to perform text rendering without platform          
// paint then it's likely much more efficient to use that method in the         
// case where this function returns false.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_device.cc
File skia/ext/platform_device.cc (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_device.c...
skia/ext/platform_device.cc:79: bool PlatformDevice::IsNativeRenderingAllowed()
{
On 2012/09/17 16:29:30, Alok Priyadarshi wrote:
> Can we get rid of IsNativeFontRenderingAllowed?

We can't get rid of it as it's used to determine if platform paint is supported.
I think it makes sense to rename it so it matches the PlatformCanvas API:

s/IsNativeFontRenderingAllowed/SupportsDirectPlatformPaint/
s/IsNativeRenderingAllowed/SupportsPlatformPaint/

I'll create a separate CL for this though as I think it should be done
independently.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
File skia/ext/platform_picture_skia.cc (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
skia/ext/platform_picture_skia.cc:17: record_->unref();
On 2012/09/17 16:29:30, Alok Priyadarshi wrote:
> can we use SkAutoRef to do this automatically?

Yea, I don't see why not.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
File skia/ext/platform_picture_skia.h (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
skia/ext/platform_picture_skia.h:18: SK_API PlatformPictureSkia();
On 2012/09/17 16:29:30, Alok Priyadarshi wrote:
> Don't we export the whole class?

yea, I'll move the export directive to the class.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
File skia/ext/recording_platform_device_skia.cc (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:51: // Copy picture and playback to
raster surface.
On 2012/09/17 16:51:24, reed1 wrote:
> Why do you need to make a copy of the picture?

Is there another way to playback a SkPicture without stopping the current
recording?

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:65: // recorded matrix operations.
On 2012/09/17 16:51:24, reed1 wrote:
> If you're done with the raster after this call, it will be more efficient to
> mark that bitmap has being "immutable" before it is recorded, otherwise the
> picture-recording code will make a deep-copy of it.

Ah, thanks for pointing this out! It would be nice to avoid a deep-copy here.

Looks like I'll have to add some PlatformDevice API to support this though. I'd
like this initial patch to not grow too large. Would it be OK that I add a TODO
here and follow up with this improvement in a separate patch?

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:67: SkCanvas* canvas =
picture_->getRecordingCanvas();
On 2012/09/17 16:51:24, reed1 wrote:
> Shouldn't we "reset" the recording at this point, since the bitmap, by
> definition, contains all of the necessary data for the drawing up to this
point?

We need to maintain all the state that's been set by previous commands. Also any
kind of absolute matrix modification here would break the typical per-tile
painting usage pattern of SkPicture where we playback with a different initial
X/Y translation.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:77: // PlatformCanvas matches begin
and end calls.
On 2012/09/17 16:51:24, reed1 wrote:
> 1. not sure what that comment means.

hm, I copied that from vector_platform_device_skia. I don't think the comment is
needed. I assumed it was supposed to mean that this is the PlatformCanvas call
that matches our previous BeginPlatformPaint call.

> 2. who deletes the canvas that raster_canvas_ points to?

It's a SkRefPtr so my assumption was that this is done automatically when all
references are dropped.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:79: raster_canvas_ = NULL;
On 2012/09/17 16:51:24, reed1 wrote:
> At this point, perhaps we should just covert over to a raster from now own.
Why
> have a raster + more picture opts, when we could just become a raster?

The current patch is creating temporary bitmaps the size of the device but
that's just a detail of the current implementation that I intend to improve soon
after this lands as that's not really an option when recording full pages.

My current plan is to limit the size of the temporary bitmap to the current
clip. Seems like that would work well as we already propagate the clip to
platform device.

[nduca, 2012-09-18 09:54:23]

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_canvas.h...
skia/ext/platform_canvas.h:124: // given canvas without any indirect rendering
taking place.
On 2012/09/17 21:22:16, David Reveman wrote:
> On 2012/09/17 16:51:24, reed1 wrote:
> > On 2012/09/17 16:29:30, Alok Priyadarshi wrote:
> > > I am not quite clear on the differences between these two functions. May
be
> a
> > > comment about usage would help.
> > 
> > +1 -- not sure the diff between these two functions.
> 
> On 2012/09/17 16:29:30, Alok Priyadarshi wrote:
> > I am not quite clear on the differences between these two functions. May be
a
> > comment about usage would help.
> 
> Sure, does updating the comment to the following make it clear?
> 
> // Returns true if native platform routines can be used to draw on the        
 
> // given canvas without any indirect rendering taking place. Indirect         
 
> // rendering can be expensive so it's recommended to avoid using              
 
> // platform paint if possible when direct platform paint isn't supported.     
 
> // E.g. if there's a method to perform text rendering without platform        
 
> // paint then it's likely much more efficient to use that method in the       
 
> // case where this function returns false.

Done.

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_canvas.h...
skia/ext/platform_canvas.h:124: // given canvas without any indirect rendering
taking place.
Would a better tweak for this be to change SupportsPlatformPaint to be more like
a capabilities query that returns an enum? PlatformPaintIndirect  |
PlatformPaintDirect | Unsupported?

[nduca, 2012-09-18 09:58:22]

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
File skia/ext/platform_picture_skia.cc (right):

http://codereview.chromium.org/10915065/diff/10003/skia/ext/platform_picture_...
skia/ext/platform_picture_skia.cc:22: SkCanvas*
PlatformPictureSkia::beginRecording(int width, int height) {
alokp/reveman, does the microbenchmark api needs to be adjusted to do the same
thing here?

[reveman, 2012-09-18 17:56:15]

https://chromiumcodereview.appspot.com/10915065/diff/10003/skia/ext/platform_...
File skia/ext/platform_canvas.h (right):

https://chromiumcodereview.appspot.com/10915065/diff/10003/skia/ext/platform_...
skia/ext/platform_canvas.h:124: // given canvas without any indirect rendering
taking place.
On 2012/09/18 09:54:24, nduca wrote:
> Would a better tweak for this be to change SupportsPlatformPaint to be more
like
> a capabilities query that returns an enum? PlatformPaintIndirect  |
> PlatformPaintDirect | Unsupported?

I left this as SupportsPlatformPaint/SupportsDirectPlatformPaint in the latest
patch but I'd be fine changing this to:

enum PlatformPaintType {
  PlatformPaintUnsupported = 0,
  PlatformPaintIndirect,
  PlatformPaintDirect
};
PlatformPaintType PlatformPaintSupport();

if that would be preferred?

[reveman, 2012-09-18 19:45:16]

https://chromiumcodereview.appspot.com/10915065/diff/10003/skia/ext/platform_...
File skia/ext/platform_picture_skia.cc (right):

https://chromiumcodereview.appspot.com/10915065/diff/10003/skia/ext/platform_...
skia/ext/platform_picture_skia.cc:22: SkCanvas*
PlatformPictureSkia::beginRecording(int width, int height) {
On 2012/09/18 09:58:23, nduca wrote:
> alokp/reveman, does the microbenchmark api needs to be adjusted to do the same
> thing here?

Yes, we should switch to using PlatformPictureSkia there too. Do you want that
as part of this patch? I think it's OK to do that in a follow up.

[reveman, 2012-09-19 23:42:53]

Need the following questions answered to move forward:

Is SupportsPlatformPaint/SupportsDirectPlatformPaint OK or should I add an enum?

Can I follow up with a no deep-copy improvement or does that have to be in this
CL?

Is it OK that I do the microbenchmark API update in a separate CL?

[reed1, 2012-09-20 13:42:28]

I think this has two large inefficiencies. I'll leave it to other reviews and
the author to decide when those should be addressed.

1. We allocate a very large offscreen in BeginPaint, limited only by the current
clip (I think). We then make a copy of that and stuff it into the picture. This
is a performance and memory hit.

2. We might potentially record lots of these, depending on how often BeginPaint
is called before we can start recording a new picture. This feels unbounded and
risky.

https://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

https://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_device....
skia/ext/platform_device.h:128: // without indirect rendering taking place.
I honestly don't know what "without indirect rendering taking place" means in
this context. I presume that is the difference between this method and
SupportsPlatformPaint, but I don't know what it means from the comment.

https://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_picture...
File skia/ext/platform_picture_skia.h (right):

https://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_picture...
skia/ext/platform_picture_skia.h:26: SkPicture picture_;
We are desperately trying to remove SkRefPtr from our public APIs in Skia.
Please use * or some other construct here.

https://codereview.chromium.org/10915065/diff/21001/skia/ext/recording_platfo...
File skia/ext/recording_platform_device_skia.cc (right):

https://codereview.chromium.org/10915065/diff/21001/skia/ext/recording_platfo...
skia/ext/recording_platform_device_skia.cc:51: // Copy picture and playback to
raster surface.
you're right about needing to copy so we can continue to record. Lets say that
here in the comment.
// We make a copy so that our original picture_ can continue to record. Normally
if you "draw" a picture, that forces its recording to stop.

https://codereview.chromium.org/10915065/diff/21001/skia/ext/recording_platfo...
File skia/ext/recording_platform_device_skia.h (right):

https://codereview.chromium.org/10915065/diff/21001/skia/ext/recording_platfo...
skia/ext/recording_platform_device_skia.h:49: private:
No SkRefPtr please.

[reed1, 2012-09-20 13:44:35]

Regarding the size of the offscreen, I think I may have underestimated its size.
Is it really the size of the original picture? Is that the size of the entire
page, or just a tile, or what?

[alokp, 2012-09-20 15:47:51]

I am fine with updating microbenchmarks in a subsequent patch.

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_canvas.h...
skia/ext/platform_canvas.h:123: // Returns true if native platform routines can
be used to draw on the
I am still not quite clear on the differences between direct and indirect paint.
What exactly is indirect?

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_picture_...
File skia/ext/platform_picture_skia.h (right):

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_picture_...
skia/ext/platform_picture_skia.h:23: void drawPicture(SkCanvas* canvas);
I think a better name for this function would be drawInto?

[reveman, 2012-09-20 17:01:26]

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_canvas.h...
skia/ext/platform_canvas.h:123: // Returns true if native platform routines can
be used to draw on the
On 2012/09/20 15:47:51, Alok Priyadarshi wrote:
> I am still not quite clear on the differences between direct and indirect
paint.
> What exactly is indirect?

In the current implementation this means use of a temporary bitmap and the need
to playback previous commands to this bitmap. I was being a bit vague here as it
could potentially mean something else. The important part is, indirect is not as
efficient as direct.

I'll improve the comment. I guess there's no need to be this vague here.

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_device.h...
skia/ext/platform_device.h:128: // without indirect rendering taking place.
On 2012/09/20 13:42:28, reed1 wrote:
> I honestly don't know what "without indirect rendering taking place" means in
> this context. I presume that is the difference between this method and
> SupportsPlatformPaint, but I don't know what it means from the comment.

I'll improve the language here.

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_picture_...
File skia/ext/platform_picture_skia.h (right):

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_picture_...
skia/ext/platform_picture_skia.h:23: void drawPicture(SkCanvas* canvas);
On 2012/09/20 15:47:51, Alok Priyadarshi wrote:
> I think a better name for this function would be drawInto?

I agree. That's much better!

http://codereview.chromium.org/10915065/diff/21001/skia/ext/platform_picture_...
skia/ext/platform_picture_skia.h:26: SkPicture picture_;
On 2012/09/20 13:42:28, reed1 wrote:
> We are desperately trying to remove SkRefPtr from our public APIs in Skia.
> Please use * or some other construct here.

ok, np.

http://codereview.chromium.org/10915065/diff/21001/skia/ext/recording_platfor...
File skia/ext/recording_platform_device_skia.cc (right):

http://codereview.chromium.org/10915065/diff/21001/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.cc:51: // Copy picture and playback to
raster surface.
On 2012/09/20 13:42:28, reed1 wrote:
> you're right about needing to copy so we can continue to record. Lets say that
> here in the comment.
> // We make a copy so that our original picture_ can continue to record.
Normally
> if you "draw" a picture, that forces its recording to stop.

sure.

http://codereview.chromium.org/10915065/diff/21001/skia/ext/recording_platfor...
File skia/ext/recording_platform_device_skia.h (right):

http://codereview.chromium.org/10915065/diff/21001/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.h:49: private:
On 2012/09/20 13:42:28, reed1 wrote:
> No SkRefPtr please.

will fix.

[reveman, 2012-09-20 17:07:31]

On 2012/09/20 13:42:28, reed1 wrote:
> I think this has two large inefficiencies. I'll leave it to other reviews and
> the author to decide when those should be addressed.
> 
> 1. We allocate a very large offscreen in BeginPaint, limited only by the
current
> clip (I think). We then make a copy of that and stuff it into the picture.
This
> is a performance and memory hit.
> 
> 2. We might potentially record lots of these, depending on how often
BeginPaint
> is called before we can start recording a new picture. This feels unbounded
and
> risky.

Yes, the current implementation is VERY inefficient. It's important to
understand that this will only be used for windowless NPAPI plugins, which at
the moment don't render at all when SkPictures are used. This initial CL will
take us from NPAPI plugin support being broken to actually working.

I'll start working on the clipping and the "avoid deep-copy" improvements once
this lands. Together they should give us reasonably windowless NPAPI plugin
performance when using SkPicture. With flapper being used by default windowless
NPAPI plugin performance isn't as critical.

[reveman, 2012-09-20 17:13:22]

On 2012/09/20 13:44:35, reed1 wrote:
> Regarding the size of the offscreen, I think I may have underestimated its
size.
> Is it really the size of the original picture? Is that the size of the entire
> page, or just a tile, or what?

Yes, it's the size of the original picture in this first CL. Which is the bounds
for the invalidated layer area in the current version of the compositor.

[reed1, 2012-09-24 15:23:35]

Just capturing my take on our recent hangout.

1. rename support query so we're sure we've updated all current call-sites
2. clearly list in a new bug or Cl the planned optimizations (and why they are
important).
3. agree to sign my "kill npapi once a for all" petition :)

[reveman, 2012-09-24 20:40:43]

On 2012/09/24 15:23:35, reed1 wrote:
> Just capturing my take on our recent hangout.
> 
> 1. rename support query so we're sure we've updated all current call-sites

Took nduca's suggestion and made this an enum. Function name is now:
PlatformPaintSupport().

> 2. clearly list in a new bug or Cl the planned optimizations (and why they are
> important).

Created https://code.google.com/p/chromium/issues/detail?id=152011 and added
TODO that mentions planned optimizations.

> 3. agree to sign my "kill npapi once a for all" petition :)

Happily :)

[reed1, 2012-09-24 21:04:01]

https://codereview.chromium.org/10915065/diff/37001/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/10915065/diff/37001/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:117: 
So now a canvas/device will report DIRECT or INDIRECT support. Does this mean
that if we have a device that returns DIRECT (the new code written for NPAPI),
that we must always do that, even for callers like Form Controls that would be
fine with the cheaper INDIRECT?

Might it make sense for the caller to specify their requirements, so that more
efficient clients need never trigger the DIRECT path?

https://codereview.chromium.org/10915065/diff/37001/skia/ext/platform_device.h
File skia/ext/platform_device.h (right):

https://codereview.chromium.org/10915065/diff/37001/skia/ext/platform_device....
skia/ext/platform_device.h:97: class SK_API PlatformDevice {
I don't know the naming patterns for enums in Chrome, but I find these very
opaque, since the enum's name does not tell you what its scope is.

UNSUPPORTED is only scoped by PlatformDevice. Could it be named more like

PLATFORM_PAINT_UNSUPPORTED
PLATFORM_PAINT_DIRECT_SUPPORT
PLATFORM_PAINT_INDIRECT_SUPPORT

Verbose, yes. But it makes the call-sites very clear. Think of the case where a
class has several different enum groups. Suddenly UNSUPPORTED or DIRECT may need
more scope. Just a thought.

[reveman, 2012-09-24 23:09:40]

On 2012/09/24 21:04:01, reed1 wrote:
> https://codereview.chromium.org/10915065/diff/37001/skia/ext/platform_canvas.h
> File skia/ext/platform_canvas.h (right):
> 
>
https://codereview.chromium.org/10915065/diff/37001/skia/ext/platform_canvas....
> skia/ext/platform_canvas.h:117: 
> So now a canvas/device will report DIRECT or INDIRECT support. Does this mean
> that if we have a device that returns DIRECT (the new code written for NPAPI),
> that we must always do that, even for callers like Form Controls that would be
> fine with the cheaper INDIRECT?

A device is not allowed to return different values at different times and
there's no way currently to support both. A device support one or the other.
INDIRECT is not as efficient as DIRECT so if INDIRECT is returned, callers like
Form Controls will use their own indirect rendering mechanism which was
previously used when platform paint was not supported at all.

> 
> Might it make sense for the caller to specify their requirements, so that more
> efficient clients need never trigger the DIRECT path?

A device that supports DIRECT will always want to use DIRECT. There's no need to
support both at this point and doing so would require refactoring the
Begin/EndPlatformPaint API.

> 
> https://codereview.chromium.org/10915065/diff/37001/skia/ext/platform_device.h
> File skia/ext/platform_device.h (right):
> 
>
https://codereview.chromium.org/10915065/diff/37001/skia/ext/platform_device....
> skia/ext/platform_device.h:97: class SK_API PlatformDevice {
> I don't know the naming patterns for enums in Chrome, but I find these very
> opaque, since the enum's name does not tell you what its scope is.
> 
> UNSUPPORTED is only scoped by PlatformDevice. Could it be named more like
> 
> PLATFORM_PAINT_UNSUPPORTED
> PLATFORM_PAINT_DIRECT_SUPPORT
> PLATFORM_PAINT_INDIRECT_SUPPORT
> 
> Verbose, yes. But it makes the call-sites very clear. Think of the case where
a
> class has several different enum groups. Suddenly UNSUPPORTED or DIRECT may
need
> more scope. Just a thought.

I like it. I think the enum naming pattern in chromium is flexible enough to
allow this and it does make it clearer in this case.

[reed1, 2012-09-25 11:53:59]

we see do a lot of this

DCHECK(skia::PlatformPaintSupport(canvas));

but that routine now returns an enum. Seems like we should DCHECK against one or
more enum values.

https://chromiumcodereview.appspot.com/10915065/diff/52003/ui/base/native_the...
File ui/base/native_theme/native_theme_win.cc (right):

https://chromiumcodereview.appspot.com/10915065/diff/52003/ui/base/native_the...
ui/base/native_theme/native_theme_win.cc:486: DCHECK(offscreen_canvas.get());
shouldn't we DCHECK against one or more enum values here, instead of relying on
passive conversion to bool?

[reveman, 2012-09-25 12:04:13]

https://chromiumcodereview.appspot.com/10915065/diff/52003/ui/base/native_the...
File ui/base/native_theme/native_theme_win.cc (right):

https://chromiumcodereview.appspot.com/10915065/diff/52003/ui/base/native_the...
ui/base/native_theme/native_theme_win.cc:486: DCHECK(offscreen_canvas.get());
On 2012/09/25 11:53:59, reed1 wrote:
> shouldn't we DCHECK against one or more enum values here, instead of relying
on
> passive conversion to bool?

I explicitly made PLATFORM_PAINT_UNSUPPORTED = 0 so code that doesn't care about
the paint type can do this check.

I can change this to an explicit enum value check if you prefer that?

[reed1, 2012-09-25 12:05:30]

I understand why it works today, but I think that is very fragile, and not very
clear at the call-site (what are we asserting?).

[reed1, 2012-09-25 12:06:01]

lgtm after we can fixed the DCHECKs

[reveman, 2012-09-25 12:50:55]

On 2012/09/25 12:05:30, reed1 wrote:
> I understand why it works today, but I think that is very fragile, and not
very
> clear at the call-site (what are we asserting?).

Ok, changing to explicit enum checks. I'm curious though, how is this very
fragile?

[reed1, 2012-09-25 13:05:44]

fragile if a later editor reorders or inserts new enums.

[tfarina, 2012-09-25 16:43:09]

http://codereview.chromium.org/10915065/diff/39010/skia/ext/recording_platfor...
File skia/ext/recording_platform_device_skia.h (right):

http://codereview.chromium.org/10915065/diff/39010/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.h:55: SK_API SkDevice*
CreateRecordingPlatformDeviceSkia(
why not fold this into the class?

static RecordingPlatformDeviceSkia* Create(...);

This is the pattern we use most in chrome, take a look at content/ for example.

[tfarina, 2012-09-25 16:43:45]

http://codereview.chromium.org/10915065/diff/39010/skia/ext/recording_platfor...
File skia/ext/recording_platform_device_skia.h (right):

http://codereview.chromium.org/10915065/diff/39010/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.h:55: SK_API SkDevice*
CreateRecordingPlatformDeviceSkia(
On 2012/09/25 16:43:09, tfarina wrote:
> why not fold this into the class?
> 
> static RecordingPlatformDeviceSkia* Create(...);
> 
ops, static SkDevice*

[reveman, 2012-09-25 18:07:11]

http://codereview.chromium.org/10915065/diff/39010/skia/ext/recording_platfor...
File skia/ext/recording_platform_device_skia.h (right):

http://codereview.chromium.org/10915065/diff/39010/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.h:55: SK_API SkDevice*
CreateRecordingPlatformDeviceSkia(
On 2012/09/25 16:43:09, tfarina wrote:
> why not fold this into the class?
> 
> static RecordingPlatformDeviceSkia* Create(...);
> 
> This is the pattern we use most in chrome, take a look at content/ for
example.

Not sure why I didn't do that. I guess I was trying to be consistent with
PlatformCanvas but it makes more sense to be consistent with
BitmapPlatformDevice which already use the static Create() pattern.
I'll fix this. Thanks for pointing it out.

http://codereview.chromium.org/10915065/diff/39010/skia/ext/recording_platfor...
skia/ext/recording_platform_device_skia.h:55: SK_API SkDevice*
CreateRecordingPlatformDeviceSkia(
On 2012/09/25 16:43:45, tfarina wrote:
> On 2012/09/25 16:43:09, tfarina wrote:
> > why not fold this into the class?
> > 
> > static RecordingPlatformDeviceSkia* Create(...);
> > 
> ops, static SkDevice*

I think it might actually be more correct with RecordingPlatformDeviceSkia*
here. SkDevice* only makes sense if we have different platform implementations
of this class. right?

[reveman, 2012-09-25 23:36:59]

I included the the clip optimization we talked about in this CL. This is only
the RecordingPlatformDeviceSkia part. Additional changes to the plugin proxy
code are also required for this optimization to make a difference but those
changes will be done using a separate CL.

[reed1, 2012-09-26 13:30:39]

lgtm

[commit-bot: I haz the power, 2012-09-26 13:49:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/10915065/56003

[commit-bot: I haz the power, 2012-09-26 13:49:27]

Presubmit check for 10915065-56003 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    cc
    ui/gfx
    ui
    content/renderer
    webkit/plugins/npapi
    webkit/plugins/ppapi

Presubmit checks took 2.2s to calculate.

[reveman, 2012-09-26 14:13:07]

Need OWNER lgtm for s/SupportsPlatformPaint/PlatformPaintSupport/ change. No
structural changes except in skia/ext which have already been reviewed.

[Bernhard Bauer, 2012-09-26 14:22:42]

webkit/plugins/npapi LGTM.

[Alexei Svitkine (slow), 2012-09-26 14:29:32]

LGTM with a nit.

Nit: You seem to be doing a lot of:

"skia::PlatformPaintSupport(src_canvas) !=
skia::PlatformDevice::PLATFORM_PAINT_UNSUPPORTED"
and
"skia::PlatformPaintSupport(src_canvas) ==
skia::PlatformDevice::PLATFORM_PAINT_UNSUPPORTED"

I'd suggest having a helper call in the skia:: namespace that returns a bool, so
that users of the API don't have to do the comparison themselves. This should
simplify a lot of these checks.

e.g. something like:

bool skia::PlatformPaintIsSupported(canvas)

[tfarina, 2012-09-26 14:31:42]

On Wed, Sep 26, 2012 at 11:29 AM,  <asvitkine@chromium.org> wrote:
> e.g. something like:
>
> bool skia::PlatformPaintIsSupported(canvas)
>
More natural/readable would be: IsPlatformPaintSupported

-- 
Thiago

[reed1, 2012-09-26 14:40:18]

+1

and/or

IsPlatformPaintSupported(canvas, DIRECT | INDIRECT);

On Wed, Sep 26, 2012 at 10:31 AM, Thiago Farina <tfarina@chromium.org> wrote:
> On Wed, Sep 26, 2012 at 11:29 AM,  <asvitkine@chromium.org> wrote:
>> e.g. something like:
>>
>> bool skia::PlatformPaintIsSupported(canvas)
>>
> More natural/readable would be: IsPlatformPaintSupported
>
> --
> Thiago

[piman, 2012-09-26 16:05:34]

lgtm

[jamesr, 2012-09-26 17:39:10]

I think a better strategy for windowless plugins would be to give them a fully
transparent backbuffer to render into and see if that breaks anyone.  That will
give us much more flexibility going forward.  See
http://code.google.com/p/chromium/issues/detail?id=123392#c3 - is there a
specific reason you didn't want to do this?  Was there some information not
recorded in the bug?

[reveman, 2012-09-26 19:03:23]

On 2012/09/26 17:39:10, jamesr wrote:
> I think a better strategy for windowless plugins would be to give them a fully
> transparent backbuffer to render into and see if that breaks anyone.  That
will
> give us much more flexibility going forward.  See
> http://code.google.com/p/chromium/issues/detail?id=123392#c3 - is there a
> specific reason you didn't want to do this?  Was there some information not
> recorded in the bug?

Plugins assume there's no such thing as an alpha channel. The windowless plugin
API was afaik created to provide translucency without requiring an alpha
channel. You'll need to do something funky like what's described in
crbug.com/12586 to extract alpha.

Sorry for not getting this recorded in the bug. I'll update it.

[jamesr, 2012-09-26 19:14:49]

On 2012/09/26 19:03:23, David Reveman wrote:
> On 2012/09/26 17:39:10, jamesr wrote:
> > I think a better strategy for windowless plugins would be to give them a
fully
> > transparent backbuffer to render into and see if that breaks anyone.  That
> will
> > give us much more flexibility going forward.  See
> > http://code.google.com/p/chromium/issues/detail?id=123392#c3 - is there a
> > specific reason you didn't want to do this?  Was there some information not
> > recorded in the bug?
> 
> Plugins assume there's no such thing as an alpha channel. The windowless
plugin
> API was afaik created to provide translucency without requiring an alpha
> channel. You'll need to do something funky like what's described in
> crbug.com/12586 to extract alpha.
> 
> Sorry for not getting this recorded in the bug. I'll update it.

Is there any plugin (besides flash, which isn't NPAPI) for which this matters? 
Looking at http://code.google.com/p/chromium/issues/detail?id=12586 and
http://benjamin.smedbergs.us/blog/2010-11-18/asynchronous-plugin-layer-painting/
it sounds like Firefox has done something much simpler which is working fine and
they don't have flapper.

I don't think we should attempt to add this complexity unless we know of a
windowless NPAPI plugin that is broken on a page that we care about supporting. 
If we do, we need to record that information (plugin+page+bustage) so we can
revisit later.

[jamesr, 2012-09-26 19:25:42]

From https://bugzilla.mozilla.org/show_bug.cgi?id=611698 it looks like what
Firefox did is force ClearType off completely for NPAPI Flash on windows. 
Everything else was (according to them) alpha-aware enough to Just Work.  We
don't support NPAPI Flash on windows, so we should be good to go right?

[jamesr, 2012-09-26 19:28:18]

On 2012/09/26 19:25:42, jamesr wrote:
> From https://bugzilla.mozilla.org/show_bug.cgi?id=611698 it looks like what
> Firefox did is force ClearType off completely for NPAPI Flash on windows. 
> Everything else was (according to them) alpha-aware enough to Just Work.  We
> don't support NPAPI Flash on windows, so we should be good to go right?

Actually looks like they did end up somewhere more complex.  See
https://bugzilla.mozilla.org/show_bug.cgi?id=626602#c2.

So the key question is, what windowless plugin on windows will this impact?

[reveman, 2012-09-26 20:20:56]

Seems hard to know what plugins and pages this will impact until per-tile
painting is out in the wild.

Lets wait until we turn on per-tile painting by default on a platform that
supports windowless npapi plugins before landing this.

[jamesr, 2012-09-26 20:35:37]

On 2012/09/26 20:20:56, David Reveman wrote:
> Seems hard to know what plugins and pages this will impact until per-tile
> painting is out in the wild.
> 
> Lets wait until we turn on per-tile painting by default on a platform that
> supports windowless npapi plugins before landing this.

That sounds good to me.  If something does break when we try to ship per-tile
painting on windows, then we'll know what it is and have this patch to look at.

[sky, 2012-09-26 21:17:32]

https://codereview.chromium.org/10915065/diff/37048/skia/ext/platform_canvas.h
File skia/ext/platform_canvas.h (right):

https://codereview.chromium.org/10915065/diff/37048/skia/ext/platform_canvas....
skia/ext/platform_canvas.h:119: SK_API PlatformDevice::PlatformPaint
PlatformPaintSupport(
Can we have a Get in here, eg GetPlatformPaintSupport? Or just GetPlatformPaint
which matches the name of what is returned.

[Alexei Svitkine (slow), 2012-10-04 16:07:55]

Removing myself as reviewer since I'm OOO on vacation.