Closed Bug 932888 Opened 6 years ago Closed 6 years ago

Take the Thebes out of ThebesLayerBuffer

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: nrc, Assigned: nrc)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(7 files, 1 obsolete file)

Now we have Azure everywhere, we can rip out a whole bunch of code from TLB. May as well rename it at the same time as step 1 towards not using 'Thebes' to mean 'content'. Then, rejoice.
Just to be clear: there is currently code that is completely not being used anywhere by any other code, and it can be removed.  Right?  What's getting renamed?
(In reply to Milan Sreckovic [:milan] from comment #1)
> Just to be clear: there is currently code that is completely not being used
> anywhere by any other code, and it can be removed.  Right?

Right. Needs some untangling too, and a little bit of Thebes->Azure conversion, but basically dead code removal.

> What's getting renamed?

ThebesLayerBuffer -> ContentLayerBuffer because it will no longer use Thebes, and to be more in line with ContentClient/Host.
Summary: Take the Thebess out of ThebesLayerBuffer → Take the Thebes out of ThebesLayerBuffer
Depends on: 924403
We have an order in which we want to do the remaining -> Moz2D work.  If possible, I'd rather not skip steps or take them out of order, and we should have separate bugs for all of them.  This bug encompasses way too much inside of it, and it isn't clear what exactly will be going on.  Really, a meta bug.

For Moz2D, the order is:

Remove asking Moz2D for Thebes (helper functions)
Remove Moz2D Thebes interop / stop using gfxContext where equivalent Moz2D API exists
Turn on D2D 1.1
Clear manual creation of gfxContext
Simplify/de-branch gfxContext
...

What you're mentioning in here fits into that picture, or did we miss something?
(In reply to Milan Sreckovic [:milan] from comment #3)
> We have an order in which we want to do the remaining -> Moz2D work.  If
> possible, I'd rather not skip steps or take them out of order, and we should
> have separate bugs for all of them.  This bug encompasses way too much
> inside of it, and it isn't clear what exactly will be going on.  Really, a
> meta bug.
> 
> For Moz2D, the order is:
> 
> Remove asking Moz2D for Thebes (helper functions)
> Remove Moz2D Thebes interop / stop using gfxContext where equivalent Moz2D
> API exists
> Turn on D2D 1.1
> Clear manual creation of gfxContext
> Simplify/de-branch gfxContext
> ...
> 
> What you're mentioning in here fits into that picture, or did we miss
> something?

I think this is separate from that lot. It is looking only at one class - ThebesLayerBuffer - which currently has separate code paths for Thebes and Moz2D and removing the Thebes one. In terms of order it is neither blocked by nor blocks any of those steps.
OK, we missed one.  Bas, where should we insert this?  I understand there are no dependencies, but we didn't sort the list just on dependencies, but on other benefits as well.
Flags: needinfo?(bas)
Blocks: 933549
Not for landing, just for if anyone wants to apply this patch queue and build, you will need it. For landing we have to block on removing LayerManagerOGL.
Attachment #825643 - Attachment description: disable some OGL stuff (don't land this!) → patch 0: disable some OGL stuff (don't land this!)
Attachment #825644 - Flags: review?(matt.woodrow)
note to self: need to land this in the Moz2D repo too.
Attachment #825645 - Flags: review?(matt.woodrow)
Attachment #825646 - Flags: review?(matt.woodrow)
Attachment #825649 - Flags: review?(matt.woodrow)
Attachment #825653 - Flags: review?(matt.woodrow)
Comment on attachment 825644 [details] [diff] [review]
patch 1: remove the thebes paths from TLB

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

We also probably need a patch to remove the azure.content.enabled pref, and also ensure that all platforms have a fallback azure backend that can't be removed with pref changes.
Attachment #825644 - Flags: review?(matt.woodrow) → review+
Attachment #825645 - Flags: review?(matt.woodrow) → review+
Attachment #825646 - Flags: review?(matt.woodrow) → review+
Attachment #825649 - Flags: review?(matt.woodrow) → review+
Comment on attachment 825653 [details] [diff] [review]
patch 5: rename TLB

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

::: gfx/layers/RotatedBuffer.h
@@ +77,5 @@
>                                const gfx::Matrix* aMaskTransform = nullptr) const;
>  
>    /**
>     * |BufferRect()| is the rect of device pixels that this
> +   * RotatedContentBuffer covers.  That is what DrawBufferWithRotation()

RotatedBuffer
Attachment #825653 - Flags: review?(matt.woodrow) → review+
(In reply to Milan Sreckovic [:milan] from comment #5)
> OK, we missed one.  Bas, where should we insert this?  I understand there
> are no dependencies, but we didn't sort the list just on dependencies, but
> on other benefits as well.

I think this is mainly a cleanup operation. In some place we added branches because we were using Moz2D and Thebes in parallel, I think a separate point would probably be:

'Remove existing branches to accommodate Thebes/Moz2D parallel usage'

And it can probably go fairly high up as it will increase our ability to detect places accidentally still using a thebes gfxContext, as well as provide a large amount of code cleanup.
Flags: needinfo?(bas)
So looks like I was a bit optimistic - patch 3 doesn't work because we can pass in gfxContexts from layout/SVG to basic layers which aren't backed by a DrawTarget, and these find their way to TLB. I will try to land patch 1 and rebased versions of 4 and 5. I'll move patches 2 and 3 to another bug, although I bet by the time they can be applied, they will be rotten beyond use.
Blocks: 934276
(In reply to Nick Cameron [:nrc] from comment #15)
> So looks like I was a bit optimistic - patch 3 doesn't work because we can
> pass in gfxContexts from layout/SVG to basic layers which aren't backed by a
> DrawTarget, and these find their way to TLB. I will try to land patch 1 and
> rebased versions of 4 and 5. I'll move patches 2 and 3 to another bug,
> although I bet by the time they can be applied, they will be rotten beyond
> use.

Filed bug 934276 for this followup work.
Looks like I was still too optimistic and that even patch 1 fails due to getting non-DrawTarget gfxContexts from layout. So. Much. Sadness.
Try push for patch 1 only: https://tbpl.mozilla.org/?tree=Try&rev=9289fa217685
(In reply to Nick Cameron [:nrc] from comment #17)
> Looks like I was still too optimistic and that even patch 1 fails due to
> getting non-DrawTarget gfxContexts from layout. So. Much. Sadness.

Actually, maybe not too optimistic - the test failures involve perspective, so maybe the gfxContext is due to an intermediate surface, not layout.
Attachment #826588 - Flags: review?(matt.woodrow)
Attachment #826588 - Attachment is obsolete: true
Attachment #826588 - Flags: review?(matt.woodrow)
Attachment #827062 - Flags: review?(matt.woodrow)
Comment on attachment 827062 [details] [diff] [review]
patch 1.5 more azurification for patch 1 to work with 3d transforms

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

::: gfx/layers/basic/BasicLayerManager.cpp
@@ +794,5 @@
>    // Create a surface the size of the transformed object.
>    nsRefPtr<gfxASurface> dest = aDest->CurrentSurface();
> +  nsRefPtr<gfxImageSurface>  destImage = new gfxImageSurface(gfxIntSize(aDestRect.width,
> +                                                                        aDestRect.height),
> +                                                             gfxImageFormatARGB32);

Use Factory::CreateDataSourceSurface() here instead, and make the return value a TemporaryRef<SourceSurface>

You'll need to add a gfxContext::SetSource overload that just takes a SourceSurface, but that should be easy.
Attachment #827062 - Flags: review?(matt.woodrow) → review+
Patches 2,3,4 have been reorganised a bit and moved to bug 934276.
Whiteboard: [qa-]
Depends on: 967763
You need to log in before you can comment on or make changes to this bug.