gfxImageFormat can be replaced with gfx::SurfaceFormat.

cairo_format_t and gfxImageFormat have their equivalent constants in the same order, so you can just cast between them, which is kind of nasty. This patch replaces all such casts with explicit conversions via calls to new conversion functions. These functions will be removed in a subsequent patch.

This value is never written anywhere, so it's not needed, and gfx::Surface doesn't have an equivalent.

I did this patch mechanically with |sed|. It won't compile on its own; it needs the next patch as well to fix up the compile errors. I will fold the two patches together before landing, but I separated the automated changes from the manual changes to make reviewing easier.

This patch does the following. - Changes gfxImageFormat to be a typedef to gfx::SurfaceFormat. This will be removed in the next patch. - Removes gfxCairoFormatToImageFormat() and gfxImageFormatToCairoFormat() and replace calls to them with CairoFormatToGfxFormat() and GfxFormatToCairoFormat(). - Removes SurfaceFormatToImageFormat() and ImageFormatToSurfaceFormat() and calls to them, because they're now both no-ops. - Removes ParamTraits<gfxImageFormat>. - Add namespace qualifiers to SurfaceFormat instances where necessary.

Now that gfxImageFormat is just a typedef for SurfaceFormat, it's straightforward to remove it. The patch also removes some typedefs such as TextureImage::ImageFormat.

Comment on attachment 8667640 [details] [diff] [review] (part 3a) - Convert all gfxImageFormat values to SurfaceFormat equivalents Review of attachment 8667640 [details] [diff] [review]: ----------------------------------------------------------------- I don't fully remember the details but we already bumped into complications when attempting to remove gfxImageFormat due to the fact that cairo/gfxImageSurface deals with endianness differently than Moz2D (or the latter doesn't at all, I don't remember exactly). We talked about this and the closest thing written down that I can find is somewhat formulated by Jeff here: https://bugzilla.mozilla.org/show_bug.cgi?id=1021628#c30 This patch (and subsequent versions since I think something has to be done about the endinanness business) should have Jeff's review.

Comment on attachment 8667640 [details] [diff] [review] (part 3a) - Convert all gfxImageFormat values to SurfaceFormat equivalents Review of attachment 8667640 [details] [diff] [review]: ----------------------------------------------------------------- As Nical wrote, gfxImageFormat::ARGB32 is only equivalent to SurfaceFormat::B8G8R8A8 on little endian. This patch needs to deal with this problem.

Let me see if I understand. Basically the problem is this: - cairo_format_t and gfxImageFormat describe values that are stored as native-endian; - SurfaceFormat, in contrast, describes values that are store as little-endian. So the obvious conversions (as done by CairoFormatToGfxformat(), GfxFormatToCairoFormat(), SurfaceFormatToImageFormat(), and ImageFormatToSurfaceFormat()) are only safe on little-endian. Presumably the existing code use gfxImageFormat enough that these bogus conversions are currently avoided? Using native endianness seems useful, because you don't need two constants. I guess there must be a reason why Moz2D doesn't do that. Perhaps the interactions with the actual OS graphics APIs require it? (I see constants like LOCAL_GL_BGRA.) Maybe Cairo internally converts CAIRO_FORMAT_ARGB32 to one or the other as necessary? How might I go about fixing this? Bug 1021628 comment 30 describes the necessary new values, but once they're added I'm not sure what to do after that. Does it require looking at every occurrence of B8G8R8A8 and R8G8B8A8 and seeing if something needs to be done? If that's the case I think it might be beyond me at the moment. BTW, are any of our standard platforms big-endian? If not, how would this new code be tested? Thank you.

Nicholas, as you have surmised, this is a bit involved and in a nasty state to start. Is this a background task for you, or are you depending on this bug to be fixed for something else?

