Handle big size image #1564

JerryShih · 2017-08-09T18:31:57Z

This patch try to handle the #1405.
If we hit the maximum texture size, turn to use a 1x1 rgba(255,255,255,255) dummy texture.

This change is

glennw · 2017-08-09T21:19:24Z

I'm working on a large refactoring of the texture cache at the moment - if possible, I'd like to hold off on merging this until that's done. Is that going to be OK? (I would expect it to be done early next week). Handling this case should be much simpler with the changes I'm working on, too.

bors-servo · 2017-08-10T00:34:19Z

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

JerryShih · 2017-08-11T02:59:34Z

@glennw
That's fine. If the refactor is done, please show the note in this thread.

glennw · 2017-08-17T03:17:10Z

@JerryShih The texture cache refactor has landed now. Instead of trying to use a dummy texture for this case, could we just not add the item to any batches when we find an invalid texture?

JerryShih · 2017-08-17T06:10:09Z

@JerryShih The texture cache refactor has landed now. Instead of trying to use a dummy texture for this case, could we just not add the item to any batches when we find an invalid texture?

@glennw
That could be another approach. I think I will still use a special id for the big size allocation(or failed allocation). Then, we could skip the item with that id in batch list or draw with a special color texture for debugging(with a 1x1 black dummy texture).

glennw · 2017-08-17T06:37:46Z

I think the simplest approach will be to early exit from request_image in resource cache, before even getting to texture cache. Then, get_cached_image could just return SourceTexture::Invalid. But let's see how this looks after rebasing to the new texture cache and decide from there :)

JerryShih · 2017-08-17T09:53:04Z

@glennw r?

And I need the help for the wrench reftest. I got the different result from the standalone yaml replay and the reftest result.

standalone:
I could see a black Rect. That's what I want.
cargo run show reftests/image/very-big-tile-size.yaml

reftest:
I got a gray Rect.
./headless.py reftest reftests/image/very-big-tile-size.yaml

I don't know the difference between them.

bors-servo · 2017-08-17T17:37:25Z

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

JerryShih · 2017-08-18T03:37:31Z

@glennw r? again.
rebase.
I still hit the reftest problem with wrench. Maybe you have some hints for this problem.

glennw · 2017-08-18T05:05:11Z

@JerryShih I'm not really sure why it would be different between standalone and reftest. Perhaps the wrong texture is getting bound or something like that?

I wrote up a prototype of what I was discussing above here glennw@9ca1214

I think if we use a solution like this, where it doesn't involve the texture cache at all, the code is quite a bit simpler. And when we want to display the bad texture, we could do that as a follow up in the renderer (handle the case where a texture resolves to SourceTexture::Invalid during batching). What do you think?

JerryShih · 2017-08-18T05:31:45Z

@JerryShih I'm not really sure why it would be different between standalone and reftest. Perhaps the wrong texture is getting bound or something like that?

I wrote up a prototype of what I was discussing above here glennw/webrender@9ca1214

I think if we use a solution like this, where it doesn't involve the texture cache at all, the code is quite a bit simpler. And when we want to display the bad texture, we could do that as a follow up in the renderer (handle the case where a texture resolves to SourceTexture::Invalid during batching). What do you think?

Your prototype is much simpler than me. So, yours is a better solution. I add a comment in glennw/webrender@9ca1214. I will create another follow up if we need to show the skipped image.

glennw · 2017-08-18T06:31:09Z

@JerryShih Sounds good, thanks! Do you want me to follow up your comments and open a PR, or should I leave that for you?

JerryShih · 2017-08-18T06:32:23Z

@glennw
I would like to do that!

JerryShih · 2017-08-18T06:39:20Z

@glennw has a better solution. Close this pr.

JerryShih · 2017-09-05T09:15:51Z

@glennw @kvark r? again.
This version just skip the image request and return a invalid CacheItem for the big size image request.

kvark · 2017-09-06T01:18:35Z

webrender/src/resource_cache.rs

