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.

f+

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).

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

Backed out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=38383126&repo=mozilla-inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/6f5c85e2f3f8

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.

(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?

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.

Hi Paul, I'm away for a week, I'll get to these reviews early next week, sorry about the delay.

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).

Try push with the 3 patches currently r+'ed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4ef20255a45eed4813ca8cf20f52c5cdafe341f8

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.

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

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.

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.

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

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.

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.

https://hg.mozilla.org/mozilla-central/rev/70c511388557 https://hg.mozilla.org/mozilla-central/rev/0447969a62c2 https://hg.mozilla.org/mozilla-central/rev/990c876f2c10 https://hg.mozilla.org/mozilla-central/rev/ad10adb391af https://hg.mozilla.org/mozilla-central/rev/ecc2d2326a0e

try push with the dom/plugins patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=74d4f4326a13e4262b495c7ed91394c58ffb946d

This should let us access the software backend on the GPU process at least (and maybe on the plugin process? I don't know).

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.

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.

Pushed by nsilva@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/60cb618034c4 Expose the software backend in gfxVars. r=dvander

Nical, Can be it be closed now? thanks

We still hard-code cairo in a few places so let's keep it open for now.