1195653 - [LayerScope] Dump SecondaryEffects (EffectTypes::MASK) on Layerscope viewer

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeremy Chen ([e-mail])]

In current version, we only dump PrimaryEffects on Layerscope. It would be great if we dump SecondaryEffects as well. SecondaryEffects provide additional information, such like Mask Layer, which can help engineers to verify whether a mask is correct or not.

Comment 1

[Jeremy Chen ([e-mail])]

Attachment: [wip] Dump SecondaryEffects EffectTypes::Mask on LayerScope viewer

With this wip, we can see mask layer attribute on LayerScope viewer now.

Comment 2

[u459114]

Comment on attachment 8649854 [details] [diff] [review]
[wip] Dump SecondaryEffects EffectTypes::Mask on LayerScope viewer

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

::: gfx/layers/LayerScope.cpp
@@ +1068,5 @@
> +
> +    GLuint texID = GetTextureID(aGLContext, source);
> +    if (IsTextureIdContainsInList(texID)) {
> +        return;
> +    }

Can we move IsTextureIdContainsInList into SendTextureSource?

@@ +1144,5 @@
>  
> +    if (aEffectChain.mSecondaryEffects[EffectTypes::MASK]) {
> +        const Effect* secondaryEffect = aEffectChain.mSecondaryEffects[EffectTypes::MASK];
> +        const EffectMask* effectMask =
> +            static_cast<const EffectMask*>(secondaryEffect);

const EffectMask* effectMask = static_cast<const EffectMask*>(aEffectChain.mSecondaryEffects[EffectTypes::MASK].get());

Comment 3

[Jeremy Chen ([e-mail])]

Attachment: Part1.1: Dump SecondaryEffects (EffectTypes::MASK) on Layerscope viewer (v2)

Use SendTextureSource() to send mask effect to layerscope viewer.

Comment 4

[Jeremy Chen ([e-mail])]

Attachment: Part2: Move IsTextureIdContainsInList into SendTextureSource. (v1)

Since we always check IsTextureIdContainsInList before SendTextureSource(), move IsTextureIdContainsInList into SendTextureSource().

Comment 5

[u459114]

Comment on attachment 8662827 [details] [diff] [review]
Part2: Move IsTextureIdContainsInList into SendTextureSource. (v1)

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

::: gfx/layers/LayerScope.cpp
@@ +1117,5 @@
>          return;
>      }
>  
> +#ifdef MOZ_WIDGET_GONK
> +    GLuint texID = GetTextureID(aGLContext, aSource);

Move this statement into SendGraphicBuffer?

Comment 6

[u459114]

Comment on attachment 8662825 [details] [diff] [review]
Part1.1: Dump SecondaryEffects (EffectTypes::MASK) on Layerscope viewer (v2)

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

::: gfx/layers/LayerScope.cpp
@@ +1145,5 @@
> +    }
> +
> +    SendTextureSource(aGLContext, aLayerRef, source, texID, false);
> +}
> +

How to know a texture is a color one or a mask one at viewer side?

Comment 7

[Jeremy Chen ([e-mail])]

(In reply to C.J. Ku[:cjku] from comment #6)
> Comment on attachment 8662825 [details] [diff] [review]
> Part1: Dump SecondaryEffects (EffectTypes::MASK) on Layerscope viewer (v2)
> 
> Review of attachment 8662825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/LayerScope.cpp
> @@ +1145,5 @@
> > +    }
> > +
> > +    SendTextureSource(aGLContext, aLayerRef, source, texID, false);
> > +}
> > +
> 
> How to know a texture is a color one or a mask one at viewer side?

Good idea. Thanks for the feedback! :)

Comment 8

[Jeremy Chen ([e-mail])]

(In reply to C.J. Ku[:cjku] from comment #5)
> Comment on attachment 8662827 [details] [diff] [review]
> Part2: Move IsTextureIdContainsInList into SendTextureSource. (v1)
> 
> Review of attachment 8662827 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/LayerScope.cpp
> @@ +1117,5 @@
> >          return;
> >      }
> >  
> > +#ifdef MOZ_WIDGET_GONK
> > +    GLuint texID = GetTextureID(aGLContext, aSource);
> 
> Move this statement into SendGraphicBuffer?


Good idea. Thanks for the feedback! :)

Comment 9

[Jeremy Chen ([e-mail])]

(In reply to C.J. Ku[:cjku] from comment #6)
> Comment on attachment 8662825 [details] [diff] [review]
> Part1: Dump SecondaryEffects (EffectTypes::MASK) on Layerscope viewer (v2)
> 
> Review of attachment 8662825 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/LayerScope.cpp
> @@ +1145,5 @@
> > +    }
> > +
> > +    SendTextureSource(aGLContext, aLayerRef, source, texID, false);
> > +}
> > +
> 
> How to know a texture is a color one or a mask one at viewer side?

I'm considering sending this information from gecko to layerscope viewer through protobuf. Still debug my wip. I'll send feedback request once it is ready.

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Part1.2: Move IsTextureIdContainsInList into SendTextureSource. (carry f+:cku, v2)

Since we always check IsTextureIdContainsInList before SendTextureSource(), move IsTextureIdContainsInList into SendTextureSource().

Comment 11

[Jeremy Chen ([e-mail])]

Attachment: Part1.3: Add isMask attribute field to LayerScopePacket.proto. (v1)

Comment 12

[Jeremy Chen ([e-mail])]

Attachment: Part1.4: Generated file diffs from patched LayerScopePacket.proto. (v1)

Auto generated files.

Comment 13

[Jeremy Chen ([e-mail])]

Attachment: Part1.5: Send isMask info to LayerScope viewer by TexturePacket. (v1)

Comment 14

[Jeremy Chen ([e-mail])]

Attachment: PR for LayerScope: Show mask effect on LayerScope viewer. (v2)

Comment 15

[u459114]

Comment on attachment 8663592 [details] [diff] [review]
Part1.5: Send isMask info to LayerScope viewer by TexturePacket. (v1)

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

::: gfx/layers/LayerScope.cpp
@@ +531,5 @@
>            mTarget(target),
>            mName(name),
>            mContextAddress(reinterpret_cast<intptr_t>(cx)),
> +          mDatasize(0),
> +          mPacket(Move(aPacket))

Why don't you just pass a boolean parameter,aIsMaskTexture, here?

@@ +1168,5 @@
>      TextureSourceOGL* sourceCr = sourceYCbCr->GetSubSource(Cr)->AsSourceOGL();
>  
> +    auto packet = MakeUnique<layerscope::Packet>();
> +
> +    SendTextureSource(aGLContext, aLayerRef, sourceY, false, Move(packet));

Move the same unique_ptr three times?

Comment 16

[u459114]

Comment on attachment 8663588 [details] [diff] [review]
Part1.3: Add isMask attribute field to LayerScopePacket.proto. (v1)

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

::: gfx/layers/protobuf/LayerScopePacket.proto
@@ +31,5 @@
>    optional uint64 glcontext = 8;
>    optional bytes data = 9;
> +
> +  // Texture effect attributes (10 to 19)
> +  // TODO: reserved for texture effect attributes, see Bug 1205521

reserved for primary textured effect.

Comment 17

[Jeremy Chen ([e-mail])]

(In reply to C.J. Ku[:cjku] from comment #15)
> Comment on attachment 8663592 [details] [diff] [review]
> Part1.5: Send isMask info to LayerScope viewer by TexturePacket. (v1)
> 
> Review of attachment 8663592 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/LayerScope.cpp
> @@ +531,5 @@
> >            mTarget(target),
> >            mName(name),
> >            mContextAddress(reinterpret_cast<intptr_t>(cx)),
> > +          mDatasize(0),
> > +          mPacket(Move(aPacket))
> 
> Why don't you just pass a boolean parameter,aIsMaskTexture, here?

In this bug, I think only passing a boolean is sufficient. However, if I intend to dump secondary mask effect attributes in SendMaskEffect() (see Bug 1205521) by texture packet, the unique_ptr and move operator would be needed. So, I kind of doing this to create the infrastructure for Bug 1205521. Is that okay if I keep the infrastructure but adding some comments here?

> @@ +1168,5 @@
> >      TextureSourceOGL* sourceCr = sourceYCbCr->GetSubSource(Cr)->AsSourceOGL();
> >  
> > +    auto packet = MakeUnique<layerscope::Packet>();
> > +
> > +    SendTextureSource(aGLContext, aLayerRef, sourceY, false, Move(packet));
> 
> Move the same unique_ptr three times?

This is definitely wrong.

Comment 18

[Jeremy Chen ([e-mail])]

Attachment: Part1.3: Add isMask attribute field to LayerScopePacket.proto. (carry f+:cku, v2)

Comment 19

[Jeremy Chen ([e-mail])]

Attachment: Part1.5: Send isMask info to LayerScope viewer by TexturePacket. (v2)

Comment 20

[Jeremy Chen ([e-mail])]

Comment on attachment 8663596 [details] [review]
PR for LayerScope: Show mask effect on LayerScope viewer. (v2)

Address the feedback comments.

Comment 21

[Jeremy Chen ([e-mail])]

Attachment: Part1.5: Send isMask info to LayerScope viewer by TexturePacket. (v2)

I intend to primary/secondary effect attributes by texture packet (see Bug 1205521). So, the unique_ptr and move operator is used here as an infrastructure for Bug 1205521. Comments would be removed after Bug 1205521.

Comment 22

[u480271]

Comment on attachment 8662825 [details] [diff] [review]
Part1.1: Dump SecondaryEffects (EffectTypes::MASK) on Layerscope viewer (v2)

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

Comments aren't directly related to the code in the patch.

::: gfx/layers/LayerScope.cpp
@@ +1139,5 @@
> +        return;
> +    }
> +
> +    GLuint texID = GetTextureID(aGLContext, source);
> +    if (IsTextureIdContainsInList(texID)) {

`IsTextureIdContainsInList` is a bad name for this function.

I've looked over the use of the texture id list and would like to suggest some changes:

* change std::list<GLuint> SenderHelper::sTextureIdList to std::vector<GLuint> SenderHelper::sSentTextureIds
  (A vector is fine here and preferable to list. sTextureIdList doesn't document
   what the data is, which is a texture ids that have already been sent)

* SenderHelper::ClearTextureIdList() can become:

void
SenderHelper::ClearSentTextureIds()
{
    sSentTextureIds.clear();
}

or, just inline sSentTextureIds.clear() instead of calling ClearSentTextureIds()

* SenderHelper::IsTextureIdContainsInList() can become:

bool
SenderHelper::HasTextureIdBeenSent(GLuint aTextureId)
{
    return std::find(sSentTextureIds.begin(), sSentTextureIds.end(), aTextureId) != sSentTextureIds.end();
}

Comment 23

[u480271]

Comment on attachment 8664115 [details] [diff] [review]
Part1.5: Send isMask info to LayerScope viewer by TexturePacket. (v2)

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

A lot of changes and duplication of packet creation/ownership to just set `ismask` flag. I would prefer to pass flag to SendTextureSource().

::: gfx/layers/LayerScope.cpp
@@ +1069,5 @@
>                                                           size,
>                                                           shaderConfig, aFlipY);
>      gLayerScopeManager.GetSocketManager()->AppendDebugData(
>          new DebugGLTextureData(aGLContext, aLayerRef, textureTarget,
> +                               texID, img, Move(aPacket)));

Does this need to be a Move? The parameter is Move'd again in the initializer list of DebugGLTextureData.

@@ +1149,5 @@
>  
> +    auto packet = MakeUnique<layerscope::Packet>();
> +    layerscope::TexturePacket* texturePacket = packet->mutable_texture();
> +
> +    texturePacket->set_ismask(true);

Exposing packet creation seems like overkill to have this one place that sets a flag.
Can't `ismask` become a parameter to SendTextureSource() instead?

Comment 24

[Jeremy Chen ([e-mail])]

Attachment: Part1.1: Dump SecondaryEffects (EffectTypes::MASK) on Layerscope viewer. r=dglastonbury (v2, carry r+)

Rebased.

Comment 25

[Jeremy Chen ([e-mail])]

Attachment: Part1.2: Use sSentTextureIds instead of sTextureIdList and move HasTextureIdBeenSent into SendTextureSource. r=dglastonbury (v3, carry r+)

Rebased. Address reviewer's comments.

Comment 26

[Jeremy Chen ([e-mail])]

Attachment: Part1.3: Add isMask attribute field to LayerScopePacket.proto. r=dglastonbury (v2, carry r+)

Rebased.

Comment 27

[Jeremy Chen ([e-mail])]

Attachment: Part1.4: Generated file diffs from patched LayerScopePacket.proto. r=dglastonbury (v1, carry r+)

Rebased.

Comment 28

[Jeremy Chen ([e-mail])]

Attachment: Part1.5: Send isMask info to LayerScope viewer by TexturePacket. r=dglastonbury (v3)

Rebased. Use bool parameter isMask instead of exposing packet creation in SendMaskEffect().

Exposing packet creation in SendTexturedEffect(), SendMaskEffect() is a solution for Bug 1205521, but not for this bug. Perhaps it would be better to do it in Bug 1205521.

Comment 29

[Jeremy Chen ([e-mail])]

Hi Dan, could you take a look at Part1.5 again? Your comments have been addressed. Thanks.

Comment 30

[u480271]

Jeremy,

  Sorry about delay on review.

Comment 31

[Jeremy Chen ([e-mail])]

try looks positove: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a6139d0e1872
Please check-in gecko patchs (Part1.1~1.5). Thanks.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c94845fed57a
https://hg.mozilla.org/integration/mozilla-inbound/rev/05e87501ca2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9bd3f3709c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/de66b7117fb4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2aeb63e0f392

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c94845fed57a
https://hg.mozilla.org/mozilla-central/rev/05e87501ca2f
https://hg.mozilla.org/mozilla-central/rev/e9bd3f3709c3
https://hg.mozilla.org/mozilla-central/rev/de66b7117fb4
https://hg.mozilla.org/mozilla-central/rev/2aeb63e0f392

Comment 34

[Jeremy Chen ([e-mail])]

Hi boris, could you help merge PR for layerscope viewer? Thanks.

Comment 35

[Boris Chiou [:boris]]

Layerscope viewer code merged:
https://github.com/mozilla/layerscope/commit/bb45cbce62f43bfa82d5accbe521846498c9b9ad