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?

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 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.

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 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

(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 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.

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.

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 .

(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.

Michael, did I get this straight?

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.

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)

Pull requests for android-sdk merged.

Filed rel-eng bug 910383.

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

OK, updated pull request: https://github.com/mozilla-b2g/b2g-manifest/pull/95

b2g-manifest pull request merged. Hopefully it sticks.

Land this on all the trees!

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