WIP: Adjust externally allocated memory when ImageBitmap is transferred

WIP: Adjust externally allocated memory when ImageBitmap is transferred

TODO: fill in later

BUG=705718
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

[jbroman, 2017-04-04 15:44:55]

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

[jbroman, 2017-04-04 15:44:56]

a few comments by inspection, but I haven't debugged

https://codereview.chromium.org/2796103002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp
(right):

https://codereview.chromium.org/2796103002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/SerializedScriptValue.cpp:455:
m_transferablesNeedExternalAllocationRegistration = true;
This would need to be fused with the one above. Otherwise an array buffer being
transferred will prevent image bitmaps from being properly transferred.

https://codereview.chromium.org/2796103002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2796103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:986:
m_image->unregisterExternalAllocationWithCurrentContext();
Is this balanced by a corresponding increase somewhere in the non-transfer case?

https://codereview.chromium.org/2796103002/diff/1/third_party/WebKit/Source/p...
File
third_party/WebKit/Source/platform/graphics/UnacceleratedStaticBitmapImage.cpp
(right):

https://codereview.chromium.org/2796103002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/UnacceleratedStaticBitmapImage.cpp:51:
int externallyAllocatedMemory = m_image->width() * m_image->height() * 4;
probably not your problem, but beware overflow in this calculation

[Justin Novosad, 2017-04-04 16:15:14]

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

[Justin Novosad, 2017-04-04 16:15:15]

https://codereview.chromium.org/2796103002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/ImageBitmap.cpp (right):

https://codereview.chromium.org/2796103002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/ImageBitmap.cpp:986:
m_image->unregisterExternalAllocationWithCurrentContext();
On 2017/04/04 15:44:56, jbroman wrote:
> Is this balanced by a corresponding increase somewhere in the non-transfer
case?

I think It would be cleaner to take care of register/unregister in
StaticBitmapImage constructor/destructor.

https://codereview.chromium.org/2796103002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h (right):

https://codereview.chromium.org/2796103002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/graphics/StaticBitmapImage.h:43: virtual void
unregisterExternalAllocationWithCurrentContext() {}
Would be better to register/unregister right here in StaticBitmapImage instead
of delegating to subclass via virtual.  Rationale: AcceleratedStaticBitmapImage
should also trigger GCs,  even though it does not technically consume RAM, GPU
memory is still a precious resource that needs to be freed.

[xidachen, 2017-05-05 15:31:23]

Description was changed from

==========
WIP: Adjust externally allocated memory when ImageBitmap is transferred

TODO: fill in later

BUG=705718
==========

to

==========
WIP: Adjust externally allocated memory when ImageBitmap is transferred

TODO: fill in later

BUG=705718
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[xidachen, 2017-05-05 15:40:23]

Description was changed from

==========
WIP: Adjust externally allocated memory when ImageBitmap is transferred

TODO: fill in later

BUG=705718
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
WIP: Adjust externally allocated memory when ImageBitmap is transferred

TODO: fill in later

BUG=705718
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[Justin Novosad, 2017-05-09 15:20:07]

@xidachen: ping!

[xidachen, 2017-05-09 15:28:13]

On 2017/05/09 15:20:07, Justin Novosad wrote:
> @xidachen: ping!

Without this CL, the bug is already fixed, which means that in
SerializedScriptValue we are already unregister the externally allocated amount
for ImageBitmap case.

[xidachen, 2017-05-09 15:30:35]

On 2017/05/09 15:28:13, xidachen wrote:
> On 2017/05/09 15:20:07, Justin Novosad wrote:
> > @xidachen: ping!
> 
> Without this CL, the bug is already fixed, which means that in
> SerializedScriptValue we are already unregister the externally allocated
amount
> for ImageBitmap case.

Sorry that I forgot to mention it in this CL, I commented on the bug that the
bug is already fixed with the current status.

[Justin Novosad, 2017-05-09 15:30:47]

Right. Okay, sorry.