942501 - Move everything TextureImage-related out of GLContext

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

GLContext should just be a lower-level tool used by TextureImage. There's no reason why GLContext would have any TextureImage-related method, such as CreateTextureImage.

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

I'll do this one next.

Comment 3

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 1: remove some dead code around TextureImage functions

We had some dead code around there, and removing it first made the rest easier to reason about.

It later turned out that we had some more dead code: see Part 5.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 2: move TextureImageGLX to separate files

Before moving further we have a nontrivial problem to solve:

Having the TextureImage-related functions implemented in GLContextProvider* files solved the problem of where to put platform-specific code.

We probably don't want to put all that code in a single file with #ifdefs. That might even not be practical e.g. if Mac code really needs to be Objective C++.

But at the same time I'm really serious about wanting to rid the GLContextProvider* files of these functions, because GLContextProvider* files are really central code that we often need to stare at, and much of the GLContext implementation lives there, so I want these files to be as small as possible.

I noted that we already have TextureImageEGL.{cpp,h} files.

That's why I resorted to creating new TextureImageGLX.{cpp,h} files, and the next patch will introduce TextureImageCGL.{cpp,h}.

I know that you are not a fan of file proliferation but I didn't see a better alternative. I think that we agree that now that we have UNIFIED_SOURCES, the biggest remaining problem with file proliferation is that it's hard to find what one is looking for in a directory. Here, the present code (TextureImage) is deprecateed and is expected to go away in the future, so maybe it would be acceptable to move these TextureImage* files to a textureimage/ subdirectory, so we can forget about these files? --- if you don't want more files directly in gfx/gl.

Comment 5

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 3: move TextureImageCGL to separate filesTextureImageCGL

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 4: move CreateTextureImage and TileGenFunc out of GLContext

This is the main patch.

The polymorphism that was previously implemented by having CreateTextureImage be a virtual method on GLContext, is now implemented by a switch statement on the ContextType() --- EGL vs CGL, etc.

Not very pretty or high-tech, but again since this code is legacy and should go away, I didn't want to worry too much about that.

Comment 7

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 5: remove the unused ReadTextureImage function

The cherry on the cake: ReadTextureImage had zero callers!

Comment 8

[Benoit Jacob [:bjacob] (mostly away)]

Even with the addition of several new files, this patch queue is still a net removal of 170 lines of code!

$ hg diff -r 0ae53698b8a0..tip --stat
 gfx/gl/GLContext.cpp                           |  149 -----------------
 gfx/gl/GLContext.h                             |   67 +-------
 gfx/gl/GLContextProviderCGL.mm                 |  188 +---------------------
 gfx/gl/GLContextProviderEGL.cpp                |   50 -----
 gfx/gl/GLContextProviderGLX.cpp                |  186 +---------------------
 gfx/gl/GLTextureImage.cpp                      |   49 +++++-
 gfx/gl/GLTextureImage.h                        |   24 ++
 gfx/gl/GLXLibrary.h                            |    1 +
 gfx/gl/TextureImageCGL.h                       |   66 +++++++
 gfx/gl/TextureImageCGL.mm                      |  158 ++++++++++++++++++
 gfx/gl/TextureImageEGL.cpp                     |   39 ++++
 gfx/gl/TextureImageEGL.h                       |   14 +
 gfx/gl/TextureImageGLX.cpp                     |   88 ++++++++++
 gfx/gl/TextureImageGLX.h                       |   78 +++++++++
 gfx/gl/moz.build                               |    7 +-
 gfx/layers/composite/LayerManagerComposite.cpp |    8 -
 gfx/layers/composite/LayerManagerComposite.h   |   10 -
 gfx/layers/ipc/ShadowLayerUtilsD3D10.cpp       |    8 -
 gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp     |   13 -
 gfx/layers/ipc/ShadowLayerUtilsMac.cpp         |    8 -
 gfx/layers/ipc/ShadowLayerUtilsX11.cpp         |    9 -
 gfx/layers/opengl/TextureHostOGL.cpp           |   29 +-
 22 files changed, 540 insertions(+), 709 deletions(-)



If we focus on the central files that we care about un-cluttering (GLContext.*, GLContext*), this is a *** net removal of 630 lines of code! ***

