1021628 - [Moz2Dification] Replace gfxImageFormat by gfx::SurfaceFormat in gfx code

Status: RESOLVED DUPLICATE of bug 1209812

(Core :: Graphics, defect)

Description

[Nicolas Silva [:nical]]

"grep -r gfxImageFormat gfx/ | wc -l" gives ~275 occurences of gfxImageFormat in gfx code. This should probably be split in several smaller bugs. There are also quite a few occurences in layout, widget and media code.

The correspondance between gfxImageFormat and gfx::SurfaceFormat can be found in http://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfx2DGlue.h?from=gfx2DGlue.h#213

Comment 1

[u538282]

Hi, I would like to work on this bug !

Comment 2

[u538282]

Should the comment in file https://dxr.mozilla.org/mozilla-central/source/gfx/ipc/GfxMessageUtils.h#287 be removed ? It appears to be dead code.

Comment 3

[Nicolas Silva [:nical]]

(In reply to thibaud.backenstrass from comment #2)
> Should the comment in file
> https://dxr.mozilla.org/mozilla-central/source/gfx/ipc/GfxMessageUtils.h#287
> be removed ? It appears to be dead code.

Yeah, good idea.

Comment 4

[u538282]

Attachment: gfxImageFormat.patch

I know this patch is quite huge, but I didn't manage to split it into several smaller patches since the bug affects both functions declarations and functions calls. I hope it will be ok however...
This patch replaces all the occurences of gfxImageFormat in gfx/, dom/ and widget/ code. The build finished successfully.

Some stuff still needs to be done, but I'll do it after this first patch has landed into mozilla-central : now that there is no more call to gfxImageFormat, the class can be removed and the functions ImageFormatToSurfaceFormat and SurfaceFormatToImageFormat (file gfx2DGlue.h) are no longer needed.

Comment 5

[u538282]

Attachment: gfxImageFormat.rev2.patch

Sorry, the last patch contained a mistake (SourceFormat instead of SurfaceFormat), this one is better :)

Comment 6

[Nicolas Silva [:nical]]

Wow indeed. Let's run this monster on the try servers: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5453a1f8f7fd

Comment 7

[Nicolas Silva [:nical]]

There are a few build errors to fix.

Comment 8

[u538282]

Attachment: gfxImageFormat.rev3.patch

My monster fell in one swoop!
I fixed several build failures with this new patch.

Comment 9

[u538282]

Attachment: gfxImageFormat.rev4.patch

Comment 10

[u538282]

Attachment: gfxImageFormat.rev5.patch

Comment 11

[u538282]

Attachment: gfxImageFormat.rev6.patch

Comment 12

[u538282]

Attachment: gfxImageFormat.rev7.patch

Comment 13

[u538282]

Attachment: gfxImageFormat.rev8.patch

Comment 14

[u538282]

Attachment: gfxImageFormat.rev9.patch

Fix build failures on Windows.

Comment 15

[u538282]

Attachment: gfxImageFormat.rev10.patch

With this revision I fixed the problem with Cairo by using https://dxr.mozilla.org/mozilla-central/source/gfx/2d/HelpersCairo.h#202
Let's see if it'll work...

Comment 16

[Nicolas Silva [:nical]]

Comment on attachment 8609588 [details] [diff] [review]
gfxImageFormat.rev10.patch

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

We will probably need to add SurfaceFormat::A1 since we apparently support that in cairo.
There are places where we test against, say RGB24, and we used to have only one way to describe 24 bit depth opaque color, but now we have the RGB and the BGR versions in the enum so in a lot of these places it's quite risky to just replace RGB24 into BGRX without testing against the RGB equivalent.

