Move everything TextureImage-related out of GLContext

RESOLVED FIXED in mozilla28

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bjacob, Assigned: bjacob)

Tracking

Trunk
mozilla28
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(4 attachments, 5 obsolete attachments)

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.
I'll do this one next.
Assignee: nobody → bjacob
Duplicate of this bug: 942504
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.
Attachment #8340720 - Flags: review?(bgirard)
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.
Attachment #8340721 - Flags: review?(bgirard)
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.
Attachment #8340723 - Flags: review?(bgirard)
The cherry on the cake: ReadTextureImage had zero callers!
Attachment #8340724 - Flags: review?(bgirard)
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(-)
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!
Attachment #8340720 - Attachment is obsolete: true
Attachment #8340721 - Attachment is obsolete: true
Attachment #8340720 - Flags: review?(bgirard)
Attachment #8340721 - Flags: review?(bgirard)
Attachment #8340726 - Flags: review?(bgirard)
Attachment #8340726 - Attachment description: remove-dead-code → Part 1: remove some dead code around TextureImage functions
Attachment #8340722 - Attachment is obsolete: true
Attachment #8340722 - Flags: review?(bgirard)
Attachment #8340727 - Flags: review?(bgirard)
Attachment #8340723 - Attachment is obsolete: true
Attachment #8340723 - Flags: review?(bgirard)
Attachment #8340728 - Flags: review?(bgirard)
Attachment #8340724 - Attachment is obsolete: true
Attachment #8340724 - Flags: review?(bgirard)
Attachment #8340729 - Flags: review?(bgirard)
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 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
Attachment #8340726 - Flags: review?(bgirard) → review+
Attachment #8340727 - Flags: review?(bgirard) → review+
Attachment #8340729 - Flags: review?(bgirard) → review+
Attachment #8340728 - Flags: review?(bgirard) → review+
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.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.