$ hg diff -r 0ae53698b8a0..tip --stat gfx/gl/GLContext*
 gfx/gl/GLContext.cpp            |  149 -----------------------------
 gfx/gl/GLContext.h              |   67 +------------
 gfx/gl/GLContextProviderCGL.mm  |  188 +------------------------------------
 gfx/gl/GLContextProviderEGL.cpp |   50 ---------
 gfx/gl/GLContextProviderGLX.cpp |  186 +------------------------------------
 5 files changed, 5 insertions(+), 635 deletions(-)

Comment 9

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=473b2761fe60 (red on mac)
https://tbpl.mozilla.org/?tree=Try&rev=ecf30b8a7a48 (mac-only)

Comment 10

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 1: remove some dead code around TextureImage functions

Wait --- TextureImageGLX is unused, as the only codepath in CreateTextureImage creating one was part of the dead code that was removed --- so we can completely remove it!

Comment 11

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 2: move TextureImageCGL to separate files

Comment 12

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 3: move CreateTextureImage and TileGenFunc out of GLContext

Comment 13

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Part 4: remove the unused ReadTextureImage function

Comment 14

[Benoit Jacob [:bjacob] (mostly away)]

The total diff is now a net removal of 340 lines of code! And still a net removal of 630 lines of code in the files that we care about.

$ hg diff -r 0ae53698b8a0..tip --stat
 gfx/gl/GLContext.cpp                           |  149 -----------------
 gfx/gl/GLContext.h                             |   67 +-------
 gfx/gl/GLContextProviderCGL.mm                 |  188 +---------------------
 gfx/gl/GLContextProviderEGL.cpp                |   50 -----
 gfx/gl/GLContextProviderGLX.cpp                |  184 ---------------------
 gfx/gl/GLTextureImage.cpp                      |   49 +++++-
 gfx/gl/GLTextureImage.h                        |   24 ++
 gfx/gl/GLXLibrary.h                            |    1 +
 gfx/gl/TextureImageCGL.h                       |   66 +++++++
 gfx/gl/TextureImageCGL.mm                      |  158 ++++++++++++++++++
 gfx/gl/TextureImageEGL.cpp                     |   39 ++++
 gfx/gl/TextureImageEGL.h                       |   14 +
 gfx/gl/moz.build                               |    6 +-
 gfx/layers/composite/LayerManagerComposite.cpp |    8 -
 gfx/layers/composite/LayerManagerComposite.h   |   10 -
 gfx/layers/ipc/ShadowLayerUtilsD3D10.cpp       |    8 -
 gfx/layers/ipc/ShadowLayerUtilsGralloc.cpp     |   13 -
 gfx/layers/ipc/ShadowLayerUtilsMac.cpp         |    8 -
 gfx/layers/ipc/ShadowLayerUtilsX11.cpp         |    9 -
 gfx/layers/opengl/TextureHostOGL.cpp           |   29 +-
 20 files changed, 371 insertions(+), 709 deletions(-)

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=81c4c0638a07

Comment 16

[Benoit Girard (:BenWa)]

Comment on attachment 8340726 [details] [diff] [review]
Part 1: remove some dead code around TextureImage functions

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

bjacob++

BTW description is wrong

Comment 17

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=eb3be360fb34

Comment 18

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=1b48093b8244

Comment 19

[Benoit Jacob [:bjacob] (mostly away)]

Part 4 broke with the landing of LayerScope, which uses ReadTextureImage. The new trypush is adjusted for that. Bug 945586 has been filed about taking ReadTextureImage out of GLContext.

Comment 20

[Benoit Jacob [:bjacob] (mostly away)]

https://tbpl.mozilla.org/?tree=Try&rev=01672e3e55a0

http://hg.mozilla.org/integration/mozilla-inbound/rev/93d12e87d7d6
http://hg.mozilla.org/integration/mozilla-inbound/rev/55109f1334e7
http://hg.mozilla.org/integration/mozilla-inbound/rev/17964f955e0c

Part 4 here won't land, as explained in comment 19.

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93d12e87d7d6
https://hg.mozilla.org/mozilla-central/rev/55109f1334e7
https://hg.mozilla.org/mozilla-central/rev/17964f955e0c