(In reply to Milan Sreckovic [:milan] from comment #9) > Nicholas, as you have surmised, this is a bit involved and in a nasty state > to start. Is this a background task for you, or are you depending on this > bug to be fixed for something else? It's not blocking anything. I just thought I'd try to help the gfx team a bit so I've been working on some Moz2Dification bugs. I'd be interested if you can point to any other gfx bugs that might be suitable for non-gfx experts.

I am still interested in hearing answers to the questions in comment 8, if anybody knows them.

(In reply to Nicholas Nethercote [:njn] from comment #8) > Let me see if I understand. Basically the problem is this: > > - cairo_format_t and gfxImageFormat describe values that are stored as > native-endian; > > - SurfaceFormat, in contrast, describes values that are store as > little-endian. > > So the obvious conversions (as done by CairoFormatToGfxformat(), > GfxFormatToCairoFormat(), SurfaceFormatToImageFormat(), and > ImageFormatToSurfaceFormat()) are only safe on little-endian. Presumably the > existing code use gfxImageFormat enough that these bogus conversions are > currently avoided? Hopefully, yes. > > Using native endianness seems useful, because you don't need two constants. > I guess there must be a reason why Moz2D doesn't do that. Perhaps the > interactions with the actual OS graphics APIs require it? > (I see constants like LOCAL_GL_BGRA.) Maybe Cairo internally converts > CAIRO_FORMAT_ARGB32 to one or the other as necessary? > > How might I go about fixing this? Bug 1021628 comment 30 describes the > necessary new values, but once they're added I'm not sure what to do after > that. Does it require looking at every occurrence of B8G8R8A8 and R8G8B8A8 > and seeing if something needs to be done? If that's the case I think it > might be beyond me at the moment. I'll try to get consensus on the correct approach here during our meeting tomorrow. > > BTW, are any of our standard platforms big-endian? If not, how would this > new code be tested? No, none of our standard platforms are big-endian. However, there are people around that do test on big endian.

So we talked about this. Here's an untested patch that should let you just search and replace gfxImageFormat::ARGB32 with SurfaceFormat::ARGB32

> So we talked about this. Here's an untested patch that should let you just > search and replace gfxImageFormat::ARGB32 with SurfaceFormat::ARGB32 Thank you for posting the patch. I will incorporate and and rework my existing patches soon.

> Thank you for posting the patch. I will incorporate and and rework my > existing patches soon. Hmm, I think R5G6B5 needs similar handling.

> Hmm, I think R5G6B5 needs similar handling. Specifically, I *think* the following is necessary: - Need to add a G3B5R5G3 value to SurfaceFormat, to go alongside the existing R5G6B5 value. - Need to add an alias value to SurfaceFormat: - On big-endian: RGB16_565 = R5G6B5 - On little-endian: RGB16_565 = G3B5R5G3 Am I on the right track here? Also, I feel like gfx::SurfaceFormat desperately needs a comment explaining how to interpret the names of the its various values. E.g. what does "B8G8R8A8" mean? As a value in a register, which is the low byte and which is the high byte? As a value in memory, which byte comes first in memory? I think in both cases the leftmost part of the name represents the "low" part, e.g. for B8G8R8A8: - register: (a << 24) | (r << 16) | (g << 8) | (b << 0) - memory: [0: b, 1: g, 2: r, 3: a] Is that right? Maybe not... I'm now horribly confused by this.

AIUI, the SurfaceFormat value names use the OpenGL convention(?) of being named for the memory ordering. E.g. "B8G8R8A8" means the low byte in memory is blue and the high byte is alpha, and this is independent of the machine endianness. Which explains why we need new alias such as "ARGB32" whose value changes depending on endianness. I'm still totally confused by R5G6B5, though. I'm wondering if it should really be called B5G6R5.

(In reply to Nicholas Nethercote [:njn] from comment #16) > > Hmm, I think R5G6B5 needs similar handling. > > Specifically, I *think* the following is necessary: > > - Need to add a G3B5R5G3 value to SurfaceFormat, to go alongside the > existing R5G6B5 value. > > - Need to add an alias value to SurfaceFormat: > - On big-endian: RGB16_565 = R5G6B5 > - On little-endian: RGB16_565 = G3B5R5G3 > > Am I on the right track here? The R5G6B5 is afaik always a 16bit format in native endian so we don't need an additional enum. You could certainly make the argument for renaming the existing one to RGB16_565 so that's it's more clear in comparison to the format names. However, in practice it's not much of a problem because 565 tends to be unambiguous.

https://hg.mozilla.org/integration/mozilla-inbound/rev/feb0f7522ece30fdcc264ccb8df5e580c4f27a07 Bug 1209812 (part 1) - Remove casts between cairo_format_t and gfxImageFormat. r=nical. https://hg.mozilla.org/integration/mozilla-inbound/rev/2708f83e4c15836f7925e69a4227778ae9a1809e Bug 1209812 (part 2) - Remove gfxImageFormat::A1. r=nical.

https://hg.mozilla.org/mozilla-central/rev/feb0f7522ece https://hg.mozilla.org/mozilla-central/rev/2708f83e4c15

This is Jeff's patch, which needs review from another gfx peer.

As before, this is the automated part, and will be merged with part 6b before landing.

The manual fix-ups for part 6a.

Bas: comment 18 (and the preceding three or four comments) explain the rationale for the R5G6B5 change. The comments added in part 5 also help explain it.

Comment on attachment 8677873 [details] [diff] [review] (part 3) - Rename SurfaceFormat::R5G6B5 as RGB16_565 Review of attachment 8677873 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a big fan of this. I 'sort of' understand the reasoning, so I won't protest too hard, I would actually prefer something like R5G6B5_UINT16 or something like that.

> I'm not a big fan of this. I 'sort of' understand the reasoning, so I won't > protest too hard, I would actually prefer something like R5G6B5_UINT16 or > something like that. Is that an r+ if I change it to R5G6B5_UINT16? I just want to land these patches.

(In reply to Nicholas Nethercote [:njn] from comment #29) > > I'm not a big fan of this. I 'sort of' understand the reasoning, so I won't > > protest too hard, I would actually prefer something like R5G6B5_UINT16 or > > something like that. > > Is that an r+ if I change it to R5G6B5_UINT16? I just want to land these > patches. Sure, yes. Although maybe we should name it B5G6R5 then because that's what it would look like when viewed as a UINT16 on a little Endian platform.

There are three naming schemes used in SurfaceFormat, once these patches have been applied. (a) The values like B8G8R8A8 are named according to byte-by-byte ordering in memory. (b) The values like RGB24 are named according to full-value (16-bit or 32-bit) ordering. (Some of these are aliases to category (a) ones.) (c) The values like YUV and NV12 are odd "other" cases. Currently, R5G6B5 looks like a category (a) value, but it's really a category (b) value. This confused me deeply (see comment 16, comment 17, comment 18). That's why changing it to RGB16_565 is a good idea, because it makes it actually look like a category (b) one. (It's also the same name that Cairo uses, so there's consistency and precedence there.) Changing it to R5G6B5_UINT16 or B5G6R5 makes it still look like a category (a) value.

(In reply to Nicholas Nethercote [:njn] from comment #31) > There are three naming schemes used in SurfaceFormat, once these patches > have been applied. > > (a) The values like B8G8R8A8 are named according to byte-by-byte ordering > in memory. > > (b) The values like RGB24 are named according to full-value (16-bit or > 32-bit) ordering. (Some of these are aliases to category (a) ones.) > > (c) The values like YUV and NV12 are odd "other" cases. > > Currently, R5G6B5 looks like a category (a) value, but it's really a > category (b) value. This confused me deeply (see comment 16, comment 17, > comment 18). > > That's why changing it to RGB16_565 is a good idea, because it makes it > actually look like a category (b) one. (It's also the same name that Cairo > uses, so there's consistency and precedence there.) > > Changing it to R5G6B5_UINT16 or B5G6R5 makes it still look like a category > (a) value. So, my opinion on this is that we should fix all of them to the following convention: 1) If there's no suffix, they are in order as viewed as bytes. 2) If there -is- a suffix it as in order when viewed as -that- suffix. RGB24 should not exist in the new SurfaceFormat. RGB24 in particular is a disaster. If I reviewed that I apologize, let's call those: X8R8G8B8_UINT32 A8R8G8B8_UINT32 That would be consistent with naming: R5G6B5_UINT16

Bas and I agreed on the following. > enum class SurfaceFormat : int8_t { > // The following values are named to reflect layout of colors in memory, from > // lowest byte to highest byte. The 32-bit value layout depends on machine > // endianness. > // in-memory 32-bit LE value 32-bit BE value > B8G8R8A8, // [BB, GG, RR, AA] 0xAARRGGBB 0xBBGGRRAA > B8G8R8X8, // [BB, GG, RR, 00] 0x00RRGGBB 0xBBGGRR00 > R8G8B8A8, // [RR, GG, BB, AA] 0xAABBGGRR 0xRRGGBBAA > R8G8B8X8, // [RR, GG, BB, 00] 0x00BBGGRR 0xRRGGBB00 > A8R8G8B8, // [AA, RR, GG, BB] 0xBBGGRRAA 0xAARRGGBB > X8R8G8B8, // [00, RR, GG, BB] 0xBBGGRR00 0x00RRGGBB > > // The following values are endian-independent synonyms. The _UINT32 suffix > // indicates that the name reflects the layout when viewed as a uint32_t > // value. > #if MOZ_LITTLE_ENDIAN > A8R8G8B8_UINT32 = B8G8R8A8, // 0xAARRGGBB > X8R8G8B8_UINT32 = B8G8R8X8, // 0x00RRGGBB > #elif MOZ_BIG_ENDIAN > A8R8G8B8_UINT32 = A8R8G8B8 // 0xAARRGGBB > X8R8G8B8_UINT32 = X8R8G8B8, // 0x00RRGGBB > #else > # error "bad endianness" > #endif > > // The _UINT16 suffix here indicates that the name reflects the layout when > // viewed as a uint16_t value. In memory these values are stored using native > // endianness. > R5G6B5_UINT16, // 0bRRRRRGGGGGGBBBBB > > // This one is a single-byte, so endianness isn't an issue. > A8, > > // These ones are their own special cases. > YUV, > NV12, > > // This represents the unknown format. > UNKNOWN > }; I'll update my patches and hopefully land them soon.

Updated patch. Carrying over implicit r+ from Bas (see comment 33).

(reordering a couple of patches)

https://hg.mozilla.org/integration/mozilla-inbound/rev/c566d5a42e84a116835b574bc69b72f4a2ab64ea Bug 1209812 (part 3) - Rename SurfaceFormat::R5G6B5 as R5G6B5_UINT16. r=Bas. https://hg.mozilla.org/integration/mozilla-inbound/rev/3b546c86c824d971b8a888a9772bd4c241958880 Bug 1209812 (part 4) - Add comments to SurfaceFormat. r=jrmuizel,Bas.

https://hg.mozilla.org/integration/mozilla-inbound/rev/593bccc56191fe7d1fc0757257de5be103f142a8 Bug 1209812 (follow-up) - Android bustage fix on a CLOSED TREE. r=me.

I just landed parts 3 and 4, which rename SurfaceFormat::R5G6B5 and add comments to SurfaceFormat. I haven't yet landed the rest because, after some thought, I have zero confidence that they won't horribly break everything on big-endian platforms. Part of that is because I really don't understand part 5, which Jeff wrote. It just seems like not nearly enough changes to work as claimed, though I fully admit I am no expert on this. I also realized that this bit of part 6b: > Removes SurfaceFormatToImageFormat() and ImageFormatToSurfaceFormat() and > calls to them, because they're now both no-ops. is valid on little-endian, but not valid on big-endian. So that's bad.

Thinking some more: part 5 is the most dangerous patch in terms of possibly breaking big-endian platforms; the subsequent patches are almost entirely mechanical. Are there people we can ask to test part 5 for us, ahead of time? Alternatively, could we land part 5 and give it some time to bake (possibly let it ride the trains) and then if things go well land the later patches? I'm reluctant to land all of them at once because the later ones touch enough lines that backing them out later could be painful.

https://hg.mozilla.org/mozilla-central/rev/c566d5a42e84 https://hg.mozilla.org/mozilla-central/rev/3b546c86c824 https://hg.mozilla.org/mozilla-central/rev/593bccc56191

(In reply to Nicholas Nethercote [:njn] from comment #42) > Thinking some more: part 5 is the most dangerous patch in terms of possibly > breaking big-endian platforms; the subsequent patches are almost entirely > mechanical. Are there people we can ask to test part 5 for us, ahead of time? > > Alternatively, could we land part 5 and give it some time to bake (possibly > let it ride the trains) and then if things go well land the later patches? > I'm reluctant to land all of them at once because the later ones touch > enough lines that backing them out later could be painful. I think the second option is better. I wouldn't be surprised if we see some breakage on big endian so it would be good to discover it before changing everything.

And given the early branch, sounds like we're waiting for 45 before landing anything else...

Part 5 seems to be ok on try, though it's a bit hard to tell with so much intermittent orange at the moment: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d9d9cb0ab5e

https://hg.mozilla.org/integration/mozilla-inbound/rev/aa15cfe46e9cee89c0bca5a46c0d7ff4c0230f8b Bug 1209812 (part 5) - Add endian-neutral variants to SurfaceFormat. r=nical,Bas.

Now that part 5 has successfully made it to mozilla-central, I will wait for a while before landing parts 6 and 7, to give time for any problems caused by part 5 to surface. Is six weeks (one dev cycle) long enough to reliable expose any such problems?

(In reply to Nicholas Nethercote [:njn] from comment #49) > Now that part 5 has successfully made it to mozilla-central, I will wait for > a while before landing parts 6 and 7, to give time for any problems caused > by part 5 to surface. > > Is six weeks (one dev cycle) long enough to reliable expose any such > problems? That should be enough to expose anything severe.

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

jrmuizel: any signs of problems here? Is it reasonable to proceed with the remaining patches? (See comment 50 for context.)

(In reply to Nicholas Nethercote [:njn] from comment #54) > jrmuizel: any signs of problems here? Is it reasonable to proceed with the > remaining patches? (See comment 50 for context.) I haven't seen anything. I think it's good to proceed.

Here's patches 6a and 6b combined, with the SurfaceFormatToImageFormat() and ImageFormatToSurfaceFormat() changes removed (as per comment 41).

Comment on attachment 8706177 [details] [diff] [review] (part 6) - Convert all gfxImageFormat values to SurfaceFormat equivalents Review of attachment 8706177 [details] [diff] [review]: ----------------------------------------------------------------- Carrying over r+ from patches 6a and 6b.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e8b7fe096ec7829790bdfaa02bf31ab002356fe Bug 1209812 (part 6) - Convert all gfxImageFormat values to SurfaceFormat equivalents. r=jrmuizel.

One remaining problem with this bug is that SurfaceFormatToImageFormat() and ImageFormatToSurfaceFormat() still exist. On little-endian platforms they are a no-op, but on big-endian they are not. I guess they should be renamed... maybe as DeEndianize() and Endianize()? Not sure. Even if they are renamed, it's an awkward situation: the SurfaceFormat type has two subtly different meanings, but those meanings only differ in practice on big-endian platforms, which get very little testing coverage. Hmm. jrmuizel, any thoughts on a good way to deal with this?

It seems like we should just keep it as marker in places that still need to be converted: e.g. FormatsAreCompatible should have its types changed and we should drop the call to SurfaceFormatToImageFormat.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #61) > It seems like we should just keep it as marker in places that still need to > be converted: e.g. FormatsAreCompatible should have its types changed and we > should drop the call to SurfaceFormatToImageFormat. Ok. I filed bug 1243605 about removing those conversions, and bug 1243606 about subsequently removing gfxImageFormat. And I will now close this bug because it's seen more than enough action.

Comment on attachment 8677880 [details] [diff] [review] (part 7) - Remove gfxImageFormat I moved this patch over to bug 1243606. It won't be directly usable, but might be a useful reference.

I haven't seen this mentioned anywhere, so yes, this did break big-endian, very badly. I'm trying to back it out on the TenFourFox tree.

I should add, though, that this could be an OS X thing, and we're forking at 45, so maybe Steve has a better idea of how badly it's busted on Linux. Maybe it's just OS X/ppc. Crash Annotation GraphicsCriticalError: |[0][GFX1]: Unknown image format 0[GFX1] : Unknown image format 0 Assertion failure: false (An assert from the graphics logger), at /Volumes/Bruce Deuce/src/esr45c/gfx/2d/Logging.h:523

linux lots a lot of 'unknown image format' errors that seem harmless. I wonder if the changes from this bug were the root cause of what I started to see in bug 1244398 which I never did track down.

Maybe, though what triggered it for me was nsSVGMaskFrame.cpp, and it was SurfaceFormat::B8G8R8A8. Currently my workaround is static inline cairo_format_t GfxFormatToCairoFormat(SurfaceFormat format) { switch (format) { case SurfaceFormat::A8R8G8B8_UINT32: case SurfaceFormat::B8G8R8A8: // XXX? return CAIRO_FORMAT_ARGB32; This gets around the assertion. I haven't done much more testing yet since I'm unsure this is the correct (or at least not a bad) fix.

(In reply to Cameron Kaiser [:spectre] from comment #65) > I should add, though, that this could be an OS X thing, and we're forking at > 45, so maybe Steve has a better idea of how badly it's busted on Linux. > Maybe it's just OS X/ppc. > > Crash Annotation GraphicsCriticalError: |[0][GFX1]: Unknown image format > 0[GFX1] > : Unknown image format 0 > Assertion failure: false (An assert from the graphics logger), at > /Volumes/Bruce > Deuce/src/esr45c/gfx/2d/Logging.h:523 Gentoo linux is running into this on PPC as well, see http://bugs.gentoo.org/594864 for more info.