1296443 - Check if we're using Skia content when ImageLib is initialized so we don't have to ask gfxPlatform later

Status: RESOLVED INVALID

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Because Skia doesn't support RGBX surfaces, we have to check whether we're using the Skia content backend before creating them. (See bug 1282496 for the nitty gritty.) However, checking if we're using Skia content requires interacting with gfxPlatform, which in turn requires that we're on the main thread and that shutdown hasn't started. That's not ideal, because it forces us to do more work on the main thread, and in situations where we can't guarantee that we're on the main thread we've had to penalize non-Skia backends. (Specifically, we do a memset() to ensure that RGBX surfaces have 0xFF alpha, but only Skia requires it; the problem is that we can't safely check whether we're using Skia content off-main-thread right now, so we always have to do it.)

Let's just fix this by checking if we're using Skia content at ImageLib module initialization time.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Determine if we're using Skia content at ImageLib initialization so we don't have to interact with gfxPlatform later.

Here's the patch. We just check the content backend when the ImageLib module is
initialized and cache it. By handling this as a getter that transparently does
the caching the first time it's called, we eliminate the need for tests to
initialize the ImageLib module just because they use imgFrame. (They'll still
need to be on the main thread, of course, but they are.)

The lambda in ContentBackendIsSkia() is just there to let us assert that we're
on the main thread the first time the function is called. If we used a separate
function instead, we'd then have to worry that some knucklehead would call that
function by mistake and introduce a hard-to-reproduce crash at shutdown.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Try job here:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b24d98f4984

Comment 3

[Seth Fowler [:seth] [:s2h]]

Second try job here, because this needs a Windows (aka, non-Skia) try job as well:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dd5474851d3a

Comment 4

[Mason Chang [Inactive] [:mchang]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #0)
> Because Skia doesn't support RGBX surfaces, we have to check whether we're
> using the Skia content backend before creating them. (See bug 1282496 for
> the nitty gritty.) However, checking if we're using Skia content requires
> interacting with gfxPlatform, which in turn requires that we're on the main
> thread and that shutdown hasn't started. That's not ideal, because it forces
> us to do more work on the main thread, and in situations where we can't
> guarantee that we're on the main thread we've had to penalize non-Skia
> backends. (Specifically, we do a memset() to ensure that RGBX surfaces have
> 0xFF alpha, but only Skia requires it; the problem is that we can't safely
> check whether we're using Skia content off-main-thread right now, so we
> always have to do it.)
> 
> Let's just fix this by checking if we're using Skia content at ImageLib
> module initialization time.

When does ImageLib init? The problem is with d2d, we will default to cairo, do some tests to see if D2D is enabled, and otherwise go back to cairo. Once skia on Windows lands, the default will be cairo. What I'm worried about is that we'll check if skia exists, go to d2d after the tests are done, and you'll have cached the wrong value.

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Mason Chang [:mchang] from comment #4)
> When does ImageLib init? The problem is with d2d, we will default to cairo,
> do some tests to see if D2D is enabled, and otherwise go back to cairo. Once
> skia on Windows lands, the default will be cairo. What I'm worried about is
> that we'll check if skia exists, go to d2d after the tests are done, and
> you'll have cached the wrong value.

It inits late in initialization of the layout module. I'm not sure when that is relative to the D2D tests you're talking about.

Even more fun, I see from the try failures that gfxPlatform attempts to initialize ImageLib. This creates a circular dependency. We're doing that here:

https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxPlatform.cpp?q=%2Bfunction%3AgfxPlatform%3A%3AInit%28%29&redirect_type=single#752

I'm not sure *why* we're doing that, really, but probably some code somewhere depends on it. =\

At any rate, I'm sure we can cut that knot. Are the tests that you're talking about done by the time gfxPlatform::Init() finishes? If so, then there's no problem. If not, what's a more appropriate time to grab this information?

Comment 6

[Mason Chang [Inactive] [:mchang]]

From irc, we should double check to see when we decide the d2d backend is turned on in the initialization phase. Clearing ni.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:aosmond, could you have a look please?
For more information, please visit auto_nag documentation.