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

Apply opacity bindings in vertex shader, rather than CPU. #3257

Merged
merged 2 commits into from Nov 1, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Collaborator

gw3583 commented Nov 1, 2018

Previously, opacity bindings were calculated on the CPU and
stored in the GPU cache as part of the primitive color. This
required the GPU cache location to be invalidated when the
opacity binding changed.

This complicates primitive interning, where we want to be able
to share the GPU cache data for interned primitives, and not
need to update it during frame building.

Instead, provide the opacity for rectangles and images as part
of the primitive instance header, and apply in the vertex
shader.

This will simplify porting rectangles and images to use
primitive interning.


This change is Reviewable

@gw3583

This comment has been minimized.

@kvark

This comment has been minimized.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 1, 2018

Fixed the shader compile errors - needed a float cast.

@kvark

Have one concern, plus red CI

Reviewed 4 of 4 files at r1, 2 of 2 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gw3583)


webrender/res/brush_image.glsl, line 150 at r1 (raw file):

    float opacity = user_data.z / 65535.0;
    image_data.color *= opacity;

Doesn't this imply PremultipliedAlpha blending? What if a different mode is used?

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 1, 2018

Try run looks good so far. There is an off-by-one alpha fuzziness in two tests in R1, however the same fuzziness exists in D2D backend, according to the reftest.list

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 1, 2018

Good point about the premultiplied alpha blend - I'll check up on that.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 1, 2018

Pushed a fix for blend modes in image shader. Good catch!

As a follow up, we should port all the batching code that uses an image shader to use a common function for getting the batch params, and add a wrench test for images with alpha blend mode (I'll open issues for these).

New pending try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=97ee08f71c2078113d68798aecb8697d721374e7

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 1, 2018

New try run looks good. Just need a review of the follow up commit that fixes the blend mode case in the image shader. The only failure is a 1/255 pixel difference in a couple of tests, but these are also fuzzy in D2D, so we can just mark them as fuzzy.

cc @staktrace

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 1, 2018

☔️ The latest upstream changes (presumably #3254) made this pull request unmergeable. Please resolve the merge conflicts.

gw3583 added some commits Oct 31, 2018

Apply opacity bindings in vertex shader, rather than CPU.
Previously, opacity bindings were calculated on the CPU and
stored in the GPU cache as part of the primitive color. This
required the GPU cache location to be invalidated when the
opacity binding changed.

This complicates primitive interning, where we want to be able
to share the GPU cache data for interned primitives, and not
need to update it during frame building.

Instead, provide the opacity for rectangles and images as part
of the primitive instance header, and apply in the vertex
shader.

This will simplify porting rectangles and images to use
primitive interning.
@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 1, 2018

Rebased 👍

@kvark

kvark approved these changes Nov 1, 2018

:lgtm:

Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gw3583)

@kvark

This comment has been minimized.

Member

kvark commented Nov 1, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 1, 2018

📌 Commit 3c59d24 has been approved by kvark

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 1, 2018

⌛️ Testing commit 3c59d24 with merge 340c25f...

bors-servo added a commit that referenced this pull request Nov 1, 2018

Auto merge of #3257 - gw3583:opacity-instancing, r=kvark
Apply opacity bindings in vertex shader, rather than CPU.

Previously, opacity bindings were calculated on the CPU and
stored in the GPU cache as part of the primitive color. This
required the GPU cache location to be invalidated when the
opacity binding changed.

This complicates primitive interning, where we want to be able
to share the GPU cache data for interned primitives, and not
need to update it during frame building.

Instead, provide the opacity for rectangles and images as part
of the primitive instance header, and apply in the vertex
shader.

This will simplify porting rectangles and images to use
primitive interning.

<!-- 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/3257)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 1, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 340c25f to master...

@bors-servo bors-servo merged commit 3c59d24 into servo:master Nov 1, 2018

3 of 4 checks passed

code-review/reviewable 1 discussion left (gw3583)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment