998749 - SVG filters are busted on big-endian

Status: NEW

(Core :: SVG, defect)

Description

[Cameron Kaiser [:spectre]]

Attachment: Endian big on thing right the do

After the Moz2D switchover in bug 924103, SVG filters went bad on big-endian. The problem is even though the code uses offsets for selecting the channel, it has the wrong offsets for big-endian. Simple fix.

Comment 1

[Markus Stange [:mstange]]

Comment on attachment 8409407 [details] [diff] [review]
Endian big on thing right the do

Huh. So, on big endian, SurfaceFormat::B8G8R8A8 actually means A8R8G8B8? There are tons of places in FilterProcessingSIMD-inl.h, for example, where I don't make use of the B8G8R8A8_COMPONENT_BYTEOFFSET_* consts, and all those places would also need a big endian version. Can we instead make sure that the input surfaces come with their data laid out in such a way that the existing code just works?

Bas, how do you think this should work?

Comment 2

[Bas Schouten (:bas.schouten)]

(In reply to Markus Stange [:mstange] from comment #1)
> Comment on attachment 8409407 [details] [diff] [review]
> Endian big on thing right the do
> 
> Huh. So, on big endian, SurfaceFormat::B8G8R8A8 actually means A8R8G8B8?
> There are tons of places in FilterProcessingSIMD-inl.h, for example, where I
> don't make use of the B8G8R8A8_COMPONENT_BYTEOFFSET_* consts, and all those
> places would also need a big endian version. Can we instead make sure that
> the input surfaces come with their data laid out in such a way that the
> existing code just works?
> 
> Bas, how do you think this should work?

So, in my opinion we should offer consistent -byte- offsets. I don't care what it looks like when you look at it as a uint32_t. I'm not sure we could teach cairo to do that though, sadly.

Comment 3

[Cameron Kaiser [:spectre]]

(In reply to Markus Stange [:mstange] from comment #1)
> Huh. So, on big endian, SurfaceFormat::B8G8R8A8 actually means A8R8G8B8?

I'm not sure if that's the intention, but that appears to be the case in practice (i.e., I'm getting a host-ordered surface back, which on big-endian CPUs would indeed be ARGB instead of BGRA).

There are several other related bugs to this I'm trying to solve. One of them that's driving me up the wall is the New Tab thumbnails turning blue (because the alpha is always 255, so it makes the blue channel always 255 in the thumbnail which has a different channel order). If you have an idea about where I can look for this, please advise.

Anyway, I agree this is probably a bigger issue than this particular manifestation, but I just don't know this code very well to come up with more than bandaids.

> There are tons of places in FilterProcessingSIMD-inl.h, for example, where I
> don't make use of the B8G8R8A8_COMPONENT_BYTEOFFSET_* consts, and all those
> places would also need a big endian version.

We don't use the SIMD stuff yet, just the Scalar version (unless the Scalar one uses the inlines in that file). I *am* working on an AltiVec version, though, so it's helpful to know in advance that this will be an issue.

Comment 4

[Cameron Kaiser [:spectre]]

Followup: it does look like other things predicated on this assumption *are* breaking; bug 933030 was the cause of my "blue" thumbnails (the old Thebes code worked; the new one, which apparently assumes an underlying BGRA representation, does not). I can file a separate bug on that unless you want to make this a bigger bug about problems with underlying representation. For the time being, I've put the Thebes code back in the TenFourFox local changesets for shipping purposes.

Comment 5

[Markus Stange [:mstange]]

Jeff, do you have any thoughts on how this should work?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

(In reply to Markus Stange [:mstange] from comment #5)
> Jeff, do you have any thoughts on how this should work?

Let me think it over.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Bas and I talked this over a bit. It seems like the right thing to do is to have CAIRO_FORMAT_ARGB32 convert to SurfaceFormat::A8R8G8B8 on big endian and deal with the fall out from that. I'm told that skia currently has trouble supporting big endian so some effort may need to be invested there.

Comment 8

[Cameron Kaiser [:spectre]]

Not knowing that code well, I assume you mean putting a byteswap routine in somewhere?

Skia does have big-endian issues. However, the OS X/ppc port doesn't use Skia (we're 100% Cairo with a modified CG target as a fallback). This would be mostly an issue for Linux/ppc et al.

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Cameron Kaiser [:spectre] from comment #8)
> Not knowing that code well, I assume you mean putting a byteswap routine in
> somewhere?
> 
> Skia does have big-endian issues. However, the OS X/ppc port doesn't use
> Skia (we're 100% Cairo with a modified CG target as a fallback). This would
> be mostly an issue for Linux/ppc et al.

No, I mean more something like having a SurfaceFormat::ARGB32 that is equal to SurfaceFoomat::B8G8R8A8 on little endian and SurfaceFormat::A8R8G8B8 on big endian. Then replacing the uses of SurfaceFormat::B8G8R8A8 with SurfaceFormat::ARGB32 where the data is actually stored in native endian.

e.g. ImageFormatToSurfaceFormat convert gfxImageFormat::ARGB32 to SurfaceFormat::B8G8R8A8 or SurfaceFormat::A8R8G8B8 depending on the endianness.

Comment 10

[Cameron Kaiser [:spectre]]

I see. How would I know which places those are? Are there any tipoffs in the code that would suggest this can stay native-endian?

Comment 11

[Steve Singer (:stevensn)]

Skia on big-endian (linux) builds with my patch on Bug 1005535  seems to be working with gfx.content.azure.backends=skia  (though 3b5fb4abaa3f did break some things, see my comments on the above mentioned bug)

I haven't come across any blue thumbnails lately but if there is anything you want me to test let me know.

Comment 12

[Cameron Kaiser [:spectre]]

I'm not currently working on this; I'm not sure if it's still broken.

Comment 13

[msirringhaus]

Attachment: mozilla-bmo998749.patch

Yes, this is still an issue and yes the original patch still more or less works.
It has to be slightly adopted for the new macros.

Comment 14

[Cameron Kaiser [:spectre]]

Unfortunately I'm not running trunk on any big-endian system currently. I'd have to defer this to someone who is.