900201 - Implement GfxInfo on gonk

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

Attachment: Implement GfxInfo on gonk

I don't like GfxInfo... but the Canvas 2D mochitest relies on it to determine where to increase tolerance to accomodate inaccuracies of certain rendering paths. That is the reason for at last some (and possibly all) of our canvas 2D mochitest errors on TBPL, with Skia/GL.

GfxInfo is not currently implemented on Gonk. So the path of least resistance is likely to add a dummy implementation there. All the nontrivial stuff needed for the mochitest, comes from gfxPlatform manually adding properties to the JS object returned by GfxInfo::GetInfo(). So I expect that just this naive, trivial implementation will be enough.

Will ask for review after I get results from this try-run, as I don't know how to exercise GfxInfo locally (short of learning how to run mochitests locally on device... not in a rush).

https://tbpl.mozilla.org/?tree=Try&rev=e8dbc935a286

Comment 1

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

M1 is green with this patch, asking for review!

Comment 2

[(no longer active)]

Comment on attachment 783989 [details] [diff] [review]
Implement GfxInfo on gonk

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

::: widget/gonk/GfxInfo.cpp
@@ +10,5 @@
> +// Grrr, this should never have ended up #ifdef DEBUG... but it's how it is in
> +// existing GfxInfo implementations.
> +#ifdef DEBUG
> +NS_IMPL_ISUPPORTS_INHERITED1(GfxInfo, GfxInfoBase, nsIGfxInfoDebug)
> +#endif

This should go away as well.

@@ +14,5 @@
> +#endif
> +
> +GfxInfo::GfxInfo()
> +{
> +}

So should this.

::: widget/gonk/GfxInfo.h
@@ +48,5 @@
> +
> +#ifdef DEBUG
> +  NS_DECL_ISUPPORTS_INHERITED
> +  NS_DECL_NSIGFXINFODEBUG
> +#endif

Let's not repeat the same mistake here, I think you should remove these, and then you can remove the three using declarations above as well.

Comment 3

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

Thanks for the useful review comments: at first I didn't realize that I could simply omit these stupid ifdef DEBUG blocks.

http://hg.mozilla.org/integration/mozilla-inbound/rev/716e54fb5d09

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Funny story, backed out for B2G debug bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0dde7bd3e303

https://tbpl.mozilla.org/php/getParsedLog.php?id=26461278&tree=Mozilla-Inbound

Comment 5

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

Ouch, forgot that part.

This should stick:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0eeacb439f2

Comment 6

[Ed Morley [:emorley]]

Backed out for B2G emulator build failures:
https://tbpl.mozilla.org/php/getParsedLog.php?id=26535990&tree=Mozilla-Inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e980d89c9e1e

Please can you test on try (or locally) before trying again?

Comment 7

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

Sorry... I tried locally, but not with the right .userconfig.

Comment 8

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

Try push: https://tbpl.mozilla.org/?tree=Try&rev=f33e6435f3f6

Comment 9

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

Attachment: final patch that should hopefully stick!

Comment 10

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

There were 2 problems with the reftests:
 - reftest.js still had a #ifdef B2G causing it to not even try GfxInfo on B2G.
 - with SkiaGL, the canvas reftest.list needs some fuzzy.

mozilla-graphics try push: https://tbpl.mozilla.org/?tree=Try&rev=975bfb05089d
mozilla-central try push: https://tbpl.mozilla.org/?tree=Try&rev=96d3afc30262

Comment 11

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

Alright. There were 2 issues:
 - another change was needed in a SVG reftest.list;
 - an unwanted android.json change corrupted the mochitest results.

mozilla-graphics try push: https://tbpl.mozilla.org/?tree=Try&rev=38dc16729eeb
mozilla-central try push: https://tbpl.mozilla.org/?tree=Try&rev=c9d231e7a278

(reftest orange is still expected on graphics pending the push of bug 905141 to the slaves)

Comment 12

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

greeeeeeeeeeeeeeen! push!

Comment 13

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

