Closed Bug 1223736 Opened 4 years ago Closed 4 years ago

Assertion failure: !effectMask->mIs3D, at c:/Users/mozilla/debug-builds/mozilla-central/gfx/layers/basic/BasicCompositor.cpp:395

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox45 + wontfix
firefox46 + wontfix
firefox47 - affected
firefox48 --- fixed

People

(Reporter: cbook, Assigned: mattwoodrow)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion, crash)

Attachments

(9 files, 6 obsolete files)

155.64 KB, text/plain
Details
14.35 KB, patch
mattwoodrow
: review-
Details | Diff | Splinter Review
5.02 KB, patch
Details | Diff | Splinter Review
3.14 KB, patch
Details | Diff | Splinter Review
345 bytes, text/html
Details
3.43 KB, patch
sinker
: review+
Details | Diff | Splinter Review
248 bytes, text/html
Details
4.92 KB, patch
lsalzman
: review+
Details | Diff | Splinter Review
453.42 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
Attached file bughunter crash stack
Found by bughunter and reproduced on a windows 7 trunk debug build 

Steps to reproduce:

--> Load http://www.stroitel.club/
---> Assertion failure: !effectMask->mIs3D, at c:/Users/mozilla/debug-builds/mozilla-central/gfx/layers/b asic/BasicCompositor.cpp:395

Assertion failure: !effectMask->mIs3D, at c:/Users/mozilla/debug-builds/mozilla-central/gfx/layers/b
asic/BasicCompositor.cpp:395
#01: <lambda_50c881b21f2dc08c90ad7c7d1171733e>::operator() (c:\users\mozilla\debug-builds\mozilla-ce
ntral\gfx\layers\composite\containerlayercomposite.cpp:725)
#02: mozilla::layers::RenderWithAllMasks<<lambda_50c881b21f2dc08c90ad7c7d1171733e> > (c:\users\mozil
la\debug-builds\mozilla-central\firefox-debug\dist\include\mozilla\layers\layermanagercomposite.h:55
7)
#03: mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite> (c:\users\mozilla\de
bug-builds\mozilla-central\gfx\layers\composite\containerlayercomposite.cpp:721)
#04: mozilla::layers::ContainerLayerComposite::RenderLayer (c:\users\mozilla\debug-builds\mozilla-ce
ntral\gfx\layers\composite\containerlayercomposite.cpp:802)
#05: mozilla::layers::RenderLayers<mozilla::layers::ContainerLayerComposite> (c:\users\mozilla\debug
-builds\mozilla-central\gfx\layers\composite\containerlayercomposite.cpp:564)
#06: mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite> (c:\users\mozilla\de
bug-builds\mozilla-central\gfx\layers\composite\containerlayercomposite.cpp:728)
#07: mozilla::layers::ContainerLayerComposite::RenderLayer (c:\users\mozilla\debug-builds\mozilla-ce
ntral\gfx\layers\composite\containerlayercomposite.cpp:802)
#08: mozilla::layers::RenderLayers<mozilla::layers::ContainerLayerComposite> (c:\users\mozilla\debug
-builds\mozilla-central\gfx\layers\composite\containerlayercomposite.cpp:564)
#09: mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite> (c:\users\mozilla\de
bug-builds\mozilla-central\gfx\layers\composite\containerlayercomposite.cpp:728)
#10: mozilla::layers::ContainerLayerComposite::RenderLayer (c:\users\mozilla\debug-builds\mozilla-ce
ntral\gfx\layers\composite\containerlayercomposite.cpp:802)
#11: mozilla::layers::LayerManagerComposite::Render (c:\users\mozilla\debug-builds\mozilla-central\g
fx\layers\composite\layermanagercomposite.cpp:791)
#12: mozilla::layers::LayerManagerComposite::UpdateAndRender (c:\users\mozilla\debug-builds\mozilla-
central\gfx\layers\composite\layermanagercomposite.cpp:365)
#13: mozilla::layers::LayerManagerComposite::EndTransaction (c:\users\mozilla\debug-builds\mozilla-c
entral\gfx\layers\composite\layermanagercomposite.cpp:288)
#14: mozilla::layers::CompositorParent::CompositeToTarget (c:\users\mozilla\debug-builds\mozilla-cen
tral\gfx\layers\ipc\compositorparent.cpp:1144)
#15: mozilla::layers::CompositorVsyncScheduler::Composite (c:\users\mozilla\debug-builds\mozilla-cen
tral\gfx\layers\ipc\compositorparent.cpp:425)
#16: DispatchTupleToMethod<mozilla::layers::CompositorVsyncScheduler,void (__thiscall mozilla::layer
s::CompositorVsyncScheduler::*)(mozilla::TimeStamp),mozilla::TimeStamp> (c:\users\mozilla\debug-buil
ds\mozilla-central\ipc\chromium\src\base\task.h:48)
#17: RunnableMethod<mozilla::layers::CompositorVsyncScheduler,void (__thiscall mozilla::layers::Comp
ositorVsyncScheduler::*)(mozilla::TimeStamp),mozilla::Tuple<mozilla::TimeStamp> >::Run (c:\users\moz
illa\debug-builds\mozilla-central\ipc\chromium\src\base\task.h:307)
#18: MessageLoop::DeferOrRunPendingTask (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromium\
src\base\message_loop.cc:375)
#19: MessageLoop::DoWork (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromium\src\base\messag
e_loop.cc:459)
#20: base::MessagePumpForUI::DoRunLoop (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromium\s
rc\base\message_pump_win.cc:211)
#21: base::MessagePumpWin::Run (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromium\src\base\
message_pump_win.h:78)
#22: MessageLoop::RunInternal (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromium\src\base\m
essage_loop.cc:234)
#23: MessageLoop::RunHandler (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromium\src\base\me
ssage_loop.cc:228)
#24: MessageLoop::Run (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromium\src\base\message_l
oop.cc:202)
#25: base::Thread::ThreadMain (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromium\src\base\t
hread.cc:175)
#26: `anonymous namespace'::ThreadFunc (c:\users\mozilla\debug-builds\mozilla-central\ipc\chromium\s
rc\base\platform_thread_win.cc:27)
#27: BaseThreadInitThunk[kernel32 +0x4ee1c]
#28: RtlInitializeExceptionChain[ntdll +0x637eb]
#29: RtlInitializeExceptionChain[ntdll +0x637be]
still able to reproduce with todays builds based on m-c tip

Assertion failure: !effectMask->mIs3D, at c:/Users/mozilla/debug-build/mozilla-central/gfx/layers/basic/BasicCompositor.cpp:394
[4864] WARNING: NS_ENSURE_SUCCESS(status, status) failed with result 0x804B0002: file c:/Users/mozilla/debug-build/mozilla-central/netwerk/protocol/http/nsCORSListenerPro
xy.cpp, line 530
[4864] WARNING: NS_ENSURE_SUCCESS(status, status) failed with result 0x804B0002: file c:/Users/mozilla/debug-build/mozilla-central/netwerk/protocol/http/nsCORSListenerPro
xy.cpp, line 530
[4864] WARNING: NS_ENSURE_SUCCESS(status, status) failed with result 0x804B0002: file c:/Users/mozilla/debug-build/mozilla-central/netwerk/protocol/http/nsCORSListenerPro
xy.cpp, line 530
#01: mozilla::layers::Compositor::DrawQuad (c:\users\mozilla\debug-build\mozilla-central\firefox-debug\dist\include\mozilla\layers\compositor.h:320)
#02: <lambda_50c881b21f2dc08c90ad7c7d1171733e>::operator() (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\containerlayercomposite.cpp:739)
#03: mozilla::layers::RenderWithAllMasks<<lambda_50c881b21f2dc08c90ad7c7d1171733e> > (c:\users\mozilla\debug-build\mozilla-central\firefox-debug\dist\include\mozilla\laye
rs\layermanagercomposite.h:567)
#04: mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite> (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\containerlayercomposite
.cpp:734)
#05: mozilla::layers::ContainerLayerComposite::RenderLayer (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\containerlayercomposite.cpp:815)
#06: mozilla::layers::RenderLayers<mozilla::layers::ContainerLayerComposite> (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\containerlayercomposite.cp
p:580)
#07: mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite> (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\containerlayercomposite
.cpp:741)
#08: mozilla::layers::ContainerLayerComposite::RenderLayer (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\containerlayercomposite.cpp:815)
#09: mozilla::layers::RenderLayers<mozilla::layers::ContainerLayerComposite> (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\containerlayercomposite.cp
p:580)
#10: mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite> (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\containerlayercomposite
.cpp:741)
#11: mozilla::layers::ContainerLayerComposite::RenderLayer (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\containerlayercomposite.cpp:815)
#12: mozilla::layers::LayerManagerComposite::Render (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\layermanagercomposite.cpp:873)
#13: mozilla::layers::LayerManagerComposite::UpdateAndRender (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\layermanagercomposite.cpp:449)
#14: mozilla::layers::LayerManagerComposite::EndTransaction (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\composite\layermanagercomposite.cpp:368)
#15: mozilla::layers::CompositorParent::CompositeToTarget (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\ipc\compositorparent.cpp:1182)
#16: mozilla::layers::CompositorVsyncScheduler::ComposeToTarget (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\ipc\compositorparent.cpp:569)
#17: mozilla::layers::CompositorVsyncScheduler::Composite (c:\users\mozilla\debug-build\mozilla-central\gfx\layers\ipc\compositorparent.cpp:442)
#18: details::CallMethod<0,mozilla::layers::CompositorVsyncScheduler,void (__thiscall mozilla::layers::CompositorVsyncScheduler::*)(mozilla::TimeStamp),mozilla::TimeStamp
> (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\task.h:29)
#19: DispatchTupleToMethod<mozilla::layers::CompositorVsyncScheduler,void (__thiscall mozilla::layers::CompositorVsyncScheduler::*)(mozilla::TimeStamp),mozilla::TimeStamp
> (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\task.h:47)
#20: RunnableMethod<mozilla::layers::CompositorVsyncScheduler,void (__thiscall mozilla::layers::CompositorVsyncScheduler::*)(mozilla::TimeStamp),mozilla::Tuple<mozilla::T
imeStamp> >::Run (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\task.h:307)
#21: MessageLoop::RunTask (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_loop.cc:365)
#22: MessageLoop::DeferOrRunPendingTask (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_loop.cc:375)
#23: MessageLoop::DoWork (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_loop.cc:459)
#24: base::MessagePumpForUI::DoRunLoop (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_pump_win.cc:210)
#25: base::MessagePumpWin::RunWithDispatcher (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_pump_win.cc:56)
#26: base::MessagePumpWin::Run (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_pump_win.h:78)
#27: MessageLoop::RunInternal (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_loop.cc:235)
#28: MessageLoop::RunHandler (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_loop.cc:228)
#29: MessageLoop::Run (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\message_loop.cc:202)
#30: base::Thread::ThreadMain (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\thread.cc:175)
#31: `anonymous namespace'::ThreadFunc (c:\users\mozilla\debug-build\mozilla-central\ipc\chromium\src\base\platform_thread_win.cc:27)
#32: BaseThreadInitThunk[kernel32 +0x4ee6c]
#33: RtlInitializeExceptionChain[ntdll +0x63ab3]
#34: RtlInitializeExceptionChain[ntdll +0x63a86]
Assignee: nobody → nical.bugzilla
So it appears that 3D transforms for masks aren't implemented in the basic compositor. Looking at the GL compositor, there's some code that handles 3d transforms for masks, generates the adequate shader, etc.

