1209812 - Remove gfxImageFormat

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Nicholas Nethercote [inactive]]

gfxImageFormat can be replaced with gfx::SurfaceFormat.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Remove casts between cairo_format_t and gfxImageFormat

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.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Remove gfxImageFormat::A1

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

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3a) - Convert all gfxImageFormat values to SurfaceFormat equivalents

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.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 3b) - Fix up compile errors from the previous patch

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.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove gfxImageFormat

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 6

[Nicolas Silva [:nical]]

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 7

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 8

[Nicholas Nethercote [inactive]]

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.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

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?

Comment 10

[Nicholas Nethercote [inactive]]

(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.

Comment 11

[Nicholas Nethercote [inactive]]

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

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 13

[Jeff Muizelaar [:jrmuizel]]

Attachment: Add some better enums

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

Comment 14

[Nicholas Nethercote [inactive]]

> 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.

Comment 15

[Nicholas Nethercote [inactive]]

> Thank you for posting the patch. I will incorporate and and rework my
> existing patches soon.

Hmm, I think R5G6B5 needs similar handling.

Comment 16

[Nicholas Nethercote [inactive]]

> 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.

Comment 17

[Nicholas Nethercote [inactive]]

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.

Comment 18

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 19

[Nicholas Nethercote [inactive]]

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.

Comment 20

[Carsten Book [:Tomcat]]

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

Comment 21

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Rename SurfaceFormat::R5G6B5 as RGB16_565

Comment 22

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Add endian-neutral variants to SurfaceFormat

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

Comment 23

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Add comments to SurfaceFormat

Comment 24

[Nicholas Nethercote [inactive]]

Attachment: (part 6a) - Convert all gfxImageFormat values to SurfaceFormat equivalents

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

Comment 25

[Nicholas Nethercote [inactive]]

Attachment: (part 6b) - Fix up compile errors from the previous patch

The manual fix-ups for part 6a.

Comment 26

[Nicholas Nethercote [inactive]]

Attachment: (part 7) - Remove gfxImageFormat

Comment 27

[Nicholas Nethercote [inactive]]

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 28

[Bas Schouten (:bas.schouten)]

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.

Comment 29

[Nicholas Nethercote [inactive]]

> 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.

Comment 30

[Bas Schouten (:bas.schouten)]

(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.

Comment 31

[Nicholas Nethercote [inactive]]

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.

Comment 32

[Bas Schouten (:bas.schouten)]

(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

Comment 33

[Nicholas Nethercote [inactive]]

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.

Comment 34

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Rename SurfaceFormat::R5G6B5 as R5G6B5_UINT16

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

Comment 35

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Add endian-neutral variants to SurfaceFormat

Comment 36

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Add comments to SurfaceFormat

Comment 37

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Add comments to SurfaceFormat

(reordering a couple of patches)

Comment 38

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Add endian-neutral variants to SurfaceFormat

Comment 39

[Nicholas Nethercote [inactive]]

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.

Comment 40

[Nicholas Nethercote [inactive]]

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

Comment 41

[Nicholas Nethercote [inactive]]

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.

Comment 42

[Nicholas Nethercote [inactive]]

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.

Comment 43

[Carsten Book [:Tomcat]]

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

Comment 44

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 45

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 46

[Nicholas Nethercote [inactive]]

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

Comment 47

[Nicholas Nethercote [inactive]]

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

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa15cfe46e9c

Comment 49

[Nicholas Nethercote [inactive]]

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?

Comment 50

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 51

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/aa15cfe46e9c

Comment 52

[Carsten Book [:Tomcat]]

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

Comment 54

[Nicholas Nethercote [inactive]]

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

Comment 55

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 56

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Convert all gfxImageFormat values to SurfaceFormat equivalents

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

Comment 57

[Nicholas Nethercote [inactive]]

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.

Comment 58

[Nicholas Nethercote [inactive]]

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

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e8b7fe096ec

Comment 60

[Nicholas Nethercote [inactive]]

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?

Comment 61

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 62

[Nicholas Nethercote [inactive]]

(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 63

[Nicholas Nethercote [inactive]]

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.

Comment 64

[Cameron Kaiser [:spectre]]

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.

Comment 65

[Cameron Kaiser [:spectre]]

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

Comment 66

[Steve Singer (:stevensn)]

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.

Comment 67

[Cameron Kaiser [:spectre]]

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.

Comment 68

[Ian Stakenvicius]

(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.