Remove GLImage::Destroy().

Remove GLImage::Destroy().

Since there are no longer any implementations that depend on the GL
context for destruction, we can remove this and simplify the GLImage
interface. In particular, it will now be possible to hand ownership
of GLImages entirely to a Texture.

BUG=602708
CQ_INCLUDE_TRYBOTS=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

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

[sandersd (OOO until July 31), 2016-10-26 20:20:25]

sandersd@chromium.org changed reviewers:
+ piman@chromium.org

[sandersd (OOO until July 31), 2016-10-26 20:20:26]

piman@: This is the same strategy we used for AVDACodecImage, and for the same
purpose.

If we accept this strategy and want to fully implement it (that is to say, spec
it on the GLImage interface), one approach would be a construction parameter (or
other metadata) to request this mode. Then other paths would retain the existing
invariant, and GLImage implementations could opt out of this mode.

[piman, 2016-10-26 21:28:29]

piman@chromium.org changed reviewers:
+ reveman@chromium.org

[piman, 2016-10-26 21:28:30]

+reveman
In general I am very supportive of not having a Destroy() on GLImage, especially
because they are refcounted, so it makes it hard to reason about transient
states (image is still referenced but Destroy'ed).
I would prefer not to have subtly different behavior on different platforms
though, especially if the behavior leaks to the client side. I don't know that a
constructor parameter makes it any easier. How hard would it be for all GLImage
implementations to follow that pattern?

[reveman, 2016-10-27 04:35:14]

The GLImage code has changed significantly since Destroy function was introduced
and it looks like none of the implementations use GL state anymore so it should
be safe to remove the Destroy function completely and I agree that this would be
a nice cleanup.

[sandersd (OOO until July 31), 2016-10-27 17:41:09]

On 2016/10/27 04:35:14, reveman wrote:
> The GLImage code has changed significantly since Destroy function was
introduced
> and it looks like none of the implementations use GL state anymore so it
should
> be safe to remove the Destroy function completely and I agree that this would
be
> a nice cleanup.

I looked at every subclass, and there are two that stand out in that they do
nontrivial work in Destroy():

  - GLImageEGL: calls eglDestroyImageKHR()
  - GLImageGLX: calls glXDestroyGLXPixmap()

If these are safe to simply move to the destructor, I can create a CL for that.

Notably, there are no implementations that look at |have_context|.

[reveman, 2016-10-27 17:53:33]

On 2016/10/27 at 17:41:09, sandersd wrote:
> On 2016/10/27 04:35:14, reveman wrote:
> > The GLImage code has changed significantly since Destroy function was
introduced
> > and it looks like none of the implementations use GL state anymore so it
should
> > be safe to remove the Destroy function completely and I agree that this
would be
> > a nice cleanup.
> 
> I looked at every subclass, and there are two that stand out in that they do
nontrivial work in Destroy():
> 
>   - GLImageEGL: calls eglDestroyImageKHR()
>   - GLImageGLX: calls glXDestroyGLXPixmap()

Those calls are not tied to a specific GL context so safe to move to dtor. We
used to have implementations with GL textures and that's when Destroy made
sense.

> 
> If these are safe to simply move to the destructor, I can create a CL for
that.
> 
> Notably, there are no implementations that look at |have_context|.

[reveman, 2016-10-27 17:53:34]

On 2016/10/27 at 17:41:09, sandersd wrote:
> On 2016/10/27 04:35:14, reveman wrote:
> > The GLImage code has changed significantly since Destroy function was
introduced
> > and it looks like none of the implementations use GL state anymore so it
should
> > be safe to remove the Destroy function completely and I agree that this
would be
> > a nice cleanup.
> 
> I looked at every subclass, and there are two that stand out in that they do
nontrivial work in Destroy():
> 
>   - GLImageEGL: calls eglDestroyImageKHR()
>   - GLImageGLX: calls glXDestroyGLXPixmap()

Those calls are not tied to a specific GL context so safe to move to dtor. We
used to have implementations with GL textures and that's when Destroy made
sense.

> 
> If these are safe to simply move to the destructor, I can create a CL for
that.
> 
> Notably, there are no implementations that look at |have_context|.

[sandersd (OOO until July 31), 2016-10-27 19:06:23]

Description was changed from

==========
GLImageIOSurface: Allow destruction without Destroy().

Since GLImageIOSurface does not rely on any GPU resources to destruct
cleanly, we can safely remove this requirement. It is then safe to use
Textures to keep such images alive, and we can do so in VTVDA to enable
video frames to outlive the VDA.

BUG=602708
==========

to

==========
GLImageIOSurface: Allow destruction without Destroy().

Since GLImageIOSurface does not rely on any GPU resources to destruct
cleanly, we can safely remove this requirement. It is then safe to use
Textures to keep such images alive, and we can do so in VTVDA to enable
video frames to outlive the VDA.

BUG=602708
CQ_INCLUDE_TRYBOTS=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
==========

[sandersd (OOO until July 31), 2016-10-27 19:10:06]

Description was changed from

==========
GLImageIOSurface: Allow destruction without Destroy().

Since GLImageIOSurface does not rely on any GPU resources to destruct
cleanly, we can safely remove this requirement. It is then safe to use
Textures to keep such images alive, and we can do so in VTVDA to enable
video frames to outlive the VDA.

BUG=602708
CQ_INCLUDE_TRYBOTS=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

==========
Remove GLImage::Destroy().

Since there are no longer any implementations depend on the GL context,
we can remove this and simplify the GLImage interface. In particular,
it will now be possible to hand ownership of GLImages entirely to a
Texture.

BUG=602708
CQ_INCLUDE_TRYBOTS=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
==========

[sandersd (OOO until July 31), 2016-10-27 19:10:28]

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

[commit-bot: I haz the power, 2016-10-27 19:10:58]

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

[sandersd (OOO until July 31), 2016-10-27 19:15:49]

sandersd@chromium.org changed reviewers:
+ hshi@google.com

[sandersd (OOO until July 31), 2016-10-27 19:15:49]

hshi@: Please review ui/ozone/.

all: This CL now removes GLImage::Destroy() entirely (and
ImageManager::Destroy() too). PTAL.

[piman, 2016-10-27 19:39:57]

lgtm, very nice!

[commit-bot: I haz the power, 2016-10-27 19:43:30]

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

[commit-bot: I haz the power, 2016-10-27 19:43:31]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  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...)