Matt, this assertion is from your original implementation of the BasicCompositor. Did it mean that we really expect to not have 3d transforms for masks (as in, some other code makes sure that it is handled at another level), or is it more intended as a TODO item?
Flags: needinfo?(matt.woodrow)
This reproduces on Linux as well, and more generally I assume it'll reproduce on any platform if BasicCompositor is enabled.
I believe this changed with thinkers work in bug 1097474, so we might need to implement support for 3d masks in BasicCompositor.
Flags: needinfo?(matt.woodrow) → needinfo?(tlee)
Since it always has an intermediate surface for this case, it should not be with 3d transform applied on the masks.  Maybe we should check if the mask's parent using an intermediate surface for EffectChain and effective transform of mask layers.
Flags: needinfo?(tlee)
still reproducible with todays build, is there any eta when we get this fixed ?
Flags: needinfo?(tlee)
Yes! We should fix it before next beta.  I take it.
Assignee: nical.bugzilla → tlee
Flags: needinfo?(tlee)
Tracking it.
Thinker, do you have an eta? We are in the last week of the beta cycle.
Flags: needinfo?(tlee)
Matt,
Is this line necessary?

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp?case=true&from=BasicCompositor.cpp#435

I have found other compositor implementations allowing mIs3D and switching between 2D and 3D mask.

