Closed Bug 1205521 Opened 4 years ago Closed 4 years ago

[LayerScope] Dump mask/texture attributes on Layerscope viewer

Categories

(Core :: Graphics, defect)

ARM
All
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jeremychen, Assigned: jeremychen)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(5 files, 10 obsolete files)

45 bytes, text/x-github-pull-request
boris
: review+
u459114
: feedback+
Details | Review
2.00 KB, patch
jeremychen
: review+
Details | Diff | Splinter Review
96.26 KB, patch
jeremychen
: review+
Details | Diff | Splinter Review
5.27 KB, patch
jeremychen
: review+
Details | Diff | Splinter Review
15.00 KB, patch
jeremychen
: review+
Details | Diff | Splinter Review
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
Assignee: nobody → jeremychen
Summary: [LayerScope] Dump attributes of textures on Layerscope viewer → [LayerScope] Dump mask/texture attributes on Layerscope viewer
I'm working on 2 WIPs:
Part1: Send texture attributes to layerscope viewer.
Part2: Let viewer display the attributes finely.
1. Expose packet creation in SendTexturedEffect(), SendMaskEffect(), so we can dump texture/mask attributes.
2. Add few helper functions to dump texture/mask attributes.
Hi CJ, could you take a look at these patches and give me some feedbacks? Thanks.
Attachment #8668214 - Flags: feedback?(cku)
Attachment #8668215 - Flags: feedback?(cku)
Attachment #8668216 - Flags: feedback?(cku)
Attachment #8668222 - Flags: feedback?(cku)
Attachment #8668214 - Flags: feedback?(cku) → feedback+
Attachment #8668215 - Flags: feedback?(cku) → feedback+
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
Attachment #8668216 - Flags: feedback?(cku) → feedback+
Attachment #8668222 - Flags: feedback?(cku) → feedback-
Attachment #8668214 - Attachment is obsolete: true
Attachment #8668215 - Attachment is obsolete: true
Attachment #8668216 - Attachment is obsolete: true
Attachment #8672474 - Attachment is obsolete: true
Attachment #8672475 - Flags: review?(dglastonbury)
Attachment #8672476 - Flags: review?(dglastonbury)
Attachment #8672477 - Flags: review?(dglastonbury)
Attachment #8672478 - Flags: review?(dglastonbury)
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 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.
Attachment #8668222 - Flags: feedback- → feedback?(cku)
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 on attachment 8668222 [details] [review]
PR for LayerScope: Display texture/mask effect attributes on LayerScope viewer.

Cool, thanks for making this tool better
Attachment #8668222 - Flags: feedback?(cku) → feedback+
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?
Attachment #8668222 - Flags: review?(boris.chiou)
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.
Attachment #8668222 - Flags: review?(boris.chiou)
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?
Attachment #8672475 - Flags: review?(dglastonbury) → review+
Attachment #8672476 - Flags: review?(dglastonbury) → review+
Attachment #8672477 - Flags: review?(dglastonbury) → review+
Attachment #8672478 - Flags: review?(dglastonbury) → review+
(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
As discussed with Boris offline, we should keep the backward compatibility while using protobuf.
Attachment #8675477 - Flags: review+
Attachment #8672475 - Attachment is obsolete: true
Attachment #8672476 - Attachment is obsolete: true
Attachment #8672477 - Attachment is obsolete: true
Keep the backward compatibility while using protobuf.
Attachment #8675482 - Flags: review+
Attachment #8672478 - Attachment is obsolete: true
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.
Attachment #8668222 - Flags: review?(boris.chiou)
Attachment #8668222 - Flags: review?(boris.chiou) → review+
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.
sorry had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=15928403&repo=mozilla-inbound
Flags: needinfo?(jeremychen)
Attachment #8675480 - Attachment is obsolete: true
Attachment #8676094 - Attachment is obsolete: true
(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.
Flags: needinfo?(jeremychen)
Boris, please check-in viewer side patches, thank you. :)
Flags: needinfo?(boris.chiou)
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.