Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/b164441d329b1260970621adbe9046ee79917aea
Cr-Commit-Position: refs/heads/master@{#413555}

[danakj, 2016-08-19 03:24:48]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

Didn't implement readback for software WebGL (swiftshader yet.. trying
bots).

R=kbr@chromium.org, piman, xidachen
BUG=
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

Didn't implement readback for software WebGL (swiftshader yet.. trying
bots).

R=kbr@chromium.org, piman, xidachen
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[danakj, 2016-08-19 03:24:49]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

Didn't implement readback for software WebGL (swiftshader yet.. trying
bots).

R=kbr@chromium.org, piman, xidachen
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

Didn't implement readback for software WebGL (swiftshader yet.. trying
bots).

R=kbr@chromium.org, piman, xidachen
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-08-19 03:24:52]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 03:25:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 03:38:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 03:38:59]

Dry run: Try jobs failed on following builders:
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)

[danakj, 2016-08-19 18:19:01]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

Didn't implement readback for software WebGL (swiftshader yet.. trying
bots).

R=kbr@chromium.org, piman, xidachen
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

Didn't implement readback for software WebGL (swiftshader yet.. trying
bots).

R=kbr@chromium.org, piman, xidachen, junov
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-08-19 18:19:02]

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

[danakj, 2016-08-19 18:20:42]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 18:21:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 18:24:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 18:24:29]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[danakj, 2016-08-19 18:33:30]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 18:34:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-19 18:57:26]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 18:57:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-19 19:43:16]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2016-08-19 19:43:20]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

Didn't implement readback for software WebGL (swiftshader yet.. trying
bots).

R=kbr@chromium.org, piman, xidachen, junov
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

Didn't implement readback for software WebGL (swiftshader yet.. trying
bots).

R=kbr@chromium.org, piman, xidachen, junov, pdr
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-08-19 19:43:20]

danakj@chromium.org changed reviewers:
+ pdr@google.com

[danakj, 2016-08-19 19:43:40]

danakj@chromium.org changed reviewers:
+ pdr@chromium.org
- pdr@google.com

[danakj, 2016-08-19 19:43:53]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

Didn't implement readback for software WebGL (swiftshader yet.. trying
bots).

R=kbr@chromium.org, piman, xidachen, junov, pdr
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

R=kbr@chromium.org, piman, xidachen, junov, pdr
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-08-19 19:44:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danakj, 2016-08-19 19:46:06]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

R=kbr@chromium.org, piman, xidachen, junov, pdr
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr@chromium.org, piman, xidachen, junov, pdr
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-08-19 19:46:54]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr@chromium.org, piman, xidachen, junov, pdr
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-08-19 19:46:55]

danakj@chromium.org changed reviewers:
+ tkent@chromium.org

[danakj, 2016-08-19 19:49:12]

Hi! May I please get a review of the following?

xidachen: The most interesting bits are in code you wrote pls have a look.

OWNERS:
kbr: platform/graphics/gpu/ modules/webgl/ (and platform/graphics/ in general)
junov: platform/graphics/
tkent: wtf/ and components/test_runner/
piman: cc/ and content/

[danakj, 2016-08-19 19:49:25]

Oh, and

pdr: public/

[danakj, 2016-08-19 19:51:03]

https://codereview.chromium.org/2261623002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp
(left):

https://codereview.chromium.org/2261623002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/webgl/WebGLRenderingContextBase.cpp:649:
WebExternalTextureMailbox mailbox;
This code moved into DrawingBuffer because using cc types from modules/ isn't
allowed and DrawingBuffer::PrepareTextureMailbox uses them.

[danakj, 2016-08-19 19:54:30]

