868556 - We are not using gralloc on b2g anymore

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

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

The gralloc compositing path (GrallocTextureHostOGL) is unused in a B2G build with today's mozilla-central.

Logcat has this,

[Parent 928] ###!!! ASSERTION: GrallocBufferConstructor failed by returned null: 'Error', file /hack/mozilla-central/gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp, line 350

And the code there is

  PGrallocBufferChild* gc = AllocGrallocBuffer(aSize, aContent, &handle);
  if (!gc) {
    NS_ERROR("GrallocBufferConstructor failed by returned null");
    return false;
  }

And this is in ISurfaceAllocator, and ISurfaceAllocator::AllocGrallocBuffer is a stub that just returns null, so indeed there is no chance that we'd ever use gralloc with that code.

Comment 1

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

This must be a regression over the past week, even over the past 3-4 days. mozilla-central of monday or tuesday ran fine with gralloc.

Comment 2

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

I'm guessing the inclusion of TextureHost.h in the ShadowLayerUtilsGralloc.h before the #define of MOZ_HAVE_SURFACEDESCRIPTORGRALLOC could be related to this?

Comment 3

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

(In reply to Milan Sreckovic [:milan] from comment #2)
> I'm guessing the inclusion of TextureHost.h in the ShadowLayerUtilsGralloc.h
> before the #define of MOZ_HAVE_SURFACEDESCRIPTORGRALLOC could be related to
> this?

Or not...

Comment 4

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

Holy cow, good catch!!

So it would've been my fault... trying that.

Comment 5

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

Hm, no, makes no difference.

Comment 6

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

Right, I imagine this is set as a build flag somewhere?  Anyway, probably a red herring anyway.

Comment 7

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

This does feel like it should be set by the build system, but in fact, it isn't. It is just set in one particular header, ShadowLayerUtilsGralloc.h, and so code will or will not use the correct AllocGrallocBuffer function depending on whether it #included that header.

Pure craziness.

Comment 8

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

Attachment: crazyness

There is enough crazyness here to generate a gravitational field.

In fact, this caused our entire gralloc path to collapse.

Comment 9

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

Note to self: we want to fix this in a better way. But I also want to leave the office now.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 745397 [details] [diff] [review]
crazyness

Go, but please fix better next week :)

Comment 11

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

So related question -- how did we not catch this via any kind of testing?  Shouldn't -some- kind of perf test or pageload or something have shot way up?

Comment 12

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/529526e6e659

(In reply to Vladimir Vukicevic [:vlad] [:vladv] from comment #11)
> So related question -- how did we not catch this via any kind of testing? 
> Shouldn't -some- kind of perf test or pageload or something have shot way up?

I am not aware of performance (Talos or whatever) tests being run on B2G at the moment. A quick look at TBPL suggests we only run crashtests, reftests, xpcshell, and something called 'marionette'. Some googling suggests that marionette is able to do performance tests,
  https://developer.mozilla.org/en-US/docs/Marionette/Performance_tests
so perhaps this could be addressed.

Comment 13

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/deea5f24e17a for astonishingly bad timing. I noticed earlier today that someone had finally fixed the permaorange on b2g reftest-1, so I unhid it maybe 5 minutes before you broke bmp-size-1x1-1bpp.bmp which runs in reftest-1. 

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

Comment 14

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

I understand, but let me suggest that it is the re-enabling-of-the-reftest, not the re-enabling-of-gralloc, that should have been backed out. The present bug makes b2g-on-mozilla-central use a different graphics path than what we actually want to use (and use on consumer devices) and is considered to block investigation of other b2g-on-mozilla-central bugs (see blockees).

Comment 15

[Phil Ringnalda (:philor)]

The suite having been hidden is just an amusing coincidence, it has no actual practical meaning.

I don't care about that particular reftest (in fact, I hate *all* tests), caring about it is your job, but there's absolutely no way that I should be deciding that your patch is more important than passing tests. If you don't care about it, you just reland with a fails-if(B2G) annotation for it.

Comment 16

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

ok, will do as soon as possible.

Comment 17

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

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

The failure is limited to 1x1 images and is that the pixel is RGB(255,1,255) in one image and RGB(255,0,255) in the other image. So it's not the end of the world.

Comment 18

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

Green, relanded with that test marked as failing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/39c44b2b1ef5

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/39c44b2b1ef5

Comment 21

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

Filed bug 870193 about the pretty fix.

Comment 22

[Ed Morley [:emorley]]

(In reply to Benoit Jacob [:bjacob] from comment #18)
> Green, relanded with that test marked as failing:

The failing test is now passing again, as of the landing of bug 879149. fails-if annotation removed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78214f394a97