Closed Bug 1309200 Opened 8 years ago Closed 2 years ago

Avoid hard-coding the choice of the cairo drawing backend whenever possible.

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: nical, Assigned: paul.bignier)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(10 files, 6 obsolete files)

3.35 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.61 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.06 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
1.20 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
3.07 KB, patch
nical
: review+
nical
: checkin+
Details | Diff | Splinter Review
1.31 KB, patch
nical
: review-
Details | Diff | Splinter Review
1.85 KB, patch
nical
: feedback+
Details | Diff | Splinter Review
4.99 KB, patch
Details | Diff | Splinter Review
3.11 KB, patch
nical
: feedback+
Details | Diff | Splinter Review
2.06 KB, patch
dvander
: review+
Details | Diff | Splinter Review
In most places where we want a software Moz2D backend, we hard-code cairo, which doesn't much sense anymore now that we are switching most platforms to skia.

Let's add a BackendType gfxContext::GetSoftwareContentBacakend() method which will use skia in most platforms (cairo where we have not switched over yet), and progressively move our code to using that instead of hard-coding.
Attachment #8799835 - Flags: review?(bas)
Assignee: nobody → nical.bugzilla
Attachment #8799837 - Flags: review?(bas)
Comment on attachment 8799836 [details] [diff] [review]
an equivalent of gfxPlatform GetSoftwareBackend that can be called from inside Moz2D

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

::: gfx/2d/Factory.cpp
@@ +222,5 @@
>  
> +BackendType
> +Factory::GetDefaultSoftwareBackend()
> +{
> +  return sConfig ? sConfig->mDefaultSoftwareBackend : BackendType::CAIRO;

When do we not have sConfig, and should it be an error when we call this if we haven't configured the backend(s)?

::: gfx/thebes/gfxPlatform.cpp
@@ +675,5 @@
>      GLContext::StaticInit();
>  #endif
>  
> +
> +    mozilla::gfx::Config cfg;

Why this change?  As in, why did this code have to move, and what are the implications now that this comes after the Mutex lock above?
Comment on attachment 8799835 [details] [diff] [review]
gfxPlatform::GetSoftwareBackend

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

::: gfx/thebes/gfxPlatform.h
@@ +822,5 @@
>      mozilla::gfx::BackendType mFallbackCanvasBackend;
>      // The backend to use for content
>      mozilla::gfx::BackendType mContentBackend;
> +    // The backend to use when we need it not to be accelerated.
> +    mozilla::gfx::BackendType mSoftwareBackend;

Naming nit - should there be a word "content" somewhere in the name?  Also, should the canvas fallback and content fallback be consistently named, as in, should this be the mFallbackContentBackend?
(In reply to Milan Sreckovic [:milan] from comment #6)
> When do we not have sConfig, and should it be an error when we call this if
> we haven't configured the backend(s)?

gtests is the only thing that comes to mind.

> > +
> > +    mozilla::gfx::Config cfg;
> 
> Why this change?  As in, why did this code have to move,

Had to move it down after gfxPlatform::InitBackendPrefs is run from gfxPlatform's ctor so that the software backend has been chosen when we init gfx::Factory.

> and what are the
> implications now that this comes after the Mutex lock above?

I haven't seen any interaction with this mutex in the initialization of gfx::Factory.

> Naming nit - should there be a word "content" somewhere in the name?

I don't feel very strongly about the naming here, but this backend isn't intended to be specific to content. It is the backend we choose when we need the target to be in CPU memory in the places where we would usually hard-code cairo to that effect, regardless of whether the temporary DrawTarget is used by content or canvas (Internally I chose to call GetContentBackendPref because we tend to be more conservative about content backends than canvas backends which is probably what we want here, and the code would be the same if we added another Get*Pref function).

> Also,
> should the canvas fallback and content fallback be consistently named, as
> in, should this be the mFallbackContentBackend?

It's not a fallback, it's about the cases where we want to use a CPU backend because we wrap it around existing CPU-side memory, or need to access the result on the CPU right after (example: when rendering a tab in a video stream, the resulting frame will be encoded on the CPU so we don't want to render with D2D just to do a read-back immediately after).
Attachment #8799839 - Flags: review?(bas) → review+
Attachment #8799837 - Flags: review?(bas) → review+
Attachment #8799836 - Flags: review?(bas) → review+
Attachment #8799835 - Flags: review?(bas) → review+
See Also: → 1310064
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1563a66b64d8
Add gfxPlatfrom::GetSoftwareBackend. r=Bas
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5ad7d04ed43
Add gfx::Factory::GetDefaultSoftwareBackend. r=Bas
Whiteboard: [gfx-noted][leave-open]
Comment on attachment 8799836 [details] [diff] [review]
an equivalent of gfxPlatform GetSoftwareBackend that can be called from inside Moz2D

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

::: gfx/thebes/gfxPlatform.cpp
@@ +679,5 @@
> +    mozilla::gfx::Config cfg;
> +    cfg.mLogForwarder = fwd;
> +    cfg.mMaxTextureSize = gfxPrefs::MaxTextureSize();
> +    cfg.mMaxAllocSize = gfxPrefs::MaxAllocSize();
> +    cfg.mDefaultSoftwareBackend = gPlatform->GetSoftwareBackend();

Do we actually have an instance of gfxPlatform in the GPU process?
I'll get back to this in the short/medium term. In the mean time if anyone is blocked on parts of this, the patch that adds the gfxPlatform method is fine to land, while the moz2d one is probably not.
Flags: needinfo?(nical.bugzilla)
(In reply to Nicolas Silva [:nical] from comment #12)
> I'll get back to this in the short/medium term. In the mean time if anyone
> is blocked on parts of this, the patch that adds the gfxPlatform method is
> fine to land, while the moz2d one is probably not.

For Bug 1310064, I'd tried to apply parts of patches that adds the gfxPlatform method locally and it if fine as expected. Will you land this part? Or do you mind if I picked them into Bug 1310064?
Flags: needinfo?(nical.bugzilla)
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a687eff8e5
Add gfxPlatfrom::GetSoftwareBackend. r=Bas
(In reply to Vincent Liu[:vliu] from comment #13)
> Will you land this part? Or do you mind if I picked them into Bug 1310064?

Just landed it, thanks for the reminder I had forgotten about this.
Flags: needinfo?(nical.bugzilla)
Attached patch Use GetSoftwareBackend in dom (obsolete) — Splinter Review
Attachment #8841365 - Flags: review?(nical.bugzilla)
Attachment #8841366 - Flags: review?(nical.bugzilla)
Attachment #8841365 - Attachment is obsolete: true
Attachment #8841365 - Flags: review?(nical.bugzilla)
Attachment #8841532 - Flags: review?(nical.bugzilla)
Attachment #8841539 - Flags: review?(nical.bugzilla)
Hi Paul, I'm away for a week, I'll get to these reviews early next week, sorry about the delay.
Attachment #8841913 - Flags: review?(nical.bugzilla)
Attached patch Use GetSoftwareBackend in widget (obsolete) — Splinter Review
Attachment #8841917 - Flags: review?(nical.bugzilla)
Attachment #8841532 - Flags: review?(nical.bugzilla) → review+
Attachment #8841913 - Flags: review?(nical.bugzilla) → review+
Attachment #8841366 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8841539 [details] [diff] [review]
Use GetSoftwareBackend in dom/plugin

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

Please don't modify white spaces all over the file far from the thing you are fixing in your patch (as much as I dislike trailing spaces, it's important to keep the history easy to browse through).
Comment on attachment 8841913 [details] [diff] [review]
Use GetSoftwareBackend in image

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

Removing the r+ on this one because it breaks the test toolkit/components/places/tests/favicons/test_favicons_conversions.js (see try push). I bisected the patch queue locally to make sure that it was indeed this one. I'm not sure why the test fails with skia.
Attachment #8841913 - Flags: review+ → feedback+
Attachment #8847761 - Flags: review?(nical.bugzilla)
Attachment #8841539 - Attachment is obsolete: true
Attachment #8841539 - Flags: review?(nical.bugzilla)
Thanks for the reviews so far and here's the dom/plugin patch without whitespace removal.
Btw it looks like mercurial doesn't support the -w option that git has (at least not for qnew) to ignore whitespaces at patch creation.

Although I found the reference to the file you mentioned, I can't make much of the treeherder report so I'll proceed with the other patches if that's ok with you.

Regards,
Paul
Attachment #8847780 - Flags: review?(nical.bugzilla)
Attachment #8841917 - Attachment is obsolete: true
Attachment #8841917 - Flags: review?(nical.bugzilla)
Attachment #8847782 - Flags: review?(nical.bugzilla)
Attachment #8847761 - Attachment is obsolete: true
Attachment #8847761 - Flags: review?(nical.bugzilla)
Comment on attachment 8847782 [details] [diff] [review]
Use GetSoftwareBackend in dom/plugins

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

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +3512,5 @@
>          // Certain backends (i.e. Skia) may not truly support BGRX formats, so they must
>          // be emulated by filling the alpha channel opaque as if it was BGRA data. Cairo
>          // leaves the alpha zeroed out for BGRX, so we cause Cairo to fill it as opaque
>          // by handling the copy target as a BGRA surface.
> +        dt = Factory::CreateDrawTargetForData(gfxPlatform::GetPlatform()->GetSoftwareBackend(),

This one is most likely going to be trickier, as the comment above suggests. The problem is that the skia backend assumes that the BGRX format is in fact BGRA with A always equal to 255, while cairo really ignores the X channel values. This means that we should not pass a BGRX image to skia that doesn't have its X channel properly set to 255. before this patch cairo automatically fills the X channel when copying the BRGA surface into a BGRX one below, so we have to preserve this behavior somewhere.

A simple way to go here is to check if the backend is skia and if he format is BGRX, and fix the X channel before creating the DrawTarget around it.

I think that we have a function somewhere in gecko to fix up the X channel.
Attachment #8849978 - Flags: review?(nical.bugzilla)
Attachment #8847782 - Attachment is obsolete: true
Attachment #8847782 - Flags: review?(nical.bugzilla)
Comment on attachment 8849978 [details] [diff] [review]
Use GetSoftwareBackend in dom/plugins

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

Looks good, just a few nits to fix.

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +3510,5 @@
>            aSurface->GetSurfaceFormat() == SurfaceFormat::B8G8R8X8) {
>          gfxImageSurface* imageSurface = static_cast<gfxImageSurface*>(aSurface);
>          // Bug 1196927 - Reinterpret target surface as BGRA to fill alpha with opaque.
>          // Certain backends (i.e. Skia) may not truly support BGRX formats, so they must
>          // be emulated by filling the alpha channel opaque as if it was BGRA data. Cairo

Please remove the part "Cairo eaves the alpha zeroed out for BGRX, so we cause Cairo to fill it as opaque by handling the copy target as a BGRA surface." of this comment, since your patch fixes the issue without doing that hack.

@@ +3515,5 @@
>          // leaves the alpha zeroed out for BGRX, so we cause Cairo to fill it as opaque
>          // by handling the copy target as a BGRA surface.
> +        BackendType backendType = gfxPlatform::GetPlatform()->GetSoftwareBackend();
> +        if (backendType == BackendType::SKIA) {
> +            // Overwrite from BGRX to BGRA format

Maybe: "Overwrite from BGRX to BGRA format in order to fill the X channel with opaque alpha values."

@@ +3525,4 @@
>                                                imageSurface->Data(),
>                                                imageSurface->GetSize(),
>                                                imageSurface->Stride(),
>                                                SurfaceFormat::B8G8R8A8);

Here we actually want BGRX I believe. We previously used BGRA as a hack to force the cairo backend to fill the X/A channel, but now that you do this explicitly with SwizzleData we should stick to BGRX.
Attachment #8849978 - Flags: feedback+
Attachment #8847780 - Flags: review?(nical.bugzilla) → review+
Attachment #8847780 - Flags: checkin+
Attachment #8841532 - Flags: checkin+
Attachment #8841366 - Flags: checkin+
Assignee: nical.bugzilla → paul.bignier
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad10adb391af
Backout changeset 990c876f2c10 because of test failures. r=me
Comment on attachment 8847780 [details] [diff] [review]
Use GetSoftwareBackend in widget

removing the r+ from this one since it appears to have caused some failures, see https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=8a543634c62025922b824819751ce8f77edfe57b&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&selectedJob=86192884

These are caused by the same skia issue with RGBX. It means one of these surfaces come from a source that does not enforce the X channel to be filled with 255, we need to find where these surfaces come from and do the swizzle thing.
Attachment #8847780 - Flags: review+
Attachment #8847780 - Flags: checkin+
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc2d2326a0e
Backed out changeset 0447969a62c2 due to test failures. r=me
Comment on attachment 8841532 [details] [diff] [review]
Use GetSoftwareBackend in dom/media

I was pretty sure this patch had passed the tests a week ago, but it happens to cause failures because it calls into gfxPlatform from the GPU process which we apparently don't allow.

I had to back it out, sorry.
Paul, let's leave this one on the side until we get a chance to talk about it on irc since it needs more thinking than expected to figure out a good way to select the backend on the GPU process.
Attachment #8841532 - Flags: review-
Attachment #8841532 - Flags: review+
Attachment #8841532 - Flags: checkin+
Attachment #8851216 - Flags: review?(nical.bugzilla)
Attachment #8849978 - Attachment is obsolete: true
Attachment #8849978 - Flags: review?(nical.bugzilla)
Attachment #8851216 - Flags: review?(nical.bugzilla) → review+
This should let us access the software backend on the GPU process at least (and maybe on the plugin process? I don't know).
Attachment #8851950 - Flags: review?(dvander)
Comment on attachment 8851216 [details] [diff] [review]
Use GetSoftwareBackend in dom/plugins

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

Clearing the review on this patch because it turns out we can't access the prefs on the plugin process (where PluginInstanceChild is). I'm not sure whether we can get away with using gfxVars instead (probably not), if not we'll have to hard-code the backend to skia. I'll  come back to you when I have the more information about this.
Attachment #8851216 - Flags: review+ → feedback+
Comment on attachment 8851950 [details] [diff] [review]
expose the software backend on gfxVars.

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

I don't think you'd get the plugin process for free. You'd have to hook it up like PContent/PGPU are, by making gfxVars send updates and the initial list through PPluginModuleParent.

Or you could just have the plugin process grab the variable as needed via IPDL.
Attachment #8851950 - Flags: review?(dvander) → review+
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/60cb618034c4
Expose the software backend in gfxVars. r=dvander
Depends on: 1351725
Nical, Can be it be closed now? thanks
Flags: needinfo?(nical.bugzilla)
Keywords: leave-open
Whiteboard: [gfx-noted][leave-open] → [gfx-noted]
We still hard-code cairo in a few places so let's keep it open for now.
Flags: needinfo?(nical.bugzilla)
Assignee: paul.bignier → nobody

The leave-open keyword is there and there is no activity for 6 months.
:jbonisteel, maybe it's time to close this bug?

Flags: needinfo?(jbonisteel)

It's not high priority but there are still places where we hard-code cairo so let's keep it open.

Flags: needinfo?(jbonisteel)

The leave-open keyword is there and there is no activity for 6 months.
:jbonisteel, maybe it's time to close this bug?

Flags: needinfo?(jbonisteel)

We can leave it open

Flags: needinfo?(jbonisteel)

The leave-open keyword is there and there is no activity for 6 months.
:ktaeleman, maybe it's time to close this bug?

Flags: needinfo?(ktaeleman)

Let's leave it open :)

Flags: needinfo?(ktaeleman)

The leave-open keyword is there and there is no activity for 6 months.
:jimm, maybe it's time to close this bug?

Flags: needinfo?(jmathies)

This looks tech debt related, lets keep it open.

Flags: needinfo?(jmathies)

The leave-open keyword is there and there is no activity for 6 months.
:jimm, maybe it's time to close this bug?

Flags: needinfo?(jmathies)

The leave-open keyword is there and there is no activity for 6 months.
:bhood, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.

Flags: needinfo?(bhood)
Flags: needinfo?(bhood)
Flags: needinfo?(jmathies)

Nical, any reason to keep this open?

Flags: needinfo?(nical.bugzilla)

I'not sure whether we still have cairo in a few places where it's not wanted but if so it hasn't been enough of an issue for us to bother over many yers so let's close.

Status: NEW → RESOLVED
Closed: 2 years ago
Flags: needinfo?(nical.bugzilla)
Resolution: --- → FIXED
Assignee: nobody → paul.bignier
You need to log in before you can comment on or make changes to this bug.