Cache image data objects.

Cache image data objects.

This will re-use ImageData and mapped memory when you do ReplaceContents within a few seconds of doing a previous one. Hopefully this will speed up 2D painting.

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

[brettw, 2012-08-21 20:56:09]

I did not do the resource ID remapping. I started doing this and was not happy
with the code. It is kind of a complicated bolt-on thing that feels out of place
and fragile (there are 3 places that need to be updated). I'm also concerned
about adding more complexity if this is going to be merged to 22.

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/examples/2d/p...
File ppapi/examples/2d/paint_manager_example.cc (right):

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/examples/2d/p...
ppapi/examples/2d/paint_manager_example.cc:82: pp::Rect
paint_bounds(paint_manager_.GetEffectiveSize());
Ignore the changes in this file, this is for testing.

[brettw, 2012-08-21 21:18:04]

Hi Yuzhu,

Can you look at this? Trung says he's busy. I'm going to write a test but
thought I would send this first since it's on the complicated side.

[yzshen, 2012-08-21 21:23:46]

Sure. Working on it.

On Tue, Aug 21, 2012 at 2:18 PM, <brettw@chromium.org> wrote:

> Hi Yuzhu,
>
> Can you look at this? Trung says he's busy. I'm going to write a test but
> thought I would send this first since it's on the complicated side.
>
>
http://codereview.chromium.**org/10823378/<http://codereview.chromium.org/108...
>



-- 
Best regards,
Yuzhu Shen.

[yzshen1, 2012-08-21 22:07:24]

Cool! Thanks!

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/proxy/ppapi_m...
File ppapi/proxy/ppapi_messages.h (right):

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/proxy/ppapi_m...
ppapi/proxy/ppapi_messages.h:438:
IPC_MESSAGE_ROUTED1(PpapiPluginMsg_PPBImageData_NotifyUnusedImageData,
nit: most of the messages sent to the ppapi process are called PpapiMsg_XXX.

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/proxy/ppb_ima...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/proxy/ppb_ima...
ppapi/proxy/ppb_image_data_proxy.cc:63: //    is as usable.
is -> it.

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/proxy/ppb_ima...
ppapi/proxy/ppb_image_data_proxy.cc:93: // plugin has to be painting at at least
two frames for this time interfacl to
interfacl -> interval

at at -> at

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/proxy/ppb_ima...
ppapi/proxy/ppb_image_data_proxy.cc:227: base::WeakPtrFactory<ImageDataCache>
weak_factory_;
Why does it need to be a weak ptr factory?

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/proxy/ppb_ima...
ppapi/proxy/ppb_image_data_proxy.cc:583: return;  // This may happen if the
plugin is malixiously sending this
malixiously -> maliciously

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/thunk/ppb_gra...
File ppapi/thunk/ppb_graphics_2d_api.h (right):

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/thunk/ppb_gra...
ppapi/thunk/ppb_graphics_2d_api.h:33: // When |*old_image_data| is non-null and
the flush is executing a replace
nit: |*old_image_data| looks like "deref old_image_data", so the first and the
third |*old_image_data| are better changed to |old_image_data|.

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/thunk/ppb_gra...
ppapi/thunk/ppb_graphics_2d_api.h:40: PP_Resource* old_image_data) = 0;
Please comment that |old_image_data| is used 'synchronously'. It can be freed as
soon as this method returns.

https://chromiumcodereview.appspot.com/10823378/diff/3012/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right):

https://chromiumcodereview.appspot.com/10823378/diff/3012/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:350: old_image_data);
There might be more than one ReplaceContents(). If that is the case, we are
leaking resource ref here.

[brettw, 2012-08-21 23:24:58]

New upload with test, PTAL.

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/proxy/ppb_ima...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://chromiumcodereview.appspot.com/10823378/diff/3012/ppapi/proxy/ppb_ima...
ppapi/proxy/ppb_image_data_proxy.cc:227: base::WeakPtrFactory<ImageDataCache>
weak_factory_;
I added a comment

https://chromiumcodereview.appspot.com/10823378/diff/3012/webkit/plugins/ppap...
File webkit/plugins/ppapi/ppb_graphics_2d_impl.cc (right):

https://chromiumcodereview.appspot.com/10823378/diff/3012/webkit/plugins/ppap...
webkit/plugins/ppapi/ppb_graphics_2d_impl.cc:350: old_image_data);
Good catch!

[yzshen1, 2012-08-21 23:42:44]

LGTM

Thanks!

https://chromiumcodereview.appspot.com/10823378/diff/6018/ppapi/proxy/ppb_ima...
File ppapi/proxy/ppb_image_data_proxy.cc (right):

https://chromiumcodereview.appspot.com/10823378/diff/6018/ppapi/proxy/ppb_ima...
ppapi/proxy/ppb_image_data_proxy.cc:93: // plugin has to be painting at least
two frames for this time interfacl to
interfacl -> interval. :)

https://chromiumcodereview.appspot.com/10823378/diff/6018/ppapi/tests/test_gr...
File ppapi/tests/test_graphics_2d.cc (right):

https://chromiumcodereview.appspot.com/10823378/diff/6018/ppapi/tests/test_gr...
ppapi/tests/test_graphics_2d.cc:702: // re-used objects. Howevever, our current
implementation does this so its
its->it is

https://chromiumcodereview.appspot.com/10823378/diff/6018/ppapi/tests/test_gr...
ppapi/tests/test_graphics_2d.cc:715: // Do two replace contentses, adding the
shared memory pointers to our map.
please update the comment: we are not adding shared memory pointers.