Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC, alpha] Capturing infrastructure #2232

Merged
merged 13 commits into from Dec 20, 2017
Merged

[RFC, alpha] Capturing infrastructure #2232

merged 13 commits into from Dec 20, 2017

Conversation

@kvark
Copy link
Member

kvark commented Dec 18, 2017

As we start re-focusing on the correctness issues, it becomes vital to be able to capture a bug and replay it in a reduced standalone environment. Unfortunately, the current binary/player infrastructure does not provide a solid way to achieve that, see #2231.

Changes to existing logic:

  • internal representation of Document on the WR backend side is changed to have a new DocumentView item.
  • BuiltDisplayList/DisplayItemRef serialization (used for JSON/RON) is rewritten to use stronger typing (with CompletelySpecificDisplayItem enum) and consistent/robust code. Result also looks a bit better now, since items have less nesting.
  • WR API got "debug-serialization" feature, and WR itself got "capture" feature

New API calls:

  • save_capture(path)
  • load_capture(path) - still not sure if we might instead just always create a new WR backend instead of trying to overwrite the current state with a capture (comments are welcome!)
  • In the examples, we can now hit 'C' to save a capture or load one, if it's already at "captures/example".

Here is what a capture looks like on disk:
capture-files

This is just the start. What will follow is a series of issues/PRs to extend the function in the following ways:

  1. full external/blob image serialization (this is the biggest piece missing to call this a "beta"). Currently, the following paths are incomplete:
  2. #2234: storing images as PNG
  3. #2235: supporting other serial formats for the scenes, like JSON, with configurable pretty-printing
  4. wrench integration, removal of the existing JSON/RON writers
  5. #2238, #2239: more exciting stuff ;)

This change is Reviewable

@kvark kvark requested a review from glennw Dec 18, 2017
@glennw
Copy link
Member

glennw commented Dec 18, 2017

Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


webrender_api/Cargo.toml, line 11 at r1 (raw file):

nightly = ["euclid/unstable", "serde/unstable"]
ipc = ["ipc-channel"]
serial = []

I'm not sure about the naming of this - perhaps we can either come up with something a bit more descriptive or add a comment explaining what this feature provides?


webrender_api/src/display_item.rs, line 58 at r1 (raw file):

#[derive(Clone, Copy, Debug, Deserialize, PartialEq, Serialize)]
pub struct GenericDisplayItem<T> {
    pub item: T,

Do we need @gankro or @jrmuizel to check that this won't have any bad side effects on serde codegen for (de)serialization times?


Comments from Reviewable

@glennw
glennw approved these changes Dec 18, 2017
Copy link
Member

glennw left a comment

A couple of very minor comments, otherwise this looks great to me.

external texture buffer support
renamed WR API feature to "debug-serialization"
@kvark
Copy link
Member Author

kvark commented Dec 18, 2017

@glennw thanks for the review!
I've implemented a bit of missing critical logic for blobs and external buffers, renamed the wr-api feature to "debug-serialization", and confirmed with Jeff about the GenericDisplayItem changes being harmless.

@glennw
Copy link
Member

glennw commented Dec 18, 2017

@kvark r=me if you want to get this merged now then, or we can revisit if there are still more commits to come first.

@kvark
Copy link
Member Author

kvark commented Dec 18, 2017

@glennw thank you!
I believe this is a fairly complete PR that is ready to merge. But I don't want to rush it in (and now would be a perfect moment to do so, given how everyone is still getting relaxed after All Hands). Let's have a few more interested parties taking a look and then proceed.
r? @jrmuizel or @gankro

@jrmuizel
Copy link
Contributor

jrmuizel commented Dec 19, 2017

I've only skimmed it but it seems fine to me.

@kvark
Copy link
Member Author

kvark commented Dec 19, 2017

@jrmuizel please take a look at the last commit with an extra raw test, as requested. Afterwards, it should be ready to sail.

@jrmuizel
Copy link
Contributor

jrmuizel commented Dec 19, 2017

Looks good to me.

@kvark
Copy link
Member Author

kvark commented Dec 19, 2017

Alright-y, thanks everyone who looked at the code!
@bors-servo r=glennw,jrmuizel

@bors-servo
Copy link
Contributor

bors-servo commented Dec 19, 2017

📌 Commit 1088d90 has been approved by glennw,jrmuizel

@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2017

Testing commit 1088d90 with merge afaa8bb...

bors-servo added a commit that referenced this pull request Dec 20, 2017
[RFC, alpha] Capturing infrastructure

As we start re-focusing on the correctness issues, it becomes vital to be able to capture a bug and replay it in a reduced standalone environment. Unfortunately, the current binary/player infrastructure does not provide a solid way to achieve that, see #2231.

Changes to existing logic:
  - internal representation of `Document` on the WR backend side is changed to have a new `DocumentView` item.
  - `BuiltDisplayList/DisplayItemRef` serialization (used for JSON/RON) is rewritten to use stronger typing (with `CompletelySpecificDisplayItem` enum) and consistent/robust code. Result also looks a bit better now, since items have less nesting.
  - WR API got "debug-serialization" feature, and WR itself got "capture" feature

New API calls:
  - `save_capture(path)`
  - `load_capture(path)` - still not sure if we might instead just always create a new WR backend instead of trying to overwrite the current state with a capture (comments are welcome!)
  - In the examples, we can now hit 'C' to save a capture or load one, if it's already at "captures/example".

Here is what a capture looks like on disk:
![capture-files](https://user-images.githubusercontent.com/107301/34118462-dcb8518c-e3ec-11e7-854c-14b31039bb58.png)

This is just the start. What will follow is a series of issues/PRs to extend the function in the following ways:
  1. full external/blob image serialization (this is the biggest piece missing to call this a "beta"). Currently, the following paths are incomplete:
      - #2236: tiled blobs
      - #2237: external GL textures
  2. #2234: storing images as PNG
  3. #2235: supporting other serial formats for the scenes, like JSON, with configurable pretty-printing
  4. wrench integration, removal of the existing JSON/RON writers
  5. #2238, #2239: more exciting stuff ;)

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2232)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Dec 20, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw,jrmuizel
Pushing afaa8bb to master...

@bors-servo bors-servo merged commit 1088d90 into servo:master Dec 20, 2017
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 12 files, 2 discussions left (glennw, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@kvark kvark deleted the kvark:capture branch Dec 20, 2017
@pyfisch pyfisch mentioned this pull request Jan 2, 2018
3 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.