1208661 - Dump client-side textures and integrate them into the HTML display list dump

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

This bug is to track landing the patches I used to produce the HTML display list dumps in bug 1205630 (e.g. [1]).

This basically involves:

  - Fixing the dumping of client-side textures for individual
    display items.

  - Dumping client-side textures for layers.

  - Fixing the integration of these into the HTML display list
    dump, and making said dump nicer by showing the images in-line.

[1] https://bug1205630.bmoattachments.org/attachment.cgi?id=8665158

Comment 1

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1208661 - Move Dump() up from ContentClient to CompositableClient. r=BenWa

Bug 1208661 - Move Dump() up from ContentClient to CompositableClient. r=mattwoodrow

Only some ContentClient implementations implement it, but it allows it to be
called from more general code. Other CompositableClient implementations can
be provided later.

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1208661 - Make ContentClient dumping play nicely with HTML dumping. r=BenWa

Bug 1208661 - Make ContentClient dumping play nicely with HTML dumping. r=mattwoodrow

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1208661 - Support dumping client-side layer textures without compression. r=BenWa

Bug 1208661 - Support dumping client-side layer textures without compression. r=mattwoodrow

Compression is used by the profiler, but we need uncompressed textures for the
browser to be able to render them when we include them in the HTML paint dump.

Comment 4

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1208661 - Do not assume that PaintRoot()'s contributions to the HTML paint dump will all be inside a <script> tag. r=BenWa

Bug 1208661 - Do not assume that PaintRoot()'s contributions to the HTML paint dump will all be inside a <script> tag. r=mattwoodrow

Comment 5

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1208661 - Dump client-side layer textures. r=BenWa

Bug 1208661 - Dump client-side layer textures. r=mattwoodrow

Comment 6

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1208661 - Implement SourceSurfaceDual::GetDataSurface() for debugging purposes. r=BenWa

Bug 1208661 - Implement SourceSurfaceDual::GetDataSurface() for debugging purposes. r=mattwoodrow

Comment 7

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1208661 - Show display list and layer textures in-line in the HTML paint dump. r=BenWa

Bug 1208661 - Show display list and layer textures in-line in the HTML paint dump. r=mattwoodrow

Comment 8

[Botond Ballo [:botond]]

These patches are an attempt to turn my local changes into something landable.

Before posting them for review, I'd like to ask Matt's feedback on a couple of a things:

  1) There are currently two places where we dump compositor-side textures
     for individual layers.

      - One is in Layer::Dump(), where we call CompositableHost::Dump() [1].

      - The other is the calls to WriteSnapshotToDumpFile() in the various 
        XXXLayerComposite::Render() implementations [2] [3] [4] [5], which
        are meant to be used  together with the call to 
        WriteSnapshotLinkToDumpFile() in Layer::Dump() [6].

     Right now, my client-side dumping code is a mismash of these two, using
     WriteSnapshotLinkToDumpFile() to emit the link, and 
     CompositableClient::Dump() to emit the texture itself.

     Do you know why we have these two compositor-side paths? Ideally I'd
     like to unify them, and then use the same approach on the client side.

  2) To ensure that every display item that's painted gets an image in the
     dump, I had to implement SourceSurfaceDual::GetDataSurface(). I
     implemented it by returning |mA->GetDataSurface()|, which caused
     *something* to show up in the dumps, which was good enough for my
     purposes.

     Is this remotely appropriate? Should I be doing something else instead?

[1] https://dxr.mozilla.org/mozilla-central/rev/5abe3c4deab94270440422c850bbeaf512b1f38d/gfx/layers/Layers.cpp#1685
[2] https://dxr.mozilla.org/mozilla-central/rev/5abe3c4deab94270440422c850bbeaf512b1f38d/gfx/layers/composite/CanvasLayerComposite.cpp?offset=0#94
[3] https://dxr.mozilla.org/mozilla-central/rev/5abe3c4deab94270440422c850bbeaf512b1f38d/gfx/layers/composite/ContainerLayerComposite.cpp#713
[4] https://dxr.mozilla.org/mozilla-central/rev/5abe3c4deab94270440422c850bbeaf512b1f38d/gfx/layers/composite/ImageLayerComposite.cpp?offset=0#92
[5] https://dxr.mozilla.org/mozilla-central/rev/5abe3c4deab94270440422c850bbeaf512b1f38d/gfx/layers/composite/PaintedLayerComposite.cpp?offset=0#121
[6] https://dxr.mozilla.org/mozilla-central/rev/5abe3c4deab94270440422c850bbeaf512b1f38d/gfx/layers/Layers.cpp#1676

