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 2.5 #2326

Merged
merged 5 commits into from Jan 22, 2018
Merged

Capture infrastructure, part 2.5 #2326

merged 5 commits into from Jan 22, 2018

Conversation

@kvark
Copy link
Member

kvark commented Jan 19, 2018

The PR rewrites the way capturing interacts with external images and buffers. For the matter of consistency with the frame-level capturing, and respecting the given UV ranges, the external stuff now goes through an extra level of indirection:

  • Old: "scene-1-0.ron" -> "blobs/4.raw"
  • New: "scene-1-0.ron" -> "externals/4.ron" -> "externals/t2.raw"

This is a WIP for early feedback about the refactor/API changes, comes with absolutely no warranty! Ultimately, the PR will be ready when replaying a capture from Gecko works edit: is ready 🎉 .

Public API changes:

  • TexelRect is exposed and used in ExternalImage
  • TextureTarget is exposed and used in ExternalImageType
  • wrench compact profiler key is now "S" (same as the examples use), while "C" is reserved for capturing

The PR also contains a bunch of internal refactoring to drop some of the technological debt accumulated, including but not limited to:

  • use of strongly typed RectWithEndpoint in ImageResource GLSL code
  • private contents of GpuBlockData, better conversions to it
  • more use of Self in method returns, plus some associated constants
  • simplification of a quite a bit of conversion code thanks to the public API changes ^
  • wrench window size handling is streamlined a bit, it's also getting re-sized on capture load

This change is Reviewable

@kvark kvark force-pushed the kvark:capture branch from fd45138 to 3f34869 Jan 22, 2018
@kvark kvark changed the title [WIP] Capture infrastructure, part 2.5 Capture infrastructure, part 2.5 Jan 22, 2018
@kvark kvark force-pushed the kvark:capture branch from 3f34869 to f1aabc2 Jan 22, 2018
@kvark
Copy link
Member Author

kvark commented Jan 22, 2018

The main problem I had with Gecko was caused by the fact I used to issue a capture from the Browser Console, which is in turn a window also using WR, so all the captured data was messed up. When capturing with a key combination (thanks to @mstange for help setting this up), wrench shows the captured FF frame just fine:
ff-wrench

With this in, the PR is ready for review. I'm going to follow up with the FF patch and a gecko try push before this is merged.

@glennw
Copy link
Member

glennw commented Jan 22, 2018

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


webrender/src/capture.rs, line 72 at r1 (raw file):

        let color_type = match format {
            ImageFormat::BGRA8 => ColorType::RGBA, //TODO?

Could you expand on what the TODO here means?


webrender/src/capture.rs, line 104 at r1 (raw file):

    pub data: String,
    pub id: ExternalImageId,
    pub channel_index: u8,

What does channel_index refer to here?


webrender/src/renderer.rs, line 494 at r1 (raw file):

}

//TODO: those types are the same, can we merge them?

I think so, yep (doesn't need to be in this PR).


Comments from Reviewable

@glennw
Copy link
Member

glennw commented Jan 22, 2018

@kvark Looks good! r=me once those minor comments are addressed, and try is green.

@kvark
Copy link
Member Author

kvark commented Jan 22, 2018

@glennw thanks for taking a look! The try push shows some JS errors that may be related to the JS code I put in (unlikely though) but have nothing to do with WR side of things, so I consider it green for the matter of this PR (unless @staktrace says otherwise ;) ).

Wrench: window size refactor
@kvark kvark force-pushed the kvark:capture branch from f1aabc2 to 32c7de1 Jan 22, 2018
@kvark
Copy link
Member Author

kvark commented Jan 22, 2018

@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

📌 Commit 32c7de1 has been approved by glennw

@staktrace
Copy link
Contributor

staktrace commented Jan 22, 2018

The try push looks good to me

@bors-servo
Copy link
Contributor

bors-servo commented Jan 22, 2018

Testing commit 32c7de1 with merge 9a73429...

bors-servo added a commit that referenced this pull request Jan 22, 2018
Capture infrastructure, part 2.5

The PR rewrites the way capturing interacts with external images and buffers. For the matter of consistency with the frame-level capturing, and respecting the given UV ranges, the external stuff now goes through an extra level of indirection:
  - Old: "scene-1-0.ron" -> "blobs/4.raw"
  - New: "scene-1-0.ron" -> "externals/4.ron" -> "externals/t2.raw"

This ~~is a WIP for early feedback about the refactor/API changes,~~ comes with absolutely no warranty! Ultimately, the PR ~~will be ready when replaying a capture from Gecko works~~ edit: is ready 🎉 .

Public API changes:
  - `TexelRect` is exposed and used in `ExternalImage`
  - `TextureTarget` is exposed and used in `ExternalImageType`
  - `wrench` compact profiler key is now "S" (same as the examples use), while "C" is reserved for capturing

The PR also contains a bunch of internal refactoring to drop some of the technological debt accumulated, including but not limited to:
  - use of strongly typed `RectWithEndpoint` in `ImageResource` GLSL code
  - private contents of `GpuBlockData`, better conversions to it
  - more use of `Self` in method returns, plus some associated constants
  - simplification of a quite a bit of conversion code thanks to the public API changes ^
  - wrench window size handling is streamlined a bit, it's also getting re-sized on capture load

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

bors-servo commented Jan 22, 2018

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

@bors-servo bors-servo merged commit 32c7de1 into servo:master Jan 22, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 3 files, 3 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 Jan 23, 2018
@kvark
Copy link
Member Author

kvark commented Jan 23, 2018

Please not that this is not the end of story. I noticed some external images not showing up in Gecko captures, so I'll follow up with more fixes. This PR main goal is to have the Gecko integration finally set up, and I'm sure it's helpful in the current shape already.

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.