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.

Sign up for GitHub

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

Jump to bottom

Remove ResourceList structure and usage. #523

Merged

bors-servo merged 2 commits into servo:master from glennw:resource-list-2

Nov 4, 2016

Member

glennw commented Nov 3, 2016 •

edited by larsbergstrom

Loading

Previously, a resource list would be built at the start of the
frame. This would allow glyphs missing from the texture cache
to be rasterized.

However, sometimes (e.g. with subpixel antialiasing) it's not possible
to know right at the start of the frame exactly what needs to be
rasterized yet.

Instead, we now allow the resource cache to add requests during
the prepare_prim_for_render() stage. Primitives that request
resources are marked as needing a resolve operation. This allows
primitives that request resources to update their UV coords etc
before batching occurs (since texture IDs for resources must be
known before batching can occur).

This also lays most of the groundwork for running the resource
cache rasterizer as a separate thread in the future. This will be
used to ensure that if too many glyphs are requested in one frame,
WR can continue running without blocking, using older glyphs and
then re-render the scene when newer, high resolution glyphs are
available.

This change is

Member Author

glennw commented Nov 3, 2016

r? @pcwalton (also @kvark particularly for the clip-related changes).

Member Author

glennw commented Nov 3, 2016

Note: Let's get #512 reviewed and merged before landing this (and I'll rebase mine on top of it). This can still be reviewed now though :)

pcwalton reviewed

View reviewed changes

webrender/src/prim_store.rs

+                              if let Some(address) = metadata.clip_index {
+                                  let clip = self.gpu_data32.get_slice_mut(address, 6);
+                                  let old = clip[5].data; //TODO: avoid retaining the screen rectangle
+                                  clip[5] = GpuBlock32::from(ImageMaskInfo {

Contributor

pcwalton Nov 3, 2016

At some point I'd like to factor all these indices out into constants. (You don't have to do this now)

Maybe we could even make macros to make this stuff easier.

webrender/src/prim_store.rs

+                                                                             &text.glyph_indices, |index, uv0, uv1| {
+                                      let dest_glyph = &mut dest_glyphs[index];
+                                      let dest: &mut GlyphPrimitive = unsafe {
+                                          mem::transmute(dest_glyph)

Contributor

pcwalton Nov 3, 2016

What is this transmute for?

Member Author

glennw Nov 3, 2016

The existing data is a GpuBlock32 - I need to access it as the original type, in order to modify some fields of it. Is there a better way to express this (the From trait is used to create the GpuBlock32 initially)?

kvark approved these changes

View reviewed changes

Member

kvark left a comment

got a few notes, otherwise looks good!

webrender/src/prim_store.rs

-                                                        Size2D::new(old[6], old[7])),
-                              });
-                          }
+                          resource_cache.request_image(mask_key, ImageRendering::Auto);

Member

kvark Nov 3, 2016

As a next step, I suppose moving the request_* logic out of ResourceCache into a separate type/interface, to be used here.

Member Author

glennw Nov 3, 2016

Yup, agreed.

webrender/src/resource_cache.rs

               thread_local!(pub static FONT_CONTEXT: RefCell<FontContext> = RefCell::new(FontContext::new()));
+              pub struct CacheItem {
+                  pub texture_id: TextureId,
+                  pub uv0: Point2D<f32>,

Member

kvark Nov 3, 2016

would be great to specify that this is in texels, not actual normalized UVs, either by better naming, typing, or just comments

Member Author

glennw Nov 3, 2016

Added comment explaining that it is in texels, and why.

webrender/src/resource_cache.rs

+              }
+              enum ResourceRequest {
+                  Image(ImageResourceRequest),

Member

kvark Nov 3, 2016

I think it would be clearer to just have Image(ImageKey, ImageRendeing), since ImageResourceRequest is not used in any other context anyway

Member Author

glennw Nov 3, 2016

Fixed

webrender/src/resource_cache.rs

               pub struct ResourceCache {
                   cached_glyphs: ResourceClassCache<GlyphKey, Option<TextureCacheItemId>>,
                   cached_images: ResourceClassCache<(ImageKey, ImageRendering), CachedImageInfo>,
                   // TODO(pcwalton): Figure out the lifecycle of these.
-                  webgl_textures: HashMap<WebGLContextId, TextureId, BuildHasherDefault<FnvHasher>>,
+                  webgl_textures: HashMap<WebGLContextId, (TextureId, Size2D<i32>), BuildHasherDefault<FnvHasher>>,

Member

kvark Nov 3, 2016

This asks for a new type

Member Author

glennw Nov 3, 2016

Fixed

webrender/src/resource_cache.rs

+                  pub fn request_image(&mut self,
+                                       key: ImageKey,
+                                       rendering: ImageRendering) {
+                      debug_assert!(self.state == State::AddResources);

Member

kvark Nov 3, 2016

I assume this will be converted to a type system rule/restriction later on

webrender/src/resource_cache.rs

+                              let uv1 = Point2D::new(cache_item.pixel_rect.bottom_right.x as f32,
+                                                     cache_item.pixel_rect.bottom_right.y as f32);
+                              f(loop_index, uv0, uv1);
+                              texture_id = cache_item.texture_id;

Member

kvark Nov 3, 2016

is this texture_id guaranteed to be the same for all glyphs?

Member Author

glennw Nov 3, 2016

Yes.

webrender/src/tiling.rs

                   layer_store: &'a Vec<StackingContext>,
                   prim_store: &'a PrimitiveStore,
                   render_task_id_counter: AtomicUsize,
               }
+              struct RenderTargetContext<'a> {
+                  layer_store: &'a Vec<StackingContext>,

Member

kvark Nov 3, 2016

Any reason not to use &'a [StackingContext]? I think that would reduce the indirections on access

Member Author

glennw Nov 3, 2016

Fixed

webrender/src/tiling.rs

-                                          color: color,
+                          // Build list of passes, target allocs that each tile needs.
+                          for screen_tile in screen_tiles {
+                              let rect = screen_tile.rect;        // TODO(gw): Remove clone here

Member

kvark Nov 3, 2016

where is the clone?

Member Author

glennw Nov 3, 2016

Removed comment

Member Author

glennw commented Nov 3, 2016

@pcwalton @kvark OK, addressed those review comments. I rebased kvark's PR on top of this locally, and the conflicts looked straightforward. So it probably doesn't matter which one lands first.

Contributor

bors-servo commented Nov 3, 2016

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


          Remove ResourceList structure and usage.

bb53fab

Previously, a resource list would be built at the start of the
frame. This would allow glyphs missing from the texture cache
to be rasterized.

However, sometimes (e.g. with subpixel antialiasing) it's not possible
to know right at the start of the frame exactly what needs to be
rasterized yet.

Instead, we now allow the resource cache to add requests during
the prepare_prim_for_render() stage. Primitives that request
resources are marked as needing a resolve operation. This allows
primitives that request resources to update their UV coords etc
before batching occurs (since texture IDs for resources must be
known before batching can occur).

This also lays most of the groundwork for running the resource
cache rasterizer as a separate thread in the future. This will be
used to ensure that if too many glyphs are requested in one frame,
WR can continue running without blocking, using older glyphs and
then re-render the scene when newer, high resolution glyphs are
available.

Member

kvark commented Nov 3, 2016

In that case, you may add an assert to enforce this

On Nov 3, 2016, at 18:33, Glenn Watson [email protected] wrote:

@glennw commented on this pull request.

In webrender/src/resource_cache.rs:

   let mut glyph_key = GlyphKey::new(font_key,

                                     size,

                                     blur_radius,

0);

   let mut texture_id = TextureId::invalid();

   for (loop_index, glyph_index) in glyph_indices.iter().enumerate() {

```
       glyph_key.index = *glyph_index;
```

       let image_id = self.cached_glyphs.get(&glyph_key, self.current_frame_id);

       let cache_item = image_id.map(|image_id| self.texture_cache.get(image_id));

       if let Some(cache_item) = cache_item {

           let uv0 = Point2D::new(cache_item.pixel_rect.top_left.x as f32,

                                  cache_item.pixel_rect.top_left.y as f32);

           let uv1 = Point2D::new(cache_item.pixel_rect.bottom_right.x as f32,

                                  cache_item.pixel_rect.bottom_right.y as f32);

```
           f(loop_index, uv0, uv1);
```

           texture_id = cache_item.texture_id;

Yes.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.


          Rebase and address review comments.

17a3629

glennw force-pushed the resource-list-2 branch from e27cb5c to 17a3629 Compare

November 4, 2016 00:00

Contributor

pcwalton commented Nov 4, 2016

@bors-servo: r+

Contributor

bors-servo commented Nov 4, 2016

📌 Commit 17a3629 has been approved by pcwalton

Contributor

bors-servo commented Nov 4, 2016

⌛ Testing commit 17a3629 with merge e7a66c5...

bors-servo pushed a commit that referenced this pull request


          Auto merge of #523 - glennw:resource-list-2, r=pcwalton

e7a66c5

Remove ResourceList structure and usage.

Previously, a resource list would be built at the start of the
frame. This would allow glyphs missing from the texture cache
to be rasterized.

However, sometimes (e.g. with subpixel antialiasing) it's not possible
to know right at the start of the frame exactly what needs to be
rasterized yet.

Instead, we now allow the resource cache to add requests during
the prepare_prim_for_render() stage. Primitives that request
resources are marked as needing a resolve operation. This allows
primitives that request resources to update their UV coords etc
before batching occurs (since texture IDs for resources must be
known before batching can occur).

This also lays most of the groundwork for running the resource
cache rasterizer as a separate thread in the future. This will be
used to ensure that if too many glyphs are requested in one frame,
WR can continue running without blocking, using older glyphs and
then re-render the scene when newer, high resolution glyphs are
available.

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

bors-servo mentioned this pull request

Add API for tweaking debug options. #526

Merged

Contributor

bors-servo commented Nov 4, 2016

☀️ Test successful - status-travis

bors-servo merged commit 17a3629 into servo:master

bors-servo mentioned this pull request

Major refactoring of how clips are processed down the pipeline #512

Merged

glennw deleted the resource-list-2 branch

December 12, 2016 20:42

glennw pushed a commit to glennw/saltfs that referenced this pull request


          [Proposal] Give @kvark reviewer privileges.

ce4e2e3

kvark has make several important contributions to webrender,
both as a reviewer and committer. He also has significant
Rust experience previously as the author of gfx-rs.

Some examples of recent reviews:

servo/webrender#546
servo/webrender#523
servo/webrender#561
servo/webrender#554

glennw mentioned this pull request

[Proposal] Give @kvark reviewer privileges. servo/saltfs#551

Merged

bors-servo pushed a commit to servo/saltfs that referenced this pull request


          Auto merge of #551 - glennw:kvark-reviewer, r=larsbergstrom

1724ff3

[Proposal] Give @kvark reviewer privileges.

kvark has make several important contributions to webrender,
both as a reviewer and committer. He also has significant
Rust experience previously as the author of gfx-rs.

Some examples of recent reviews:

servo/webrender#546
servo/webrender#523
servo/webrender#561
servo/webrender#554

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/551)
<!-- Reviewable:end -->

Coder206 pushed a commit to Coder206/saltfs that referenced this pull request


          [Proposal] Give @kvark reviewer privileges.

7d2f864

kvark has make several important contributions to webrender,
both as a reviewer and committer. He also has significant
Rust experience previously as the author of gfx-rs.

Some examples of recent reviews:

servo/webrender#546
servo/webrender#523
servo/webrender#561
servo/webrender#554

choudhary001 pushed a commit to choudhary001/saltfs that referenced this pull request


          [Proposal] Give @kvark reviewer privileges.

b8e4dc6

kvark has make several important contributions to webrender,
both as a reviewer and committer. He also has significant
Rust experience previously as the author of gfx-rs.

Some examples of recent reviews:

servo/webrender#546
servo/webrender#523
servo/webrender#561
servo/webrender#554

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet