Skip to content

Conversation

@nical
Copy link
Contributor

@nical nical commented Apr 27, 2018

This PR implements decomposing tiled images during frame building using visibility information to avoid issues with very large blob images.
This version does not use segments, avoiding issues with the maximum contiguous gpu cache alloc size as well as a few hacks that I had to write to get repetitions to work with segments.

As a bonus the non-brush image primitive and ps_image shader are now obsolete so I removed them.


This change is Reviewable

@nical
Copy link
Contributor Author

nical commented Apr 27, 2018

Sorry about closing/recreating the PR, turns out git push -f and git push -d are very close on a qwerty keyboard.
I rebased the PR with #2644 now in master, although we still need #2664 to land before merging this.

@nical
Copy link
Contributor Author

nical commented Apr 27, 2018

@nical nical requested a review from glennw April 27, 2018 16:35
@glennw
Copy link
Member

glennw commented Apr 27, 2018

cargo test is failing on CI. I suspect it's just that you need to remove ps_image from the list of shaders that run through the validation pass:

thread 'validate_shaders' panicked at 'Shader compilation failed: ps_image
WARNING: 0:5: '' : unexpected end of file found in directive
ERROR: 0:? : '' : syntax error
', webrender/tests/angle_shader_validation.rs:146:13
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

We'll also want to resolve the reftest failures @staktrace mentioned here #2644 (comment).

Thanks for addressing all those comments from the previous review!

@glennw
Copy link
Member

glennw commented Apr 29, 2018

There's a heap of CI tests failing too - maybe something went wrong during rebasing?

Probably around half of the reftests fail, until it crashes at:

REFTEST TEST-END | reftests/gradient/repeat-border-radius.yaml == reftests/gradient/repeat-border-radius.png
REFTEST reftests/image/tile-size.yaml == reftests/image/tile-size-ref.yaml
thread 'WRRenderBackend#0' panicked at 'handle not requested or allocated!', libcore/option.rs:917:5
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::_print
             at libstd/sys_common/backtrace.rs:71
   2: std::panicking::default_hook::{{closure}}
             at libstd/sys_common/backtrace.rs:59
             at libstd/panicking.rs:380
   3: std::panicking::default_hook
             at libstd/panicking.rs:396
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:576
   5: std::panicking::begin_panic
             at libstd/panicking.rs:537
   6: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:521
   7: rust_begin_unwind
             at libstd/panicking.rs:497
   8: core::panicking::panic_fmt
             at libcore/panicking.rs:71
   9: core::option::expect_failed
             at libcore/option.rs:917
  10: <unknown>
             at /checkout/src/libcore/option.rs:302
             at webrender/src/gpu_cache.rs:647
             at webrender/src/batch.rs:624
Traceback (most recent call last):
  File "script/headless.py", line 75, in <module>
    subprocess.check_call(['../target/release/wrench', '--no-scissor', '-h'] + sys.argv[1:])
  File "/usr/lib/python2.7/subprocess.py", line 541, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['../target/release/wrench', '--no-scissor', '-h', 'reftest']' returned non-zero exit status -6
[taskcluster 2018-04-27 17:07:05.232Z] === Task Finished ===

https://tools.taskcluster.net/groups/CfiHc6UoR4eB0t4h5N45aw/tasks/CfiHc6UoR4eB0t4h5N45aw/runs/0/logs/public%2Flogs%2Flive.log

@glennw
Copy link
Member

glennw commented May 1, 2018

Getting close!

REFTEST INFO | 276 passing, 2 failing
Reftests with unexpected results:
	reftests/filters/blend-clipped.yaml == reftests/filters/blend-clipped.png
	reftests/image/tile-with-spacing.yaml == reftests/image/tile-with-spacing-ref.yaml
Traceback (most recent call last):

@nical
Copy link
Contributor Author

nical commented May 2, 2018

@nical
Copy link
Contributor Author

nical commented May 3, 2018

@nical
Copy link
Contributor Author

nical commented May 4, 2018

The remaining 3 failing reftests all go through the the tile decomposition code path with a large scroll offset applied. All of them render nothing on screen (they do produce tiles but I suspect these tiles are off-screen). The tests pass if I remove the scroll offset in the tests and the references which seems to imply I got this piece wrong somehow:

In prim_store.rs:

fn compute_conservative_visible_rect(
    prim_run_context: &PrimitiveRunContext,
    frame_context: &FrameBuildingContext,
    local_clip_rect: &LayoutRect,
) -> LayoutRect {
    let world_screen_rect = prim_run_context
        .clip_chain.combined_outer_screen_rect
        .to_f32() / frame_context.device_pixel_scale;

    if let Some(layer_screen_rect) = prim_run_context
        .scroll_node
        .world_content_transform
        .unapply(&world_screen_rect) {

        return local_clip_rect.intersection(&layer_screen_rect).unwrap_or(LayoutRect::zero());
    }

    *local_clip_rect
}

If anyone understands webrender's coordinate systems somewhat and has a vague idea of what I missed, please let me know.

@jrmuizel
Copy link
Collaborator

jrmuizel commented May 8, 2018

It looks like it was actually for_each_tile() that was wrong.

@jrmuizel
Copy link
Collaborator

jrmuizel commented May 8, 2018

This fixes all the failing tests:

diff --git a/gfx/webrender/src/image.rs b/gfx/webrender/src/image.rs
index 181cb566fd1e..d1f3811b3631 100644
--- a/gfx/webrender/src/image.rs
+++ b/gfx/webrender/src/image.rs
@@ -195,22 +195,22 @@ pub fn for_each_tile(
         if y == y_count - 1 {
             row_flags |= EdgeAaSegmentMask::BOTTOM;
         }
 
         for x in 0..x_count {
             let tile_offset = t0 + vec2(x, y);
 
 
             let mut segment_rect = LayoutRect {
                 origin: LayoutPoint::new(
-                    x0 + tile_offset.x as f32 * layer_tile_size.width,
-                    y0 + tile_offset.y as f32 * layer_tile_size.height,
+                    prim_rect.origin.x + tile_offset.x as f32 * layer_tile_size.width,
+                    prim_rect.origin.y + tile_offset.y as f32 * layer_tile_size.height,
                 ),
                 size: layer_tile_size,
             };
 
             if tile_offset.x == leftover_offset.x {
                 segment_rect.size.width = leftover_layer_size.width;
             }
 
             if tile_offset.y == leftover_offset.y {
                 segment_rect.size.height = leftover_layer_size.height;

@kvark
Copy link
Member

kvark commented May 8, 2018

Also, the compute_conservative_visible_rect function shouldn't be introduced - instead we should re-use the clip_chain_rect already computed in prepare_prim_runs.

@jrmuizel
Copy link
Collaborator

jrmuizel commented May 8, 2018

Here's a version of this that's been rebased and should pass the tests:
https://github.com/jrmuizel/webrender/tree/tile-decomposition

@jrmuizel
Copy link
Collaborator

jrmuizel commented May 8, 2018

I'm going to move to a different pull request for now.

bors-servo pushed a commit that referenced this pull request May 10, 2018
Decompose tiled images during frame building (v4)

This is a continuation of #2704

<!-- 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/2742)
<!-- Reviewable:end -->
@gw3583
Copy link
Contributor

gw3583 commented May 11, 2018

Closing since tiled images v4 PR landed.

@gw3583 gw3583 closed this May 11, 2018
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.

5 participants