567421 - Layers: Add basic logging

Status: RESOLVED FIXED

(Core :: Graphics, enhancement)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I need to check that layer trees are properly synced up across processes.  stdout log is probably the easiest spot check.  The soon-to-be-attached patch adds logging output like the following when then env var NSPR_LOG_MODULES="Layers:5" is set.

-1239615488[214dab0]: ===== BeginTransaction() ====================
-1239615488[214dab0]: BasicLayerManager (0x339d060)
-1239615488[214dab0]:   Subtree:
-1239615488[214dab0]:   ____
-1239615488[214dab0]:   ContainerLayer (0x33b08d0) {
-1239615488[214dab0]:       visible region .. <nsIntRegion [ <nsIntRect @ (x=0, y=0); size (w=891, h=842)> ]>
-1239615488[214dab0]:       transform ....... <gfx3DMatrix [ 1 0 0 0;  0 1 0 0;  0 0 1 0;  0 0 0 1; ]>
-1239615488[214dab0]:       opacity ......... (unused) 1
-1239615488[214dab0]:       clip rect ....... (unused) <nsIntRect @ (x=0, y=0); size (w=0, h=0)>
-1239615488[214dab0]:   }
-1239615488[214dab0]:     |--____
-1239615488[214dab0]:     |--ThebesLayer (0x33b0a20) {
-1239615488[214dab0]:     |--    visible region .. <nsIntRegion [ <nsIntRect @ (x=0, y=0); size (w=891, h=842)> ]>
-1239615488[214dab0]:     |--    transform ....... <gfx3DMatrix [ 1 0 0 0;  0 1 0 0;  0 0 1 0;  0 0 0 1; ]>
-1239615488[214dab0]:     |--    opacity ......... (unused) 1
-1239615488[214dab0]:     |--    clip rect ....... (unused) <nsIntRect @ (x=0, y=0); size (w=0, h=0)>
-1239615488[214dab0]:     |--    valid region .... <nsIntRegion [ <nsIntRect @ (x=0, y=0); size (w=891, h=32)> <nsIntRect @ (x=0, y=32); size (w=891, h=71)> <nsIntRect @ (x=0, y=103); size (w=247, h=1)> <nsIntRect @ (x=247, y=103); size (w=644, h=18)> <nsIntRect @ (x=0, y=104); size (w=24, h=16)> <nsIntRect @ (x=24, y=104); size (w=223, h=17)> <nsIntRect @ (x=0, y=120); size (w=24, h=1)> <nsIntRect @ (x=0, y=121); size (w=891, h=721)> ]>
-1239615488[214dab0]:     |--}
-1239615488[214dab0]: ----- EndTransaction() --------------------
-1239615488[214dab0]: BasicLayerManager (0x339d060)
-1239615488[214dab0]:   Subtree:
-1239615488[214dab0]:   ____
-1239615488[214dab0]:   ContainerLayer (0x33b08d0) {
-1239615488[214dab0]:       visible region .. <nsIntRegion [ <nsIntRect @ (x=0, y=0); size (w=891, h=842)> ]>
-1239615488[214dab0]:       transform ....... <gfx3DMatrix [ 1 0 0 0;  0 1 0 0;  0 0 1 0;  0 0 0 1; ]>
-1239615488[214dab0]:       opacity ......... (unused) 1
-1239615488[214dab0]:       clip rect ....... (unused) <nsIntRect @ (x=0, y=0); size (w=0, h=0)>
-1239615488[214dab0]:   }
-1239615488[214dab0]:     |--____
-1239615488[214dab0]:     |--ThebesLayer (0x33b0a20) {
-1239615488[214dab0]:     |--    visible region .. <nsIntRegion [ <nsIntRect @ (x=0, y=0); size (w=891, h=842)> ]>
-1239615488[214dab0]:     |--    transform ....... <gfx3DMatrix [ 1 0 0 0;  0 1 0 0;  0 0 1 0;  0 0 0 1; ]>
-1239615488[214dab0]:     |--    opacity ......... (unused) 1
-1239615488[214dab0]:     |--    clip rect ....... (unused) <nsIntRect @ (x=0, y=0); size (w=0, h=0)>
-1239615488[214dab0]:     |--    valid region .... <nsIntRegion [ <nsIntRect @ (x=0, y=0); size (w=891, h=32)> <nsIntRect @ (x=0, y=32); size (w=700, h=26)> <nsIntRect @ (x=887, y=32); size (w=4, h=26)> <nsIntRect @ (x=0, y=58); size (w=891, h=45)> <nsIntRect @ (x=0, y=103); size (w=247, h=1)> <nsIntRect @ (x=247, y=103); size (w=644, h=18)> <nsIntRect @ (x=0, y=104); size (w=24, h=16)> <nsIntRect @ (x=24, y=104); size (w=223, h=17)> <nsIntRect @ (x=0, y=120); size (w=24, h=1)> <nsIntRect @ (x=0, y=121); size (w=891, h=721)> ]>
-1239615488[214dab0]:     |--}

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Add basic logging for layers

This patch leaves logging capabilities enabled for opt and debug builds.  The "log-enabled" check is a single branch taken once for BeginTransaction() and once for EndTransaction(*), so the cost should be minimal.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I've got some layer-dumping code in FrameLayerBuilder in my patch queue ... we need to merge that with this somehow. What do you think?

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I agree.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Rename some stuff and add debug names for BasicLayers

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Implement Dump[Self](FILE*) and Log[Self]() methods for layers

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Rename some stuff and add debug names for BasicLayers (updated)

These have rotted pretty thoroughly.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Implement Dump[Self](FILE*) and Log[Self]() methods for layers

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is it useful to have layer-manager-specific layer names? If it is, why bother with names for the base layer classes?

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #8)
> Is it useful to have layer-manager-specific layer names? If it is, why bother
> with names for the base layer classes?

I had in mind coexisting Basic/OGL managers.  Mostly to avoid touching layers/opengl and layers/d3d9, initially ;) (I didn't care about either when I wrote these patches).  But I'd be happy to switch the abstract bases to a new LAYER_DECL_TYPE() macro or somesuch and add DECL_NAME() to the other backends, if you prefer that.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Personally I don't think we need per-layer-manager names. We could have names for the layer managers and then logging code would just get the manager from the layer and then get the manager's name.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Something like |printf("layer name: %s%s", layer->Manager()->TypeName(), layer->Name())| ?  That suits me.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yeah

Comment 13

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Rename NS_* macros to MOZ_* and add LayerManager::Name() interface

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 2: Implement Dump[Self](FILE*) and Log[Self]() methods for layers

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Rob, due to the unholy mess that is our logging system, it turns out that rocket science is needed to conditionally declare these logging functions in a way that compiles on all platforms.  To keep this code simple, do you mind having these symbols always declared?  If not, then we have two options for their implementation: either always define them too, or define no-ops in Layers.cpp under the original |ifdef DEBUG || PR_LOGGING| condition.  Do you have a preference?

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The latter.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Rollup patch of parts 1 and 2

Requesting approval2.0 for this logging code because it's

  (1) adds interfaces and a new file that I would like to share between m-c and [mumble] when the cross-process layers work "branches" into [mumble].  Not taking it will cause merge headaches.
  (2) is mostly not compiled for release builds and NPOTB regardless.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/1c0e2921fc65
http://hg.mozilla.org/mozilla-central/rev/b0f37b33246e