1205521 - [LayerScope] Dump mask/texture attributes on Layerscope viewer

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeremy Chen ([e-mail])]

After Bug 1195653, we may see all textures (including mask) dumped on layerscope viewer. However, attributes of these textures are not shown. With these info, we can track these attributes and observe the rendered texture by layerscope at the same time.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/layers/Effects.h#57-99

Comment 1

[Jeremy Chen ([e-mail])]

I'm working on 2 WIPs:
Part1: Send texture attributes to layerscope viewer.
Part2: Let viewer display the attributes finely.

Comment 2

[Jeremy Chen ([e-mail])]

Attachment: Part1: (v1) Add texture/mask attribute fields in LayerScopePacket.proto.

Comment 3

[Jeremy Chen ([e-mail])]

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

Comment 4

[Jeremy Chen ([e-mail])]

Attachment: Part3: (v1) Dump texture/mask attributes on layerscope viewer.

1. Expose packet creation in SendTexturedEffect(), SendMaskEffect(), so we can dump texture/mask attributes.
2. Add few helper functions to dump texture/mask attributes.

Comment 5

[Jeremy Chen ([e-mail])]

Attachment: PR for LayerScope: Display texture/mask effect attributes on LayerScope viewer.

Comment 6

[Jeremy Chen ([e-mail])]

Hi CJ, could you take a look at these patches and give me some feedbacks? Thanks.

Comment 7

[u459114]

Comment on attachment 8668216 [details] [diff] [review]
Part3: (v1) Dump texture/mask attributes on layerscope viewer.

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

::: gfx/layers/LayerScope.cpp
@@ +1191,5 @@
>      TextureSourceOGL* sourceY =  sourceYCbCr->GetSubSource(Y)->AsSourceOGL();
>      TextureSourceOGL* sourceCb = sourceYCbCr->GetSubSource(Cb)->AsSourceOGL();
>      TextureSourceOGL* sourceCr = sourceYCbCr->GetSubSource(Cr)->AsSourceOGL();
>  
> +    // Expose packet creation here, so we could dump effect attributes.

TextureSourceOGL *sources[] = {
  sourceYCbCr->GetSubSource(Y)->AsSourceOGL(),
  sourceYCbCr->GetSubSource(cb)->AsSourceOGL(),
  sourceYCbCr->GetSubSource(Cr)->AsSourceOGL()};

// You can move code in for loop into another function, since you need it in SenderHelper::SendMaskEffect as well. 
for (auto source: sources) {
auto packetY = MakeUnique<layerscope::Packet>();
    SendTextureSource(aGLContext, aLayerRef, sourceCr, false, false);	
    layerscope::TexturePacket* texturePacketY = packetY->mutable_texture();
    texturePacketY->set_mpremultiplied(aEffect->mPremultiplied);
    DumpFilter(texturePacketY, aEffect->mFilter);
    DumpRect(texturePacketY->mutable_mtexturecoords(), aEffect->mTextureCoords);
    SendTextureSource(aGLContext, aLayerRef, source, false, false, Move(packetY));
}

@@ +1259,5 @@
>  
> +// The static helper function sets the transform matrix into the packet
> +void
> +SenderHelper::DumpMatrix(TexturePacket::Matrix* aTextureMatrix,
> +                            const Matrix4x4& aMatrix)

Please keep the way to dump matrix of secondary texture effect be consisted with DebugGLDrawData::mMVMatrix.

@@ +1284,5 @@
> +}
> +
> +// The static helper function sets the rect into the packet
> +void
> +SenderHelper::DumpRect(TexturePacket::Rect* aTextureRect, const Rect& aRect)

Please also use this new added function here:
https://dxr.mozilla.org/mozilla-central/source/gfx/layers/LayerScope.cpp#724

@@ +1307,5 @@
> +    case Filter::POINT:
> +      aTexturePacket->set_mfilter(TexturePacket::POINT);
> +      break;
> +    default:
> +      // ignore it

assertion here

Comment 8

[Jeremy Chen ([e-mail])]

Attachment: Part4: (v1) Align LayerScopePacket to fit changes in Bug 1211324.

Comment 9

[Jeremy Chen ([e-mail])]

Attachment: Part1: (v2) Add texture/mask attribute fields in LayerScopePacket.proto.

Comment 10

[Jeremy Chen ([e-mail])]

Attachment: Part2: (v2) Generated file diffs from patched LayerScopePacket.proto.

Comment 11

[Jeremy Chen ([e-mail])]

Attachment: Part3: (v2) Dump texture/mask attributes on layerscope viewer.

Comment 12

[Jeremy Chen ([e-mail])]

Attachment: Part4: (v1) Align LayerScopePacket to fit changes in Bug 1211324.

Comment 13

[Jeremy Chen ([e-mail])]

Cj, thank you for the feedbacks. For the viewer patch, I'll address your comments and send a feedback request then.


Hi Dan, could you help review gecko patches (Part1~Part4)? Thanks.

Comment 14

[Jeremy Chen ([e-mail])]

Comment on attachment 8668222 [details] [review]
PR for LayerScope: Display texture/mask effect attributes on LayerScope viewer.

1. Let EffectMask be a sub structure of TexturePacket.
2. Polish sortSpriteByLayerID().
3. Let treeview_renderer.js handle all the manipulations of $("#texture-property-table").

CJ, could you take a look at this again? Thanks.

Comment 15

[u459114]

Comment on attachment 8672477 [details] [diff] [review]
Part3: (v2) Dump texture/mask attributes on layerscope viewer.

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

