1206915 - Fix paint dumping

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

We have code to dump display item and layer textures for debugging purposes. However, it apparently hasn't been used for a while and has been broken along the way. Let's fix it because it's a useful debugging tool (and I'd like to use it to debug 1205630).

Comment 1

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1206915 - Move dumping of compositor textures under its own environment variable. r=mattwoodrow

Bug 1206915 - Move dumping of compositor textures under its own environment variable. r=mattwoodrow

The rationale is that it's broken at least on some platforms (e.g.
TextureHost::GetAsSurface() is not implemented), and moving it under its own
environment variable allows us to use the client-side parts of paint dumping
without crashing while attempting to do the compositor-side parts.

Comment 2

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1206915 - Avoid reordering of different parts of paint dump output. r=mattwoodrow

Bug 1206915 - Avoid reordering of different parts of paint dump output. r=mattwoodrow

Comment 3

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1206915 - Make paint dumping to a file e10s-friendly. r=mattwoodrow

Bug 1206915 - Make paint dumping to a file e10s-friendly. r=mattwoodrow

Comment 4

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1206915 - Handle nested PaintFrame() calls correctly during paint dumping. r=mattwoodrow

Bug 1206915 - Handle nested PaintFrame() calls correctly during paint dumping. r=mattwoodrow

Comment 5

[Botond Ballo [:botond]]

Attachment: MozReview Request: [WIP] Bug 1206915 - Dump client-side layer textures

[WIP] Bug 1206915 - Dump client-side layer textures

Comment 6

[Botond Ballo [:botond]]

The first four patches fix the dumping of display items with links to their rendered textures in an HTML file.

The fifth patch aims to dump layer textures on the client side (as dumping on the compositor side is not implemented on all platforms and requires more infrastructure to implement), but isn't quite working yet.

Comment 7

[Markus Stange [:mstange]]

https://reviewboard.mozilla.org/r/19887/#review17933

::: layout/base/nsLayoutUtils.cpp:2886
(Diff revision 1)
> +// nsLayoutUtils::PaintFrame() can call itself recursively, so rather than

What causes these nested paint calls? It shouldn't call itself recursively.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Markus Stange [:mstange] from comment #7)
> https://reviewboard.mozilla.org/r/19887/#review17933
> 
> ::: layout/base/nsLayoutUtils.cpp:2886
> (Diff revision 1)
> > +// nsLayoutUtils::PaintFrame() can call itself recursively, so rather than
> 
> What causes these nested paint calls? It shouldn't call itself recursively.

It shouldn't, but it does.

External svg documents as images is the case we were seeing, but I think -moz-element does it too. There may be other cases.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8663958 [details]
MozReview Request: Bug 1206915 - Move dumping of compositor textures under its own environment variable. r=mattwoodrow

https://reviewboard.mozilla.org/r/19881/#review17959

Comment 10

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8663959 [details]
MozReview Request: Bug 1206915 - Avoid reordering of different parts of paint dump output. r=mattwoodrow

https://reviewboard.mozilla.org/r/19883/#review17961

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8663960 [details]
MozReview Request: Bug 1206915 - Make paint dumping to a file e10s-friendly. r=mattwoodrow

https://reviewboard.mozilla.org/r/19885/#review17963

Comment 12

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8663961 [details]
MozReview Request: Bug 1206915 - Handle nested PaintFrame() calls correctly during paint dumping. r=mattwoodrow

https://reviewboard.mozilla.org/r/19887/#review17965

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8663962 [details]
MozReview Request: [WIP] Bug 1206915 - Dump client-side layer textures

Let's land the patches that have been reviewed. I'll file a new bug for client-side texture dumping.

Comment 14

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5df560d1d3f8

Comment 15

[Botond Ballo [:botond]]

The Try push is indicating that I'm leaking a (single) nsTArray object. Presumably it's the

  StaticAutoPtr<nsTArray<int>> gPaintCountStack;

that I've added in nsLayoutUtils.cpp. Am I supposed to clear that somewhere?

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #15)
> The Try push is indicating that I'm leaking a (single) nsTArray object.
> Presumably it's the
> 
>   StaticAutoPtr<nsTArray<int>> gPaintCountStack;
> 
> that I've added in nsLayoutUtils.cpp. Am I supposed to clear that somewhere?

Apparently yes, with ClearOnShutdown().

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8663958 [details]
MozReview Request: Bug 1206915 - Move dumping of compositor textures under its own environment variable. r=mattwoodrow

Bug 1206915 - Move dumping of compositor textures under its own environment variable. r=mattwoodrow

The rationale is that it's broken at least on some platforms (e.g.
TextureHost::GetAsSurface() is not implemented), and moving it under its own
environment variable allows us to use the client-side parts of paint dumping
without crashing while attempting to do the compositor-side parts.

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8663959 [details]
MozReview Request: Bug 1206915 - Avoid reordering of different parts of paint dump output. r=mattwoodrow

Bug 1206915 - Avoid reordering of different parts of paint dump output. r=mattwoodrow

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8663960 [details]
MozReview Request: Bug 1206915 - Make paint dumping to a file e10s-friendly. r=mattwoodrow

Bug 1206915 - Make paint dumping to a file e10s-friendly. r=mattwoodrow

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8663961 [details]
MozReview Request: Bug 1206915 - Handle nested PaintFrame() calls correctly during paint dumping. r=mattwoodrow

Bug 1206915 - Handle nested PaintFrame() calls correctly during paint dumping. r=mattwoodrow

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8663961 [details]
MozReview Request: Bug 1206915 - Handle nested PaintFrame() calls correctly during paint dumping. r=mattwoodrow

Updated to add ClearOnShutdown() call. Re-requesting review.

Comment 22

[Botond Ballo [:botond]]

New Try push that includes this patch:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39e65f2343a1

Comment 23

[Botond Ballo [:botond]]

Comment on attachment 8663961 [details]
MozReview Request: Bug 1206915 - Handle nested PaintFrame() calls correctly during paint dumping. r=mattwoodrow

Asking BenWa for review instead as Matt is on PTO.

Comment 24

[Benoit Girard (:BenWa)]

Comment on attachment 8663961 [details]
MozReview Request: Bug 1206915 - Handle nested PaintFrame() calls correctly during paint dumping. r=mattwoodrow

https://reviewboard.mozilla.org/r/19887/#review18437

Looks good

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8778868f4830
https://hg.mozilla.org/integration/mozilla-inbound/rev/29cfc8773048
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b455fe27440
https://hg.mozilla.org/integration/mozilla-inbound/rev/31364c76dd97

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8778868f4830
https://hg.mozilla.org/mozilla-central/rev/29cfc8773048
https://hg.mozilla.org/mozilla-central/rev/2b455fe27440
https://hg.mozilla.org/mozilla-central/rev/31364c76dd97