https://codereview.chromium.org/2261623002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2261623002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:288: if
(useSharedMemory) {
I verified the hardware paths but AFAICT there's no tests for swiftshader, so
I'm not sure how to really verify this code works other than it compiles and I
mostly just copied it from WebExternalTextureLayerImpl. Any advice on what to do
to verify this would be welcome.

https://codereview.chromium.org/2261623002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:310: return
true;
Note that the old code would do the software readback then also make a GPU
mailbox, which would not be used by WebExternalTextureLayerImpl other than to
pass it back to the mailboxReleased method. Now we return here.

[pdr., 2016-08-19 20:00:05]

On 2016/08/19 at 19:49:25, danakj wrote:
> Oh, and
> 
> pdr: public/

LGTM for public/, very straightforward.

(I did not review the rest of the patch)

[commit-bot: I haz the power, 2016-08-19 20:41:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 20:41:35]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[danakj, 2016-08-19 20:57:30]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel,master.tryserver.chromium.android:android_optional_gpu_tests_rel,master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[danakj, 2016-08-19 20:59:59]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 21:00:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 21:00:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 21:00:36]

Dry run: Your CL can not be processed by CQ because of:
 * Failed to parse additional trybots

[danakj, 2016-08-19 21:01:38]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel,master.tryserver.chromium.android:android_optional_gpu_tests_rel,master.tryserver.chromium.linux:linux_optional_gpu_tests_rel,master.tryserver.chromium.mac:mac_optional_gpu_tests_rel,master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
==========

[danakj, 2016-08-19 21:01:44]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2016-08-19 21:01:53]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-08-19 21:02:25]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 21:03:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-19 21:17:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-19 21:17:57]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)

[danakj, 2016-08-19 21:35:47]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[danakj, 2016-08-19 21:35:48]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[danakj, 2016-08-19 21:35:52]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[danakj, 2016-08-19 21:36:00]

The CQ bit was unchecked by danakj@chromium.org

[danakj, 2016-08-19 21:36:29]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 21:37:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Justin Novosad, 2016-08-19 21:51:19]

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp
(right):

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:98:
return fromSkSp(SkImage::MakeFromAdoptedTexture(sharedGrContext,
backendTexture));
Would it be useful to set m_image to this newly created SkImage, so that we can
set m_imageIsForSharedMainThreadContext to true? That way, next time this method
is called, we would not have to redo the mailbox dance.

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h
(right):

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:29:
static PassRefPtr<AcceleratedStaticBitmapImage> create(PassRefPtr<SkImage>,
PassRefPtr<GrContext>, const gpu::Mailbox&, const gpu::SyncToken&);
If it's not too inconvenient, in new code we should use sk_sp instead of RefPtr
for skia objects.

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:52:
RefPtr<GrContext> m_grContext;
What are the use case where taking a ref here is necessary?

[danakj, 2016-08-19 21:57:57]

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp
(right):

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:98:
return fromSkSp(SkImage::MakeFromAdoptedTexture(sharedGrContext,
backendTexture));
On 2016/08/19 21:51:18, Justin Novosad wrote:
> Would it be useful to set m_image to this newly created SkImage, so that we
can
> set m_imageIsForSharedMainThreadContext to true? That way, next time this
method
> is called, we would not have to redo the mailbox dance.

Hm ya ok we could do that, and drop the ref on the GrContext then. I don't think
any code cares about the old context anymore.

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h
(right):

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:29:
static PassRefPtr<AcceleratedStaticBitmapImage> create(PassRefPtr<SkImage>,
PassRefPtr<GrContext>, const gpu::Mailbox&, const gpu::SyncToken&);
On 2016/08/19 21:51:18, Justin Novosad wrote:
> If it's not too inconvenient, in new code we should use sk_sp instead of
RefPtr
> for skia objects.

Oh I wasn't sure and went out of my way to RefPtr. Thanks.

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:52:
RefPtr<GrContext> m_grContext;
On 2016/08/19 21:51:18, Justin Novosad wrote:
> What are the use case where taking a ref here is necessary?

When the m_image is from the non-main-thread-shared-context (the webgl context
in this patch). If that context is destroyed, the m_image is no longer valid.
This makes the m_image be valid as long as the AcceleratedStaticBitmapImage is
alive.

[danakj, 2016-08-19 22:06:21]