::: gfx/thebes/gfxASurface.cpp
@@ +429,2 @@
>  {
>      return cairo_format_stride_for_width((cairo_format_t)(int)format, (int)width);

There is an incorrect cast from SurfaceFormat to cairo_format_t here

@@ +514,4 @@
>  {
>      switch (format) {
> +        case SurfaceFormat::B8G8R8A8:
> +        case SurfaceFormat::B8G8R8X8:

please remove this switch and call mozilla::gfx::BytesPerPixel here (you will find this function in gfx/2d/Tools.h.

As a side note, there are missing Formats here (such as R8G8B8A8, etc.) that did not exist in gfxImageFormat but do in SurfaceFormat. This is probably part of the reason for the gtest timeout, because we return 0 in these cases and we have code using the bytes-per-pixel to advance loops in different places.

@@ +663,5 @@
>      return aMallocSizeOf(this) + SizeOfExcludingThis(aMallocSizeOf);
>  }
>  
>  /* static */ uint8_t
> +gfxASurface::BytesPerPixel(SurfaceFormat aImageFormat)

Same thing here, please use gfx::BytesPerPixel (not even sure why we have both this function and the other one, they seem to be doing the same thing).

::: gfx/thebes/gfxUtils.cpp
@@ +674,3 @@
>  {
>      switch (aFormat) {
> +        case SurfaceFormat::B8G8R8A8:

To be safe, please add the other SurfaceFormat types here, like R8B8G8A8, etc.

::: gfx/thebes/gfxWindowsSurface.cpp
@@ +81,5 @@
>      Init(surf);
>  
>      if (mSurfaceValid) {
> +        // DDBs will generally only use 3 bytes per pixel when RGB24 (B8G8R8X8)
> +        int bytesPerPixel = ((imageFormat == gfx::SurfaceFormat::B8G8R8X8) ? 3 : 4);

to be safe, also compare against R8G8B8X8.

Comment 17

[u538282]

Attachment: gfxImageFormat.rev11.patch

Comment 18

[u538282]

Attachment: gfxImageFormat.rev12.patch

I finally fixed Layers.TextureSerialization by replacing a few casts from SurfaceFormat to Cairo.

Comment 19

[u538282]

Attachment: gfxImageFormat.rev13.patch

Fix build failure on Windows

Comment 20

[u538282]

Attachment: gfxImageFormat.rev14.patch

13 was a bad number for Windows!

Comment 21

[u538282]

Attachment: gfxImageFormat.rev15.patch

Rebased the patch with recent changes to mozilla-central.

Comment 22

[Nicolas Silva [:nical]]

Comment on attachment 8612403 [details] [diff] [review]
gfxImageFormat.rev15.patch

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

::: gfx/thebes/gfxASurface.cpp
@@ +467,3 @@
>  {
>      switch (format) {
> +        case SurfaceFormat::B8G8R8A8:

add case SurfaceFormat::R8G8B8A8:

@@ +470,2 @@
>              return gfxContentType::COLOR_ALPHA;
> +        case SurfaceFormat::B8G8R8X8:

add case SurfaceFormat::R8B8G8X8:

@@ +473,3 @@
>              return gfxContentType::COLOR;
> +        case SurfaceFormat::A8:
> +// SurfaceFormat::A1 does not exist

It does now!

::: gfx/thebes/gfxImageSurface.cpp
@@ +193,2 @@
>          stride = (aSize.width + 7) / 8;
> +    } */ // SurfaceFormat::A1 does not exist

now it does.

::: gfx/thebes/gfxPlatform.cpp
@@ +2209,1 @@
>        return mozilla::gfx::SurfaceFormat::R5G6B5;

Please change this into:

gfx::SurfaceFormat offscreenFormat = GetOffscreenFormat();
swicth (offscreenFormat) {
case gfx::SurfaceFormat::B8G8R8A8:
case gfx::SurfaceFormat::R8G8B8A8:
case gfx::SurfaceFormat::B8G8R8X8:
case gfx::SurfaceFormat::R8G8B8X8:
case gfx::SurfaceFormat::R5G6B5:
  return offscreenFormat;

::: gfx/thebes/gfxXlibSurface.cpp
@@ +522,5 @@
>              green_mask = 0x7e0;
>              blue_mask = 0x1f;
>              break;
> +        case gfx::SurfaceFormat::A8:
> +//      case gfx::SurfaceFormat::A1: // unreachable case (A1 does not exist)

now it does

@@ +569,2 @@
>              return XRenderFindStandardFormat (dpy, PictStandardA8);
> +// gfx::SurfaceFormat::A1 does not exists

ad lib.

Comment 23

[u538282]

Attachment: gfxImageFormat.rev16.patch

Comment 24

[Nicolas Silva [:nical]]

Attachment: version 17

The previous version conflicted with a patch that landed recently so I rebased it. try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=b3c2e6bf3d10

Comment 25

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

This isn't ready as is, right?

Comment 26

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8613510 [details] [diff] [review]
version 17

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

We already have the problem where we improperly label gfxImageFormat::ARGB32 as B8G8R8A8 and vice-versa. This looks like it looks like this further tangles gfxImageFormat and gfx::SurfaceFormat by adding new places/moving the places we convert around. I think we need to figure out a proper strategy for big endian before we proceed with this.

Comment 27

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #25)
> This isn't ready as is, right?

There is at least R6 to fix on android.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #26)
> We already have the problem where we improperly label gfxImageFormat::ARGB32
> as B8G8R8A8 and vice-versa. This looks like it looks like this further
> tangles gfxImageFormat and gfx::SurfaceFormat by adding new places/moving
> the places we convert around. I think we need to figure out a proper
> strategy for big endian before we proceed with this.

Well, this completely wipes gfxImageFormat out of the code base.

Let's figure out our big endian strategy soon, because Thibaud spent a lot of time working on this and I doubt either he or I will have the courage to re-do this if we let the patch bitrot too much.

Comment 28

[u538282]

Attachment: YCbCrUtils-gfxUtils.patch

Let's start again with smaller patches. This one concerns YCBCrUtils and gfxUtils. Nical, could I have a try-run for that before I continue?

Comment 29

[Nicolas Silva [:nical]]

Comment on attachment 8615357 [details] [diff] [review]
YCbCrUtils-gfxUtils.patch

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

::: gfx/thebes/gfxUtils.cpp
@@ +676,3 @@
>  {
>      switch (aFormat) {
> +        case SurfaceFormat::B8G8R8A8:

Please add the other formats to this switch (SurfaceFormat::R8G8B8A8, SurfaceFormat::R8G8B8X8, A8, A1, etc.)

Comment 30

[Jeff Muizelaar [:jrmuizel]]

So at a minimum we need to add A8R8G8B8 and X8R8G8B8 formats for big endian. CAIRO_FORMAT_ARGB32 and CAIRO_FORMAT_RGB24 should be converted to the actual byte format that they represent.

i.e.

CAIRO_FORMAT_ARGB32 = R8G8B8A8 on little endian
                    = A8R8G8B8 on big endian

Depending on how many places need to do this conversion/query this value it may make sense to have a SurfaceFormat::ARGB32 that has a value matching R8G8B8A8 or A8R8G8B8 as appropriate. We should not have a separate ARGB32 format to avoid creating ambiguity about the how the data is actually stored.

Comment 31

[u538282]

Attachment: YCbCrUtils-gfxUtils.rev2.patch

Comment 32

[Nicolas Silva [:nical]]

Comment on attachment 8616355 [details] [diff] [review]
YCbCrUtils-gfxUtils.rev2.patch

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

Looks good to me. Before you go further with format conversion, we need to figure out the safest way to implement Jeff's proposition for surface formats and endianness.

Comment 33

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

Once we do resolve that, let's save it for 42 (after June 23.)

Comment 35

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.