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

Port YUV primitive to be a brush. #2424

Merged
merged 1 commit into from Feb 16, 2018
Merged

Port YUV primitive to be a brush. #2424

merged 1 commit into from Feb 16, 2018

Conversation

@glennw
Copy link
Member

glennw commented Feb 15, 2018

Also refactor and optimize the YUV shader implementation to do
less per-pixel work.

Support segment building for YUV primitive - this is a large
performance win for any YUV images with a complex clip on them.

This is part of the prep work to allow selection of whether
a stacking context is rasterized in local or screen-space.


This change is Reviewable

@glennw
Copy link
Member Author

glennw commented Feb 15, 2018

r? @nical and/or @kvark

If someone else on Gecko knows the YUV code better, please get them to take a look too as we don't have a huge amount of test coverage in-tree for YUV images.

Pending try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c5b905907fb40875452aa702cd9ff0d8cc90a96

@glennw glennw force-pushed the glennw:yuv-brush branch from b8677b9 to 10b6b39 Feb 15, 2018
@glennw
Copy link
Member Author

glennw commented Feb 15, 2018

There are a couple of failures in some of the MP4 tests, which are probably related to this. I'll investigate those and see if I can reproduce locally.

Also refactor and optimize the YUV shader implementation to do
less per-pixel work.

Support segment building for YUV primitive - this is a large
performance win for any YUV images with a complex clip on them.

This is part of the prep work to allow selection of whether
a stacking context is rasterized in local or screen-space.
@glennw glennw force-pushed the glennw:yuv-brush branch from 10b6b39 to e6dc591 Feb 15, 2018
@glennw
Copy link
Member Author

glennw commented Feb 15, 2018

Fixed a typo - was using the uv_bounds.z for the texture layer index instead of the uv.z. Another gecko try coming shortly to confirm it's all fixed.

@glennw
Copy link
Member Author

glennw commented Feb 15, 2018

Try looks good now, I think - just the expected failure in R8 from previous PR, and an unrelated intermittent in R4 (that test has no YUV images).

@kvark
Copy link
Member

kvark commented Feb 15, 2018

:lgtm:


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


webrender/res/brush_yuv_image.glsl, line 146 at r1 (raw file):

    vec2 uv_u = clamp(vUv_U.xy, vUvBounds_U.xy, vUvBounds_U.zw);
    vec2 uv_v = clamp(vUv_V.xy, vUvBounds_V.xy, vUvBounds_V.zw);
    yuv_value.x = TEX_SAMPLE(sColor0, vec3(uv_y, vUv_Y.z)).r;

it almost feels like our TEX_SAMPLE macro should get an extra parameter of UV bounds and do the clamping


webrender/res/brush_yuv_image.glsl, line 159 at r1 (raw file):

    // https://www.khronos.org/registry/OpenGL/extensions/APPLE/APPLE_rgb_422.txt
    vec2 uv_y = clamp(vUv_YUV.xy, vUvBounds_YUV.xy, vUvBounds_YUV.zw);
    yuv_value = TEX_SAMPLE(sColor0, vec3(uv_y, vUv_YUV.z)).gbr;

uh, the gbr order here is confusing


Comments from Reviewable

@kvark
Copy link
Member

kvark commented Feb 15, 2018

@bors-servo
Copy link
Contributor

bors-servo commented Feb 15, 2018

📌 Commit e6dc591 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Feb 16, 2018

Testing commit e6dc591 with merge 9291910...

bors-servo added a commit that referenced this pull request Feb 16, 2018
Port YUV primitive to be a brush.

Also refactor and optimize the YUV shader implementation to do
less per-pixel work.

Support segment building for YUV primitive - this is a large
performance win for any YUV images with a complex clip on them.

This is part of the prep work to allow selection of whether
a stacking context is rasterized in local or screen-space.

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

bors-servo commented Feb 16, 2018

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

@bors-servo bors-servo merged commit e6dc591 into servo:master Feb 16, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 2 discussions left (glennw, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@glennw glennw deleted the glennw:yuv-brush branch Feb 16, 2018
@staktrace
Copy link
Contributor

staktrace commented Feb 16, 2018

This PR seems to have introduced shader compilation failures on Windows. See https://treeherder.mozilla.org/logviewer.html#?job_id=162670488&repo=try&lineNumber=1823

This also probably explains the orange windows test jobs in the try push noted earlier in this PR. Those jobs should be green.

@kvark
Copy link
Member

kvark commented Feb 16, 2018

@staktrace sorry about that! I'm fixing this.

@kvark kvark mentioned this pull request Feb 16, 2018
bors-servo added a commit that referenced this pull request Feb 16, 2018
Fix brush YUV shader

Fixes #2424 (comment)
r? @glennw , cc @staktrace

<!-- 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/2437)
<!-- Reviewable:end -->
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

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