Sylvestre,
I need one or two days.
Flags: needinfo?(tlee) → needinfo?(matt.woodrow)
--
Attachment #8723488 [details] [diff] - Attachment is obsolete: true
Attachment #8723490 - Flags: review?(matt.woodrow)
Attachment #8723488 - Attachment is obsolete: true
Attachment #8723488 - Flags: review?(matt.woodrow)
(In reply to Thinker Li [:sinker] from comment #9)
> Matt,
> Is this line necessary?
> 
> https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/
> BasicCompositor.cpp?case=true&from=BasicCompositor.cpp#435
> 
> I have found other compositor implementations allowing mIs3D and switching
> between 2D and 3D mask.
> 
> Sylvestre,
> I need one or two days.

Yes, definitely. The following line does .As2D() on the matrix, which will truncate it and throw away any 3d components.
Flags: needinfo?(matt.woodrow)
Comment on attachment 8723490 [details] [diff] [review]
Add a separator item to avoid mask layers with 3d transform, v2

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

::: layout/generic/nsFrame.cpp
@@ +2310,5 @@
>  
>      nsDisplayTransform *transformItem =
>        new (aBuilder) nsDisplayTransform(aBuilder, this, &resultList, dirtyRect);
> +    if (transformItem->GetClip().HasClip() &&
> +        !transformItem->GetTransform().CanDraw2D()) {

I think this needs to be outside the nsDisplayPerspective to be correct, doesn't it?
--
Attachment #8723490 [details] [diff] - Attachment is obsolete: true
Attachment #8723977 - Flags: review?(matt.woodrow)
Attachment #8723490 - Attachment is obsolete: true
Attachment #8723490 - Flags: review?(matt.woodrow)
Comment on attachment 8723977 [details] [diff] [review]
Add a separator item to avoid mask layers with 3d transform, v3

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

::: layout/generic/nsFrame.cpp
@@ +2345,5 @@
>            aBuilder, this,
>            GetContainingBlock()->GetContent()->GetPrimaryFrame(), &resultList));
>      }
>  
> +    if ((hasPerspective && hasTransformClip) ||

Why do we need two separate blocks of code here?

I don't understand why we need the inner block replacing the nsDisplayTransform.

Can you explain this more please :)
(In reply to Matt Woodrow (:mattwoodrow) from comment #16)
> I don't understand why we need the inner block replacing the
> nsDisplayTransform.
> 
> Can you explain this more please :)

I have made it complicated.  Change the way to do it in next patch.
--
Attachment #8723977 [details] [diff] - Attachment is obsolete: true
Attachment #8725098 - Flags: review?(matt.woodrow)
Attachment #8723977 - Attachment is obsolete: true
Attachment #8723977 - Flags: review?(matt.woodrow)
Attachment #8725098 - Flags: review?(matt.woodrow)
Comment on attachment 8725098 [details] [diff] [review]
Add a separator item to avoid mask layers with 3d transform, v4

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

::: layout/generic/nsFrame.cpp
@@ +2331,5 @@
> +      // nsDisplayTransform gets its clip from |aBuilder| in its
> +      // constructor.  The clip should be moved to the separator, or
> +      // it might create a mask that can not be handled by
> +      // BasicCompositor.  (see BasicCompositor::DrawQuad()).
> +      transformItem->ResetClips(aBuilder);

Couldn't we skip the clipState.Restore() on line 2307 if 'hasTransformClip', and then we wouldn't need to call ResetClips?

I think if we move this block up above to 2304, make that change, and then move the line 2348 Restore() call to the start of the if (needClipSeparator) { block (line 2352) then it'll all work.
Comment on attachment 8725098 [details] [diff] [review]
Add a separator item to avoid mask layers with 3d transform, v4

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

::: layout/generic/nsFrame.cpp
@@ +2331,5 @@
> +      // nsDisplayTransform gets its clip from |aBuilder| in its
> +      // constructor.  The clip should be moved to the separator, or
> +      // it might create a mask that can not be handled by
> +      // BasicCompositor.  (see BasicCompositor::DrawQuad()).
> +      transformItem->ResetClips(aBuilder);

Do you mean the block including line 2323?
Considering the case |CanDraw2D()| is true and |hasPerspective| is false, we still need to apply the clip on the transform item, or we need an additional separator to hold the clip.  And this case is more likely to happen than other cases.
Attachment #8725098 - Attachment is obsolete: true
I meant something like this, does this still work?
Comment on attachment 8726044 [details] [diff] [review]
attachment.cgi?id=8725098

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

::: layout/generic/nsFrame.cpp
@@ +2225,5 @@
>      }
>  
> +    const bool hasPerspective = HasPerspective();
> +    const bool hasTransformClip =
> +      transformItem->GetClip().HasClip() || transformItem->ScrollClip();

