Add pref to disable SurfaceTexture attach/detach

RESOLVED FIXED in Firefox 42

Status

()

Firefox for Android
Audio/Video
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: snorp, Assigned: droeh)

Tracking

34 Branch
Firefox 42
All
Android
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Some devices have problems with us attaching and detaching the GLContext from the SurfaceTexture when we draw video. We can stop doing that, but it will make drawing the video to a canvas (or anywhere else other than the compositor) impossible. We should add a pref to enable this fallback path for debugging this issue.
Assignee: nobody → droeh
(Assignee)

Comment 1

3 years ago
Created attachment 8630118 [details] [diff] [review]
Proposed patch

Adds a preference "gfx.detachTexture.enabled" (only on Android), and checks it when AndroidSurfaceTexture::UpdateCanDetach is called.
Attachment #8630118 - Flags: review?(snorp)
Comment on attachment 8630118 [details] [diff] [review]
Proposed patch

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

Some minor issues

::: gfx/gl/AndroidSurfaceTexture.cpp
@@ +132,5 @@
>    // The API for attach/detach only exists on 16+, and PowerVR has some sort of
>    // fencing issue. Additionally, attach/detach seems to be busted on at least some
>    // Mali adapters (400MP2 for sure, bug 1131793)
> +  bool canDetach;
> +  nsresult res = Preferences::GetBool("gfx.detachTexture.enabled", &canDetach);

That pref name doesn't really say much about what it's doing. I suggest something like 'gfx.surfaceTexture.detach.enabled', so we know it's for SurfaceTexture

@@ +134,5 @@
>    // Mali adapters (400MP2 for sure, bug 1131793)
> +  bool canDetach;
> +  nsresult res = Preferences::GetBool("gfx.detachTexture.enabled", &canDetach);
> +  if (!NS_SUCCEEDED(res)) {
> +    // Leave canDetach true if something goes wrong getting the preference.

You can just initialize canDetach to true or use the other overload for GetBool as follows:

bool canDetach = Preferences::GetBool("gfx.detachTexture.enabled", false);

::: modules/libpref/init/all.js
@@ +4250,5 @@
>  pref("layers.componentalpha.enabled", true);
>  
>  #ifdef ANDROID
>  pref("gfx.apitrace.enabled",false);
> +pref("gfx.detachTexture.enabled",true);

We only have SurfaceTexture on Fennec, so we should just put this in mobile.js. I know it says "ifdef ANDROID" here, but that covers both Firefox OS and Fennec (confusingly).
Attachment #8630118 - Flags: review?(snorp) → review-
(Assignee)

Comment 3

3 years ago
Created attachment 8630135 [details] [diff] [review]
Proposed patch (updated)

Updates the previous proposed patch with suggested changes.
Attachment #8630118 - Attachment is obsolete: true
Attachment #8630135 - Flags: review?(snorp)
Comment on attachment 8630135 [details] [diff] [review]
Proposed patch (updated)

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

One more nit

::: gfx/gl/AndroidSurfaceTexture.cpp
@@ +130,5 @@
>  {
>    // The API for attach/detach only exists on 16+, and PowerVR has some sort of
>    // fencing issue. Additionally, attach/detach seems to be busted on at least some
>    // Mali adapters (400MP2 for sure, bug 1131793)
> +  bool canDetach = Preferences::GetBool("gfx.SurfaceTexture.detach.enabled", false);

You want the default to be true, right?
Attachment #8630135 - Flags: review?(snorp) → review-
(Assignee)

Comment 5

3 years ago
Created attachment 8630175 [details] [diff] [review]
Proposed patch (updated)

Updated default behavior.
Attachment #8630135 - Attachment is obsolete: true
Attachment #8630175 - Flags: review?(snorp)
Attachment #8630175 - Flags: review?(snorp) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3c3c1d858656
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.