Comment 9

[Botond Ballo [:botond]]

Asking BenWa instead as Matt is on PTO.

Comment 10

[Benoit Girard (:BenWa)]

(In reply to Botond Ballo [:botond] from comment #8)
> These patches are an attempt to turn my local changes into something
> landable.
> 
> Before posting them for review, I'd like to ask Matt's feedback on a couple
> of a things:
> 
>   1) There are currently two places where we dump compositor-side textures
>      for individual layers.
> 
>      ...
> 
>      Do you know why we have these two compositor-side paths? Ideally I'd
>      like to unify them, and then use the same approach on the client side.
> 

My understanding is that LayerManager::Dump() gives a way to recursively dump the layer tree and the texture data without painting. The WriteSnapshotToDumpFile calls all happen during the render and thus is tied to and requires a paint to happen to dump which is not ideal. WriteSnapshotToDumpFile seems to support some things that LayerManager::Dump does not such as showing intermediate surfaces and the final frames for OGL. I can't say if its worth keeping or not given that I only use the ::Dump stuff.

>   2) To ensure that every display item that's painted gets an image in the
>      dump, I had to implement SourceSurfaceDual::GetDataSurface(). I
>      implemented it by returning |mA->GetDataSurface()|, which caused
>      *something* to show up in the dumps, which was good enough for my
>      purposes.
> 
>      Is this remotely appropriate? Should I be doing something else instead?

From the code:
"This class facilitates black-background/white-background
drawing for per-component alpha extraction for backends which do not support
native component alpha."

Since we don't need to support alpha extraction for visualization I think showing just the first surface is fine. If we need to support the black/white code later we can extend the support later.

Comment 11

[Botond Ballo [:botond]]

Thanks for the feedback! I agree that dumping in Dump() rather than Render() is better, and accordingly I updated my patch to use the Dump() mechanisms for the client-side texture dumps consistently. Posting patch series for review.

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8666267 [details]
MozReview Request: Bug 1208661 - Move Dump() up from ContentClient to CompositableClient. r=BenWa

Bug 1208661 - Move Dump() up from ContentClient to CompositableClient. r=BenWa

Only some ContentClient implementations implement it, but it allows it to be
called from more general code. Other CompositableClient implementations can
be provided later.

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8666268 [details]
MozReview Request: Bug 1208661 - Make ContentClient dumping play nicely with HTML dumping. r=BenWa

Bug 1208661 - Make ContentClient dumping play nicely with HTML dumping. r=BenWa

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8666269 [details]
MozReview Request: Bug 1208661 - Support dumping client-side layer textures without compression. r=BenWa

Bug 1208661 - Support dumping client-side layer textures without compression. r=BenWa

Compression is used by the profiler, but we need uncompressed textures for the
browser to be able to render them when we include them in the HTML paint dump.

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8666270 [details]
MozReview Request: Bug 1208661 - Do not assume that PaintRoot()'s contributions to the HTML paint dump will all be inside a <script> tag. r=BenWa

Bug 1208661 - Do not assume that PaintRoot()'s contributions to the HTML paint dump will all be inside a <script> tag. r=BenWa

Comment 16

[Botond Ballo [:botond]]

Comment on attachment 8666271 [details]
MozReview Request: Bug 1208661 - Dump client-side layer textures. r=BenWa

Bug 1208661 - Dump client-side layer textures. r=BenWa

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8666272 [details]
MozReview Request: Bug 1208661 - Implement SourceSurfaceDual::GetDataSurface() for debugging purposes. r=BenWa

Bug 1208661 - Implement SourceSurfaceDual::GetDataSurface() for debugging purposes. r=BenWa

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8666273 [details]
MozReview Request: Bug 1208661 - Show display list and layer textures in-line in the HTML paint dump. r=BenWa

Bug 1208661 - Show display list and layer textures in-line in the HTML paint dump. r=BenWa

Comment 19

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1208661 - Remove some no-longer-used debugging code. r=BenWa

Bug 1208661 - Remove some no-longer-used debugging code. r=BenWa

Comment 20

[Benoit Girard (:BenWa)]

Comment on attachment 8666267 [details]
MozReview Request: Bug 1208661 - Move Dump() up from ContentClient to CompositableClient. r=BenWa

https://reviewboard.mozilla.org/r/20491/#review18477

Comment 21

[Benoit Girard (:BenWa)]

Comment on attachment 8666268 [details]
MozReview Request: Bug 1208661 - Make ContentClient dumping play nicely with HTML dumping. r=BenWa

https://reviewboard.mozilla.org/r/20493/#review18479

Comment 22

[Benoit Girard (:BenWa)]

Comment on attachment 8666269 [details]
MozReview Request: Bug 1208661 - Support dumping client-side layer textures without compression. r=BenWa

https://reviewboard.mozilla.org/r/20495/#review18465

::: gfx/layers/client/CompositableClient.h:133
(Diff revision 2)
> +                    bool aCompress=true) {};

Generally we should prefer enum over bool for this. I'll leave it up to you if you want to fix it or not.

::: gfx/layers/composite/TiledContentHost.cpp:635
(Diff revision 2)
> +      false /* compression not supported on host side */);

Doesn't the layer dumping code call this? Shoudn't compress be conditional on aDumpHtml then?

Comment 23

[Benoit Girard (:BenWa)]

Comment on attachment 8666270 [details]
MozReview Request: Bug 1208661 - Do not assume that PaintRoot()'s contributions to the HTML paint dump will all be inside a <script> tag. r=BenWa

https://reviewboard.mozilla.org/r/20497/#review18481

Comment 24

[Benoit Girard (:BenWa)]

Comment on attachment 8666271 [details]
MozReview Request: Bug 1208661 - Dump client-side layer textures. r=BenWa

https://reviewboard.mozilla.org/r/20499/#review18473

::: gfx/layers/Layers.cpp:1681
(Diff revision 2)
> +  layerId.AppendInt((uint64_t)this);

Wouldn't uintptr_t be a better cast here?

Comment 25

[Benoit Girard (:BenWa)]

Comment on attachment 8666272 [details]
MozReview Request: Bug 1208661 - Implement SourceSurfaceDual::GetDataSurface() for debugging purposes. r=BenWa

https://reviewboard.mozilla.org/r/20501/#review18475

::: gfx/2d/SourceSurfaceDual.h:32
(Diff revision 2)
> +    return mA->GetDataSurface();

Might want to add a comment like to make it clear to the reader that this shortcut is intentional:
GetDataSurface() is only meant for debugging where we're not interested in supporting component alpha so we just return mA.

Comment 26

[Benoit Girard (:BenWa)]

Comment on attachment 8666273 [details]
MozReview Request: Bug 1208661 - Show display list and layer textures in-line in the HTML paint dump. r=BenWa

https://reviewboard.mozilla.org/r/20503/#review18483

Comment 27

[Benoit Girard (:BenWa)]

Comment on attachment 8666892 [details]
MozReview Request: Bug 1208661 - Remove some no-longer-used debugging code. r=BenWa

https://reviewboard.mozilla.org/r/20615/#review18485

Comment 28

[Botond Ballo [:botond]]

(In reply to Benoit Girard (:BenWa) from comment #22)
> ::: gfx/layers/composite/TiledContentHost.cpp:635
> (Diff revision 2)
> > +      false /* compression not supported on host side */);
> 
> Doesn't the layer dumping code call this? Shoudn't compress be conditional
> on aDumpHtml then?

TileHost::DumpTexture() ignores its |aCompress| parameter, it never does compression. (The only reason it has that parameter at all is because TileClient::DumpTexture() does, and TileHost and TileClient need to present the same interface to TiledLayerBuffer which can use either one for its |Tile| template parameter).

Comment 29

[Botond Ballo [:botond]]

Comment on attachment 8666267 [details]
MozReview Request: Bug 1208661 - Move Dump() up from ContentClient to CompositableClient. r=BenWa