PTAL

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp
(right):

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:98:
return fromSkSp(SkImage::MakeFromAdoptedTexture(sharedGrContext,
backendTexture));
On 2016/08/19 21:57:57, danakj wrote:
> On 2016/08/19 21:51:18, Justin Novosad wrote:
> > Would it be useful to set m_image to this newly created SkImage, so that we
> can
> > set m_imageIsForSharedMainThreadContext to true? That way, next time this
> method
> > is called, we would not have to redo the mailbox dance.
> 
> Hm ya ok we could do that, and drop the ref on the GrContext then. I don't
think
> any code cares about the old context anymore.

Done.

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h
(right):

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:29:
static PassRefPtr<AcceleratedStaticBitmapImage> create(PassRefPtr<SkImage>,
PassRefPtr<GrContext>, const gpu::Mailbox&, const gpu::SyncToken&);
On 2016/08/19 21:57:57, danakj wrote:
> On 2016/08/19 21:51:18, Justin Novosad wrote:
> > If it's not too inconvenient, in new code we should use sk_sp instead of
> RefPtr
> > for skia objects.
> 
> Oh I wasn't sure and went out of my way to RefPtr. Thanks.

Done.

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.h:52:
RefPtr<GrContext> m_grContext;
On 2016/08/19 21:57:57, danakj wrote:
> On 2016/08/19 21:51:18, Justin Novosad wrote:
> > What are the use case where taking a ref here is necessary?
> 
> When the m_image is from the non-main-thread-shared-context (the webgl context
> in this patch). If that context is destroyed, the m_image is no longer valid.
> This makes the m_image be valid as long as the AcceleratedStaticBitmapImage is
> alive.

Done.

[danakj, 2016-08-19 22:06:45]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 22:07:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-08-19 22:30:21]

LGTM overall, one question.

https://codereview.chromium.org/2261623002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2261623002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:423: // buffer
(which was returned from PrepareTextureMailbox()).
Why do we do this dance? The texture was created on this context, right? We
already did insert a sync token and flushed this context, why do it again?

[danakj, 2016-08-19 22:37:18]

https://codereview.chromium.org/2261623002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2261623002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:423: // buffer
(which was returned from PrepareTextureMailbox()).
On 2016/08/19 22:30:21, piman wrote:
> Why do we do this dance? The texture was created on this context, right? We
> already did insert a sync token and flushed this context, why do it again?

Since this function is acting on a mailbox only it needs to get a texture id.
The function used to be outside of DrawingBuffer, now it could just use a
texture id instead of making a mailbox at all, but I left it for now creating a
new texture id from the mailbox. So I'm doing a wait (for the mailbox produce),
a consume, and then a gensynctoken (for the mailbox consume) here. I think I'd
need to do some more surgery to PrepareTextureMailbox to avoid this, or am I
missing your point? I will leave a TODO about this though.

[danakj, 2016-08-19 22:43:03]

https://codereview.chromium.org/2261623002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2261623002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:423: // buffer
(which was returned from PrepareTextureMailbox()).
On 2016/08/19 22:37:17, danakj wrote:
> On 2016/08/19 22:30:21, piman wrote:
> > Why do we do this dance? The texture was created on this context, right? We
> > already did insert a sync token and flushed this context, why do it again?
> 
> Since this function is acting on a mailbox only it needs to get a texture id.
> The function used to be outside of DrawingBuffer, now it could just use a
> texture id instead of making a mailbox at all, but I left it for now creating
a
> new texture id from the mailbox. So I'm doing a wait (for the mailbox
produce),
> a consume, and then a gensynctoken (for the mailbox consume) here. I think I'd
> need to do some more surgery to PrepareTextureMailbox to avoid this, or am I
> missing your point? I will leave a TODO about this though.

Oh or are you saying I could do the consume but not wait on the token from
produce or gen a new one. That makes sense, since same context.

[danakj, 2016-08-19 22:57:53]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-19 22:58:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[piman, 2016-08-19 23:11:35]

lgtm

