Closed Bug 1137305 Opened 5 years ago Closed 3 years ago

Remove option to build mozilla-central without skia

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: gw280, Assigned: glandium)

References

Details

(Whiteboard: gfx-noted)

Attachments

(2 files)

Do we want to continue supporting this as a build option? If so, why?

At the moment none of the tier 1 platforms are built without Skia enabled, so stuff like bug 1136958 happen.

I think we should remove the option to build without Skia, but I'd like to get an idea of the impact this would have.
Message to dev-platform is probably a good idea at this point.
(Why a separate bug?)

Another data point against this: SkUserConfig.h is arm/x86 centric. I mentioned that building skia worked on platforms where it was currently disabled, I happened to have tested on arm64... which SkUserConfig.h actually takes care of...
I wanted us to have a central place where the wider conversation of "do we want to have skia as optional" could be had, and kept track of.
Whiteboard: gfx-noted
See Also: → 1296524
Does Skia build on big endian now? Adding Steve and Landry.
Lee, Mason, this should probably be on your radar.
The one snag here is Skia is not verifiably working on big-endian builds, so whichever third party was maintaining those had to force Cairo as the backend. But I guess so long as Cairo can still be forced as the backend regardless of whether or not Skia is used, this could work. Making Skia actually work with big-endian I don't think is an undertaking we've ever investigated yet, so I can't comment on the feasibility other than that we have no official way to test it or the right dev hardware for it.
We looked into big endian and decided it wasn't in our interest to invest the engineering effort. I think the endian issue only affected SkiaGL rather than Skia itself, so this probably isn't too much of a problem.
No, I remember the SkiaGL bug, but I thought there were other issues with big endian Skia. Steve or Landry would know better because I don't use it in OS X/ppc TenFourFox, but this is also because it glitches badly with the 10.4 SDK (we use a customized CoreGraphics backend with Cairo for printing, and don't build Skia currently at all).

For the big-endian Linux/*BSD case building Skia but actually using Cairo should certainly suffice, but my years sniffing around this community makes me think two separate graphics pathways doing the same thing won't co-exist for long.
I haven't been keeping up with the routine ppc64 build breakages in the pasts few months but going back to the last version that had built on ppc64 (ddb6b30) --enable-skia results in compile failures

 error: #error "need 32bit packing to be either RGBA or BGRA"


The discussion in Bug 1005535 is worth reading
According to my comments at the time actually using skia wasn't working so well

Bug 1144632 is addresses issues getting skia to work on big endian might be what I am seeing
So, I think what we're going to do for now is to make skia opt-out instead of opt-in. That'll mean changing the configure flag from --enable-skia to --disable-skia, and setting it to disable on whichever platforms it absolutely needs to be disabled on (I'm thinking ppc for now).

We can then revisit deprecating the option to build without Skia once we have a better idea of which platforms need Skia disabled in order to function.
Probably SPARC and aarch64 also, since there are still SPARC builders (on both Linux/*BSD and Solaris, at least so far).
I think aarch64 should be fine, but I agree that SPARC will probably be an issue.
Assignee: nobody → gwright
Attachment #8784965 - Flags: review?(ted)
Comment on attachment 8784965 [details] [diff] [review]
0001-Bug-1137305-Enable-Skia-by-default-allow-opt-out-usi.patch

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

::: old-configure.in
@@ +5206,5 @@
>  
> +if test "${CPU_ARCH}" = "ppc" -o \
> +        "${CPU_ARCH}" = "ppc64" -o \
> +        "${CPU_ARCH}" = "sparc" -o \
> +        "${CPU_ARCH}" = "sparc64"; then \

There are other big-endian platforms. Also, switching the name of the variable is not something we do.

I'd rather move --enable-skia to python configure, and do the right test there (we now have a variable for endianness in python configure).

I'll take care of this.
Attachment #8784965 - Flags: review?(ted) → review-
Assignee: gwright → mh+mozilla
> Also, switching the name of the variable is not something we do.

FWIW, actually, in that case, it's fine because we weren't taking the value from the environment if there was one.
Comment on attachment 8785033 [details]
Bug 1137305 - Move --enable-skia{,-gpu} to python configure and enable skia by default on little-endian platforms.

https://reviewboard.mozilla.org/r/74338/#review72256
Attachment #8785033 - Flags: review?(cmanchester) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/d057781e9d66
Move --enable-skia{,-gpu} to python configure and enable skia by default on little-endian platforms. r=chmanchester
https://hg.mozilla.org/mozilla-central/rev/d057781e9d66
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Thanks for sorting this out.
Depends on: 1299485
You need to log in before you can comment on or make changes to this bug.