Closed Bug 1322729 Opened 3 years ago Closed 3 years ago

Remove the code that works around lack of native Moz2D PushLayer/PopLayer (gfx.content.use-native-pushlayer)


(Core :: Graphics, defect, P5)




Tracking Status
firefox53 --- fixed


(Reporter: jwatt, Assigned: lsalzman)



(Whiteboard: [gfx-noted])


(4 files)

Now that DrawTargetCG is no more, we should try to remove the code that we use when gfx.content.use-native-pushlayer is false.
Attached patch patchSplinter Review
Pushing this to Try, it looks like there are still issues with flipping gfx.content.use-native-pushlayer to false on Mac/Android.
Priority: -- → P5
Whiteboard: [gfx-noted]
A few issues:

DTT doesn't set forward clips to tiles that it has already determined are clipped out. So subsequently calling PushLayer on those tiles thus will actually make a non-empty layer on those clipped out tiles. The solution here is to neither push nor pop a layer on a tile that has been clipped out, which is just overall better.

Within DTSkia itself, if the clip is empty or otherwise clip bounds can't be determined, saveLayer may fail to actually push a new layer device/image. We just have to check that that actually happened before we start messing with the layer device.

Also, we didn't deal with the passed in bounds parameter properly, which was in device space, but Skia expected it in local space, so I fixed that too.
Assignee: nobody → lsalzman
Attachment #8818969 - Flags: review?(mchang)
A few issues here as well:

Thought DTT's mPermitSubpixelAA state was toggled by SetPermitSubpixelAA, it never forwarded this to the tile DTs, so they would never actually render with the correct subpixel AA permissions at all. This fixes that. That also involves properly toggling the subpixel AA state when we push or pop a layer, which we need to track there now like in the other DTs.

With respect to DTSkia, we were handling subpixel AA permission wrong. Per conversation with Bas, the subpixel AA permission is always authoritative, whether or not the DT is opaque or not. Also, to ensure that, when we have pushed a color-alpha layer to Skia, we need the PreserveLCDText flag, otherwise Skia will ignore subpixel AA requests.
Attachment #8818972 - Flags: review?(mchang)
DrawTargetSkia::BorrowCGContext called LockBits without checking the origin, which meant it would fail with a clipped layer. Also, it would sometimes "succeed" if the clipped layer had its origin at (0, 0), but the clipped layer's size could be smaller than what we told CG. This would mean a possible overread/crashiness since we weren't supplying the correct smaller size to CG.
Attachment #8818973 - Flags: review?(mchang)
Comment on attachment 8818972 [details] [diff] [review]
fix DrawTargetTiled to forward SetPermitSubpixelAA

Review of attachment 8818972 [details] [diff] [review]:

::: gfx/2d/DrawTargetTiled.cpp
@@ +44,5 @@
>      mTiles[i].mDrawTarget->SetTransform(Matrix::Translation(mTiles[i].mTileOrigin.x,
>                                                              mTiles[i].mTileOrigin.y));
>    }
>    mFormat = mTiles[0].mDrawTarget->GetFormat();
> +  mPermitSubpixelAA = mTiles[0].mDrawTarget->GetPermitSubpixelAA();

nit: Maybe for debug builds, assert that all drawtargets for all the tiles have the same value? It'd be worrisome if it wasn't the case.
Attachment #8818972 - Flags: review?(mchang) → review+
Attachment #8818973 - Flags: review?(mchang) → review+
Attachment #8818969 - Flags: review?(mchang) → review+
Attachment #8817661 - Flags: review+
Pushed by
fix DrawTargetTiled::PushLayer to properly handle clipping. r=mchang
fix DrawTargetTiled to forward SetPermitSubpixelAA. r=mchang
fix DrawTargetSkia::BorrowCGContext to work with PushLayer. r=mchang
Have gfxContext always use DrawTarget::PushLayer/PopLayer. r=lsalzman
You need to log in before you can comment on or make changes to this bug.