argh, closed tree, but this is ready to go as soon as the tree reopens. Will push myself though, as 4 different bugs need to be pushed simultaneously.

Comment 14

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

https://tbpl.mozilla.org/?tree=Try&rev=c9d231e7a278

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/31e7c6415e35

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 790244 [details] [diff] [review]
final patch that should hopefully stick!

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

For future reference:

::: widget/gonk/GfxInfo.cpp
@@ +53,5 @@
> +/* readonly attribute DOMString adapterRAM; */
> +NS_IMETHODIMP
> +GfxInfo::GetAdapterRAM(nsAString & aAdapterRAM)
> +{
> +  aAdapterRAM.AssignLiteral("");

Should be Truncate()

::: widget/gonk/GfxInfo.h
@@ +4,5 @@
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef __mozilla_widget_GfxInfo_h__

Should be "GfxInfo_h", as it's included as #include "GfxInfo.h"

Comment 17

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

Thanks, I'll try to think about that next time.

Comment 18

[Douglas Crosher [:dougc]]

This change seems to be breaking WebGL apps on B2G on the Nexus 4 (JB).  Builds before work, but builds just after fail with:

E/GeckoConsole(  769): [JavaScript Warning: "Error: WebGL: Can't get a usable WebGL context" {file: "app://crystalskull.gaiamobile.org/j3d.js" line: 52}]
E/GeckoConsole(  769): [JavaScript Error: "TypeError: gl is undefined" {file: "app://crystalskull.gaiamobile.org/j3d.js" line: 53}]

Comment 19

[Douglas Crosher [:dougc]]

(In reply to Douglas Crosher [:dougc] from comment #18)
> This change seems to be breaking WebGL apps on B2G on the Nexus 4 (JB). 
> Builds before work, but builds just after fail with:
> 
> E/GeckoConsole(  769): [JavaScript Warning: "Error: WebGL: Can't get a
> usable WebGL context" {file: "app://crystalskull.gaiamobile.org/j3d.js"
> line: 52}]
> E/GeckoConsole(  769): [JavaScript Error: "TypeError: gl is undefined"
> {file: "app://crystalskull.gaiamobile.org/j3d.js" line: 53}]

Simply removing the new entry to kWidgetContracts[] is enough to
get WebGL working again on m-c tip.

widget/gonk/nsWidgetFactory.cpp:

static const mozilla::Module::ContractIDEntry kWidgetContracts[] = {
...
// { "@mozilla.org/gfx/info;1", &kNS_GFXINFO_CID },

Comment 20

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

Interesting. That's likely caused by this naive GfxInfo impl not doing something it needs to do to indicate that WebGL is *not* blacklisted. My apologies, it didn't occur to me that that wouldn't be the default behavior. Anyway, the fix is likely to simply be implement GetFeatureStatusImpl setting *aStatus = FEATURE_OK unconditionally, or something like that. Sorry, I'm going to be in airports for much of the rest of the week-end, will do it if noone beats me to it.

Comment 21

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

Oh, I hadn't seen comment 19 when I wrote comment 20.

Comment 22

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

Hm, I am afraid that the fix suggested in comment 19 is just disabling GfxInfo altogether on B2G, which is not what we actually want to do; the right fix is still likely to be along the lines of what comment 20 suggests.

Comment 23

[Masatoshi Kimura [:emk]]

(In reply to Benoit Jacob [:bjacob] from comment #20)
> Interesting. That's likely caused by this naive GfxInfo impl not doing
> something it needs to do to indicate that WebGL is *not* blacklisted. My
> apologies, it didn't occur to me that that wouldn't be the default behavior.
> Anyway, the fix is likely to simply be implement GetFeatureStatusImpl
> setting *aStatus = FEATURE_OK unconditionally, or something like that.
> Sorry, I'm going to be in airports for much of the rest of the week-end,
> will do it if noone beats me to it.

GetFeatureStatusImpl already returns FEATURE_NO_INFO unconditionally. (There is no such thing as FEATURE_OK.)