882113 - Moz2Dify layers

Status: NEW

(Core :: Graphics: Layers, defect)

Description

[Nicolas Silva [:nical]]

We should use Moz2D classes rather than Thebes as much as possible in layers, especially on the compositor side where we don't do any cairo operations, hence no need for Thebes over Moz2D.

Comment 1

[CJ]

I'm interested in getting involved with graphics and contributing to the Mozilla codebase. This was listed under mentored bugs, and I was wondering if it would be a worthwhile "Good First Bug", and if so where I should start. If not, is there another Good First Bug that involves graphics and the core code base?

Comment 2

[Nicolas Silva [:nical]]

Hi Cj,
This is a good first bug because it is a rather easy task. I have to admit it is not the most fascinating thing to work on because you won't see the difference in Firefox from a user point of view, but it will make developers life nicer, and is part of making our code more awesome and modern.
So it is worthwhile, and it is something that I would eventually end up doing if nobody is interested in doing it. I'll be glad to help you get started if you have questions.

To start, you should setup a build environment, build firefox, and start looking at some code:
 * https://mxr.mozilla.org/mozilla-central/source/gfx/2d/2D.h is the API that we want to migrate to
 * https://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxASurface.h is the the legacy API that we want to move away from
(You should have a look at the other files in these two directories)

Some of the uses of the thebes API can't be moved to Moz2D without some quite involved work, so a very good start would be to find places where we use the class gfxImageSurface (inherits gfxASurface, and is the Surface class that we usually use when we need to expose the image as a buffer in memory.
A lot of the uses of gfxImageSurface can be replaced by uses of Moz2D's DataSourceSurface.
In particular we are looking for when we use gfxImageSurface just as a wrapper around image memory, without doing cairo operations on it.

Also, as much as possible we want to find places where we use the thebes classes:
 * gfxPoint, gfxIntPoint
 * gfxRect, gfxIntRect
 * gfxSize, gfxIntSize
 * gfxRGBA
And convert them to Moz2D ones:
 * gfx::Point, gfx::IntPoint
 * gfx::Rect, gfx::IntRect
 * gfx::Size, gfx::IntSize
 * gfx::Color

we have a colletion of helper functions for that here: https://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfx2DGlue.h

It is best to not do a huge patch that converts everything. instead, you can find isolated uses of thebes classes in gfx/layers/, convert them and upload separate small patches here. It will be easier for you to advance incrementally and for us to review and land.

This week I am on PTO so I am away from irc, but starting next week you can ask me (nical) questions on irc at irc.mozilla.org. Asking questions here on the bug or by email also works well.

Comment 3

[Adrian May [:adrianmay]]

Is CJ still interested in this?

Comment 4

[CJ]

Definitely still interested, but if someone else is looking to get this done or it's important that it be finished soon, someone else is definitely welcome to take it. 

I haven't had as much time as I'd like to work on this, so I'm still trying to absorb/tinker to get a better handle on things. If someone else wishes to grab this I can still continue to pursue it separately for my own edification, so no problem there.

Comment 5

[Adrian May [:adrianmay]]

I know what you mean. It's taking me a while to get orientated in this codebase as well. 

I think these mentored bugs are all considered non-urgent and there for training purposes. So if you want to keep this one, I'll do something else.

Comment 6

[Nicolas Silva [:nical]]

I Wrote a blog post with some information about Moz2Difying things that could be helpful: http://mozillagfx.wordpress.com/2013/08/21/looking-for-a-good-first-place-to-contribute-to-gecko-gfx/

Comment 7

[Nick Cameron [:nrc]]

I started work on the compositor side stuff before I found this bug. So if people are still looking at this, then please concentrate on client side (anything with 'Client' in the class) or stuff in the MTC layers code, such as Layers.h, or anything BasicLayers -ish. I haven't done anything mentioned in comment 2 so hopefully we won't have duplicated effort.

Comment 8

[Nick Cameron [:nrc]]

And if you're not in nical's timezone, please feel free to ask me questions about this on irc.

Comment 9

[Nick Cameron [:nrc]]

Attachment: Compositor classes

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 810448 [details] [diff] [review]
Compositor classes

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