https://codereview.chromium.org/2261623002/diff/120001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2261623002/diff/120001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:423: // buffer
(which was returned from PrepareTextureMailbox()).
On 2016/08/19 22:43:03, danakj wrote:
> On 2016/08/19 22:37:17, danakj wrote:
> > On 2016/08/19 22:30:21, piman wrote:
> > > Why do we do this dance? The texture was created on this context, right?
We
> > > already did insert a sync token and flushed this context, why do it again?
> > 
> > Since this function is acting on a mailbox only it needs to get a texture
id.
> > The function used to be outside of DrawingBuffer, now it could just use a
> > texture id instead of making a mailbox at all, but I left it for now
creating
> a
> > new texture id from the mailbox. So I'm doing a wait (for the mailbox
> produce),
> > a consume, and then a gensynctoken (for the mailbox consume) here. I think
I'd
> > need to do some more surgery to PrepareTextureMailbox to avoid this, or am I
> > missing your point? I will leave a TODO about this though.
> 
> Oh or are you saying I could do the consume but not wait on the token from
> produce or gen a new one. That makes sense, since same context.

It's super subtle, but this works I guess - provided the releaseCallback is run
after the CreateAndConsumeTextureCHROMIUM, then the mailbox refers to something,
and I guess you're not doing any modification to the texture itself, so it
doesn't matter if the other context runs before or after the
CreateAndConsumeTextureCHROMIUM? 
If before, there is the reference from the original texture, if after there is
(at least) the reference from the new texture

What I was thinking originally was to steal the original texture id from
DrawingBuffer, and avoid both sync token and mailbox operations.

[commit-bot: I haz the power, 2016-08-20 00:18:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-20 00:18:22]

Dry run: Try jobs failed on following builders:
  linux_precise_blink_rel on master.tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_precise_blink_rel/...)

[danakj, 2016-08-20 01:13:30]

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp
(right):

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:98:
return fromSkSp(SkImage::MakeFromAdoptedTexture(sharedGrContext,
backendTexture));
On 2016/08/19 21:51:18, Justin Novosad wrote:
> Would it be useful to set m_image to this newly created SkImage, so that we
can
> set m_imageIsForSharedMainThreadContext to true? That way, next time this
method
> is called, we would not have to redo the mailbox dance.

It looks like doing this causes crashes.

STDERR: Received signal 11 SEGV_MAPERR 000000000000
STDERR: #0 0x7fa8c6dbd4b7 base::debug::(anonymous
namespace)::StackDumpSignalHandler()
STDERR: #1 0x7fa8c87f0330 <unknown>
STDERR: #2 0x7fa8c67ea021 SkImage::isTextureBacked()
STDERR: #3 0x7fa8c1980fea
blink::ImageBitmapRenderingContext::transferFromImageBitmap()
STDERR: #4 0x7fa8c508d2ac v8::internal::FunctionCallbackArguments::Call()
STDERR: #5 0x7fa8c512771c v8::internal::(anonymous
namespace)::HandleApiCallHelper<>()
STDERR: #6 0x7fa8c51265d5 v8::internal::Builtin_Impl_HandleApiCall()
STDERR: #7 0x7fa8c51262de v8::internal::Builtin_HandleApiCall()
STDERR: #8 0x2a19dc2063a7 <unknown>

Patch sets before that have all green bots tho.

[danakj, 2016-08-20 01:26:18]

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp
(right):

