Closed Bug 924403 Opened 6 years ago Closed 6 years ago

Remove gfx/layers/opengl

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: BenWa, Assigned: nical)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(3 files, 1 obsolete file)

With the layers refactoring we're almost ready to remove the old opengl layers code. We still have OS X 10.6 using it and it's useful to have until we certain we're not going to back out OMTC on mac.

We should also completely remove the Copy2D stuff:
https://bugzilla.mozilla.org/show_bug.cgi?id=923409#c2
Nical can have this honor :)
Assignee: nobody → nical.bugzilla
Blocks: 932888
Depends on: 920123
Duplicate of this bug: 888311
nical: do you have patches ready to go for this? I hope it will be unblocked very soon and it is blocking my ThebesLayerBuffer patches (which block Azure content), so it would be great if we could land this as soon as it is unblocked. Thanks!
We haven't identified this as a priority right now - do we need nical to do this?  In my mind a more important thing is to complete the removal of the deprecated textures that nical is on the hook to do, rather than this.  Nick, if you're blocked on this, can you take this on instead?
(In reply to Milan Sreckovic [:milan] from comment #4)
> We haven't identified this as a priority right now - do we need nical to do
> this?  In my mind a more important thing is to complete the removal of the
> deprecated textures that nical is on the hook to do, rather than this. 
> Nick, if you're blocked on this, can you take this on instead?

It is only a priority because it transitively blocks my new work on new textures/removing deprecated textures.

Anyway, I am happy to do this, but I don't want to steal it from nical unless he is keen - it should be fun to do and very quick.
You're going to remove the deprecated textures?  And that needs this?  That's a different story then.
This blocks removing Thebes paths from TLB and that soft-blocks Azure content - because adding _another_ path to TLB before we remove some is madness.
Attached patch remove non-OMTC GL layers (obsolete) — Splinter Review
There you go :)
try push: https://tbpl.mozilla.org/?tree=Try&rev=c0374ad1b0e9

I am not sure whether we can land this (there was something about TART performance tests still using LayerManagerOGL).
Attachment #827985 - Flags: review?(ncameron)
Some build fixes
Attachment #827985 - Attachment is obsolete: true
Attachment #827985 - Flags: review?(ncameron)
Attachment #828029 - Flags: review?(ncameron)
With this does it make sense to eliminate the GLManager abstraction?
(In reply to Andreas Gal :gal from comment #11)
> With this does it make sense to eliminate the GLManager abstraction?

Yes, that was a stop gap until we got to this stage. I will remove it.
Comment on attachment 828029 [details] [diff] [review]
remove non-OMTC GL layers

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

r=me and \o/

As agreed with Nical and Milan, I'll land this and handle the review comments (in a separate patch), also manage the Talos dependency and deal with any fallout.

::: gfx/layers/opengl/GLManager.h
@@ -20,5 @@
>  
>  /**
>   * Minimal interface to allow widgets to draw using OpenGL. Abstracts
> - * LayerManagerOGL and CompositorOGL. Call CreateGLManager with either a
> - * LayerManagerOGL or a LayerManagerComposite backed by a CompositorOGL.

We should be able to get rid of GLManager completely

::: gfx/layers/opengl/LayerManagerOGLProgram.h
@@ -260,5 @@
> -   * a 2D transform.
> -   */
> -  bool LoadMask(Layer* aLayer);
> -
> -  /**

We should rename these files

::: widget/xpwidgets/nsBaseWidget.cpp
@@ +1404,5 @@
>  
>  uint32_t
>  nsBaseWidget::GetGLFrameBufferFormat()
>  {
> +  // XXX - We may want to do something here or just remove that code.

We should use return RGBA here, I think
Attachment #828029 - Flags: review?(ncameron) → review+
There's another GLManager subclass at http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm#328 (which I added in bug 882523) - looks like I forgot to add it to the comment in GLManager.h. So removing GLManager will require some refactoring in nsChildView.mm.
(In reply to Markus Stange [:mstange] from comment #14)
> There's another GLManager subclass at
> http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.
> mm#328 (which I added in bug 882523) - looks like I forgot to add it to the
> comment in GLManager.h. So removing GLManager will require some refactoring
> in nsChildView.mm.

ah, ok, thanks. I'll take a look and see if it is still worth removing or not.
Attachment #828277 - Flags: review?(matt.woodrow)
Blocks: 935734
I don't think we can remove GLManager, but I think it can be simplified a lot. Seeing as that doesn't block anything, I'm not going to do that now. I filed bug 935734 for that tidy up work.
Attachment #828277 - Flags: review?(matt.woodrow) → review+
Don't waste time cleaning up LayerManagerOGLProgram, I have several patches to do that (once my current set stuck). nsChildView.mm definitely needs some refactoring. It duplicates a bunch of layers code (VBO handling, draw etc).
(In reply to Andreas Gal :gal from comment #18)
> Don't waste time cleaning up LayerManagerOGLProgram, I have several patches

I don't intend to :-) (other than the patch here which just does renaming). Do you mean that you would prefer me not to land that patch?

> to do that (once my current set stuck). nsChildView.mm definitely needs some
> refactoring. It duplicates a bunch of layers code (VBO handling, draw etc).
Another try push to check we still build with patch 2: https://tbpl.mozilla.org/?tree=Try&rev=75afdf6272a0
(In reply to Nick Cameron [:nrc] from comment #19)
> (In reply to Andreas Gal :gal from comment #18)
> > Don't waste time cleaning up LayerManagerOGLProgram, I have several patches
> 
> I don't intend to :-) (other than the patch here which just does renaming).
> Do you mean that you would prefer me not to land that patch?
Flags: needinfo?(gal)
No. Land away. I can rebase on top. Thanks for the patch here :)
Flags: needinfo?(gal)
Attachment #829037 - Flags: review?(matt.woodrow) → review+
Depends on: 936339
Depends on: 914584
No longer depends on: 936339
Depends on: 936339
No longer depends on: 914584
If this sticks I am buying beer for the team.
(In reply to Hannes Verschore [:h4writer] from comment #26)
> Can I assume the Tab Animation Regression on MaxOSX of 50% - 90% is from
> this landing:
> http://hg.mozilla.org/integration/mozilla-inbound/
> pushloghtml?fromchange=d822990ba9ee&tochange=39bfcadd6492
> 
> http://mzl.la/1csWov5
> http://mzl.la/18mYqzJ

Yes, totally expected and acceptable - see dev.planning for more details.
Depends on: 943859
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.