999694 - Use fence objects in SharedSurfaceGralloc

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Michael Wu [:mwu]]

Attachment: Use fence objects in SharedSurfaceGralloc

This is basically a port of the fence support in SharedSurfaceEGL. Android native fences are also supported here since they're more likely to actually work.

Comment 1

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8410532 [details] [diff] [review]
Use fence objects in SharedSurfaceGralloc

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

Thanks for the patch. I did some general comments to the patch. Jeff Gilbert is a correct person to review the patch. I forward the review to him.

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +149,5 @@
> +    }
> +
> +    // When Android native fences are available, try
> +    // them first since they're more likely to work.
> +    if (mEGL->IsExtensionSupported(GLLibraryEGL::ANDROID_native_fence_sync)) {

In a different bug, it might be better to add direct Fence object handling. Getting the Fence and deliver it to Compositor seems better for the performance. But I do not know which hardware support this extension. Nexus-4 and Nexus-5 do not support it.

@@ +185,5 @@
>  }
>  
>  bool
>  SharedSurface_Gralloc::WaitSync()
>  {

WaitSync() is not called in normal WebGL use case. I confirmed it by using crystalskull. I am not sure why it is not called on current WebGL. Without calling WaitSync(), this patch seems not work correctly.

Comment 2

[Michael Wu [:mwu]]

(In reply to Sotaro Ikeda [:sotaro] from comment #1)
> Comment on attachment 8410532 [details] [diff] [review]
> Use fence objects in SharedSurfaceGralloc
> 
> Review of attachment 8410532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch. I did some general comments to the patch. Jeff Gilbert
> is a correct person to review the patch. I forward the review to him.
> 
> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +149,5 @@
> > +    }
> > +
> > +    // When Android native fences are available, try
> > +    // them first since they're more likely to work.
> > +    if (mEGL->IsExtensionSupported(GLLibraryEGL::ANDROID_native_fence_sync)) {
> 
> In a different bug, it might be better to add direct Fence object handling.
> Getting the Fence and deliver it to Compositor seems better for the
> performance. But I do not know which hardware support this extension.
> Nexus-4 and Nexus-5 do not support it.
> 

Hmmm, I added support for this in order to make it work on some qc device.. maybe I didn't check for the extension first?

Agreed on giving the compositor the fence. It seems like our current setup can't do that though, and having a flush and fence is probably better than relying on readback and genlock,

> @@ +185,5 @@
> >  }
> >  
> >  bool
> >  SharedSurface_Gralloc::WaitSync()
> >  {
> 
> WaitSync() is not called in normal WebGL use case. I confirmed it by using
> crystalskull. I am not sure why it is not called on current WebGL. Without
> calling WaitSync(), this patch seems not work correctly.

Very weird.. I'll take a look at what's going on..

Comment 3

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #2)
> > 
> > WaitSync() is not called in normal WebGL use case. I confirmed it by using
> > crystalskull. I am not sure why it is not called on current WebGL. Without
> > calling WaitSync(), this patch seems not work correctly.
> 
> Very weird.. I'll take a look at what's going on..

I also recognized it's weired. I am going to check if I am doing wrong.

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> (In reply to Michael Wu [:mwu] from comment #2)
> > > 
> > > WaitSync() is not called in normal WebGL use case. I confirmed it by using
> > > crystalskull. I am not sure why it is not called on current WebGL. Without
> > > calling WaitSync(), this patch seems not work correctly.
> > 
> > Very weird.. I'll take a look at what's going on..
> 
> I also recognized it's weired. I am going to check if I am doing wrong.

Sorry, It is just my fault. WaitSync() is called correctly.

Comment 5

[Michael Wu [:mwu]]

I made a typo in the android version ifdef in WaitSync.

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #5)
> I made a typo in the android version ifdef in WaitSync.

I also just recognized it.

Comment 7

[Michael Wu [:mwu]]

Attachment: Use fence objects in SharedSurfaceGralloc, v2

It turns out you can't detect ANDROID_native_fence_sync unless you use Android's alternate eglQueryString implementation.

Comment 8

[Michael Wu [:mwu]]

Attachment: Use fence objects in SharedSurfaceGralloc, v3

Reduced the number of #if blocks.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8412123 [details] [diff] [review]
Use fence objects in SharedSurfaceGralloc, v3

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

::: gfx/gl/GLLibraryEGL.cpp
@@ +187,5 @@
>          SYMBOL(SwapBuffers),
>          SYMBOL(CopyBuffers),
> +#if ANDROID_VERSION >= 17
> +        // Vanilla QueryString isn't magical enough to get ANDROID_native_fence_sync
> +        { (PRFuncPtr*) &mSymbols.fQueryString, { "_Z35eglQueryStringImplementationANDROIDPvi", nullptr } },

Fallback to regular eglQueryString here, in case the 'secret' symbol is missing.

::: gfx/gl/SharedSurfaceGralloc.cpp
@@ +147,5 @@
> +        mSync = 0;
> +    }
> +
> +    // When Android native fences are available, try
> +    // them first since they're more likely to work.

Elaborate. It seems like if they're both present, we shouldn't care which we use. In fact, KHR_fence_sync seems like it should be lighter-weight.

@@ +170,5 @@
> +            return;
> +        }
> +    }
> +
> +#if ANDROID_VERSION < 17