https://codereview.chromium.org/2261623002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/AcceleratedStaticBitmapImage.cpp:98:
return fromSkSp(SkImage::MakeFromAdoptedTexture(sharedGrContext,
backendTexture));
On 2016/08/20 01:13:30, danakj wrote:
> On 2016/08/19 21:51:18, Justin Novosad wrote:
> > Would it be useful to set m_image to this newly created SkImage, so that we
> can
> > set m_imageIsForSharedMainThreadContext to true? That way, next time this
> method
> > is called, we would not have to redo the mailbox dance.
> 
> It looks like doing this causes crashes.
> 
> STDERR: Received signal 11 SEGV_MAPERR 000000000000
> STDERR: #0 0x7fa8c6dbd4b7 base::debug::(anonymous
> namespace)::StackDumpSignalHandler()
> STDERR: #1 0x7fa8c87f0330 <unknown>
> STDERR: #2 0x7fa8c67ea021 SkImage::isTextureBacked()
> STDERR: #3 0x7fa8c1980fea
> blink::ImageBitmapRenderingContext::transferFromImageBitmap()
> STDERR: #4 0x7fa8c508d2ac v8::internal::FunctionCallbackArguments::Call()
> STDERR: #5 0x7fa8c512771c v8::internal::(anonymous
> namespace)::HandleApiCallHelper<>()
> STDERR: #6 0x7fa8c51265d5 v8::internal::Builtin_Impl_HandleApiCall()
> STDERR: #7 0x7fa8c51262de v8::internal::Builtin_HandleApiCall()
> STDERR: #8 0x2a19dc2063a7 <unknown>
> 
> Patch sets before that have all green bots tho.

Ah this was me holding PassRefPtr wrong, you can't set 2 diff RefPtrs with one
PassRefPtr. Fixed.

[danakj, 2016-08-20 01:27:05]

The CQ bit was checked by danakj@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-20 01:27:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-20 03:33:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-20 03:33:07]

Dry run: This issue passed the CQ dry run.

[Ken Russell (switch to Gerrit), 2016-08-20 03:54:01]

Thanks very much Dana for working on cleaning up this code and in particular for
thinking through the lifetime semantics so carefully. Had a few thoughts upon
revisiting the code.

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:251: --it; //
Removed this position so iterate on it again.
This won't assert if it was the first element that was removed, will it?

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:414: // The
only situation this could happen is when two or more calls to
transferToImageBitmap are made back-to-back, or when the context gets lost.
The intent of transferToStaticBitmapImage is that it takes the DrawingBuffer's
most recent rendering result and transfers it into a StaticBitmapImage -- after
it's called, the DrawingBuffer's contents are lost. This is what implements
OffscreenCanvas' transferToImageBitmap method:
https://wiki.whatwg.org/wiki/OffscreenCanvas .

I don't see where in PrepareTextureMailbox it can return false for the second
call if they're made back-to-back, before any further rendering is done to the
DrawingBuffer so it has a chance to reallocate its back buffer. Is that still
working as expected?

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:425: // avoid
this whole dance of wait for sync token, consume, generate new sync token.
It's possible this wasn't implemented as desired before -- but the intent really
is to give the sole reference to the DrawingBuffer's current texture away. Even
the compositor shouldn't be able to get a handle to it. Again, this is a
primitive only accessible to OffscreenCanvas, which can't be displayed in the
DOM in the first place, and can't be rendered directly by the compositor. One
intended way to display OffscreenCanvas' results is in an
ImageBitmapRenderingContext, importing them via transferFromImageBitmap.

Does this change or simplify any of the code below? Can the DrawingBuffer
immediately drop its reference to the texture once it's been put in the mailbox
and given to the caller in the form of an AcceleratedStaticBitmapImage?

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h (right):

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.h:219: // Returns
a StaticBitmapImage backed by a texture containing the/ current contents of
typo: the/

[tkent, 2016-08-21 23:13:02]

Updating wtf/DequeTest.cpp in this CL looks weird a little bit because it is not
related to the purpose of this CL.  It can be a separated CL.
Anyway, components/test_runner/ and wtf lgtm.

[danakj, 2016-08-22 17:56:17]

On 2016/08/21 23:13:02, tkent wrote:
> Updating wtf/DequeTest.cpp in this CL looks weird a little bit because it is
not
> related to the purpose of this CL.  It can be a separated CL.

It's because this CL uses that behaviour and there's no documentation or tests
to show that it is valid before.

> Anyway, components/test_runner/ and wtf lgtm.

Thanks :)