::: gfx/layers/LayerScope.cpp
@@ +368,5 @@
>  
> +/* The static helper functions that set data into the packet
> + * 1. DumpRect
> + * 2. DumpFilter
> + */

template <typename T, typename U>
static void Dump(T *, const U&);

template <typename T>
static void Dump(T *, const Rect&);

template <>
static void Dump(TexturePacket *, const Filter&);

Comment 16

[u459114]

Comment on attachment 8668222 [details] [review]
PR for LayerScope: Display texture/mask effect attributes on LayerScope viewer.

Cool, thanks for making this tool better

Comment 17

[Jeremy Chen ([e-mail])]

Comment on attachment 8668222 [details] [review]
PR for LayerScope: Display texture/mask effect attributes on LayerScope viewer.

CJ, thanks for the feedbacks. :)

Hi Boris, could you help review this?

Comment 18

[Boris Chiou [:boris]]

Comment on attachment 8668222 [details] [review]
PR for LayerScope: Display texture/mask effect attributes on LayerScope viewer.

Please check my comments in the github. Thanks.

Comment 19

[u480271]

Comment on attachment 8672475 [details] [diff] [review]
Part1: (v2) Add texture/mask attribute fields in LayerScopePacket.proto.

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

::: gfx/layers/protobuf/LayerScopePacket.proto
@@ +20,5 @@
>  }
>  
>  message TexturePacket {
> +  enum Filter {
> +    GOOD = 0;

What is "GOOD" filter?

Comment 20

[Jeremy Chen ([e-mail])]

(In reply to Dan Glastonbury :djg :kamidphish from comment #19)
> Comment on attachment 8672475 [details] [diff] [review]
> Part1: (v2) Add texture/mask attribute fields in LayerScopePacket.proto.
> 
> Review of attachment 8672475 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/protobuf/LayerScopePacket.proto
> @@ +20,5 @@
> >  }
> >  
> >  message TexturePacket {
> > +  enum Filter {
> > +    GOOD = 0;
> 
> What is "GOOD" filter?

Dan, thanks for the review. According to [1], "GOOD" filter is a reasonable-performance filter, with quality similar to FILTER_BILINEAR. But, I have no idea what GOOD filter is in detail.

[1] https://dxr.mozilla.org/mozilla-central/source/gfx/cairo/cairo/src/cairo.h#2497

Comment 21

[Jeremy Chen ([e-mail])]

Attachment: Part1: (v3 carry r+:djg) Add texture/mask attribute fields in LayerScopePacket.proto.

As discussed with Boris offline, we should keep the backward compatibility while using protobuf.

Comment 22

[Jeremy Chen ([e-mail])]

Attachment: Part2: (v3 carry r+:djg) Generated file diffs from patched LayerScopePacket.proto.

Update with Part1 patch.

Comment 23

[Jeremy Chen ([e-mail])]

Attachment: Part3: (v3 carry r+:djg) Dump texture/mask attributes on layerscope viewer.

Fix few coding style nit.

Comment 24

[Jeremy Chen ([e-mail])]

Attachment: Part4: (v2 carry r+:djg) Align LayerScopePacket to fit changes in Bug 1211324.

Keep the backward compatibility while using protobuf.

Comment 25

[Jeremy Chen ([e-mail])]

Comment on attachment 8668222 [details] [review]
PR for LayerScope: Display texture/mask effect attributes on LayerScope viewer.

Boris, your comments have been addressed. Could you check again? Thanks.

Comment 26

[Jeremy Chen ([e-mail])]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b2bd53ec2f5 except [OS X 10.6 XPCShell test], which is a known issue and not related to this bug. Please check-in part1-part4 patches, thank you.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ae51f091661
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff444636fbd
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8769c3e5d4f
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3c00d122c5

Comment 28

[Carsten Book [:Tomcat]]

sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=15928403&repo=mozilla-inbound

Comment 29

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a5670ba45ae

Comment 30

[Jeremy Chen ([e-mail])]

Attachment: Part2: (v3) Generated file diffs from patched LayerScopePacket.proto.

Comment 31

[Jeremy Chen ([e-mail])]

Attachment: Part3: (v3 carry r+:djg) Dump texture/mask attributes on layerscope viewer.

Comment 32

[Jeremy Chen ([e-mail])]

(In reply to Carsten Book [:Tomcat] from comment #28)
> sorry had to back this out for bustage like
> https://treeherder.mozilla.org/logviewer.html#?job_id=15928403&repo=mozilla-
> inbound

I'm so sorry. I forgot to update Part3 patch. I'm updating the Part3 patch which is now agree with that pushed to try with good results. Please check-in again.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b612fdf3136a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a78cd7af3c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/919edc70d66a
https://hg.mozilla.org/integration/mozilla-inbound/rev/d26f66856c38

Comment 34

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

https://hg.mozilla.org/mozilla-central/rev/b612fdf3136a
https://hg.mozilla.org/mozilla-central/rev/7a78cd7af3c0
https://hg.mozilla.org/mozilla-central/rev/919edc70d66a
https://hg.mozilla.org/mozilla-central/rev/d26f66856c38

Comment 35

[Jeremy Chen ([e-mail])]

Boris, please check-in viewer side patches, thank you. :)

Comment 36

[Boris Chiou [:boris]]

Merged: https://github.com/mozilla/layerscope/commit/e7b662598d06816bd59043d7122dbc66d4d75c29

Comment 37

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.