The default bug view has changed. See this FAQ.

Remove SharedTextureHandle and friends

RESOLVED FIXED in Firefox 34

Status

()

Core
Graphics
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jgilbert, Assigned: jgilbert)

Tracking

unspecified
mozilla35
Points:
---

Firefox Tracking Flags

(firefox34+ fixed, firefox35 fixed)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8454016 [details] [diff] [review]
kill-shtex
Attachment #8454016 - Flags: review?(snorp)
Attachment #8454016 - Flags: review?(matt.woodrow)
(Assignee)

Comment 2

3 years ago
I haven't done testing on this yet, so there probably issues, but let's get the reviews rolling.
(Assignee)

Updated

3 years ago
Blocks: 1037151
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.
Attachment #8454016 - Flags: review?(snorp) → review+
(Assignee)

Comment 4

3 years ago
Created attachment 8454025 [details] [diff] [review]
kill-shtex

Now with the three files that were missing from the previous patch!
Attachment #8454016 - Attachment is obsolete: true
Attachment #8454016 - Flags: review?(matt.woodrow)
Attachment #8454025 - Flags: review?(snorp)
Attachment #8454025 - Flags: review?(matt.woodrow)
(Assignee)

Updated

3 years ago
No longer blocks: 1037151
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?
Attachment #8454025 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 6

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

Oh, snorp already r+'d this.
Attachment #8454025 - Flags: review?(snorp) → review+
(Assignee)

Comment 7

3 years ago
Created attachment 8454203 [details] [diff] [review]
patch 0: whitespace

r=mattwoodrow
Split out from the main patch.
Attachment #8454025 - Attachment is obsolete: true
Attachment #8454203 - Flags: review+
Attachment #8454203 - Flags: checkin?(jgilbert)
(Assignee)

Comment 8

3 years ago
(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 on attachment 8454203 [details] [diff] [review]
patch 0: whitespace

Please provide a Try link if you've got one.
Attachment #8454203 - Flags: checkin?(jgilbert)
(Assignee)

Comment 10

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/40cdc0d326d7
Keywords: leave-open
(Assignee)

Updated

3 years ago
Attachment #8454203 - Flags: checkin+
(Assignee)

Comment 11

3 years ago
Created attachment 8454744 [details] [diff] [review]
patch 0.1: purge more ws

r=mattwoodrow
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c3b1dd01c50f
Attachment #8454744 - Flags: review+
(Assignee)

Updated

3 years ago
Attachment #8454744 - Flags: checkin+
(Assignee)

Comment 12

3 years ago
Created attachment 8454837 [details] [diff] [review]
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?
Attachment #8454837 - Flags: review+
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 13

3 years ago
Also, I'm on PTO through the 25th. If someone else wants to finish this up (snorp?), be my guest.
https://hg.mozilla.org/mozilla-central/rev/40cdc0d326d7
https://hg.mozilla.org/mozilla-central/rev/c3b1dd01c50f
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.
Flags: needinfo?(matt.woodrow)
Created attachment 8488675 [details] [diff] [review]
Remove SharedTextureHandle

Not sure what changed, but I un-rotted the 'kill SharedTexture' patch and everything seems to be working fine.
Attachment #8488675 - Flags: review?(jgilbert)
Jeff r+ed on irc:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a84f9edfe968
Attachment #8488675 - Flags: review?(jgilbert) → review+
Backed out because I missed a file. Sigh.

https://hg.mozilla.org/integration/mozilla-inbound/rev/92bed1b7dcc3
Presumably fixed version of patch on Try here: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=472e0625e0d7
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
https://hg.mozilla.org/mozilla-central/rev/57e058be11ff
(Assignee)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Keywords: leave-open
Resolution: --- → FIXED
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.
Flags: needinfo?(jgilbert)
(Assignee)

Comment 23

2 years ago
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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!)
Flags: needinfo?(jgilbert)
(Assignee)

Updated

2 years ago
Target Milestone: --- → mozilla35
(Assignee)

Comment 24

2 years ago
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
Flags: needinfo?(snorp)
Attachment #8488675 - Flags: approval-mozilla-release?
Attachment #8488675 - Flags: approval-mozilla-beta?
(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
Blocks: 1014614
Flags: needinfo?(snorp)
status-firefox34: --- → affected
status-firefox35: --- → fixed
tracking-firefox34: --- → +
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.
Attachment #8488675 - Flags: approval-mozilla-release?
Attachment #8488675 - Flags: approval-mozilla-release-
Attachment #8488675 - Flags: approval-mozilla-beta?
Attachment #8488675 - Flags: approval-mozilla-beta+
https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=5811de401315
status-firefox34: affected → fixed
This was backed out, but relanded now

https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?changeset=53692e16c248
You need to log in before you can comment on or make changes to this bug.