[danakj, 2016-08-22 18:45:25]

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:251: --it; //
Removed this position so iterate on it again.
On 2016/08/20 03:54:01, Ken Russell wrote:
> This won't assert if it was the first element that was removed, will it?

The DequeTest I added deletes element 0 and passes. It's because the iterator
uses an unsigned index:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/Deque.h?rc...

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:414: // The
only situation this could happen is when two or more calls to
transferToImageBitmap are made back-to-back, or when the context gets lost.
On 2016/08/20 03:54:01, Ken Russell wrote:
> The intent of transferToStaticBitmapImage is that it takes the DrawingBuffer's
> most recent rendering result and transfers it into a StaticBitmapImage --
after
> it's called, the DrawingBuffer's contents are lost. This is what implements
> OffscreenCanvas' transferToImageBitmap method:
> https://wiki.whatwg.org/wiki/OffscreenCanvas .
> 
> I don't see where in PrepareTextureMailbox it can return false for the second
> call if they're made back-to-back, before any further rendering is done to the
> DrawingBuffer so it has a chance to reallocate its back buffer. Is that still
> working as expected?

So I think how this works is if |m_preserveDrawingBuffer| is |Discard|, then
PrepareTextureMailbox will throw the last recycled mailbox into the
m_colorBuffer spot.

Since transferToStaticBitmapImage always "loses" the mailboxes its given (since
it wants to retain ownership forever), they do not get recycled. So as long as
the compositor isn't also calling PrepareTextureMailbox and returning them
unlost, each call to the method makes a new texture via
createTextureAndAllocateMemory(m_size), gets a mailbox for it via
createNewMailbox(), and makes that the new m_colorBuffer. So a 2nd call would
give you a black texture.

[danakj, 2016-08-22 19:01:04]

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:425: // avoid
this whole dance of wait for sync token, consume, generate new sync token.
On 2016/08/20 03:54:01, Ken Russell wrote:
> It's possible this wasn't implemented as desired before -- but the intent
really
> is to give the sole reference to the DrawingBuffer's current texture away.
Even
> the compositor shouldn't be able to get a handle to it. Again, this is a
> primitive only accessible to OffscreenCanvas, which can't be displayed in the
> DOM in the first place, and can't be rendered directly by the compositor. One
> intended way to display OffscreenCanvas' results is in an
> ImageBitmapRenderingContext, importing them via transferFromImageBitmap.
> 
> Does this change or simplify any of the code below? Can the DrawingBuffer
> immediately drop its reference to the texture once it's been put in the
mailbox
> and given to the caller in the form of an AcceleratedStaticBitmapImage?

I think this is what is happening. We PrepareTextureMailbox, and discard the
m_colorBuffer (replace it with a new texture). Then we call
releaseCallback->Run() with lostResource=true which means the DrawingBuffer can
not reuse it, and it deletes its client id.

I think this can be simpler if we just dont deal with PrepareTextureMailbox tho,
which is what TODO on 424 is trying to get at.

[Ken Russell (switch to Gerrit), 2016-08-22 19:03:11]

Thanks again for cleaning this up and for our offline discussion. LGTM

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp (right):

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:414: // The
only situation this could happen is when two or more calls to
transferToImageBitmap are made back-to-back, or when the context gets lost.
On 2016/08/22 18:45:24, danakj wrote:
> On 2016/08/20 03:54:01, Ken Russell wrote:
> > The intent of transferToStaticBitmapImage is that it takes the
DrawingBuffer's
> > most recent rendering result and transfers it into a StaticBitmapImage --
> after
> > it's called, the DrawingBuffer's contents are lost. This is what implements
> > OffscreenCanvas' transferToImageBitmap method:
> > https://wiki.whatwg.org/wiki/OffscreenCanvas .
> > 
> > I don't see where in PrepareTextureMailbox it can return false for the
second
> > call if they're made back-to-back, before any further rendering is done to
the
> > DrawingBuffer so it has a chance to reallocate its back buffer. Is that
still
> > working as expected?
> 
> So I think how this works is if |m_preserveDrawingBuffer| is |Discard|, then
> PrepareTextureMailbox will throw the last recycled mailbox into the
> m_colorBuffer spot.
> 
> Since transferToStaticBitmapImage always "loses" the mailboxes its given
(since
> it wants to retain ownership forever), they do not get recycled. So as long as
> the compositor isn't also calling PrepareTextureMailbox and returning them
> unlost, each call to the method makes a new texture via
> createTextureAndAllocateMemory(m_size), gets a mailbox for it via
> createNewMailbox(), and makes that the new m_colorBuffer. So a 2nd call would
> give you a black texture.

