1037147 - Remove SharedTextureHandle and friends

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kelsey Gilbert [:jgilbert]]

SharedTextureHandle and friends are in a sad state. They require manual lifetime management, and a quick MXR text search shows that we no longer call Release() for them, so we're probably very slowly leaking them.

We also have a surplus of texture-like objects, so if we can remove one of them, we should be better off.

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: kill-shtex

Comment 2

[Kelsey Gilbert [:jgilbert]]

I haven't done testing on this yet, so there probably issues, but let's get the reviews rolling.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8454016 [details] [diff] [review]
kill-shtex

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

Looks pretty good to me! The whitespace changes are distracting, but cleanup is good, right?

As discussed on IRC, I think you can go ahead and nuke the EGLImage path. It's only used for Flash on Honeycomb, which is very small number of users.

Comment 4

[Kelsey Gilbert [:jgilbert]]

Attachment: kill-shtex

Now with the three files that were missing from the previous patch!

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8454025 [details] [diff] [review]
kill-shtex

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

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ -582,5 @@
>          if (!newIOSurface) {
>              NS_WARNING("Got bad IOSurfaceDescriptor in RecvShow");
>              return false;
>          }
> -      

Please land whitespace changes separately to the code changes.

::: gfx/layers/composite/TextureHost.cpp
@@ +206,1 @@
>      case SurfaceDescriptor::TNewSurfaceDescriptorGralloc:

Don't we need to add the two new surface descriptor types here so that we fall through to CreateTextureHostOGL (where they are properly handled)?

::: gfx/layers/opengl/TextureClientOGL.cpp
@@ +88,3 @@
>  {
> +  // Our data is always owned externally.
> +  AddFlags(TextureFlags::DEALLOCATE_CLIENT);

Let's assert that XRE_GetProcessType() == GeckoProcessType_Default here. Trying to pass a raw pointer between processes would be sad.

::: gfx/layers/opengl/TextureClientOGL.h
@@ +106,5 @@
> +    return false;
> +  }
> +
> +protected:
> +  nsSurfaceTexture* const mSurfTex;

nsSurfaceTexture is refcounted, why don't we make use of that here?

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +486,5 @@
> +  gl()->GetAndClearError();
> +#endif
> +  // FIXME: SurfaceTexture provides a transform matrix which is supposed to
> +  // be applied to the texture coordinates. We should return that here
> +  // so we can render correctly. Bug 775083

How is this different to GetTextureTransform below?

Comment 6

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8454025 [details] [diff] [review]
kill-shtex

Oh, snorp already r+'d this.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0: whitespace

r=mattwoodrow
Split out from the main patch.

Comment 8

