905141 - B2G emulator's OpenGL ES implementation needs to support BGRA textures

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Add support for the BGRA OpenGL extension to the B2G emulator

The OpenGL extension adding support for BGRA textures seems to be a hard requirement for Skia/GL, and seem to be supported by every device that we care about, except by the OpenGL ES environment simulated by the emulator that we use to run B2G reftests.

It would be quite bad to add special rendering paths avoiding the use of BGRA just to be able to run in this emulator: if we did that, we would effectively be running reftests in an artificial setup, using separate code paths from what is used in the real world.

So instead, the better solution is for us to patch the emulator to add support for this missing OpenGL extension. The attached patch does that.

Without this patch, with SkiaGL on B2G, all reftests fail.
With this patch, with SkiaGL on B2G, all reftests succeed.

Questions are:
 - how do we deploy this to our own test slaves?
 - how do we land this in the b2g repo?
 - how do we attempt to upstream this?

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Note: if this patch seems surprisingly small, that is because the BGRA extension was already mostly implemented. It just was missing a small piece of code to accomodate a difference between OpenGL ES and non-ES: whereas OpenGL ES always wants internalformat==format, OpenGL non-ES always wants internalformat==RGBA even with format==BGRA. That is presumably the reason why this extension wasn't exposed in the extensions string, so the other hunk in this patch just appends it to the extensions string.

Comment 2

[Benoit Jacob [:bjacob] (mostly away)]

Comment on attachment 790176 [details] [diff] [review]
Add support for the BGRA OpenGL extension to the B2G emulator

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

Adding James as reviewer for the OpenGL aspects.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Vicamo & Michael: please accept this patch in the B2G/sdk/ git repository. Landing this, and deploying this on our test slaves, is a prerequisite for our project to turn on Skia/GL on B2G, which we aim to complete in 3 weeks from now for B2G 1.2. The reason why this is required is that Skia/GL does require BGRA support (James just confirmed he checked that with the Skia developers).
Other reasons why landing this patch on the git repo is reasonable:
 - It is very small;
 - It is likely to be upstreamed soon (although we don't want to be blocked on upstreaming it) because the Skia team at Google is likely to want it --- as without this patch, it is impossible to use Skia/GL in the Android emulator.

Comment 4

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

Comment on attachment 790176 [details] [diff] [review]
Add support for the BGRA OpenGL extension to the B2G emulator

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

Yup, looks good

Comment 5

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Benoit Jacob [:bjacob] from comment #3)
> Vicamo & Michael: please accept this patch in the B2G/sdk/ git repository.

We're directly using some pinned commit 0d8f973[1](2 commits behind AOSP ics-plus-aosp branch) for ICS Gonk and AOSP tag "android-4.2.2_r1" for JB.  To land this patch we need:

  1. create branches in android-sdk[2].  Branch names has to be determined before
     you can proceed to following steps.
  2. send a PR to b2g-manifest.git[3], update emulator.xml for ICS Gonk and emulator-jb.xml for JB Gonk.
  3. send two PRs to android-sdk, for ICS/JB separately.

I can help the process if mwu has a positive response to 1).

[1]: https://android.googlesource.com/platform/sdk/+/0d8f973905a4925e00eb9e32887c192b2148b29f
[2]: https://github.com/mozilla-b2g/android-sdk
[3]: https://github.com/mozilla-b2g/b2g-manifest

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

Comment on attachment 790176 [details] [diff] [review]
Add support for the BGRA OpenGL extension to the B2G emulator

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

Have no solid idea what's this for, but will help if mwu approves it.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

OK, thanks for the feedback, based on it I'll wait for Michael's feedback before taking further action; in parallel, I'm also looking into upstreaming this change.

Comment 8

[Michael Wu [:mwu]]

For ICS:

Create a branch based on the current hardcoded revision named emu-fix. (this just copies branch name used for jrmuizel's fix - see bug 716859 )

For JB:

Create a branch based on the tag android-4.3_r2.1 named b2g-4.3_r2.1 .

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

(In reply to Michael Wu [:mwu] from comment #8)
> For ICS:
> 
> Create a branch based on the current hardcoded revision named emu-fix. (this
> just copies branch name used for jrmuizel's fix - see bug 716859 )

https://github.com/mozilla-b2g/android-sdk/pull/2

> 
> For JB:
> 
> Create a branch based on the tag android-4.3_r2.1 named b2g-4.3_r2.1 .

https://github.com/mozilla-b2g/android-sdk/pull/1

Note that these pull requests are 'big' because I had to pull from the upstream Google repository to get the right base changesets, as they didn't exist already in the mozilla-b2g/ repo.

All of the changesets from these pull requests are from the upstream Google repo, except for the last one which is the patch discussed here.

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Michael, did I get this straight?

Comment 11

[Michael Wu [:mwu]]

I've generated new branches you can use for your pull requests, which create a much cleaner result.

You can tie pull requests back to the bug using https://addons.mozilla.org/en-US/firefox/addon/github-tweaks-for-bugzilla/ .

Releng also needs to add the android-sdk repo to their mirror list. (so it's on git.mozilla.org)

Once we have these things, we can modify the emulator manifests to point to the appropriate repos.

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Thanks! Here are the pull requests for android-sdk:

https://github.com/mozilla-b2g/android-sdk/pull/3
https://github.com/mozilla-b2g/android-sdk/pull/4

(Sorry that I didn't use the firefox extension)

Comment 13

[Michael Wu [:mwu]]

Pull requests for android-sdk merged.

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

Filed rel-eng bug 910383.

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

Manifests pull requests:

https://github.com/mozilla-b2g/b2g-manifest/pull/94

Comment 16

[Benoit Jacob [:bjacob] (mostly away)]

OK, updated pull request:

https://github.com/mozilla-b2g/b2g-manifest/pull/95

Comment 17

[Michael Wu [:mwu]]

b2g-manifest pull request merged. Hopefully it sticks.

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Increase fuzziness of some WebGL reftests on B2G

Land this on all the trees!

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Pushed to b2g-inbound: https://hg.mozilla.org/integration/b2g-inbound/rev/1be9ddeefd99

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1be9ddeefd99