Implement repetition and spacing for tiled images. #972

nical · 2017-03-08T15:58:48Z

Followup of PR #897. This should cover the all of the remaining cases when rendering tiled images (image display item specifically).
If the image is tiled and repeated along the same dimension it is decomposed into a primitive for each repetition, taking spacing into account. These primitives are then split in a per tile basis.

This change is

nical · 2017-03-08T16:11:28Z

r? @kvark

nical · 2017-03-09T13:59:08Z

Sadly the test tile-repeat-prim-or-decompose.yaml which I just added fails on travis (but not on my computer) due to rounding differences. It would be great to be able to fuzz tests (my attempt at using gecko's fuzzing syntax didn't work).

nical · 2017-03-09T15:13:22Z

I just added some basic support for fuzzy reftests in wrench using the same syntax as gecko. I'll move the commit into a separate PR but just pushing it here temporarily to check that it fixes the test.

kvark · 2017-03-09T19:36:53Z

due to rounding differences

Did you find out what exactly those differences are?

glennw · 2017-03-09T19:40:35Z

Specifically - the reftests are run with OSMesa, so if you're running the reftests locally with the headless.py script, they should match exactly - and if they don't we should find out why. I'm not against adding fuzzy tests (we will need them), but in this case it'd be good to know what the cause is - since I've come across several super subtle bugs recently that caused fuzzy results.

kvark · 2017-03-09T20:20:45Z

webrender/src/frame.rs

+
+        // Decompose each vertical repetition into rows.
+        let layout_stride = info.stretch_size.height + info.tile_spacing.height;
+        let num_repetitions = (item_rect.size.width / layout_stride).ceil() as u32;


shouldn't this be item_rect.size.height / layout_stride?

kvark · 2017-03-09T20:20:45Z

webrender/src/frame.rs

+                item_rect.size.width,
+                info.stretch_size.height
+            ).intersection(&item_rect) {
+                self.decompose_image_row(scroll_layer_id, context, &row_rect, item_clip, info, image_size, tile_size);


it's a bit weird to see decompose_image_row called from inside of this loop, since it's supposed to handle image repetitions (which is not the case for this loop, is it?)

It is (or maybe I misunderstand your question). decompose_image_row handles horizontal repetition (of a given row) and this loop takes care of the vertical repetition by splitting the primitive into rows.

kvark · 2017-03-09T20:20:45Z

webrender/src/frame.rs

+        let layout_stride = info.stretch_size.width + info.tile_spacing.width;
+        let num_repetitions = (item_rect.size.width / layout_stride).ceil() as u32;
+        for i in 0..num_repetitions {
+            if let Some(img_rect) = rect(


rename to column_rect for consistency?

I find column_rect a bit misleading in the sense that the rect doesn't describe an entire column of the image but just a single repetition within the current row. Granted img_rect isn't great either since it is not the rect for the entire image.

nical · 2017-03-10T13:06:00Z

For the fuzzy test, if you want to have a look the easiest is to copy paste the entire log in reftest analyzer. The max difference is 1 for a lot of pixels and you can see bands of differing pixels in lines perpendicular to the stretching direction.
It and looks very much like the sampling interpolation rounding differently. The "correct" value for these pixels isn't strictly defined, it's really up to the precision/rounding behavior of the sampler.
I'm currently battling with the headless.py locally (for some reason I'm getting a max texture size of zero which causes a panic as soon as we try to add something to the texture caches).

bors-servo · 2017-03-10T15:38:28Z

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


        Implement repetition and spacing for tiled images.


        Add two image tiling tests.


        Fuzz test tile-repeat-prim-or-decompose.yaml

kvark

Thanks for moving the fuzzing out to a separate PR.

I'm currently battling with the headless.py locally

I guess that's the last point of concern then. Please confirm that your headless OSmesa tests match the Travis ones.

nical · 2017-03-10T16:52:20Z

Yep, OSMesa gives me the same result on my desktop as travis (unsurprisingly).

kvark · 2017-03-10T18:03:45Z

Great, thanks!
@bors-servo r+

bors-servo · 2017-03-10T18:03:46Z

📌 Commit 4ebb047 has been approved by kvark

bors-servo · 2017-03-10T18:03:49Z

⌛ Testing commit 4ebb047 with merge 461f188...


        Auto merge of #972 - nical:image-repeat-spacing, r=kvark

Implement repetition and spacing for tiled images. Followup of PR #897. This should cover the all of the remaining cases when rendering tiled images (image display item specifically). If the image is tiled and repeated along the same dimension it is decomposed into a primitive for each repetition, taking spacing into account. These primitives are then split in a per tile basis.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/972)

bors-servo · 2017-03-10T20:07:32Z

☀️ Test successful - status-travis
Approved by: kvark
Pushing 461f188 to master...

nical force-pushed the nical:image-repeat-spacing branch from 546f47e to 72d274b Mar 8, 2017

kvark requested changes Mar 9, 2017

View changes

nical force-pushed the nical:image-repeat-spacing branch from fcb3c39 to 9c8a125 Mar 10, 2017

nical added 3 commits Mar 8, 2017

Implement repetition and spacing for tiled images.

86aacbf

Add two image tiling tests.

7776927

Fuzz test tile-repeat-prim-or-decompose.yaml

Loading status checks…

4ebb047

nical force-pushed the nical:image-repeat-spacing branch from 9c8a125 to 4ebb047 Mar 10, 2017

kvark approved these changes Mar 10, 2017

View changes

bors-servo merged commit 4ebb047 into servo:master Mar 10, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / webrender

Implement repetition and spacing for tiled images. #972

Implement repetition and spacing for tiled images. #972

nical commented Mar 8, 2017 •

edited by larsbergstrom

nical commented Mar 8, 2017

nical commented Mar 9, 2017

nical commented Mar 9, 2017

kvark commented Mar 9, 2017

glennw commented Mar 9, 2017 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nical commented Mar 10, 2017

bors-servo commented Mar 10, 2017

kvark left a comment

nical commented Mar 10, 2017

kvark commented Mar 10, 2017

bors-servo commented Mar 10, 2017

bors-servo commented Mar 10, 2017

bors-servo commented Mar 10, 2017

servo / webrender

Join GitHub today

Implement repetition and spacing for tiled images. #972

Implement repetition and spacing for tiled images. #972

Conversation

nical commented Mar 8, 2017 • edited by larsbergstrom

nical commented Mar 8, 2017

nical commented Mar 9, 2017

nical commented Mar 9, 2017

kvark commented Mar 9, 2017

glennw commented Mar 9, 2017 • edited

This comment has been minimized.

kvark Mar 9, 2017

This comment has been minimized.

nical Mar 10, 2017

This comment has been minimized.

kvark Mar 9, 2017

This comment has been minimized.

nical Mar 10, 2017

This comment has been minimized.

kvark Mar 9, 2017

This comment has been minimized.

nical Mar 10, 2017

nical commented Mar 10, 2017

bors-servo commented Mar 10, 2017

kvark left a comment

nical commented Mar 10, 2017

kvark commented Mar 10, 2017

bors-servo commented Mar 10, 2017

bors-servo commented Mar 10, 2017

bors-servo commented Mar 10, 2017

nical commented Mar 8, 2017 •

edited by larsbergstrom

glennw commented Mar 9, 2017 •

edited