1385929 - Find a way to prevent duplicate gfxContext::Save/Restore

Status: RESOLVED FIXED

(Core :: Web Painting, enhancement, P2)

Description

[u459114]

In bug 1385861, I notice again duplicate gfxContext::Save/Restore.

We probabaly should find a mechanism to prevent it. For example, showing a warning massage(via NS_ASSERTION) if the state of a context is not changed between to gfxContext::Save calls.

Comment 1

[u459114]

Attachment: WIP

Several test cases crashed by applying this patch
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a14988676a61d13f17691ebdc03c14e79623541&selectedJob=119924163

Comment 2

[u459114]

gfxContext::Save in gfxContext::PushGroupForBlendBack gfxContext::PushGroupAndCopyBackground
gfxContext::Restore in gfxContext::PopGroupAndBlend()

Actually, gfxContext::PushGroupForBlendBack/PushGroupAndCopyBackground do not change AzureState state at all.

Take gfxContext::PushGroupForBlendBack as an example
void
gfxContext::PushGroupForBlendBack(gfxContentType content, Float aOpacity, SourceSurface* aMask, const Matrix& aMaskTransform)
{
  mDT->PushLayer(content == gfxContentType::COLOR, aOpacity, aMask, aMaskTransform);
}

Since the context of mDT is not part of AzureState, this function does not change the content of gfxContext::CurrentState()

Comment 3

[u459114]

Attachment: Bug 1385929 - Part 1. Check whether the content of the persisted state change.

Review commit: https://reviewboard.mozilla.org/r/163392/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163392/

Comment 4

[u459114]

Attachment: Bug 1385929 - Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1.

gfxContext::PushGroupAndCopyBackground & PushGroupForBlendBack do not change the
content of AzureState, we should remove gfxContext::Save in these two functions.

Review commit: https://reviewboard.mozilla.org/r/163394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/163394/

Comment 5

[u459114]

Comment on attachment 8892440 [details]
Bug 1385929 - Part 1. Check whether the content of the persisted state change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163392/diff/1-2/

Comment 6

[u459114]

Comment on attachment 8892441 [details]
Bug 1385929 - Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163394/diff/1-2/

Comment 7

[u459114]

Comment on attachment 8892440 [details]
Bug 1385929 - Part 1. Check whether the content of the persisted state change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163392/diff/2-3/

Comment 8

[u459114]

Comment on attachment 8892441 [details]
Bug 1385929 - Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163394/diff/2-3/

Comment 9

[u459114]

Comment on attachment 8892440 [details]
Bug 1385929 - Part 1. Check whether the content of the persisted state change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163392/diff/3-4/

Comment 10

[u459114]

Comment on attachment 8892441 [details]
Bug 1385929 - Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163394/diff/3-4/

Comment 11

[u459114]

Comment on attachment 8892440 [details]
Bug 1385929 - Part 1. Check whether the content of the persisted state change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163392/diff/4-5/

Comment 12

[u459114]

Comment on attachment 8892441 [details]
Bug 1385929 - Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163394/diff/4-5/

Comment 13

[u459114]

Comment on attachment 8892440 [details]
Bug 1385929 - Part 1. Check whether the content of the persisted state change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163392/diff/5-6/

Comment 14

[u459114]

Comment on attachment 8892441 [details]
Bug 1385929 - Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163394/diff/5-6/

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8892440 [details]
Bug 1385929 - Part 1. Check whether the content of the persisted state change.

https://reviewboard.mozilla.org/r/163392/#review170946

::: gfx/thebes/gfxContext.cpp:143
(Diff revision 6)
>  void
>  gfxContext::Restore()
>  {
> +#ifdef DEBUG
> +  // gfxContext::Restore is used to restore AzureState. We need to restore it
> +  // only if it was altered. The following APIs do change the context of

do change the content?

::: gfx/thebes/gfxContext.cpp:156
(Diff revision 6)
> +  //    only thing that you altered in the target context.
> +  // 3. Function of setup transform matrix, such as Multiply() and
> +  //    SetMatrix(). Using gfxContextMatrixAutoSaveRestore is more recommended
> +  //    if transform data is the only thing that you are going to alter.
> +  //
> +  // You will hit the assertion message bellow if there is no above functions

below

::: gfx/thebes/gfxContext.cpp:1038
(Diff revision 6)
>   * the state and might be false for the restored AzureState.
>   */
>  void
>  gfxContext::ChangeTransform(const Matrix &aNewMatrix, bool aUpdatePatternTransform)
>  {
> +  CURRENTSTATE_CHANGED(aUpdatePatternTransform)

Can we just move this down into the if() statement below where we actually change the state?

Seems like we could get rid of the parameter to CURRENTSTATE_CHANGED then too.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8892441 [details]
Bug 1385929 - Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1.

https://reviewboard.mozilla.org/r/163394/#review170948

Comment 17

[u459114]

Comment on attachment 8892440 [details]
Bug 1385929 - Part 1. Check whether the content of the persisted state change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163392/diff/6-7/

Comment 18

[u459114]

Comment on attachment 8892441 [details]
Bug 1385929 - Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/163394/diff/6-7/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/027153981c66
Part 1. Check whether the content of the persisted state change. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/fd50ab554397
Part 2. Remove unecessary gfxContext::Save/Restore found by Part 1. r=mattwoodrow

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/027153981c66
https://hg.mozilla.org/mozilla-central/rev/fd50ab554397