Closed Bug 1153848 Opened 7 years ago Closed 7 years ago

Add pref to disable SurfaceTexture attach/detach

Categories

(Firefox for Android Graveyard :: Audio/Video, defect)

34 Branch
All
Android
defect
Not set
normal

Tracking

(firefox42 fixed)

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: snorp, Assigned: droeh)

References

Details

Attachments

(1 file, 2 obsolete files)

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
Attached patch Proposed patch (obsolete) — Splinter Review
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-
Attached patch Proposed patch (updated) (obsolete) — Splinter Review
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-
Updated default behavior.
Attachment #8630135 - Attachment is obsolete: true
Attachment #8630175 - Flags: review?(snorp)
Attachment #8630175 - Flags: review?(snorp) → review+
https://hg.mozilla.org/mozilla-central/rev/3c3c1d858656
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.