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

Remove AlphaBatchTask, and introduce start of raster mode. #2053

Closed
wants to merge 2 commits into from

Conversation

glennw
Copy link
Member

@glennw glennw commented Nov 17, 2017

Most of the difficult work related to this was done in the
large picture PR that landed previously.

This removes the remnants of AlphaBatch tasks, and makes them
all run through Picture render tasks.

This also introduces the first part of the picture and shader
side support for selecting whether primitives on a given picture
are to be rasterized in screen space or a local space.


This change is Reviewable

Some render tasks can have a source task of different kinds. The
most common example is that the source task to a blur task can
be any of (a) a picture (b) an alpha task (c) a downscaling task.

To ensure this works correctly, we separate render task data into
a common base, followed by a small amount of task-specific data.

The blur tasks now only fetch the common task data, which is all
that they require.

This fixes a bug (that I don't think is actually occurring in any
tests), and is also prep work for the next step. Now that everything
is using Picture internally, we can remove the AlphaBatchTask
completely. This will allow us to start converting more primitives
over to use a smaller subset of brush shaders, which will alow us
to start doing the primitive segmentation and also get better
batching results.
Most of the difficult work related to this was done in the
large picture PR that landed previously.

This removes the remnants of AlphaBatch tasks, and makes them
all run through Picture render tasks.

This also introduces the first part of the picture and shader
side support for selecting whether primitives on a given picture
are to be rasterized in screen space or a local space.
@glennw
Copy link
Member Author

glennw commented Nov 17, 2017

r? @kvark

This patch isn't as big as it seems - it just includes #2052 which is a prerequisite. If it's easier to review and land #2052 first, and then review this - that's cool, otherwise I can close #2052 and just get them both reviewed here at the same time.

I'll kick off a gecko try run with this shortly.

@glennw
Copy link
Member Author

glennw commented Nov 17, 2017

Pending try is here https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b82697f5d521d9e07fc45dd874b36c88ccd57cb - all the orange results are expected due to previous PRs, from what I can tell (but the try is not quite finished yet).

@kvark
Copy link
Member

kvark commented Nov 17, 2017

@mstange
Copy link
Contributor

mstange commented Nov 17, 2017

The first one is known, see https://bugzilla.mozilla.org/show_bug.cgi?id=1417062#c18 .

@mstange
Copy link
Contributor

mstange commented Nov 17, 2017

And same-filter.html has existing fuzz: fuzzy-if(webrender,15-15,8274-8274).
The reftest failure says max difference: 14, number of differing pixels: 8286

@kvark
Copy link
Member

kvark commented Nov 17, 2017

:lgtm: minus a few points mentioned, also the fact it's based on a PR where I requested more changes


Reviewed 14 of 14 files at r2.
Review status: 14 of 18 files reviewed at latest revision, 3 unresolved discussions.


webrender/res/prim_shared.glsl, line 290 at r1 (raw file):

}

struct AlphaBatchTask {

hooray!


webrender/src/picture.rs, line 44 at r2 (raw file):

#[repr(C)]
#[derive(Debug, Copy, Clone)]
pub enum RasterizationMode {

shouldn't this be RasterizationSpace::Local/Screen instead?


webrender/src/render_task.rs, line 286 at r2 (raw file):

        prim_index: PrimitiveIndex,
        target_kind: RenderTargetKind,
        content_origin_x: f32,

any reason this is not just LayerPoint?


Comments from Reviewable

@glennw
Copy link
Member Author

glennw commented Nov 19, 2017

Review status: 14 of 18 files reviewed at latest revision, 3 unresolved discussions.


webrender/src/picture.rs, line 44 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

shouldn't this be RasterizationSpace::Local/Screen instead?

Done.


webrender/src/render_task.rs, line 286 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

any reason this is not just LayerPoint?

When rasterization space is Screen - it's a device space content origin, not a layer origin. As a follow up, I'd like to make this type safe - by moving content origin to be part of the rasterization space enum.


Comments from Reviewable

@glennw
Copy link
Member Author

glennw commented Nov 19, 2017

I messed up and pushed this branch to the #2052 after addressing review comments. Closing this as #2052 has both commits and comments addressed from both reviews.

@glennw glennw closed this Nov 19, 2017
@glennw glennw deleted the remove-alpha-task2 branch November 19, 2017 20:42
bors-servo pushed a commit that referenced this pull request Nov 21, 2017
DocumentAPI support on the Renderer side

This is a second batch of changes related to Document API, following #1509.

The idea is to allow Gecko to have separate documents for the chrome UI, page content, and bottom status bar, as opposed to using different pipelines in the same document. This change would allow minimal scene rebuilds per frame when UI is affected.

WIP TODO:
- [x] strict ordering API: 532bff5
- [x] Servo patch and test runs: kvark/servo@0263fa8
- [x] Firefox patch and try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f939fbdab0231e6328061c12903db0c45847dd18
  - ~~`box-shadow/boxshadow-skiprect.html`~~ (from #1943)
  - ~~`css-filter-chains/moz-element.html`~~ (same as in #2053)
  - ~~`transform/animate-layer-scale-inherit-1.html`~~ (from #2043)
- [x] example
- [x] `FrameBuilder` and `RenderedDocument` refactor
https://tools.taskcluster.net/groups/DEvThfCaQWmt5Hp806GDLg/tasks/VLCZI7hVTQCc1xrDR3PgOQ/details#
- [x] review comments

cc  @glennw  @mstange @jrmuizel

<!-- 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/2050)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants