Details
- Reviewers
gw aosmond - Commits
- rMOZILLACENTRAL12389e959ebb: Bug 1613260 - Support per-task scale for local space rasterization r=gw,aosmond
- Bugzilla Bug ID
- 1613260
Diff Detail
- Repository
- rMOZILLACENTRAL mozilla-central
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Before submitting I'd want to get some new reftests in that run this code without the need to hack up MAX_SURFACE_SIZE -- but I wanted to get your thoughts if this makes sense :) Thanks!
| gfx/wr/webrender/src/prim_store/text_run.rs | ||
|---|---|---|
| 252 | This bit missing at first was the cause of this text pop. | |
Hi Andrew, I think you're working on TextRun scaling & snapping, so I wanted to make sure this seems reasonable to you :)
The gist of the task is that a picture that wants to establish a raster root, should always be able to do so, even if the surface would be >4096 pixels. The trick is to do a last-minute downscaling to fit in 4096, using the same mechanism as DPI scaling.
I'm trying to write a reftest to expose bugs due to this scaling -- please let me know if you know of some good edge cases I can borrow from! So far all tests are passing, I need more coverage :)
Thanks!
I don't know of any test cases off hand. But I think you will want to rebase due to bug 1529260 conflicts :). You will want to add root_scaling_factor to the matrix created at:
https://searchfox.org/mozilla-central/rev/5a10be606f2d76ef22f1f44565749490de991d35/gfx/wr/webrender/src/prim_store/text_run.rs#365
I also imagine root_scaling_factor will also need to be passed into the text shader and applied as part of raster_scale?
I'll take a closer look today, but this generally looks good to me from a quick scan! The main things I think we need to work out:
- answer to Andrew's text scaling question above.
- set of extra test cases in wrench - covering various text rendering modes, filters, mix-blend-modes and transforms would probably be what to target.
- work out if/what we need to do to handle mix-blend-mode readback correctly (if nothing, that's great, but we should be able to explain why / what happens).
Thanks for the comments!
I also imagine root_scaling_factor will also need to be passed into the text shader and applied as part of raster_scale?
I think this is already working -- the shader (ps_text_run) multiplies raster_scale by task.device_pixel_scale, which is updated by adjust_scale_for_max_surface_size. Eg. if we need to scale from 8192 to 4096 and desktop DPI is at 150%, the shader will see 0.75 (via new_picture to PictureTask.device_pixel_scale to write_task_data to prim_header.user_data.z). So, uh, I think all the hard work was already done by whomever added support for arbitrary DPI scaling :) I've added a comment to clarify this.
set of extra test cases in wrench - covering various text rendering modes, filters, mix-blend-modes and transforms would probably be what to target.
I've added 3 new tests with clipping, difference-blends, text, line decorations etc. Most pixels are off by a slight amount due to the different resolutions involved; a few outliers on the edge bi-lerp to a bigger difference, so in the end the fuzzy directive looks a lot worse than it is, eg. fuzzy(89,16134). I think the important part is that the complex clip, the text location and size, and the difference-blend are all in the same place and the same size -- as opposed to shifting around or scaling up due to UVs not adjusting properly to on-the-fly DPI hackery.
TLDR I think the tests are acceptable and do test what we want?
work out if/what we need to do to handle mix-blend-mode readback correctly (if nothing, that's great, but we should be able to explain why / what happens).
Indeed -- please take a look at the use of mix-blend-mode: in the new tests, are they at the right place in the stack to test what we want to test? Thanks!
(I'll add the images to bugzilla to help visualize the YAML).
I will rebase and try run on Monday. Cheers.
Looks good to me - I've marked as accepted, but I'm unsure if you still need signoff / fixup from aosmond.
One thing I wonder - should we do this in two stages, by leaving the assign_raster_roots code in this patch, and have the behavior configurable by a bool in RendererOptions? That way, if we _do_ run into any bugs with this that are tricky to fix, we can easily disable it in the interim, while avoiding having to back the whole patch out. Then we could remove the old code paths in a couple weeks after we're more confident that no edge cases break. Thoughts?
| gfx/wr/webrender/src/picture.rs | ||
|---|---|---|
| 3609 | nit: it's used by the text run instance, but doesn't have any effect on the interning, right? | |
| gfx/wr/webrender/src/prim_store/mod.rs | ||
| 2971 | This could be: let root_scaling_factor = match pic.raster_config {
Some(raster_config) => ...
None => 1.0,
}; | |
Okay, it would be nice to add a comment to text_run.rs where we include the extra scaling factor, to say that this will be included already by the time we get to the shader. And a similar comment in the shader.
It builds...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=afe0828db38b1373e3fcb0afb75490407a23c96e&selectedJob=290641781
...but I was using wrench without --angle and just discovered that with it the rendered image looks spectacularly different... and Linux headless seems to agree with Angle.
So, the tests found an edge case alright :) Investigating...
Fixed the readback issues for the non-advanced-blend-GPU code path; shader and code was mixing "source" and "backdrop" but they're no longer one to one.
The more complex reftest C looks like this, similar for test_A.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=49bd12282725b07c84a81d3b9a9dd4e3fd48249f&selectedJob=291309353
https://treeherder.mozilla.org/#/jobs?repo=try&
revision=47c746d47225daa76c99accede3e140cae50bd3c
https://treeherder.mozilla.org/#/jobs?repo=try&revision=830335264d1275e8fa54495a41c7bc898acd0cab