Adds display list debugger

[gaaclarke]

Design doc: https://docs.google.com/document/d/17LiGKlt57R5CZMxcWt9wj9Z7eDihhUaUEg3oeGQAKTc/edit#heading=h.kifztz2gx2n3

This is a replacement for the CanvasRecorder. It just saves out the display list as a blob. The format is unstable and unsupported but people are allowed to use it for development / debugging purposes.

This won't suffer the same fate as CanvasRecorder since it's just tapping into serialized draw calls we already make as part of rendering. It's only a couple of lines of code and requires no special compilation.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
• I listed at least one issue that this PR fixes in the description above.
• I added new tests to check the change I am making or feature I am adding, or the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
• I updated/added relevant documentation (doc comments with ///).
• I signed the CLA.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

[jonahwilliams]

We specifically discussed this in the weekly and concluded that we should not add new debugging tools. You're free to create your own debugging tools on the side, and we could discuss checking it in if you find it useful. But otherwise this should not land.

[flar]

We specifically discussed this in the weekly and concluded that we should not add new debugging tools. You're free to create your own debugging tools on the side, and we could discuss checking it in if you find it useful. But otherwise this should not land.

I am also unsure how dumping the bytes of a DisplayList helps any test harness. You can't simply read them back and find it useful and as for debugging or inspection purposes, the bytes are unreadable without intimate knowledge of the DL which can change over time and in some cases they are mere pointers to data that is not represented. Not knowing how these plug into whatever code plans on using them, why wasn't the existing display list pretty printer used instead - which outputs readable data and has to be maintained as the DL interfaces (specifically the OpReceiver) change over time.

See:

engine/testing/display_list_testing.h

Line 33 in 900ca9a

extern std::ostream& operator<<(std::ostream& os,

[gaaclarke]

We specifically discussed this in the weekly and concluded that we should not add new debugging tools. You're free to create your own debugging tools on the side, and we could discuss checking it in if you find it useful. But otherwise this should not land.

Yes, that's why we decided that CanvasRecorder should be removed. This may be poorly named, it's the basis from which people can develop debugging tools outside of the engine repo. Originally the design doc there was an idea of about making something like canvas recorder that printed out usable information. After our discussion yesterday it was my understanding that that approach to debugging tool development isn't how we want to operate.

My understanding from the conversation about CanvasRecorder is that it was getting in peoples way and provided little value considering it's complexity and footprint. I don't think any of that applies here. It's 50 lines of code that just writes a region of memory to disk verbatim when signaled.

I am also unsure how dumping the bytes of a DisplayList helps any test harness. You can't simply read them back and find it useful and as for debugging or inspection purposes, the bytes are unreadable without intimate knowledge of the DL which can change over time and in some cases they are mere pointers to data that is not represented.

This decision was made because of our discussion at the sync to not add debugging tools in the engine. Anything that parses or translates the display list I would consider debugging tool in the engine. The fact that it is an obtuse and unsupported file format was a decision to keep it in line with our discussion.

[flar]

My understanding from the conversation about CanvasRecorder is that it was getting in peoples way and provided little value considering it's complexity and footprint. I don't think any of that applies here. It's 50 lines of code that just writes a region of memory to disk verbatim when signaled.

That region of data is not intended to be exposed or supported in any way. Writing a tool to read it would be impossible because it is not versioned and so you can't even begin to know what the bytes mean. (Even if you are fluent in the storage used by DL/Builder you wouldn't know - which engine did it come from? which platform? (layout on Windows is vastly different than layout on Linux/Mac and 32/64 bit architectures have different formats) and which engine commit was it dumped from (I'm constantly changing the internal storage formats these days, something which would continue over time)).

If this is for inspection, such a tool already exists to make debugging output more readable and useful for developers and it will be maintained because it is already used in a large number of engine unit tests and will be the preferred testing tool moving forward (MockCanvas which was not useful for debugging is slowly being replaced with this new tool). See

engine/testing/display_list_testing.h

Line 33 in 900ca9a

extern std::ostream& operator<<(std::ostream& os,

This decision was made because of our discussion at the sync to not add debugging tools in the engine. Anything that parses or translates the display list I would consider debugging tool in the engine. The fact that it is an obtuse and unsupported file format was a decision to keep it in line with our discussion.

We already have one, existing only for testing purposes, though. The bytes hidden in the DL storage are an unsupported file format, but the debugging output from the above tool is supported and used in many tests...

[flar]

If we are going to support a format that is available for external testing it would have to be:

a - architecture agnostic (DL storage is not)
b - versioned (DL storage data is not)
c - documented (Dl storage format is not)

We can achieve the short term effects of this by just pretty printing the DL OpReceiver output as is already mentioned above. If we want the ability to load it back in or express it in a more DlCanvas-like manner (the existing printer is more of an OpReceiver format which is very close to DlCanvas, but requires editing if one wants to use it to create source for a new test - something that could be fixed over time) we can put in the work for that or workin on some sort of json output that could be post-formatted in any number of ways.

[gaaclarke]

@flar the fact that the binary blob is not portable or version is fine for what I need it for. I don't think we want a portable file format. For my purposes, the author and consumer of these blobs will be the same person. The pretty printer isn't helpful in my case either since I don't want to write a parser for that output.

I think it might be cool if users could share their display list with us if they didn't want to share their source code with us or they couldn't. Maybe that's something we could do in the future if we thought it would help. I'm not proposing that.

How it plans to be used is in: https://docs.google.com/document/d/17LiGKlt57R5CZMxcWt9wj9Z7eDihhUaUEg3oeGQAKTc/edit#heading=h.kifztz2gx2n3

[gaaclarke]

I talked to @flar offline, here's my notes (jim, feel free to correct me, hopefully I get this right)

1. This data blob will be missing some relevant data since it includes pointers to other structs.
2. My plan was to create a separate executable outside of the engine repo that parses the blob. That way it isn't in anyones way and is inline with our conversation yesterday. Jim would really like to see the tool inside of the engine repo to make sure it's versioned in sync with the engine. I think that's a good point but I also understand our hesitancy given yesterdays conversation.
3. Jim mentioned that the pretty printer for the display list basically prints out c++ code, so it's close to what we want already.

Here are the action items from the meeting:

1. I'm going to add a version number to the blob. That's because the data format is really unstable (especially across platforms), in the future if we may want to use this for different use-cases. It would be nice to be able to make it more robust in a later version.
2. I'm going to start making the executable to parse the blobs in the engine repo as a dependent PR and have it try to use the pretty printer Jim has made. That will give us a better idea of how to proceed.
3. Jim had a lot of cool ideas how serialized display lists could be used to help us debug 1p and 3p customers and how they could be used for artifacts of broken tests. I encouraged him to write them down so we can have a north star about where we could go with this.
4. I need to chat with @jonahwilliams to see where his head is at since Jim's suggestions and his suggestions are at somewhat at odds. I think they can be reconciled by having a teeny tiny footprint on the engine binary itself and having debugging code living in a separate host executable and being fast to write up the vision, but slow to implement it as we have demonstrated need for it.

[gaaclarke]

I'm getting feedback that this is not enough and other feedback that this goes too far. For what I need it for, I don't even need to land it. I just need to be able to use it with my local builds so for that a separate branch suffices. The footprint of the diff is so tiny, I don't expect there to be much maintenance. I don't want this discussion to be a distraction.

We could even add comments to the source code that could make patching easier. I think adding platform channels and viewing/modifying display lists are interesting plugin hooks. We probably don't want to maintain a binary interface for those, but some tags in the code could make it easy for people to make plugins that apply as patches would be interesting.

Anyways, closing this, moving it to a branch with a script that can apply the patch, shelving the discussion for another day.

[gaaclarke]

Notes for future archeology: This approach wouldn't have worked. I thought the display list would just have pointers to external data like pictures. Display lists also have pointers to other display lists, so the blobs are more incomplete than I thought.