+                    // just early out and drop the image.
+                    if tile_size as u32 > self.texture_cache.max_texture_size() {
+                        warn!("Dropping image, tile size:{} is too big for hardware!", tile_size);
+                        return;


I'm a bit uncomfortable with these return statements as they are different from other return code paths from this method in that they are failures, while others are successes.
How about we return true/false (at least), and then also modify prepare_prim_for_render to return Option<Metadata> so that a primitive that failed to prepare can be discarded in time?

@kvark
I'm not familiar with the frame_builder::build process. The prepare_prim_for_render() is called by frame_builder::handle_primitive_run()[1]. And the handle_primitive_run() looks like to handle the mask and clip related thing. Are you thinking about skipping the render-task creation in handle_primitive_run() for the case of None Option return value?

[1]
https://dxr.mozilla.org/mozilla-central/rev/c959327c6b75cd4930a6ea087583c38b805e7524/gfx/webrender/src/frame_builder.rs#1996

The prepare_prim_for_render() is changed. The "Metadata" can't be remove now.

From the first comment:

I'm a bit uncomfortable with these return statements as they are different from other return code paths from this method in that they are failures, while others are successes.

We still have a case with a failure at [1] in request_image(). So, the new return statements are not just the new failed case. If they are still not looked good, I need to check the prepare_prim_for_render() for the "Metadata" removing.

[1]

webrender/webrender/src/resource_cache.rs

Line 553 in e11297a

None => {

kvark · 2017-09-06T01:49:25Z

webrender/src/resource_cache.rs

-                debug_assert!(texture_id == None ||
-                              texture_id == Some(cache_item.texture_id));
-                texture_id = Some(cache_item.texture_id);
+            if let Some(glyph) = glyph_key_cache.get(key) {


We need to handle the case of failure here. Does it make sense to return as usual if some of the glyphs failed to be obtained?

kvark · 2017-09-06T01:51:34Z

webrender/src/resource_cache.rs

+        //
+        // TODO(Jerry): add a debug option to fill the corresponding area for
+        // that invalid CacheItem.
+        image_info.map_or(CacheItem::invalid(), |image_info| {


we call get_glyphs when populating batches. Wouldn't it be simpler to just return an error here, so that a corresponding instance can be discarded?

We call get_glyphs at [1]. If there is any error inside get_glyphs()[2], the lambda expression at [3] will not be called(which will push the instance to draw list).

[1]

webrender/webrender/src/tiling.rs

Line 513 in e11297a

ctx.resource_cache.fetch_glyphs(

[2]
https://github.com/servo/webrender/pull/1564/files#diff-77cbdf7ba9ebae81feb38a64c21b8454R630
[3]

webrender/webrender/src/tiling.rs

Line 518 in e11297a

|texture_id, glyphs| {

bors-servo · 2017-09-12T03:27:48Z

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

glennw · 2017-10-02T21:08:14Z

@JerryShih @kvark What's the status of this?

JerryShih · 2017-10-03T06:10:01Z

@glennw I'm check an animation artifact problem with WR. I will go back to finish this soon.

JerryShih · 2017-10-11T09:29:02Z

webrender/src/tiling.rs

@@ -813,6 +817,11 @@ impl ClipBatcher {
                    ClipSource::Image(ref mask) => {
                        let cache_item =
                            resource_cache.get_cached_image(mask.image, ImageRendering::Auto, None);
+                        // Skip the invalid image mask.
+                        if cache_item.texture_id == SourceTexture::Invalid {


This image mask will be rendered at [1]. So, I think we should skip this image if the texture id is not valid.

[1]

webrender/webrender/src/renderer.rs

Line 2912 in e11297a

for (mask_texture_id, items) in target.clip_batcher.images.iter() {

JerryShih · 2017-10-11T09:47:14Z

@glennw @kvark r?

kvark

Thanks for updating the PR!
I think my only concern is that we still need the key to be present in the resources map, while turning value into Result.

kvark · 2017-10-11T14:12:46Z

webrender/src/resource_cache.rs

-        self.resources
-            .get(key)
-            .expect("Didn't find a cached resource with that ID!")
+    /// In some cases, we can't handle the resource request(e.g. the user


I think we should still differentiate between keys that didn't end up in the map for some other logical reasons, and keys that were just failing to be added. Perhaps, the resources map should contain Result<V> instead of V, so that we can handle those failures in a more explicit way?

JerryShih · 2017-10-12T12:53:07Z

webrender/src/prim_store.rs

@@ -1057,7 +1057,6 @@ impl PrimitiveStore {
        }
    }

-    /// Returns true if the bounding box needs to be updated.


This comment is not correct now.

JerryShih · 2017-10-12T12:53:34Z

webrender/src/resource_cache.rs

 pub struct ResourceClassCache<K, V> {
-    resources: FastHashMap<K, V>,
+    resources: FastHashMap<K, ResourceCacheCacheResult<V>>,


Use Result instead of V.

JerryShih · 2017-10-12T12:54:46Z

webrender/src/resource_cache.rs

+                    // just early out and drop the image.
+                    if tile_size as u32 > self.texture_cache.max_texture_size() {
+                        warn!("Dropping image, tile size:{} is too big for hardware!", tile_size);
+                        self.cached_images.insert(request, Err(ResourceClassCacheError::OverLimitSize));


If we hit the size limitation, just insert an Err() entry.

JerryShih · 2017-10-12T12:56:54Z

@kvark r?
Try to use Result in ResourceClassCache.

JerryShih · 2017-10-12T13:06:21Z

I haven't handled the Err() case for each get/get_mut() call, but I will just add some logs for these paths if we need to have these logs.

kvark

Thanks @JerryShih ! A few more things to fix before we proceed.

kvark · 2017-10-12T17:43:24Z

webrender/src/resource_cache.rs

+    OverLimitSize,
+}
+
+pub type ResourceCacheCacheResult<V> = Result<V, ResourceClassCacheError>;


perhaps you meant ResourceClassCacheResult?

kvark · 2017-10-12T17:45:03Z

webrender/src/resource_cache.rs

@@ -159,7 +174,7 @@ where
            .cloned()
            .collect::<Vec<_>>();
        for key in resources_to_destroy {
-            self.resources.remove(&key).unwrap();


why is this removed? I assume we get a warning about unused Result or Option

kvark · 2017-10-12T17:46:55Z

webrender/src/resource_cache.rs

+                    }
+                } else {
+                    // The image is too big for hardware texture size.
+                    if template.descriptor.width > self.texture_cache.max_texture_size() ||


should probably merge this block with the one above, using let side_size = image.tiling.unwrap_or(cmp::max(template.descriptor.width, template.descriptor.height));

kvark · 2017-10-12T17:47:34Z

webrender/src/resource_cache.rs

                // If this image exists in the texture cache, *and* the epoch
                // in the cache matches that of the template, then it is
                // valid to use as-is.
                let (entry, needs_update) = match self.cached_images.entry(request) {
                    Occupied(entry) => {
-                        let needs_update = entry.get().epoch != template.epoch;
+                        let needs_update = entry.get().as_ref().ok().unwrap().epoch != template.epoch;


no need for ok() here

kvark · 2017-10-12T17:48:02Z

webrender/src/resource_cache.rs

                        true,
                    ),
                };

                let needs_upload = self.texture_cache
-                    .request(&mut entry.texture_cache_handle, gpu_cache);
+                    .request(&mut entry.as_mut().ok().unwrap().texture_cache_handle, gpu_cache);


same here - no need for ok()

kvark · 2017-10-12T17:52:59Z

webrender/src/resource_cache.rs

@@ -656,7 +692,15 @@ impl ResourceCache {
            tile,
        };
        let image_info = &self.cached_images.get(&key);


would probably be cleaner with a simple match statement

kvark · 2017-10-12T17:53:35Z

webrender/src/resource_cache.rs

@@ -816,7 +860,7 @@ impl ResourceCache {
                image_template.descriptor.clone()
            };

-            let entry = self.cached_images.get_mut(&request).unwrap();
+            let entry = self.cached_images.get_mut(&request).as_mut().ok().unwrap();


no need for ok()

kvark · 2017-10-12T17:53:56Z

webrender/src/tiling.rs

@@ -516,6 +517,8 @@ impl AlphaRenderItem {
                            glyph_fetch_buffer,
                            gpu_cache,
                            |texture_id, glyphs| {
+                                debug_assert!(texture_id != SourceTexture::Invalid);


nit: debug_assert_ne!()

kvark · 2017-10-12T17:55:06Z

webrender/src/resource_cache.rs

+        //
+        // TODO(Jerry): add a debug option to fill the corresponding area for
+        // that invalid CacheItem.
+        image_info.as_ref().ok().map_or(CacheItem::invalid(), |image_info| {


let's just return Result<CacheItem, _> from this function and match it on the other side, there is no need for nullable CacheItem semantic (that invalid() does here)

JerryShih · 2017-10-13T08:12:43Z

webrender/src/resource_cache.rs

@@ -174,7 +167,7 @@ where
            .cloned()
            .collect::<Vec<_>>();
        for key in resources_to_destroy {
-            self.resources.remove(&key);
+            self.resources.remove(&key).unwrap();


I revert the change, but the CI test is failed.

error: unused std::result::Result which must be used
--> src/resource_cache.rs:170:13
|
170 | self.resources.remove(&key).unwrap();
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: -D unused-must-use implied by -D warnings

JerryShih · 2017-10-13T08:13:48Z

@kvark r? for d68cf51

kvark · 2017-10-13T15:46:29Z

Thank you for addressing my comments!

self.resources.remove(&key).unwrap();

Let's do let _ = self.resources.remove(&key).unwrap();

Please squash the commits together, and we can merge once CI is happy.

…it image request. WR can't handle the image request if it's bigger than the hardware limit. Please check texture_cache::max_texture_size(). Skip the image request for a big size image in resource_cache::request_image(). Return an Err() for a big size image in resource_cache::get_cached_image().

JerryShih · 2017-10-16T06:06:39Z

@kvark
Update for CI "self.resources.remove(&key).unwrap();" failed, and squash the patch set.

kvark · 2017-10-16T18:45:18Z

@JerryShih awesome, ! 👍
@bors-servo r+

bors-servo · 2017-10-16T18:45:19Z

📌 Commit f903161 has been approved by kvark

bors-servo · 2017-10-16T18:45:23Z

⌛ Testing commit f903161 with merge e4cca26...

@nical

Handle big size image @nical @glennw @kvark @sotaroikeda r? This patch try to handle the #1405. If we hit the maximum texture size, turn to use a 1x1 rgba(255,255,255,255) dummy texture.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1564)

bors-servo · 2017-10-16T20:56:30Z

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

kvark added the status: blocked label Aug 10, 2017

JerryShih force-pushed the handle-big-size-image branch 2 times, most recently from 56d88b5 to d58c2ff Compare August 17, 2017 09:43

JerryShih force-pushed the handle-big-size-image branch 3 times, most recently from 45c30a8 to 09a5c21 Compare August 18, 2017 03:35

JerryShih closed this Aug 18, 2017

JerryShih reopened this Sep 5, 2017

JerryShih force-pushed the handle-big-size-image branch from 09a5c21 to 152401b Compare September 5, 2017 09:14

kvark reviewed Sep 6, 2017

View reviewed changes

JerryShih force-pushed the handle-big-size-image branch from 152401b to 58a136f Compare October 11, 2017 09:05

JerryShih commented Oct 11, 2017

View reviewed changes

kvark reviewed Oct 11, 2017

View reviewed changes

JerryShih force-pushed the handle-big-size-image branch from 58a136f to 867da17 Compare October 12, 2017 12:50

JerryShih commented Oct 12, 2017

View reviewed changes

kvark suggested changes Oct 12, 2017

View reviewed changes

JerryShih force-pushed the handle-big-size-image branch from 867da17 to d68cf51 Compare October 13, 2017 08:06

JerryShih commented Oct 13, 2017

View reviewed changes

kvark approved these changes Oct 13, 2017

View reviewed changes

JerryShih force-pushed the handle-big-size-image branch from d68cf51 to f903161 Compare October 16, 2017 06:04

JerryShih and others added 2 commits October 16, 2017 14:06

Reftest for the over-limit-size texture.

3a98b0e

Remove the old comment for prepare_prim_for_render().

f903161

bors-servo merged commit f903161 into servo:master Oct 16, 2017

glennw mentioned this pull request Oct 23, 2017

Implement the fallback path for the big size image #1405

Closed

Handle big size image #1564

Handle big size image #1564

Conversation

JerryShih commented Aug 9, 2017 • edited by larsbergstrom Loading

glennw commented Aug 9, 2017

bors-servo commented Aug 10, 2017

JerryShih commented Aug 11, 2017

glennw commented Aug 17, 2017

JerryShih commented Aug 17, 2017

glennw commented Aug 17, 2017

JerryShih commented Aug 17, 2017

bors-servo commented Aug 17, 2017

JerryShih commented Aug 18, 2017

glennw commented Aug 18, 2017

JerryShih commented Aug 18, 2017

glennw commented Aug 18, 2017

JerryShih commented Aug 18, 2017

JerryShih commented Aug 18, 2017

JerryShih commented Sep 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Sep 12, 2017

glennw commented Oct 2, 2017

JerryShih commented Oct 3, 2017

Choose a reason for hiding this comment

JerryShih commented Oct 11, 2017

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JerryShih commented Oct 12, 2017

JerryShih commented Oct 12, 2017

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JerryShih commented Oct 13, 2017

kvark commented Oct 13, 2017

JerryShih commented Oct 16, 2017

kvark commented Oct 16, 2017

bors-servo commented Oct 16, 2017

bors-servo commented Oct 16, 2017

bors-servo commented Oct 16, 2017

JerryShih commented Aug 9, 2017 •

edited by larsbergstrom

Loading