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

NEW
Assigned to

Status

()

P3
normal
2 years ago
14 days ago

People

(Reporter: nical, Assigned: paul.bignier)

Tracking

({leave-open})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [gfx-noted])

Attachments

(10 attachments, 6 obsolete attachments)

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
(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 2

2 years ago
Created attachment 8799835 [details] [diff] [review]
gfxPlatform::GetSoftwareBackend
Attachment #8799835 - Flags: review?(bas)
(Reporter)

Comment 3

2 years ago
Created attachment 8799836 [details] [diff] [review]
an equivalent of gfxPlatform GetSoftwareBackend that can be called from inside Moz2D
Attachment #8799836 - Flags: review?(bas)
(Reporter)

Comment 4

2 years ago
Created attachment 8799837 [details] [diff] [review]
Use GetSoftwareBackend when rendering a tab in a video.
Assignee: nobody → nical.bugzilla
(Reporter)

Updated

2 years ago
Attachment #8799837 - Flags: review?(bas)
(Reporter)

Comment 5

2 years ago
Created attachment 8799839 [details] [diff] [review]
Use GetSoftwareBackend in DrawTargetTiled::GetDataSurface
Attachment #8799839 - 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?
(Reporter)

Comment 8

2 years ago
(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+

Updated

2 years ago
See Also: → bug 1310064

Comment 9

2 years ago
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
(Reporter)

Updated

2 years ago
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?
(Reporter)

Comment 12

2 years ago
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)

Comment 13

2 years ago
(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)

Comment 14

2 years ago
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b4a687eff8e5
Add gfxPlatfrom::GetSoftwareBackend. r=Bas
(Reporter)

Comment 15

2 years ago
(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)
(Assignee)

Comment 17

2 years ago
Created attachment 8841365 [details] [diff] [review]
Use GetSoftwareBackend in dom
Attachment #8841365 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 18

2 years ago
Created attachment 8841366 [details] [diff] [review]
Use GetSoftwareBackend in dom
Attachment #8841366 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8841365 - Attachment is obsolete: true
Attachment #8841365 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 19

2 years ago
Created attachment 8841532 [details] [diff] [review]
Use GetSoftwareBackend in dom/media
Attachment #8841532 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 20

2 years ago
Created attachment 8841539 [details] [diff] [review]
Use GetSoftwareBackend in dom/plugin
Attachment #8841539 - Flags: review?(nical.bugzilla)
(Reporter)

Comment 21

2 years ago
Hi Paul, I'm away for a week, I'll get to these reviews early next week, sorry about the delay.
(Assignee)

Comment 22

2 years ago
Created attachment 8841913 [details] [diff] [review]
Use GetSoftwareBackend in image
Attachment #8841913 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 23

2 years ago
Created attachment 8841917 [details] [diff] [review]
Use GetSoftwareBackend in widget
Attachment #8841917 - Flags: review?(nical.bugzilla)
(Reporter)

Updated

2 years ago
Attachment #8841532 - Flags: review?(nical.bugzilla) → review+
(Reporter)

Updated

2 years ago
Attachment #8841913 - Flags: review?(nical.bugzilla) → review+
(Reporter)

Updated

2 years ago
Attachment #8841366 - Flags: review?(nical.bugzilla) → review+
(Reporter)

Comment 24

2 years ago
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).
(Reporter)

Comment 26

2 years ago
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+
(Assignee)

Comment 27

2 years ago
Created attachment 8847761 [details] [diff] [review]
Use GetSoftwareBackend in dom/plugin
Attachment #8847761 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8841539 - Attachment is obsolete: true
Attachment #8841539 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 28

2 years ago
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
(Assignee)

Comment 29

2 years ago
Created attachment 8847780 [details] [diff] [review]
Use GetSoftwareBackend in widget
Attachment #8847780 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8841917 - Attachment is obsolete: true
Attachment #8841917 - Flags: review?(nical.bugzilla)
(Assignee)

Comment 30

2 years ago
Created attachment 8847782 [details] [diff] [review]
Use GetSoftwareBackend in dom/plugins
Attachment #8847782 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8847761 - Attachment is obsolete: true
Attachment #8847761 - Flags: review?(nical.bugzilla)
(Reporter)

Comment 31

2 years ago
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.
(Assignee)

Comment 32

2 years ago
Created attachment 8849978 [details] [diff] [review]
Use GetSoftwareBackend in dom/plugins
Attachment #8849978 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8847782 - Attachment is obsolete: true
Attachment #8847782 - Flags: review?(nical.bugzilla)
(Reporter)

Comment 33

2 years ago
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+

Comment 34

2 years ago
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/70c511388557
Use GetSoftwareBackend in dom. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/0447969a62c2
Use GetSoftwareBackend in dom/media. r=nical
https://hg.mozilla.org/integration/mozilla-inbound/rev/990c876f2c10
Use GetSoftwareBackend in widget. r=nical
(Reporter)

Updated

2 years ago
Attachment #8847780 - Flags: review?(nical.bugzilla) → review+
(Reporter)

Updated

2 years ago
Attachment #8847780 - Flags: checkin+
(Reporter)

Updated

2 years ago
Attachment #8841532 - Flags: checkin+
(Reporter)

Updated

2 years ago
Attachment #8841366 - Flags: checkin+
(Reporter)

Updated

2 years ago
Assignee: nical.bugzilla → paul.bignier

Comment 35

2 years ago
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad10adb391af
Backout changeset 990c876f2c10 because of test failures. r=me
(Reporter)

Comment 36

2 years ago
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+

Comment 37

2 years ago
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecc2d2326a0e
Backed out changeset 0447969a62c2 due to test failures. r=me
(Reporter)

Comment 38

2 years ago
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+
(Assignee)

Comment 40

2 years ago
Created attachment 8851216 [details] [diff] [review]
Use GetSoftwareBackend in dom/plugins
Attachment #8851216 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

2 years ago
Attachment #8849978 - Attachment is obsolete: true
Attachment #8849978 - Flags: review?(nical.bugzilla)
(Reporter)

Updated

2 years ago
Attachment #8851216 - Flags: review?(nical.bugzilla) → review+
(Reporter)

Comment 42

2 years ago
Created attachment 8851950 [details] [diff] [review]
expose the software backend on gfxVars.

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)
(Reporter)

Comment 43

2 years ago
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+

Comment 45

2 years ago
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]
(Reporter)

Comment 48

14 days ago
We still hard-code cairo in a few places so let's keep it open for now.
Flags: needinfo?(nical.bugzilla)
You need to log in before you can comment on or make changes to this bug.