Bug 1208661 - Move Dump() up from ContentClient to CompositableClient. r=BenWa

Only some ContentClient implementations implement it, but it allows it to be
called from more general code. Other CompositableClient implementations can
be provided later.

Comment 30

[Botond Ballo [:botond]]

Comment on attachment 8666268 [details]
MozReview Request: Bug 1208661 - Make ContentClient dumping play nicely with HTML dumping. r=BenWa

Bug 1208661 - Make ContentClient dumping play nicely with HTML dumping. r=BenWa

Comment 31

[Botond Ballo [:botond]]

Comment on attachment 8666269 [details]
MozReview Request: Bug 1208661 - Support dumping client-side layer textures without compression. r=BenWa

Bug 1208661 - Support dumping client-side layer textures without compression. r=BenWa

Compression is used by the profiler, but we need uncompressed textures for the
browser to be able to render them when we include them in the HTML paint dump.

Comment 32

[Botond Ballo [:botond]]

Comment on attachment 8666270 [details]
MozReview Request: Bug 1208661 - Do not assume that PaintRoot()'s contributions to the HTML paint dump will all be inside a <script> tag. r=BenWa

Bug 1208661 - Do not assume that PaintRoot()'s contributions to the HTML paint dump will all be inside a <script> tag. r=BenWa

Comment 33

[Botond Ballo [:botond]]

Comment on attachment 8666271 [details]
MozReview Request: Bug 1208661 - Dump client-side layer textures. r=BenWa

Bug 1208661 - Dump client-side layer textures. r=BenWa

Comment 34

[Botond Ballo [:botond]]

Comment on attachment 8666272 [details]
MozReview Request: Bug 1208661 - Implement SourceSurfaceDual::GetDataSurface() for debugging purposes. r=BenWa

Bug 1208661 - Implement SourceSurfaceDual::GetDataSurface() for debugging purposes. r=BenWa

Comment 35

[Botond Ballo [:botond]]

Comment on attachment 8666273 [details]
MozReview Request: Bug 1208661 - Show display list and layer textures in-line in the HTML paint dump. r=BenWa

Bug 1208661 - Show display list and layer textures in-line in the HTML paint dump. r=BenWa

Comment 36

[Botond Ballo [:botond]]

Comment on attachment 8666892 [details]
MozReview Request: Bug 1208661 - Remove some no-longer-used debugging code. r=BenWa

Bug 1208661 - Remove some no-longer-used debugging code. r=BenWa

Comment 37

[Botond Ballo [:botond]]

Addressed review comments.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=675c1828db1d

Comment 38

[Botond Ballo [:botond]]

(In reply to Benoit Girard (:BenWa) from comment #24)
> ::: gfx/layers/Layers.cpp:1681
> (Diff revision 2)
> > +  layerId.AppendInt((uint64_t)this);
> 
> Wouldn't uintptr_t be a better cast here?

I had to axe this change because it led to an ambiguous overload error on platforms where uintptr_t was neither uint32_t nor uint64_t, AppendInt() being overloaded only for the latter (and AppendInt() can't be overloaded for uintptr_t because on other platforms it can be the same as uint32_t or uint64_t).

New Try push that includes the updated patches:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4203c34c0f82

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2875fa8fa6c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/49dfa7b79a59
https://hg.mozilla.org/integration/mozilla-inbound/rev/b54a6e6d123c
https://hg.mozilla.org/integration/mozilla-inbound/rev/5605805248d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f1be5c408a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a34846e0e5f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5215fd3d3ad3
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa86e7eacdb7

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2875fa8fa6c6
https://hg.mozilla.org/mozilla-central/rev/49dfa7b79a59
https://hg.mozilla.org/mozilla-central/rev/b54a6e6d123c
https://hg.mozilla.org/mozilla-central/rev/5605805248d7
https://hg.mozilla.org/mozilla-central/rev/3f1be5c408a8
https://hg.mozilla.org/mozilla-central/rev/a34846e0e5f8
https://hg.mozilla.org/mozilla-central/rev/5215fd3d3ad3
https://hg.mozilla.org/mozilla-central/rev/aa86e7eacdb7