"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

Hi, I would like to work on this bug !

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.

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

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.

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

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

There are a few build errors to fix.

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

Fix build failures on Windows.

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

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

Fix build failure on Windows

13 was a bad number for Windows!

Rebased the patch with recent changes to mozilla-central.

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.

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

This isn't ready as is, right?

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.

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

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

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

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

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