[reveman, 2016-10-27 20:01:45]

lgtm

https://codereview.chromium.org/2449993005/diff/20001/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/2449993005/diff/20001/ui/gl/gl_image_egl.cc#n...
ui/gl/gl_image_egl.cc:24: egl_image_ = EGL_NO_IMAGE_KHR;
nit: no need for this in dtor

https://codereview.chromium.org/2449993005/diff/20001/ui/gl/gl_image_glx.cc
File ui/gl/gl_image_glx.cc (right):

https://codereview.chromium.org/2449993005/diff/20001/ui/gl/gl_image_glx.cc#n...
ui/gl/gl_image_glx.cc:109: glx_pixmap_ = 0;
nit: no need for this in dtor

[sandersd (OOO until July 31), 2016-10-27 20:13:08]

sandersd@chromium.org changed reviewers:
+ ccameron@chromium.org

[sandersd (OOO until July 31), 2016-10-27 20:13:09]

ccameron@chromium.org: Please review ui/accelerated_widget_mac/.

https://codereview.chromium.org/2449993005/diff/20001/ui/gl/gl_image_egl.cc
File ui/gl/gl_image_egl.cc (right):

https://codereview.chromium.org/2449993005/diff/20001/ui/gl/gl_image_egl.cc#n...
ui/gl/gl_image_egl.cc:24: egl_image_ = EGL_NO_IMAGE_KHR;
On 2016/10/27 20:01:44, reveman wrote:
> nit: no need for this in dtor

Done.

https://codereview.chromium.org/2449993005/diff/20001/ui/gl/gl_image_glx.cc
File ui/gl/gl_image_glx.cc (right):

https://codereview.chromium.org/2449993005/diff/20001/ui/gl/gl_image_glx.cc#n...
ui/gl/gl_image_glx.cc:109: glx_pixmap_ = 0;
On 2016/10/27 20:01:44, reveman wrote:
> nit: no need for this in dtor

Done.

[sandersd (OOO until July 31), 2016-10-27 20:13:32]

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

[commit-bot: I haz the power, 2016-10-27 20:14:18]

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

[hshi1, 2016-10-27 20:15:11]

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

[hshi1, 2016-10-27 20:15:11]

ui/ozone LGTM

[commit-bot: I haz the power, 2016-10-27 20:36:06]

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

[commit-bot: I haz the power, 2016-10-27 20:36:08]

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

[sandersd (OOO until July 31), 2016-10-27 20:39:52]

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

[commit-bot: I haz the power, 2016-10-27 20:40:39]

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

[sandersd (OOO until July 31), 2016-10-27 20:48:19]

Description was changed from

==========
Remove GLImage::Destroy().

Since there are no longer any implementations depend on the GL context,
we can remove this and simplify the GLImage interface. In particular,
it will now be possible to hand ownership of GLImages entirely to a
Texture.

BUG=602708
CQ_INCLUDE_TRYBOTS=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

==========
Remove GLImage::Destroy().

Since there are no longer any implementations that depend on the GL
context for destruction, we can remove this and simplify the GLImage
interface. In particular, it will now be possible to hand ownership
of GLImages entirely to a Texture.

BUG=602708
CQ_INCLUDE_TRYBOTS=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
==========

[sandersd (OOO until July 31), 2016-10-27 20:57:54]

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

[commit-bot: I haz the power, 2016-10-27 20:59:21]

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

[sandersd (OOO until July 31), 2016-10-27 21:49:48]

sandersd@chromium.org changed reviewers:
- hshi@google.com

[commit-bot: I haz the power, 2016-10-27 22:33:50]

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

[commit-bot: I haz the power, 2016-10-27 22:33:51]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2016-11-01 00:13:00]

On 2016/10/27 20:15:11, hshi1 wrote:
> ui/ozone LGTM

ccameron@: ping.

[ccameron, 2016-11-01 00:16:36]

lgtm

[sandersd (OOO until July 31), 2016-11-01 00:25:24]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2016-11-01 00:25:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, piman@chromium.org, hshi@chromium.org
Link to the patchset: https://codereview.chromium.org/2449993005/#ps80001
(title: "One more Destroy() call on Mac.")

[commit-bot: I haz the power, 2016-11-01 00:26:15]

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-11-01 02:24:50]

Description was changed from

==========
Remove GLImage::Destroy().

Since there are no longer any implementations that depend on the GL
context for destruction, we can remove this and simplify the GLImage
interface. In particular, it will now be possible to hand ownership
of GLImages entirely to a Texture.

BUG=602708
CQ_INCLUDE_TRYBOTS=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

==========
Remove GLImage::Destroy().

Since there are no longer any implementations that depend on the GL
context for destruction, we can remove this and simplify the GLImage
interface. In particular, it will now be possible to hand ownership
of GLImages entirely to a Texture.

BUG=602708
CQ_INCLUDE_TRYBOTS=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
==========

[commit-bot: I haz the power, 2016-11-01 02:24:52]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-01 02:26:48]

Description was changed from

==========
Remove GLImage::Destroy().

Since there are no longer any implementations that depend on the GL
context for destruction, we can remove this and simplify the GLImage
interface. In particular, it will now be possible to hand ownership
of GLImages entirely to a Texture.

BUG=602708
CQ_INCLUDE_TRYBOTS=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

==========
Remove GLImage::Destroy().

Since there are no longer any implementations that depend on the GL
context for destruction, we can remove this and simplify the GLImage
interface. In particular, it will now be possible to hand ownership
of GLImages entirely to a Texture.

BUG=602708
CQ_INCLUDE_TRYBOTS=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

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

[commit-bot: I haz the power, 2016-11-01 02:26:49]

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