959171 - Fix how to bind EGLImage in GrallocTextureHostOGL

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Sotaro Ikeda [:sotaro]]

+++ This bug was initially created as a clone of Bug #946720 +++

It is created from Bug 946720 Comment 24.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix how to bind EGLImage in GrallocTextureHostOGL

Comment 2

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8359232 [details] [diff] [review]
patch - Fix how to bind EGLImage in GrallocTextureHostOGL

Cancel to request reviewing. There seems still other cause of genlock failure.

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch v2 - Fix how to bind EGLImage in GrallocTextureHostOGL

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 8361264 [details] [diff] [review]
patch v2 - Fix how to bind EGLImage in GrallocTextureHostOGL

Review of attachment 8361264 [details] [diff] [review]:
-----------------------------------------------------------------

I am not an EGL expert so if you have doubts you may want to ask for an additional reviewer, otherwise LGTM.

::: gfx/layers/opengl/GrallocTextureHost.cpp
@@ +174,5 @@
>    }
>  
>    if (!mNeedsReset) {
> +    // Update binding to the EGLImage
> +    gl()->MakeCurrent();

nit: I don't know if MakeCurrent can fail on b2g, but I have had to deal with it failing on Linux (mostly during shutdown). If so, you should probably do:

if (gl()->MakeCurrent()) {
  // do stuff
} else {
  // print the most scary warning you can think of
}

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #4)
> 
> nit: I don't know if MakeCurrent can fail on b2g, but I have had to deal
> with it failing on Linux (mostly during shutdown). If so, you should
> probably do:
> 
> if (gl()->MakeCurrent()) {
>   // do stuff
> } else {
>   // print the most scary warning you can think of
> }

I never saw gl()->MakeCurrent() failure on gonk. We can put off this until we saw it on gonk.

Comment 6

[Sotaro Ikeda [:sotaro]]

> 
> I am not an EGL expert so if you have doubts you may want to ask for an
> additional reviewer, otherwise LGTM.

This is an easy fix. Your review seems enough.

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch v3 - Fix how to bind EGLImage in GrallocTextureHostOGL (r=nical)

Committable patch. Carry 'r=nical'.

Comment 8

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=81d5886e1762

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/60698fb6579b

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for B2G mochitest-3 crashes.
https://hg.mozilla.org/integration/mozilla-inbound/rev/62f929fab468

https://tbpl.mozilla.org/php/getParsedLog.php?id=33346478&tree=Mozilla-Inbound

Comment 11

[Sotaro Ikeda [:sotaro]]

Hmm, it's strange the patch hit the crash. The change might hit another bug.

Comment 12

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=18b069625559

Comment 13

[Sotaro Ikeda [:sotaro]]

I recognize the cause of the test crash.

During emulator test, there is a cases that compositor is not started yet even after test start.
At some point, Compositor is start to work. After that the compositor is set to TextureHost. In ImageHost::Composite() and then GrallocTextureSourceOGL::BindTexture() is called.

EGL Image is normally created at GrallocTextureSourceOGL::SetCompositableBackendSpecificData(). It is called when Texture swap. But at the time if Compositor is not present, EGLImage is not created. If it happens, GrallocTextureSourceOGL::BindTexture() does not have a EGLImage to bind.
It cause the crash.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: patch v4 - Fix how to bind EGLImage in GrallocTextureHostOGL (r=nical)

Fix mochi test failure.

Comment 15

[Sotaro Ikeda [:sotaro]]

https://tbpl.mozilla.org/?tree=Try&rev=49a3db7d76e7

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38bb623379e8

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/38bb623379e8