Remove this ifdef. We still need this if the above methods fail.

Comment 10

[Michael Wu [:mwu]]

(In reply to Jeff Gilbert [:jgilbert] from comment #9)
> Comment on attachment 8412123 [details] [diff] [review]
> Use fence objects in SharedSurfaceGralloc, v3
> 
> Review of attachment 8412123 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/gl/GLLibraryEGL.cpp
> @@ +187,5 @@
> >          SYMBOL(SwapBuffers),
> >          SYMBOL(CopyBuffers),
> > +#if ANDROID_VERSION >= 17
> > +        // Vanilla QueryString isn't magical enough to get ANDROID_native_fence_sync
> > +        { (PRFuncPtr*) &mSymbols.fQueryString, { "_Z35eglQueryStringImplementationANDROIDPvi", nullptr } },
> 
> Fallback to regular eglQueryString here, in case the 'secret' symbol is
> missing.
> 

That would never happen - it's part of the standard EGL implementation on android 4.2 and up, and we only do this when building for B2G on an appropriate android base. When porting to newer Android base versions, it's preferable to crash rather than fallback and do the wrong thing.

> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +147,5 @@
> > +        mSync = 0;
> > +    }
> > +
> > +    // When Android native fences are available, try
> > +    // them first since they're more likely to work.
> 
> Elaborate. It seems like if they're both present, we shouldn't care which we
> use. In fact, KHR_fence_sync seems like it should be lighter-weight.
> 

I used KHR_fence_sync first, but it simply didn't work. Adreno claimed to implement it, but it didn't do anything. We have to do as Android does because if Android doesn't do it, it's not tested and probably doesn't work.

> @@ +170,5 @@
> > +            return;
> > +        }
> > +    }
> > +
> > +#if ANDROID_VERSION < 17
> 
> Remove this ifdef. We still need this if the above methods fail.

I think I'd rather assert and crash rather than to do a workaround that just happens to work on Adreno. I'll remove the ifdef for now, but I'll follow up and check to see if all the major drivers implement KHR_fence_sync on jellybean and up.

Comment 11

[Kelsey Gilbert [:jgilbert]]

(In reply to Michael Wu [:mwu] from comment #10)
> (In reply to Jeff Gilbert [:jgilbert] from comment #9)
> > Comment on attachment 8412123 [details] [diff] [review]
> > Use fence objects in SharedSurfaceGralloc, v3
> > 
> > Review of attachment 8412123 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: gfx/gl/GLLibraryEGL.cpp
> > @@ +187,5 @@
> > >          SYMBOL(SwapBuffers),
> > >          SYMBOL(CopyBuffers),
> > > +#if ANDROID_VERSION >= 17
> > > +        // Vanilla QueryString isn't magical enough to get ANDROID_native_fence_sync
> > > +        { (PRFuncPtr*) &mSymbols.fQueryString, { "_Z35eglQueryStringImplementationANDROIDPvi", nullptr } },
> > 
> > Fallback to regular eglQueryString here, in case the 'secret' symbol is
> > missing.
> > 
> 
> That would never happen - it's part of the standard EGL implementation on
> android 4.2 and up, and we only do this when building for B2G on an
> appropriate android base. When porting to newer Android base versions, it's
> preferable to crash rather than fallback and do the wrong thing.
You must must leave a comment about this then. This is a very magical thing, and thus deserves a decent comment why we're doing things differently.

I would really much rather we universally do the `"_Z35eglQueryStringImplementationANDROIDPvi", "eglQueryString", nullptr` fallback.
Assert what we care about: We don't care that we get this version of QueryString, we care that we're using FenceSyncs successfully. Assert that instead.
> 
> > ::: gfx/gl/SharedSurfaceGralloc.cpp
> > @@ +147,5 @@
> > > +        mSync = 0;
> > > +    }
> > > +
> > > +    // When Android native fences are available, try
> > > +    // them first since they're more likely to work.
> > 
> > Elaborate. It seems like if they're both present, we shouldn't care which we
> > use. In fact, KHR_fence_sync seems like it should be lighter-weight.
> > 
> 
> I used KHR_fence_sync first, but it simply didn't work. Adreno claimed to
> implement it, but it didn't do anything. We have to do as Android does
> because if Android doesn't do it, it's not tested and probably doesn't work.
Then we should mark it as unsupported on that Adreno device. We rely on this exclusively in the EGLImage backend, so it's really really important that this is given its own bug!

In the long run, it is actually in our best interest to ignore what Android does, and do whatever we want, filing and working-around bugs as we encounter them. We should not blindly do what Android does, since that does not provide a path to improving beyond Android.
> 
> > @@ +170,5 @@
> > > +            return;
> > > +        }
> > > +    }
> > > +
> > > +#if ANDROID_VERSION < 17
> > 
> > Remove this ifdef. We still need this if the above methods fail.
> 
> I think I'd rather assert and crash rather than to do a workaround that just
> happens to work on Adreno. I'll remove the ifdef for now, but I'll follow up
> and check to see if all the major drivers implement KHR_fence_sync on
> jellybean and up.

This is not for just Adreno, this is for all instances where we can't (or failed to) create a sync object. This is our universal syncing fallback for this backend. We should keep it non-ifdef'd if we keep it at all. ifdefing should be avoided as much as possible.

Comment 12

[Michael Wu [:mwu]]

(In reply to Jeff Gilbert [:jgilbert] from comment #11)
> > That would never happen - it's part of the standard EGL implementation on
> > android 4.2 and up, and we only do this when building for B2G on an
> > appropriate android base. When porting to newer Android base versions, it's
> > preferable to crash rather than fallback and do the wrong thing.
> You must must leave a comment about this then. This is a very magical thing,
> and thus deserves a decent comment why we're doing things differently.
> 

Can do.

> I would really much rather we universally do the
> `"_Z35eglQueryStringImplementationANDROIDPvi", "eglQueryString", nullptr`
> fallback.

That doesn't seem useful though. Gecko binaries can't be run on different Android bases for B2G purposes, and I really do want things to crash if this magic QueryString gets changed in a new Android base.

> Assert what we care about: We don't care that we get this version of
> QueryString, we care that we're using FenceSyncs successfully. Assert that
> instead.

That I would also like to assert, after first checking that we don't crash on our current devices. I don't know if we can assert that.

> > 
> > > ::: gfx/gl/SharedSurfaceGralloc.cpp
> > > @@ +147,5 @@
> > > > +        mSync = 0;
> > > > +    }
> > > > +
> > > > +    // When Android native fences are available, try
> > > > +    // them first since they're more likely to work.
> > > 
> > > Elaborate. It seems like if they're both present, we shouldn't care which we
> > > use. In fact, KHR_fence_sync seems like it should be lighter-weight.
> > > 
> > 
> > I used KHR_fence_sync first, but it simply didn't work. Adreno claimed to
> > implement it, but it didn't do anything. We have to do as Android does
> > because if Android doesn't do it, it's not tested and probably doesn't work.
> Then we should mark it as unsupported on that Adreno device. We rely on this
> exclusively in the EGLImage backend, so it's really really important that
> this is given its own bug!
> 