Actually, I guess this bit doesn't, but you should be able to get the same information from the clipState object instead.
Comment on attachment 8726044 [details] [diff] [review]
attachment.cgi?id=8725098

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

::: layout/generic/nsFrame.cpp
@@ +2228,5 @@
> +    const bool hasTransformClip =
> +      transformItem->GetClip().HasClip() || transformItem->ScrollClip();
> +    bool needClipSeparator = false;
> +    if (hasTransformClip &&
> +        !transformItem->GetTransform().CanDraw2D()) {

The major problem is this line.  Getting transform from the frame directly is expensive.  I try to avoid it by calling GetTransform() which has a cache of the result, but GetTransform() is only available after creating the item.
Comment on attachment 8725590 [details] [diff] [review]
Add a separator item to avoid mask layers with 3d transform, v5

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

Sorry for taking so long to get to this properly.

We should fix this within BasicCompositor, in the same way that BasicLayerManager handles it.

We transform our 3d content into a separate buffer, and then blit this back to the original destination [1].

We should be apply the mask during this final blit, which is what BasicLayerManager does.

If the transform is 3d, then we need to clear out sourceMask/maskTransform so that the initial draw isn't mask. We then need to change the code at [1] to use FillRectWithMask instead so that the mask layer gets applied at that stage. The mask layer transform we are provided already includes the layer transform, so we need to multiply by the inverse of that to remove it (since the content is already transformed at this point).


[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicCompositor.cpp#567
Attachment #8725590 - Flags: review?(matt.woodrow) → review-
(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> If the transform is 3d, then we need to clear out sourceMask/maskTransform
> so that the initial draw isn't mask. We then need to change the code at [1]
> to use FillRectWithMask instead so that the mask layer gets applied at that
> stage. The mask layer transform we are provided already includes the layer
> transform, so we need to multiply by the inverse of that to remove it (since
> the content is already transformed at this point).

At first I had the same idea, but I afraid we would break the semantic of the interface of Compositor.  If it is OK to change that, this would be easier.
Too late for 45, we probably want to track it for 46.
(In reply to Thinker Li [:sinker] from comment #27)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> > If the transform is 3d, then we need to clear out sourceMask/maskTransform
> > so that the initial draw isn't mask. We then need to change the code at [1]
> > to use FillRectWithMask instead so that the mask layer gets applied at that
> > stage. The mask layer transform we are provided already includes the layer
> > transform, so we need to multiply by the inverse of that to remove it (since
> > the content is already transformed at this point).
> 
> At first I had the same idea, but I afraid we would break the semantic of
> the interface of Compositor.  If it is OK to change that, this would be
> easier.

I think we can fix this entirely within BasicCompositor, without touching the Compositor interface at all.
I had read the code again.  I found if |mIs3D| is true, |maskTransform.CanDraw2D()| will be true.

  See https://dxr.mozilla.org/mozilla-central/source/gfx/layers/Layers.cpp?from=Layers.cpp#1462 and
      https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.h#563

Why we need this assertion?  We never get a 3D |maskTransform| here.
Flags: needinfo?(matt.woodrow)
Why can't aTransformToSurface be 3d in your first link?
Flags: needinfo?(matt.woodrow)
It can be, but maskTransform would be 2D for passing ID transform to ComputeEffectiveTransformForMaskLayers() at first link and the local transform of the mask layer being always 2D.
Since maskTransform is 2D, BasicCompositor::DrawQuad() would pass no 3D transform to Moz2D.  I see no use the assertion here.
Mat, I mistook something on Monday. So, I add an assertion and some comments to make it more clear why the assertion of mIs3D can be removed.
Flags: needinfo?(matt.woodrow)
Attached file mask-test.html
Ok, I think I understand this a lot better now.

I agree with the MOZ_ASSERT and comment you added in DefaultComputeEffectiveTransforms, that seems correct.

I think the if() statement below it is still invalid though, and we should *always* be calling ComputeEffectiveTransformForMaskLayers with aTransformToSurface, never Matrix4x4().

I have attached a testcase which causes the Matrix4x4() path to be hit, and it renders wrong.

Getting rid of the if() statement, and calling it with an unconditional call to ComputeEffectiveTransformForMaskLayers(aTransformToSurface) fixes the rendering.

So we should never get a 3d mask transform, so the assertion for that in BasicCompositor is valid.

mIs3D actually means that the layer transform is 3d, not the mask transform, and we need to do perspective correct interpolation. I haven't thought about how this will work for BasicCompositor yet, but here's an example of what d3d10 needed to do for it: https://hg.mozilla.org/integration/mozilla-inbound/rev/16f39df9be8b#l1.167


I also think the current BasicCompositor code for rendering a 3d transform + a mask layer is incorrect still.

The expectation is that you transform the content by the layer transform, and the mask by the mask transform and then sample content where the mask allows.

For BasicCompositor (with 3d content) we're drawing the content *without* the layer transform, and applying the mask at this point. We then do a second pass to 3d transform the content to the destination.

So for the draw that gets masked, we're using the same mask transform as the accelerated compositors, but a different layer transform. Surely this means that the mask will be in the wrong position.

We really need a test for both of these.
Flags: needinfo?(matt.woodrow)
I've convinced myself that we don't need to worry about mIs3D for BasicCompositor, the perspective correct interpolation isn't an issue for software.

We just need to skip the mask when drawing the main contents during DrawQuad with 3d, and then apply the mask during the 3d transform step at the end.

DrawTargetSkia::MaskSurface is good example code for this.
This fixes the first bug, that the computed mask transform was sometimes wrong.

We still need to fix BasicCompositor rendering, and remove the mIs3D assertion.
Attachment #8731511 - Flags: review?(tlee)
Attached file BasicCompositor test
Another test that shows BasicCompositor's rendering to be broken.

With my Part 1 patch, this renders fine in CompositorOGL, but still very broken with BasicCompositor.
Lee, I know you're in the process of changing how 3d transforms in BasicCompositor work, but I can't find the bug again now.

We've now figured out that we need to be able to mask at the same time as doing the 3d transform, will that work with your new code?

The alternative is to mask during the final blit, but the mask transform isn't guaranteed to be a translation only (it can have scaling too), so swapping DrawSurface with MaskSurface wouldn't work.
I believe Mask() isn't fully implemented on all backends either.

The Part 1 patch fixes another bug and adds a test, this still fails with BasicCompositor. You can also check the 'BasicCompositor test' file for a clearer view of what's going wrong (part 1 bug fix still required for sanity).
Attachment #8731553 - Flags: feedback?(lsalzman)
Comment on attachment 8731511 [details] [diff] [review]
Part 1: Set correct effective mask transform

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

I have been convinced by all information collected from the code in these days.  This change is the right way, or the mask may be misplaced if the parent is not |useIntermediateSurface|.
Attachment #8731511 - Flags: review?(tlee) → review+
Comment on attachment 8731553 [details] [diff] [review]
Part 2: Fix masking in BasicCompositor - half implemented

I'm waiting for review still on bug 1252324, but the 3D transform changes are mostly done.

So as it is, DrawTarget::Draw3DTransformedSurface only 3D transforms a surface, it's not going to work with a mask explicitly, so you'd need to compose the source and mask to an intermediate surface still. But, couldn't you just essentially multiply the source by the mask before the transform?

I guess if necessary we could make Draw3DTransformedSurface take a composition operator so that it could do the multiply.
Attachment #8731553 - Flags: feedback?(lsalzman) → feedback+
(In reply to Lee Salzman [:lsalzman] from comment #40)
> Comment on attachment 8731553 [details] [diff] [review]
> Part 2: Fix masking in BasicCompositor - half implemented
> 
> I'm waiting for review still on bug 1252324, but the 3D transform changes
> are mostly done.
> 
> So as it is, DrawTarget::Draw3DTransformedSurface only 3D transforms a
> surface, it's not going to work with a mask explicitly, so you'd need to
> compose the source and mask to an intermediate surface still. But, couldn't
> you just essentially multiply the source by the mask before the transform?
> 
> I guess if necessary we could make Draw3DTransformedSurface take a
> composition operator so that it could do the multiply.

We can't compose the source and mask together beforehand, because the source has a 3d transform, and the mask doesn't (both transforms convert to destination pixels).

We'd have to apply the inverse of the source transform to the mask transform, and that puts us back into needing a function that can handle 3d transforms.

We basically need a Draw3DTransformedSurface to take a mask + maskTransform, or we need to do the mask post-transform (which requires DrawTarget::Mask to be implemented correctly everywhere).
(In reply to Matt Woodrow (:mattwoodrow) from comment #41)
> (In reply to Lee Salzman [:lsalzman] from comment #40)
> > Comment on attachment 8731553 [details] [diff] [review]
> > Part 2: Fix masking in BasicCompositor - half implemented
> > 
> > I'm waiting for review still on bug 1252324, but the 3D transform changes
> > are mostly done.
> > 
> > So as it is, DrawTarget::Draw3DTransformedSurface only 3D transforms a
> > surface, it's not going to work with a mask explicitly, so you'd need to
> > compose the source and mask to an intermediate surface still. But, couldn't
> > you just essentially multiply the source by the mask before the transform?
> > 
> > I guess if necessary we could make Draw3DTransformedSurface take a
> > composition operator so that it could do the multiply.
> 
> We can't compose the source and mask together beforehand, because the source
> has a 3d transform, and the mask doesn't (both transforms convert to
> destination pixels).
> 
> We'd have to apply the inverse of the source transform to the mask
> transform, and that puts us back into needing a function that can handle 3d
> transforms.
> 
> We basically need a Draw3DTransformedSurface to take a mask + maskTransform,
> or we need to do the mask post-transform (which requires DrawTarget::Mask to
> be implemented correctly everywhere).

Can't do a mask within Draw3DTransformedSurface. Wouldn't it work to draw the source, 3D transformed, to a temporary surface so now the 3D transform is out of the equation, and then just draw the mask over that on the temporary, then draw? Could be sped up in the no-scaling case by going straight to MaskSurface when applicable. I don't think BasicLayerManager does anything fundamentally different (or less icky) than this.
Assignee: tlee → matt.woodrow
Attachment #8731918 - Flags: review?(lsalzman)
Attachment #8731553 - Attachment is obsolete: true
Comment on attachment 8731918 [details] [diff] [review]
Part 2: Fix masking in BasicCompositor

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

It will cause some slight inconvenience for my 3D transform rewrite, but I'll live. :)
Attachment #8731918 - Flags: review?(lsalzman) → review+
backed out for reftest failures like https://treeherder.mozilla.org/logviewer.html#?job_id=24085841&repo=mozilla-inbound
Flags: needinfo?(matt.woodrow)
Attached patch Remove 3d masking (obsolete) — Splinter Review
Flags: needinfo?(matt.woodrow)
As far as I can tell, we have a lot of complexity and extra shaders for this, and the only benefit is that the non-3d version has one less division in the pixel shader (plus a few less instructions in the vertex shader).

This really doesn't seem worth it to me, so this patch just gets rid of it entirely and we always do things correctly.

The previous patches were triggering an assertion in CompositorD3D11 that 3d masking only happened on an RGBA effect, and we have it happening on a RENDER_TARGET instead.

Fixing it this way opens the door to stop forcing an intermediate surface for masks and let them be propagated down to children sometimes.
Attachment #8732661 - Attachment is obsolete: true
Attachment #8732685 - Flags: review?(bas)
Comment on attachment 8732685 [details] [diff] [review]
Part 3: Remove '3d' Masking

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

I concur with your assessment. Especially as hardware has improved over time.
Attachment #8732685 - Flags: review?(bas) → review+
https://hg.mozilla.org/mozilla-central/rev/00e1045a4e14
https://hg.mozilla.org/mozilla-central/rev/cc75c8546991
https://hg.mozilla.org/mozilla-central/rev/e7ee750067da
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Matt, is this good for uplift or do you think it is risky/not worth it?
Flags: needinfo?(matt.woodrow)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #55)
> Matt, is this good for uplift or do you think it is risky/not worth it?

It's reasonable simple and contained, so uplifting should be fine.

It's not really a recent regression though, masking of 3d content has always been broken with the software compositor.
Flags: needinfo?(matt.woodrow)
Given that we are pretty late in the beta cycle (heading towards beta 7 now) and that it isn't a recent regression, wontfix for 46.
This assertion failure has been around for a few releases now. I do not feel the need to track this for Fx47. If we do get around to fixing it in Beta47 cycle and a patch is nominated for uplift, I'd be willing to take it pending risk assessment.
You need to log in before you can comment on or make changes to this bug.