[Kelsey Gilbert [:jgilbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
> Comment on attachment 8454025 [details] [diff] [review]
> kill-shtex
> 
> Review of attachment 8454025 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/plugins/ipc/PluginInstanceParent.cpp
> @@ -582,5 @@
> >          if (!newIOSurface) {
> >              NS_WARNING("Got bad IOSurfaceDescriptor in RecvShow");
> >              return false;
> >          }
> > -      
> 
> Please land whitespace changes separately to the code changes.
Done.
> 
> ::: gfx/layers/composite/TextureHost.cpp
> @@ +206,1 @@
> >      case SurfaceDescriptor::TNewSurfaceDescriptorGralloc:
> 
> Don't we need to add the two new surface descriptor types here so that we
> fall through to CreateTextureHostOGL (where they are properly handled)?
Oops, yes.
> 
> ::: gfx/layers/opengl/TextureClientOGL.cpp
> @@ +88,3 @@
> >  {
> > +  // Our data is always owned externally.
> > +  AddFlags(TextureFlags::DEALLOCATE_CLIENT);
> 
> Let's assert that XRE_GetProcessType() == GeckoProcessType_Default here.
> Trying to pass a raw pointer between processes would be sad.
Good idea.
> 
> ::: gfx/layers/opengl/TextureClientOGL.h
> @@ +106,5 @@
> > +    return false;
> > +  }
> > +
> > +protected:
> > +  nsSurfaceTexture* const mSurfTex;
> 
> nsSurfaceTexture is refcounted, why don't we make use of that here?
Yep, we should be keeping a ref in the TexClient.
> 
> ::: gfx/layers/opengl/TextureHostOGL.cpp
> @@ +486,5 @@
> > +  gl()->GetAndClearError();
> > +#endif
> > +  // FIXME: SurfaceTexture provides a transform matrix which is supposed to
> > +  // be applied to the texture coordinates. We should return that here
> > +  // so we can render correctly. Bug 775083
> 
> How is this different to GetTextureTransform below?
I agree, I think this comment is obsolete. I removed it.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8454203 [details] [diff] [review]
patch 0: whitespace

Please provide a Try link if you've got one.

Comment 10

[Kelsey Gilbert [:jgilbert]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/40cdc0d326d7

Comment 11

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 0.1: purge more ws

r=mattwoodrow
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b1dd01c50f

Comment 12

[Kelsey Gilbert [:jgilbert]]

Attachment: patch 1: kill SharedTexture

r=mattwoodrow,snorp

This largely works, but sometimes on snorp's test page[1], I see the grey "tap here to activate the plugin" box showing 'behind' the mostly-alpha Flash frame.

[1] http://people.mozilla.org/~jwillcox/flash/flash_test.html

Any ideas Matt?

Comment 13

[Kelsey Gilbert [:jgilbert]]

Also, I'm on PTO through the 25th. If someone else wants to finish this up (snorp?), be my guest.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40cdc0d326d7
https://hg.mozilla.org/mozilla-central/rev/c3b1dd01c50f

Comment 15

[Matt Woodrow (:mattwoodrow)]

I don't have any ideas why you'd be able to see the activation prompt as well as the plugin. Just changing the plugin rendering path shouldn't change what content is drawn behind it.

Comment 16

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Remove SharedTextureHandle

Not sure what changed, but I un-rotted the 'kill SharedTexture' patch and everything seems to be working fine.

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Jeff r+ed on irc:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a84f9edfe968

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Backed out because I missed a file. Sigh.

https://hg.mozilla.org/integration/mozilla-inbound/rev/92bed1b7dcc3

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Presumably fixed version of patch on Try here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=472e0625e0d7

Comment 20

[James Willcox (:snorp) ([e-mail]) (he/him)]

I had forgotten a file and there was some 64bit pointer casting nonsense. Works now: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a0ad123b8735


https://hg.mozilla.org/integration/mozilla-inbound/rev/57e058be11ff

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/57e058be11ff

Comment 22

[James Willcox (:snorp) ([e-mail]) (he/him)]

Jeff, we're considering an uplift of the new video backend all the way to 33. It depends on this patch. Is this something we're comfortable with? If so please request approval all the way to release.

Comment 23

[Kelsey Gilbert [:jgilbert]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #22)
> Jeff, we're considering an uplift of the new video backend all the way to
> 33. It depends on this patch. Is this something we're comfortable with? If
> so please request approval all the way to release.

Yep, that sounds fine. (though crazy!)

Comment 24

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8488675 [details] [diff] [review]
Remove SharedTextureHandle

Approval Request Comment
[Feature/regressing bug #]: n/a
[User impact if declined]: Required for uplift of new video backend. Ask :snorp.
[Describe test coverage new/current, TBPL]: Ask :snorp.
[Risks and why]: This could break plugins, which don't IIRC have much for reftests. :snorp knows more.
[String/UUID change made/needed]: none

Comment 25

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Gilbert [:jgilbert] from comment #24)
> Comment on attachment 8488675 [details] [diff] [review]
> Remove SharedTextureHandle
> 
> Approval Request Comment
> [Feature/regressing bug #]: n/a
> [User impact if declined]: Required for uplift of new video backend. Ask
> :snorp.

Required by bug 1014614

> [Describe test coverage new/current, TBPL]: Ask :snorp.

Baked on m-c for a month or more

> [Risks and why]: This could break plugins, which don't IIRC have much for
> reftests. :snorp knows more.

We do have a robocop test for Flash.

> [String/UUID change made/needed]: none

Comment 26

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8488675 [details] [diff] [review]
Remove SharedTextureHandle

This amount of change this late in beta is scary but so is not supporting mp4 playback on Android until mid January. Let's take this in beta8 and evaluate how the uplift goes. If feedback is not good, we should consider spinning beta9 (which we don't normally do for Android) to take a backout.

At this point we are not taking this in 33.

Comment 27

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=5811de401315

Comment 28

[James Willcox (:snorp) ([e-mail]) (he/him)]

This was backed out, but relanded now

https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=53692e16c248