::: gfx/layers/basic/BasicCompositor.cpp
@@ +473,4 @@
>    if (mCopyTarget) {
> +    mCopyTarget->CopySurface(source,
> +                       IntRect(0, 0, mWidgetSize.width, mWidgetSize.height),
> +           IntPoint(0, 0));

Indents!

::: gfx/layers/ipc/CompositorParent.cpp
@@ +298,5 @@
>  {
>    AutoOpenSurface opener(OPEN_READ_WRITE, aInSnapshot);
> +  gfxIntSize size = opener.Size();
> +  RefPtr<DrawTarget> target =
> +    gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(opener.Get(),

This is always going to create a cairo draw target. I don't think that's really a problem, but you should add a comment to that effect.

We also should convert AutoOpenSurface to use Azure directly at some point.

Comment 11

[Nick Cameron [:nrc]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=183936932e5f

Comment 12

[Nick Cameron [:nrc]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> 
> We also should convert AutoOpenSurface to use Azure directly at some point.

Yes we should. I thought about doing it here, but I got scared by the shared surfaces stuff - not sure how to do that in Azure. I don't think it is impossible, but I couldn't see a trivial conversion.

Comment 13

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e1a3919e741

Comment 14

[Nicolas Silva [:nical]]

(In reply to Nick Cameron [:nrc] from comment #12)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #10)
> > 
> > We also should convert AutoOpenSurface to use Azure directly at some point.
> 
> Yes we should. I thought about doing it here, but I got scared by the shared
> surfaces stuff - not sure how to do that in Azure. I don't think it is
> impossible, but I couldn't see a trivial conversion.

I have been moving away from using AutoOpenSurface in the new textures. Use ImageDataSerializer and YCbCrImageDataSerializer instead, which are way simpler than AutoOpenSurface.
AutoOpenSurface should be killed along with the deprecated textures.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4e1a3919e741

Comment 16

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: moved some gfxRect to gfx::Rect

I moved CanvasBidiProcessor.mBoundingBox to gfx::Rect and some minor gfxPoint to gfx::Point. Also changed some mozilla::gfx --> mgfx which seems to be the standard in that file, please let me know if I have to sumbit this in another patch. 

Also, if this is the right kind of patches you are looking for please let me know as I can work on this a lot more.

Comment 17

[Nicolas Silva [:nical]]

Comment on attachment 8351081 [details] [diff] [review]
moved some gfxRect to gfx::Rect

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

Hi Giovanni, Thanks for this patch. Please use the "r?" flag to ask for reviews. "r+" is set by the reviewer once he thinks the patch is ready to be checked in. You don't need to ask for feedback if you are already asking for a review. Feedback is more intended for work in progress. I will look at your patch and give you a review soon.

Comment 18

[Nicolas Silva [:nical]]

Comment on attachment 8351081 [details] [diff] [review]
moved some gfxRect to gfx::Rect

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

looks good to me. I am not sold on the mgfx business but it predates your patch so if we decide to change that we'll do it as a followup (most files in gfx have "using namespace mozilla::gfx;" at the top of the .cpp file and they don't write the mozilla::gfx namespace in the .cpp file)

Comment 19

[Nicolas Silva [:nical]]

pushed attachment 8351081 [details] [diff] [review] to try servers https://tbpl.mozilla.org/?tree=Try&rev=8a20e9205df5

Comment 20

[Luis de Bethencourt [:luisbg]]

Can't see the results of the test build anymore and I don't see this patch merged in mozilla-core.

Anybody knows the status of this bug?

Comment 21

[Kartikaya Gupta (email:[e-mail])]

For comment 20

Comment 22

[Nicolas Silva [:nical]]

(In reply to Luis de Bethencourt [:luisbg] from comment #20)
> Can't see the results of the test build anymore and I don't see this patch
> merged in mozilla-core.
> 
> Anybody knows the status of this bug?

The two patches have landed. IIRC we moved them to separate bugs to keep this one as a generic tracking bug.

Comment 23

[Nicolas Silva [:nical]]

Comment on attachment 810448 [details] [diff] [review]
Compositor classes

Marked the two patches obsolete to avoid confusion.

Comment 24

[Kevin Wern]

I'd be willing to take on this bug if nobody else has expressed interest yet.  Is there anything I should know?

NOTE: Bugzilla is stating that the account :nical is disabled, so I chose the latest user I saw comment for the needinfo tag (Kartikaya Gupta (email:[e-mail])).

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Unfortunately I don't really know the state of things here, but nical is still definitely around so I'm not sure why bugzilla told you his account is disabled. Redirecting needinfo.

Comment 26

[Nicolas Silva [:nical]]

The easy parts of the Moz2Dification work is already done and the rest is not a good fit for first contributions so I just removed this bug from the list of mentored bugs. I encourage you to look at the other suggestions in http://www.joshmatthews.net/bugsahoy