Thanks for this explanation - sounds good.

https://codereview.chromium.org/2261623002/diff/150001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/gpu/DrawingBuffer.cpp:425: // avoid
this whole dance of wait for sync token, consume, generate new sync token.
On 2016/08/22 19:01:04, danakj wrote:
> On 2016/08/20 03:54:01, Ken Russell wrote:
> > It's possible this wasn't implemented as desired before -- but the intent
> really
> > is to give the sole reference to the DrawingBuffer's current texture away.
> Even
> > the compositor shouldn't be able to get a handle to it. Again, this is a
> > primitive only accessible to OffscreenCanvas, which can't be displayed in
the
> > DOM in the first place, and can't be rendered directly by the compositor.
One
> > intended way to display OffscreenCanvas' results is in an
> > ImageBitmapRenderingContext, importing them via transferFromImageBitmap.
> > 
> > Does this change or simplify any of the code below? Can the DrawingBuffer
> > immediately drop its reference to the texture once it's been put in the
> mailbox
> > and given to the caller in the form of an AcceleratedStaticBitmapImage?
> 
> I think this is what is happening. We PrepareTextureMailbox, and discard the
> m_colorBuffer (replace it with a new texture). Then we call
> releaseCallback->Run() with lostResource=true which means the DrawingBuffer
can
> not reuse it, and it deletes its client id.
> 
> I think this can be simpler if we just dont deal with PrepareTextureMailbox
tho,
> which is what TODO on 424 is trying to get at.

I see. Thanks for this explanation. Per our offline discussion it sounds fine to
rework and/or simplify this further in subsequent CLs. Agree that making
incremental non-breaking changes is the best way forward.

[Justin Novosad, 2016-08-22 19:21:01]

lgtm

[danakj, 2016-08-22 19:47:01]

The CQ bit was checked by danakj@chromium.org

[danakj, 2016-08-22 19:47:02]

The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org,
piman@chromium.org
Link to the patchset: https://codereview.chromium.org/2261623002/#ps210001
(title: "webmailbox: fix-passrefptr")

[commit-bot: I haz the power, 2016-08-22 19:48:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-22 22:30:44]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2016-08-22 22:33:13]

Description was changed from

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Make DrawingBuffer and Canvas2DLayerBridge be cc::TextureLayerClients.

This eliminates the following classes:
- WebExternalTextureMailbox (use cc::TextureMailbox and
  cc::SingleReleaseCallback directly now)
- WebExternalTextureLayerClient (use cc::TextureLayerClient directly)
- WebExternalBitmapImpl (use cc::SharedBitmap and friends directly)

While using SingleReleaseCallback, it became clear that some calls to
prepareMailbox were never calling releaseMailbox, causing the texture
to be kept alive forever (and the DrawingBuffer with it), leaking memory.
Fixing this lead to a large rewrite of the "transferToImageBitmap()"
implementation, which hopefully is simpler about its intentions and does
less work switching between mailboxes and SkImages.

R=kbr, piman, xidachen, junov, pdr, tkent
BUG=606056
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/b164441d329b1260970621adbe9046ee79917aea
Cr-Commit-Position: refs/heads/master@{#413555}
==========

[commit-bot: I haz the power, 2016-08-22 22:33:15]

Patchset 12 (id:??) landed as
https://crrev.com/b164441d329b1260970621adbe9046ee79917aea
Cr-Commit-Position: refs/heads/master@{#413555}