Closed Bug 1273990 Opened 4 years ago Closed 4 years ago

Wrap Basic Compositor's Back Buffer with Skia or Cairo

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: mchang, Assigned: mchang)

Details

(Whiteboard: gfx-noted)

Attachments

(2 files)

Right now, when we have basic layers, we mostly use cairo for everything. The back buffer is bitmap backed. When we composite with basic layers, we currently wrap the data with a cairo backend [1], then BitBlt the data into the actual window HDC. This is causing the assertion to fail when we have skia content due to the default screen backend being skia [2]. However, there's no reason we can't use skia to composite since it's just a wrapper around the bitmap data. 

[1] https://dxr.mozilla.org/mozilla-central/source/widget/windows/WinCompositorWidgetProxy.cpp?from=WinCompositorWidgetProxy.cpp#136
[2] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp?from=BasicCompositor.cpp#510
Whiteboard: gfx-noted
Comment on attachment 8753980 [details] [diff] [review]
Wrap back buffer in whatever our screen draw target is

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

::: widget/windows/WinCompositorWidgetProxy.cpp
@@ +132,5 @@
>    if (!target) {
>      return nullptr;
>    }
>  
> +  MOZ_ASSERT(target->GetBackendType() == BackendType::CAIRO ||

I don't understand who guarantees this assertion.  Is it because we "know" that the basic compositor could only send us one of these two?
(In reply to Milan Sreckovic [:milan] from comment #2)
> Comment on attachment 8753980 [details] [diff] [review]
> Wrap back buffer in whatever our screen draw target is
> 
> Review of attachment 8753980 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/WinCompositorWidgetProxy.cpp
> @@ +132,5 @@
> >    if (!target) {
> >      return nullptr;
> >    }
> >  
> > +  MOZ_ASSERT(target->GetBackendType() == BackendType::CAIRO ||
> 
> I don't understand who guarantees this assertion.  Is it because we "know"
> that the basic compositor could only send us one of these two?

It's a weird assert. I think the intent is that the backend should not be D2D (since that would imply we're using the D3D11 compositor). I'd be okay with removing it or changing it to test != DIRECT2D.
Attachment #8753980 - Flags: review?(dvander) → review+
(In reply to David Anderson [:dvander] from comment #3)
> (In reply to Milan Sreckovic [:milan] from comment #2)
> > Comment on attachment 8753980 [details] [diff] [review]
> > Wrap back buffer in whatever our screen draw target is
> > 
> > Review of attachment 8753980 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: widget/windows/WinCompositorWidgetProxy.cpp
> > @@ +132,5 @@
> > >    if (!target) {
> > >      return nullptr;
> > >    }
> > >  
> > > +  MOZ_ASSERT(target->GetBackendType() == BackendType::CAIRO ||
> > 
> > I don't understand who guarantees this assertion.  Is it because we "know"
> > that the basic compositor could only send us one of these two?
> 
> It's a weird assert. I think the intent is that the backend should not be
> D2D (since that would imply we're using the D3D11 compositor). I'd be okay
> with removing it or changing it to test != DIRECT2D.

Changed, thanks!

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=54b533a2f30b
I suspect a better way to fix this is to separate the drawtarget being used for content from the one being used for the compositor. Skia has slower composition so I don't see us switching to it for compositing anytime soon.
(In reply to Jeff Muizelaar [:jrmuizel] from comment #5)
> I suspect a better way to fix this is to separate the drawtarget being used
> for content from the one being used for the compositor. Skia has slower
> composition so I don't see us switching to it for compositing anytime soon.

Right now the basic compositor tries to reuse the draw target from the back buffer, which is usually cairo. The only time we don't do this is when we get a RecvMakeSnapshot[1], which sets up an explicit DrawTarget to composite to. But since this is also cairo, I think what we can do in WinWidgetCompositorProxy is use the target's backendtype to create the buffered DrawTarget instead of the content's default draw target type. It's already a "dummy" draw target in that it's only used as a reference to create another draw target.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp?from=CompositorBridgeParent.cpp#727
Attachment #8754949 - Flags: review?(jmuizelaar)
Attachment #8754949 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/16b370b80a93
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.