Closed Bug 868556 Opened 7 years ago Closed 7 years ago

We are not using gralloc on b2g anymore

Categories

(Core :: Graphics, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: bjacob, Assigned: bjacob)

References

(Depends on 2 open bugs)

Details

Attachments

(1 file)

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.
Blocks: 867813
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.
I'm guessing the inclusion of TextureHost.h in the ShadowLayerUtilsGralloc.h before the #define of MOZ_HAVE_SURFACEDESCRIPTORGRALLOC could be related to this?
(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...
Holy cow, good catch!!

So it would've been my fault... trying that.
Hm, no, makes no difference.
Right, I imagine this is set as a build flag somewhere?  Anyway, probably a red herring anyway.
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.
Attached patch crazynessSplinter Review
There is enough crazyness here to generate a gravitational field.

In fact, this caused our entire gralloc path to collapse.
Attachment #745397 - Flags: review?(nical.bugzilla)
Note to self: we want to fix this in a better way. But I also want to leave the office now.
Comment on attachment 745397 [details] [diff] [review]
crazyness

Go, but please fix better next week :)
Attachment #745397 - Flags: review?(nical.bugzilla) → review+
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?
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.
Assignee: nobody → bjacob
Target Milestone: --- → mozilla23
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
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).
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.
ok, will do as soon as possible.
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.
Green, relanded with that test marked as failing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/39c44b2b1ef5
https://hg.mozilla.org/mozilla-central/rev/39c44b2b1ef5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 868892
Duplicate of this bug: 869696
Depends on: 870193
Filed bug 870193 about the pretty fix.
Depends on: 869696
(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
You need to log in before you can comment on or make changes to this bug.