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

Capture infrastructure: Part II #2305

Merged
merged 16 commits into from Jan 16, 2018
Merged

Capture infrastructure: Part II #2305

merged 16 commits into from Jan 16, 2018

Conversation

@kvark
Copy link
Member

kvark commented Jan 15, 2018

Fixes #2238

The PR implements support for capturing all the built-frame stage environment:

  • built frames for each document
  • cached fonts, glyphs, images, and render targets
  • gpu cache

How to use:

  • run any example with "--features capture"
  • hit "c" to capture, the data is written into "captures/example"
  • run wrench with "load" command

TODO:

  • cleanup
  • rebase
  • reviews
  • proper external textures support (frame_output example)

This change is Reviewable

@kvark kvark force-pushed the kvark:capture2 branch from f391a18 to 2640ce8 Jan 15, 2018
@glennw
Copy link
Member

glennw commented Jan 16, 2018

Reviewed 31 of 35 files at r1.
Review status: 31 of 35 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


webrender/src/glyph_cache.rs, line 13 at r1 (raw file):

#[cfg_attr(feature = "capture", derive(Deserialize, Serialize))]
pub struct GenericCachedGlyphInfo<D> {

Maybe it will become clearer later in the review - but why does this need to be made generic?


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Jan 16, 2018

Partial review only so far - will be working through the other files today.

@kvark
Copy link
Member Author

kvark commented Jan 16, 2018

Review status: 31 of 35 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


webrender/src/glyph_cache.rs, line 13 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Maybe it will become clearer later in the review - but why does this need to be made generic?

The general approach is to have all the Arc<Vec<u8>> things replaced by String storing the relative path to a binary file (e.g. images/10.raw). In order to avoid code duplication, I made this struct generic over the data.


Comments from Reviewable

@kvark kvark force-pushed the kvark:capture2 branch from 2640ce8 to 1d39756 Jan 16, 2018
@glennw
Copy link
Member

glennw commented Jan 16, 2018

Reviewed 4 of 35 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


webrender/src/device.rs, line 132 at r2 (raw file):

#[derive(Clone, Copy, Debug, PartialEq)]
pub enum ReadPixelsFormat {

I wonder if we should remove this and just use ImageFormat?


Comments from Reviewable

@glennw glennw self-requested a review Jan 16, 2018
@glennw
glennw approved these changes Jan 16, 2018
Copy link
Member

glennw left a comment

Looks good to me!

@kvark
Copy link
Member Author

kvark commented Jan 16, 2018

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


webrender/src/device.rs, line 132 at r2 (raw file):

Previously, glennw (Glenn Watson) wrote…

I wonder if we should remove this and just use ImageFormat?

I wish we could. Problem is - the ImageFormat doesn't have RGBA versus BGRA...


Comments from Reviewable

@kvark kvark force-pushed the kvark:capture2 branch from 1d39756 to cd01eaa Jan 16, 2018
@kvark
Copy link
Member Author

kvark commented Jan 16, 2018

@glennw please take a look at the last commit dealing with external images. There is nothing particularly concerning there, but it's new code. frame_output example capturing works now 🎉 , and this should be ready to land.

@glennw
Copy link
Member

glennw commented Jan 16, 2018

The last commit looks good to me. You could perhaps expand on the TODO? comments with some more details, but otherwise looks fine.

@kvark
Copy link
Member Author

kvark commented Jan 16, 2018

Thanks @glennw ! This is done now.
@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jan 16, 2018

📌 Commit aeb2c60 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jan 16, 2018

Testing commit aeb2c60 with merge 1eb0032...

bors-servo added a commit that referenced this pull request Jan 16, 2018
Capture infrastructure: Part II

Fixes #2238

The PR implements support for capturing all the built-frame stage environment:
  - built frames for each document
  - cached fonts, glyphs, images, and render targets
  - gpu cache

How to use:
  - run any example with "--features capture"
  - hit "c" to capture, the data is written into "captures/example"
  - run wrench with "load" command

TODO:
- [x] cleanup
- [x] rebase
- [x] reviews
- [x] proper external textures support (`frame_output` example)

<!-- 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/2305)
<!-- Reviewable:end -->
@kvark
Copy link
Member Author

kvark commented Jan 16, 2018

@bors-servo r-
Didn't realize read_pixels_into is used by Gecko. Need to re-expose it.

@kvark kvark force-pushed the kvark:capture2 branch from aeb2c60 to c0eb493 Jan 16, 2018
…ead_pixels_into API
@kvark kvark force-pushed the kvark:capture2 branch from c0eb493 to 712e62f Jan 16, 2018
@kvark
Copy link
Member Author

kvark commented Jan 16, 2018

Should be good now. read_pixels_into is back to original form.
@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jan 16, 2018

📌 Commit 712e62f has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jan 16, 2018

Testing commit 712e62f with merge 39abbda...

bors-servo added a commit that referenced this pull request Jan 16, 2018
Capture infrastructure: Part II

Fixes #2238

The PR implements support for capturing all the built-frame stage environment:
  - built frames for each document
  - cached fonts, glyphs, images, and render targets
  - gpu cache

How to use:
  - run any example with "--features capture"
  - hit "c" to capture, the data is written into "captures/example"
  - run wrench with "load" command

TODO:
- [x] cleanup
- [x] rebase
- [x] reviews
- [x] proper external textures support (`frame_output` example)

<!-- 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/2305)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jan 16, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: glennw
Pushing 39abbda to master...

@bors-servo bors-servo merged commit 712e62f into servo:master Jan 16, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 15 files, 2 discussions left (glennw)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@kvark kvark deleted the kvark:capture2 branch Jan 17, 2018
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

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