AFAICT, the other use of KHR_fence_sync is only in the plugin code path, which isn't interesting.

> In the long run, it is actually in our best interest to ignore what Android
> does, and do whatever we want, filing and working-around bugs as we
> encounter them. We should not blindly do what Android does, since that does
> not provide a path to improving beyond Android.

This is true, but we don't win if we don't have Android compatibility. We're not currently in a position to deviate significantly from the policy of doing as Android does without any obvious benefits to our partners.

Comment 13

[Kelsey Gilbert [:jgilbert]]

(In reply to Michael Wu [:mwu] from comment #12)
> > > 
> > > > ::: gfx/gl/SharedSurfaceGralloc.cpp
> > > > @@ +147,5 @@
> > > > > +        mSync = 0;
> > > > > +    }
> > > > > +
> > > > > +    // When Android native fences are available, try
> > > > > +    // them first since they're more likely to work.
> > > > 
> > > > Elaborate. It seems like if they're both present, we shouldn't care which we
> > > > use. In fact, KHR_fence_sync seems like it should be lighter-weight.
> > > > 
> > > 
> > > I used KHR_fence_sync first, but it simply didn't work. Adreno claimed to
> > > implement it, but it didn't do anything. We have to do as Android does
> > > because if Android doesn't do it, it's not tested and probably doesn't work.
> > Then we should mark it as unsupported on that Adreno device. We rely on this
> > exclusively in the EGLImage backend, so it's really really important that
> > this is given its own bug!
> > 
> 
> AFAICT, the other use of KHR_fence_sync is only in the plugin code path,
> which isn't interesting.
It's primarily used for canvas2d and WebGL compositing on Android, though it's also used for plugins.
> 
> > In the long run, it is actually in our best interest to ignore what Android
> > does, and do whatever we want, filing and working-around bugs as we
> > encounter them. We should not blindly do what Android does, since that does
> > not provide a path to improving beyond Android.
> 
> This is true, but we don't win if we don't have Android compatibility. We're
> not currently in a position to deviate significantly from the policy of
> doing as Android does without any obvious benefits to our partners.
Sure, but standards are standards. If a vendor exposes an extension, we're supposed to be able to rely on it.



Some important links for this discussion:
The code: http://androidxref.com/4.4.2_r2/xref/frameworks/native/opengl/libs/EGL/eglApi.cpp#1119
The cset: https://android.googlesource.com/platform/frameworks/native/+/ca08833d5ea99130797e10ad68a651b50e99da74%5E!/

It sounds like this is because they're deliberately trying not to expose this stuff to non-system apps. This makes it more obvious that we should hack our way in through this backdoor, at least on B2G, if not also on Android.
For the versions of Android which have this backdoor (and going forward), we should assert that it exists. I'm tempted to try to assert only on known versions, so newer versions would need an investigation done, but I don't know a good way to do this, if we don't necessarily have the sources yet.

Also, they left this note in the commit message: "There is still one option though, to disable KHR_fence_sync on some devices (which are more efficient without it)". It sounds like Android always uses native fences, except when they're unavailable, since some devices have worse efficiency with fence_syncs. I'm OK with miming this if we include a comment stating why in the code.

Comment 14

[Kelsey Gilbert [:jgilbert]]

Maybe a good way to implement this is by adding a new entry in our EGL symbol table for this, and in our fQueryString function, use fQueryStringANDROID if it exists. (and assert that it does on >Android17 B2G)

Comment 15

[Sotaro Ikeda [:sotaro]]

> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +149,5 @@
> > +    }
> > +
> > +    // When Android native fences are available, try
> > +    // them first since they're more likely to work.
> > +    if (mEGL->IsExtensionSupported(GLLibraryEGL::ANDROID_native_fence_sync)) {
> 
> In a different bug, it might be better to add direct Fence object handling.
> Getting the Fence and deliver it to Compositor seems better for the
> performance.

Created Bug 1001417 for it.

Comment 16

[Sotaro Ikeda [:sotaro]]

> 
> ::: gfx/gl/SharedSurfaceGralloc.cpp
> @@ +149,5 @@
> > +    }
> > +
> > +    // When Android native fences are available, try
> > +    // them first since they're more likely to work.
> > +    if (mEGL->IsExtensionSupported(GLLibraryEGL::ANDROID_native_fence_sync)) {
> 
> In a different bug, it might be better to add direct Fence object handling.
> Getting the Fence and deliver it to Compositor seems better for the
> performance. But I do not know which hardware support this extension.
> Nexus-4 and Nexus-5 do not support it.

By applying attachment 8412123 [details] [diff] [review], I confirmed that nexus-4 and nexus-5 support ANDROID_native_fence_sync.

Comment 17

[Michael Wu [:mwu]]

Attachment: Use fence objects in SharedSurfaceGralloc, v4

Comment 18

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8413065 [details] [diff] [review]
Use fence objects in SharedSurfaceGralloc, v4

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

R- only because I'm quite worried about accidentally disabling Firefox Mobile for some devices.

::: gfx/gl/GLLibraryEGL.cpp
@@ +203,5 @@
>          return false;
>      }
>  
> +#if ANDROID_VERSION >= 17
> +    MOZ_RELEASE_ASSERT(mSymbols.fQueryStringImplementationANDROID,

I'm worried about this being potentially problematic for the long tail of Android phones, since someone hitting this would render them unable to use Firefox Mobile at all. Ideally, we'd collect info about this, via telemetry or similar.
We can probably handle this by having this be nightly-only, and bake for a cycle. If we get through a few weeks with no crashes here, we're probably safe, and can make this unconditionally assert. As such, we should wait until after uplift to take this.

We can take a version of this patch with this as DEBUG-only MOZ_ASSERT immediately, but MOZ_RELEASE_ASSERT would need to be carefully baked on Nightly.

Comment 19

[Michael Wu [:mwu]]

(In reply to Jeff Gilbert [:jgilbert] from comment #18)
> > +#if ANDROID_VERSION >= 17
> > +    MOZ_RELEASE_ASSERT(mSymbols.fQueryStringImplementationANDROID,
> 
> I'm worried about this being potentially problematic for the long tail of
> Android phones, since someone hitting this would render them unable to use
> Firefox Mobile at all.

This actually doesn't apply to Firefox for Android. ANDROID_VERSION is really for Gonk use rather than Android. I can add a check for MOZ_WIDGET_GONK if that helps you. I expect ANDROID_VERSION to be either 0 or 9 on Android.

Comment 20

[Kelsey Gilbert [:jgilbert]]

(In reply to Michael Wu [:mwu] from comment #19)
> (In reply to Jeff Gilbert [:jgilbert] from comment #18)
> > > +#if ANDROID_VERSION >= 17
> > > +    MOZ_RELEASE_ASSERT(mSymbols.fQueryStringImplementationANDROID,
> > 
> > I'm worried about this being potentially problematic for the long tail of
> > Android phones, since someone hitting this would render them unable to use
> > Firefox Mobile at all.
> 
> This actually doesn't apply to Firefox for Android. ANDROID_VERSION is
> really for Gonk use rather than Android. I can add a check for
> MOZ_WIDGET_GONK if that helps you. I expect ANDROID_VERSION to be either 0
> or 9 on Android.

MOZ_WIDGET_GONK would be nice, even if it's actually only serving as clarification. :)

Comment 21

[Michael Wu [:mwu]]

Attachment: Use fence objects in SharedSurfaceGralloc, v5

Now also checking MOZ_WIDGET_GONK for the sake of clarity.

Comment 22

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/34f9b78948b9

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out in https://hg.mozilla.org/integration/b2g-inbound/rev/dce9bf32da45 for breaking pretty much everything on Android: https://tbpl.mozilla.org/php/getParsedLog.php?id=39314225&tree=B2g-Inbound and on B2G: https://tbpl.mozilla.org/php/getParsedLog.php?id=39313513&tree=B2g-Inbound

Comment 24

[Michael Wu [:mwu]]

Attachment: Use fence objects in SharedSurfaceGralloc, v6

Accidentally made not having the backdoor QueryString fatal. This should fix it. Going to run through try.

Comment 25

[Michael Wu [:mwu]]

Attachment: Use fence objects in SharedSurfaceGralloc, v7

Passes try. The only change is to the way eglQueryStringImplementationANDROID is found.

Comment 26

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8420703 [details] [diff] [review]
Use fence objects in SharedSurfaceGralloc, v7

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

Thanks, but remove the trychooser stuff!

Comment 27

[Michael Wu [:mwu]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b1ef023f690c

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b1ef023f690c

Comment 29

[Jeff Muizelaar [:jrmuizel]]

This is needed to avoid having to wait for the current frame before continuing in the content process. Waiting for the frame to finish can cause a significant delay in more complicated webgl apps.

Comment 30

[Wayne Chen [:xwaynec]]

hi Michael,

We can't find string 'QueryStringImplementationANDROID' in android 4.2.2_r1 base.
But we can find libs/EGL/eglApi.cpp:EGLAPI const char* eglQueryStringImplementationANDROID(EGLDisplay dpy, EGLint name). in flame.

However we encounter boot problem in vixen project...

05-26 05:48:08.629 I/Gecko   (  483): Attempting load of libEGL.so
05-26 05:48:08.629 I/Gecko   (  483): Can't find symbol '_Z35eglQueryStringImplementationANDROIDPvi'.
05-26 05:48:08.629 F/MOZ_Assert(  483): Assertion failure: mSymbols.fQueryStringImplementationANDROID (Couldn't find eglQueryStringImplementationANDROID), at ../../../gecko/gfx/gl/GLLibraryEGL.cpp:224

Thanks
Sincerely,
Wayne

Comment 31

[Preeti Raghunath(:Preeti)]

Is this is a regression? OR is it needed for a partner app?

Comment 32

[Sotaro Ikeda [:sotaro]]

This is a regression caused by this bug. I checked the flatfish(android v4.2)'s libEGL.so. It does not have eglQueryStringImplementationANDROID function.

Comment 33

[Jason Smith [:jsmith]]

(In reply to Sotaro Ikeda [:sotaro] from comment #32)
> This is a regression caused by this bug. I checked the flatfish(android
> v4.2)'s libEGL.so. It does not have eglQueryStringImplementationANDROID
> function.

Okay - what's the user impact if we don't fix this?

Comment 34

[Milan Sreckovic [:milan] (needinfo for best results)]

We need this and some of the dependent bugs to reach the performance goal for a partner game.

Comment 35

[Michael Wu [:mwu]]

(In reply to Preeti Raghunath(:Preeti) from comment #31)
> Is this is a regression? OR is it needed for a partner app?

This is not a regression. Sotaro was responding to Wayne's comment. It's necessary for performance per comment 34.

Comment 36

[Sotaro Ikeda [:sotaro]]

(In reply to Michael Wu [:mwu] from comment #35)
> (In reply to Preeti Raghunath(:Preeti) from comment #31)
> > Is this is a regression? OR is it needed for a partner app?
> 
> This is not a regression. Sotaro was responding to Wayne's comment. It's
> necessary for performance per comment 34.

Thanks for the comment. My comment was confusing. mwu's comment is correct.

Comment 37

[Michael Wu [:mwu]]

Attachment: Don't assert on 4.2

Turns out that this was added for 4.3, not 4.2. We have a few devices on 4.2, though they're not phones.

Comment 38

[Michael Wu [:mwu]]

I think 4.2 also has support, but there's no way to detect it at runtime.

Comment 39

[Michael Wu [:mwu]]

Followup pushed:

https://hg.mozilla.org/integration/b2g-inbound/rev/c8b0960cc5f7

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8b0960cc5f7

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/54e94bc1fe2c