Allow rasterizing blobs on the scene builder thread

[nical]

The general idea is to move the rasterization of blob images that we think will be rendered soon to the scene builder thread so that it can happen asynchronously and not block scrolling.
The current heuristic for what "we think will be rendered soon" is to rasterize anything that is in the bounds of the display list. This is simple but will typically rasterize more than necessary when off-screen blobs are animated. We can look into tweaking this as a followup.

---

This change is 

[bors-servo]

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

[kvark]

I looked at it briefly, comments below

---

Review status: 0 of 7 files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

webrender/examples/blob.rs, line 224 at r1 (raw file):

        &mut self,
        _services: &api::BlobImageResources,
        requests: Vec<(api::BlobImageRequest, api::BlobImageDescriptor, Option<api::DeviceUintRect>)>,

given that the same trio appears in BlobImageRenderer::request signature , they deserve a struct home (unless you want to remove that other use eventually)

---

webrender/examples/blob.rs, line 226 at r1 (raw file):

        requests: Vec<(api::BlobImageRequest, api::BlobImageDescriptor, Option<api::DeviceUintRect>)>,
    ) -> Option<Box<api::BlobSceneBuilderRequest>> {
        Some(Box::new(SceneBuilderRequest {

would using an associated type be more appropriate here instead of a trait object?

---

webrender/examples/blob.rs, line 246 at r1 (raw file):

        let requests = mem::replace(&mut self.requests, Vec::new());
        let workers = Arc::clone(&self.workers);
        workers.install(||{

can't we do self.workers.install() here?

---

webrender/src/render_backend.rs, line 236 at r1 (raw file):

        document_ops: &DocumentOps,
        document_id: DocumentId,
        resource_cache: &mut ResourceCache,

this is a little concerning to me. Semantically, this is no longer just forwarding the transaction but actually starting the work on it (by rasterizing blobs)

---

webrender/src/resource_cache.rs, line 429 at r1 (raw file):

    ) {
        let mut new_updates = Vec::with_capacity(updates.len());
        for update in mem::replace(updates, Vec::new()) {

did you try going with retain() instead of doing the replace dance here?

---

webrender/src/resource_cache.rs, line 433 at r1 (raw file):

                ResourceUpdate::AddImage(ref img) => {
                    if let ImageData::Blob(ref blob_data) = img.data {
                        let tiling = None; // TODO: tiled blob images.

assert_eq!(img.tiling, None)?

---

Comments from Reviewable

[nical]

Review status: 0 of 7 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

webrender/examples/blob.rs, line 224 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

given that the same trio appears in BlobImageRenderer::request signature , they deserve a struct home (unless you want to remove that other use eventually)

I am not sure what the actual interface will be by the time this merges (do we pass a rect decide which tiles to render or do we pass tiles explicitly, etc), so this is the simple version, but It'll be a struct by the time this PR is finished.

---

webrender/examples/blob.rs, line 226 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would using an associated type be more appropriate here instead of a trait object?

I don't use associated types very often but my understanding is that the actual type needs to be known at compile time which wouldn't work with BlobImageRenderer and its request object that are selected at runtime.

---

webrender/examples/blob.rs, line 246 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can't we do self.workers.install() here?

Yeah that looks like a remnant of a previous version of the code where this was needed.

---

webrender/src/render_backend.rs, line 236 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

this is a little concerning to me. Semantically, this is no longer just forwarding the transaction but actually starting the work on it (by rasterizing blobs)

We don't necessarily start any work here. We just take the blob image renderer as &mut in order to let it do a bit of internal bookkeeping when creating the request object. We can hide that behind a Cell if you feel strongly about keeping the interface non-&mut.

---

webrender/src/resource_cache.rs, line 429 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

did you try going with retain() instead of doing the replace dance here?

I'll give it a try, there's also the option of maintaining a separate array.

---

webrender/src/resource_cache.rs, line 433 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

assert_eq!(img.tiling, None)?

There's two kinds of tiling: whether the blob image is rendered into separate tiles and whether we upload it into separate tiles in the texture cache. When we support rendering blobs in tiles I don't see a reason to keep the two separate but in the mean time we need to support splitting a big non-tiled blob image into texture cache tiles.

---

Comments from Reviewable

[nical]

Review status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @kvark and @nical)

---

webrender/src/resource_cache.rs, line 429 at r1 (raw file):

Previously, nical (Nicolas Silva) wrote…

I'll give it a try, there's also the option of maintaining a separate array.

So I looked into this again and the reason I did the replace dance was the classic "gotta call a &mut method while iterating on member array" story.

---

Comments from Reviewable

[nical]

Review status: 0 of 7 files reviewed, 6 unresolved discussions (waiting on @kvark and @nical)

---

webrender/src/resource_cache.rs, line 429 at r1 (raw file):

Previously, nical (Nicolas Silva) wrote…

So I looked into this again and the reason I did the replace dance was the classic "gotta call a &mut method while iterating on member array" story.

Sorry, actually it was because the font variants need to move non-copy stuff out of the vector.

---

Comments from Reviewable

[nical]

Progress update

• I completely removed the non-async blob image code path which simplifies things a whole lot.
• I also removed the list of blobs to render from the API, and the render backends figures this out by looking at the list of added and updated blob images in the transaction.
• Plumbing the dirty rect through is implemented.
• Most rawtests pass (all except very_large_blob, see below).
• Gecko plumbing (to a point where things render but I am sure there is more stuff to do on that front).

Outstanding missing stuff

• no tiling yet and no partial rendering. This means an image the size of the whole thing is allocated which obviously can't land as is.

Assuming gecko already has the visible rect info for each blob image, I think that the simplest approach to fixing the missing stuff is to add something like Api::set_visible_rect(ImageKey, DeviceRect), attach this information to the blob image and have the render backend request tiles depending on that. If two display items read different parts of the image we'd need the visible rect to encompass the two areas, but I don't think this will be an issue in practice (hopefully).

As a second step, to avoid rasterizing the whole thing at once we could set a tighter visible rect when building the display list, let the culling pass update the visible rect and have it schedule async rasterization as things scroll. That way we would get back some of the benefits of lazy rasterization we had while remaining fully async.

[nical]

I implemented rasterizing blob tiles and the API to specify the visible rect of an image, which restricts blob rasterization to the tiles that intersect it.
That's enough to pass the very_large_blob rawtest, but there are still some missing pieces, the main one being that the rasterized tiles are currently never discarded and getting that right requires a bit of thinking.

On the gecko side I think I figured out how to compute the intersection of the blob image and the viewport, but I haven't integrated that in a way that I can test with the webrender parts yet.

[nical]

Things are starting to look good on the WebRender side. There were some complications around invalidation, though: Currently with sync blobs we lazily rasterize and block the render backend which means that if a blob is rendered we know it will be uploaded right away, and when we update it, we can decide to render only the dirty rect if the previous version of the blob is in the texture cache.
With async blobs it's more complicated. When we schedule rasterizing an updated blobs with a dirty rect, we could ask the texture cache whether the previous epoch of the blob is uploaded, but we'd also need the guarantee that the blob will stay in the cache no matter what until we are done rasterizing the updated blob and it is requested again. Otherwise we end up loosing the parts outside of the dirty region.

So for now I disabled sending dirty rects to the blob rasterizer implementation. In order to partially redraw blobs it is still possible to tile the image, and only damaged tiles will be rasterized (although full tiles will be rendered, so we might want to try and use small tile sizes).

I tested this implementation on http://digitalocean.com/ and it successfully preserves 60fps scrolling while the blob at the bottom of the page renders at ~14fps.

So that's great however requesting rasterization for all blob images in the display list means that we rasterize off-screen blobs while we used to only render visible ones, so saving APZ comes at the cost of quite a bit of extra work when an animation keeps invalidating an off-screen blob.

I have ideas about how to bring back fine-grained invalidation and rasterize less than the full content of the view port, but I am hopeful that we can first land something that is close to the current version of this PR and add these optimizations as followups. I'll finish the gecko integration and do a try push to see how much of a regression we get from doing this extra amount of rasterization.

[nical]

The PR is not ready to land because some stuff on the gecko side still block testing it thoroughly, but it's ready for reviews.

How this works

The blob image mechanism now has two traits:

• BlobImageHandler is roughly the equivalent of the previous BlobImageRenderer except that it doesn't do any rendering (it manages the state of the blob commands, and resources like fonts).
• AsyncBlobImageRasterizer is created by the handler and sent over to the scene builder thread. the async rasterizer is meant to be a snapshot of the state of blob image commands that can execute the commands if provided some requests.

When receiving a transaction, the render backend / resource cache look at the list of added and updated blob images in that transaction, collect the list of blob images and tiles that need to be rendered, create a rasterizer, and ship the two to the scene builder.
After building the scene the rasterizer gets handed the list of blob requests and does all of the rasterization, blocking the scene builder thread until the work is done.

When the scene building and rasterization is done, the render backend receives the rasterized blobs and stores them so that they are available when frame building needs them.

Because blob images can be huge, we don't always want to rasterize them entirely during scene building. To decide what should be rasterized, we rely on gecko giving us a hint through the added set_image_visible_area API. When the render backend receives that message it decides which tiles are going to be rasterized. This information is also used to decide which tiles to evict, so that we don't keep thousands of tiles if we scroll through a massive blob image. The idea is for the visible area to correspond to the size of the display list.

Sometimes, however, Gecko gets this visible area "wrong", or at least gives webrender a certain visible area but eventually webrender requests tiles during frame building that weren't in that area. I think that this is inevitable because the culling logic in gecko and webrender works very differently, so relying on them to match exactly is fragile at best.
So to work around this type of situation, keep around the async blob rasterizer that we sent to the scene builder, and store it in the resource cache when we swap the scene. This blob rasterizer represents the state of the blob commands at the time the transaction was built (and is potentially different from the state of the blob image handler). Frame building collects a list of blob images (or blob tiles) that are not already rasterized, and asks the current async blob rasterizer to rasterize them synchronously on the render backend. The hope is that this would happen rarely.

Another important detail is that for this to work, resources that are used by blob images (so currently only fonts), need to be in sync with the blobs. Fortunately, fonts are currently immutable so we mostly need to make sure they are added before the transaction is built and removed after the transaction is swapped. If blob images were to use images, then we'd have to either do the same for these images (and disallow updating them), or maintain the state of images before and after scene building like we effectively do for blob.

Shortcomings

I'll admit this whole mechanism isn't particularly exciting or beautiful. It's the simplest I could come up with that satisfies the transaction rules and need to b, etc. In its current state it has a few ugly corners:

• Because we eagerly rasterize blobs for the entire scene during scene building, we rasterize a lot more than we do before with lazy rasterization during frame building.
• Currently restricting blob rasterization to the dirty rect relies on guarantees that we get from rasterizing synchronously on the scene builder, namely: we rasterize only if we need to upload so we know it will be uploaded, and we know at the time of rasterization whether the thing is still on the GPU. Moving this to the scene builder we don't know whether the blob will be in the texture cache by the time we finish rasterizing and therefore we have to produce the whole image or whole tile.

Both of these things can be mitigated by incorporating the same kind of optimizations we do in gecko's layers tiling logic (copy-on-write tiles, checkerboarding, clever heuristics to decide which tiles are really important to rasterize depending on scroll speed, etc). But it's not clear to me how much effort should go into these optimizations, versus into using blob images less (certainly a mix of both). More importantly it'll be miraculous if this lands as is without making waves, so let's not make it even more complicated, prepare for some regressions and land this anyway, since this makes APZ a ton smoother even if at the expense extra CPU time spent during rasterization.

[Gankra]

Because we eagerly rasterize blobs for the entire scene during scene building, we rasterize a lot more than we do before with lazy rasterization during frame building.

Checking i understand what this means: ideally we would only rasterize blobs that are actually on screen, but with this design we need to rasterize all the tiles which might be APZ scrolled onto the screen with the current scene?

[nical]

Checking i understand what this means: ideally we would only rasterize blobs that are actually on screen, but with this design we need to rasterize all the tiles which might be APZ scrolled onto the screen with the current scene?

Yes. We can make extensions to this implementation where the render backend decides what critical area needs to be rasterized eagerly and asynchronously schedule rasterization of blobs we think we will see soon because of scroll speed, etc. That would be similar to gecko's layer tiling logic.
That's possible but I hope we won't need to do it.

[bors-servo]

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

[nical]

Review ping @kvark @gankro @gw3583 @staktrace (and anyone else interested).

[gw3583]

Reviewed 16 of 16 files at r2.
Reviewable status: all files reviewed, 14 unresolved discussions (waiting on @kvark and @nical)

---

webrender/src/render_backend.rs, line 1251 at r2 (raw file):

    let mut requests = Vec::new();
    for update in updates {
        match update {

match *update is preferred style

---

webrender/src/resource_cache.rs, line 105 at r2 (raw file):

/// Pre scene building state.
/// We use this to generate the async bob rendering requests.

nit: bob

---

webrender/src/resource_cache.rs, line 758 at r2 (raw file):

        self.blob_image_handler.as_mut().unwrap().update(key, data, *dirty_rect);

        let image = match self.blob_image_templates.get_mut(&key) {

Can use expect() here

---

webrender/src/resource_cache.rs, line 901 at r2 (raw file):

            // For some reason the blob image is missing. We'll fall back to
            // rasterizing it on the render backend thread.
            if missing {

Is it worth emitting a warning in this case or recording some kind of stats? Perhaps as a follow up...

---

webrender/src/resource_cache.rs, line 990 at r2 (raw file):

                // This code tries to keep things sane if Gecko sends
                // nonsensical blob image requests.

Should we warn / error if we get a not sane request?

---

webrender/src/resource_cache.rs, line 1089 at r2 (raw file):

        );
        image.data.retain(|tile, _| {
            match tile {

match *tile

---

webrender/src/scene_builder.rs, line 153 at r2 (raw file):

                });

                let rasterized_blobs = match blob_rasterizer {

Could probably be blob_rasterizer.map_or() or similar.

---

webrender_api/src/image.rs, line 184 at r2 (raw file):

/// A handler on the render backend that can create rasterizer objects which will
/// be sent to the scene builder thread to execute teh rasterization.

nit: teh

[gw3583]

Added some minor comments in the review above, nothing major.

The overview description above sounds reasonable, although I haven't thought it through completely. It's perhaps worth including that description somewhere in the doc/ directory in tree?

Overall, I'm happy to get this merged once others are - like you said, it's probably best to get it in and working, and deal with any fallout from it.

There is a CI failure - I haven't investigated that, and I expect we'll also want a try run, plus signoff from @kvark before merging.

Nice work!

[nical]

Thanks a lot for starting the review.

Is it worth emitting a warning in this case or recording some kind of stats? Perhaps as a follow up...

It's tricky because pages that run into this might run into this continuously, so I'd rather not make it worse by spamming stdout or stderr.
Also since eagerly rasterizing has some tradeoffs (avoid jank but rasterize a whole bunch more than we if we did it lazily), we might actually want to take advantage of the possibility to rasterize missing blobs and allow ourselves to hit this code when scrolling fast to reduce the amount of offscreen rasterization we do on average, for example.

Should we warn / error if we get a not sane request?

Maybe we could warn, I'm not sure where the line but it's something to think about. But here again we have to be careful about not spamming stderr if the page runs into this continuously like that gatsbyjs.org page.

There is a CI failure

It was a build failure while compiling osmesa, let's see if the next round of CI comes greener.

[Darkspirit]

added {before the transaction] -> added [before the transaction]

[kvark]

It took me a while to go through the PR, including a force reboot, because (apparently!) address-sanitized Gecko + Reviewable is a deadly combo, literally.

Thank you for the "How it works" description, it helps a ton reviewing the big change.

we rely on gecko giving us a hint through the added set_image_visible_area

So this appears to be related to display port of Gecko, in which case - why don't we communicate it as such (i.e. a general set_display_port or something) instead of having a state per image?

I think that this is inevitable because the culling logic in gecko and webrender works very differently, so relying on them to match exactly is fragile at best.

Why would we want to rely on it? In my understanding, Gecko's viewport should be considered an optimization hint by WR, (e.g. this is how much stuff you should expect to be shown) not much more.

keep around the async blob rasterizer that we sent to the scene builder

That sounds reasonable to me.

If blob images were to use images

I don't understand this part.

Because we eagerly rasterize blobs for the entire scene during scene building, we rasterize a lot more than we do before with lazy rasterization during frame building.

Aren't we only rasterizing the current viewport blobs (as opposed to "entire scene")?
Also, it would be nice if the Renderer didn't wait for rasterization of tiles that it knows are outside of the current frame. Do you see a way for us to make it so?

Moving this to the scene builder we don't know whether the blob will be in the texture cache by the time we finish rasterizing and therefore we have to produce the whole image or whole tile.

Didn't we previously decide to force those images to be in the texture cache for the duration of async blob rasterization?

Reviewed 11 of 16 files at r2, 5 of 5 files at r3.
Reviewable status: all files reviewed, 39 unresolved discussions (waiting on @kvark, @nical, and @Darkspirit)

---

examples/blob.rs, line 147 at r3 (raw file):

    fn prepare_resources(
        &mut self,
        _services: &api::BlobImageResources,

why is the variable called _services?

---

examples/blob.rs, line 168 at r3 (raw file):

impl api::AsyncBlobImageRasterizer for Rasterizer {
    fn rasterize(&mut self, requests: &[api::BlobImageParams]) -> Vec<(api::BlobImageRequest, api::BlobImageResult)> {

similarly, why is requests an array of XxxParams?

---

examples/blob.rs, line 169 at r3 (raw file):

impl api::AsyncBlobImageRasterizer for Rasterizer {
    fn rasterize(&mut self, requests: &[api::BlobImageParams]) -> Vec<(api::BlobImageRequest, api::BlobImageResult)> {
        let requests: Vec<(&api::BlobImageParams, Arc<ImageRenderingCommands>)> = requests.into_iter().map(|params| {

nit: could be Vec<_>

---

webrender/doc/blob.md, line 4 at r3 (raw file):

The blob image mechanism now has two traits:
- [`BlobImageHandler`](https://github.com/servo/webrender/pull/2785/files#diff-2b72a28a40b83edf41a59adfd46b1a11R188) is roughly the equivalent of the previous `BlobImageRenderer` except that it doesn't do any rendering (it manages the state of the blob commands, and resources like fonts).

wouldn't this link die after a pull request is closed (and the branch is deleted)?
also, I don't think it makes sense to base the explanation of the "previous BlobImageRenderer". It'd be cleaner to just say what this is now.

---

webrender/src/image.rs, line 239 at r3 (raw file):

    let t0 = point2(
        f32::floor(visible_area.origin.x * tw) as u16,

how can we make sure this case doesn't overflow?

---

webrender/src/image.rs, line 255 at r3 (raw file):

pub fn for_each_tile_in_range(
    range: &TileRange,

looks like TileRangeshould just impl Iterator

---

webrender/src/render_backend.rs, line 229 at r3 (raw file):

        &mut self,
        transaction_msg: TransactionMsg,
        blobs_to_rasterize: Vec<ImageKey>,

could this be &[ImageKey] instead?

---

webrender/src/render_backend.rs, line 1015 at r3 (raw file):

        let mut op = initial_op;

        // We currently don't support rasterizing blob images outside of the

hold on - aren't we rasterizing them on the scene building thread now?

---

webrender/src/render_backend.rs, line 1248 at r3 (raw file):

}

fn get_blob_image_updates(updates: &[ResourceUpdate]) -> Vec<ImageKey> {

could also be impl Iterator<Item = ImageKey>, so that the caller can re-use the storage

---

webrender/src/resource_cache.rs, line 429 at r1 (raw file):

Previously, nical (Nicolas Silva) wrote…

Sorry, actually it was because the font variants need to move non-copy stuff out of the vector.

what if we go though the updates twice - first, iterating mutably, so that we can do all the things we need, including moving out the font variants (by replacing with Vec::new()), and second - with retain() to only filter things without doing any work. That should let us avoid allocating the new_updates entirely

---

webrender/src/resource_cache.rs, line 101 at r3 (raw file):

/// Post scene building state.
struct RasterizedBlobImage {
    data: FastHashMap<Option<TileOffset>, BlobImageResult>,

can it have both the tiled and untiled results? if not, it would be cleaner to make this an enum:

enum RasterizedBloobImage {
  Single(BlobImageResult),
  Tiled(FastHashMap<TileOffset, BlobImageResult>),
}

---

webrender/src/resource_cache.rs, line 489 at r3 (raw file):

                ResourceUpdate::AddFont(_) |
                ResourceUpdate::AddFontInstance(_) => {
                    // Handled in update_resources_pre_scene_building

so this is asking to split our resource updates into pre/post at the type level

---

webrender/src/resource_cache.rs, line 762 at r3 (raw file):

            .expect("Attempt to update non-existent blob image");

        let mut tiling = image.tiling;

could this be logic be shared with add_blob_image?

---

webrender/src/resource_cache.rs, line 941 at r3 (raw file):

        keys: &[ImageKey]
    ) -> (Option<Box<AsyncBlobImageRasterizer>>, Vec<BlobImageParams>) {
        if self.blob_image_handler.is_none() {

I think by this point we'd long panic if the handler is None. Perhaps, we should check for keys.is_empty() instead?

---

webrender/src/resource_cache.rs, line 983 at r3 (raw file):

                    );

                    if let Some(range) = tiles.intersection(&dirty_tiles) {

we could also have the tiles as None here in case of no viewport_tiles specified, just taking the dirty_tiles straight

---

webrender/src/resource_cache.rs, line 985 at r3 (raw file):

                    if let Some(range) = tiles.intersection(&dirty_tiles) {
                        tiles = range;
                    }

what happens if they don't intersect? seems like we are missing that code path

---

webrender/src/resource_cache.rs, line 1052 at r3 (raw file):

                );
            }
            template.dirty_rect = None;

hmm, seems not obvious that create_xxx_requests ends up mutating it's state. Should probably be documented that calling the function twice will not yield the same results.

---

webrender/src/resource_cache.rs, line 1055 at r3 (raw file):

        }
        let handler = self.blob_image_handler.as_mut().unwrap();
        handler.prepare_resources(&self.resources, &blob_request_params);

could this just return the rasterizer right away?

---

webrender/src/resource_cache.rs, line 1090 at r3 (raw file):

            match *tile {
                Some(offset) => tile_range.contains(&offset),
                // This would be a bug. If we get here the blob should be tiled.

let's shout out accordingly - at least with error!

---

webrender/src/resource_cache.rs, line 1344 at r3 (raw file):

        {
            let handler = self.blob_image_handler.as_mut().unwrap();
            handler.prepare_resources(&self.resources, &self.missing_blob_images);

does it make sense to still prepare resources if the blob rasterizer is None?

---

webrender/src/resource_cache.rs, line 1373 at r3 (raw file):

                ImageData::Blob(..) => {
                    let blob_image = self.rasterized_blob_images.get(&request.key).unwrap();
                    match &blob_image.data.get(&request.tile).as_ref() {

you shouldn't need to do &xxxoption.as_ref(). Let's just do match xxxoption right away

---

webrender/src/resource_cache.rs, line 1384 at r3 (raw file):

                        }
                        &None => {
                            debug_assert!(false, "invalid blob image request during frame building");

nit: let's print out the request
also, I think error! would be more appropriate - to see this in release builds

---

webrender/src/resource_cache.rs, line 1684 at r3 (raw file):

                ImageData::Blob(_) => {
                    assert_eq!(template.tiling, None);
                    let blob_request_params = vec![

does it have to be heap-allocated?

---

webrender_api/src/image.rs, line 191 at r3 (raw file):

    fn create_blob_rasterizer(&mut self) -> Box<AsyncBlobImageRasterizer>;

    fn prepare_resources(

let's comment the hell out of these trait methods, please

---

webrender_api/src/image.rs, line 210 at r3 (raw file):

}

// A group of rasterization requests to execute synchronously on the scene builder thread.

nit: turn into a doc comment

---

webrender_api/src/units.rs, line 119 at r3 (raw file):

}

/// Coordinates in normalized space (between zero and one).

uh, this is tricky. I think of normazilation as a property of an existing space rather than a space of it's own (e.g. normalized device coordinates)

---

wrench/src/blob.rs, line 189 at r3 (raw file):

        let requests: Vec<Command> = requests.into_iter().map(
            |item| {
                let &(color, tile_size) = self.image_cmds.get(&item.request.key).unwrap();

can just do let (color, tile_size) = self.image_cmds[item.request.key]?

---

wrench/src/rawtest.rs, line 201 at r3 (raw file):

        // make sure we didn't request too many blobs
        assert!(called.load(Ordering::SeqCst) < 20);

isn't this number deterministic? could use a stronger check

---

wrench/src/rawtest.rs, line 269 at r3 (raw file):

        txn.add_image(
            blob_img1,
            ImageDescriptor::new(800, 800, ImageFormat::BGRA8, false, false),

nit: reuse image_size

[nical]

So this appears to be related to display port of Gecko, in which case - why don't we communicate it as such (i.e. a general set_display_port or something) instead of having a state per image?

To do this we'd need to resolve the scroll tree during frame building to figure out where each image item is with respect to the display port (which long term would be my favorite solution, but requires quite a bit more work).

Why would we want to rely on it? In my understanding, Gecko's viewport should be considered an optimization hint by WR, (e.g. this is how much stuff you should expect to be shown) not much more.

The way I read your question, I think that we are agreeing: relying on gecko's culling and webrender's to match perfectly would be hard/fragile, hence the current approach being to treat the visible area information provided by gecko more as an optimization hint than a drawing parameter like a clip.

I don't understand this part.

currently (non-blob) image updates are applied to the resource cache after we swap the scene. This is because it is information relevant to frame building, so if we were to add image updates to the resource cache when the transaction arrives before scene building, we would expose APZ frames which don't have the new scene built to the updated versions of the images that should arrive later with the scene.
As a result, if blob images were to read images from the resource cache they would not see any potential image update from the same transaction. This isn't a problem with fonts because we don't have a way to update them. It's also not in practice a problem with images because blob images don't actually read from them right now but it's something to know about if we want to do that in the future.

Aren't we only rasterizing the current viewport blobs (as opposed to "entire scene")?

So there is a bit of confusion around viewport/displayport and I totally get it wrong half of the time. In this PR the rectangle that we use to decide what we rasterize is one that corresponds to the size of the entire scene, which in practice represents a few screenfuls of content (it's not the entire page).

Also, it would be nice if the Renderer didn't wait for rasterization of tiles that it knows are outside of the current frame. Do you see a way for us to make it so?

This comes back to the problem of being able to know where the image items are in screen space and requires resolving the scroll tree during scene building and doing a subset of the culling phase there. It'd be really cool because we could be much smarter about what to rasterize ahead of time (could take into account scroll direction and velocity, etc), but that requires some work.

Didn't we previously decide to force those images to be in the texture cache for the duration of async blob rasterization?

It was one of the possibilities we talked about but I believe we ended up leaning towards rasterizing more instead of uploading more. The reason is that doing extra rasterization on the scene builder is costly for the CPU but won't cause janky scrolling, while uploading too much puts us at risk of missing the frame budget in the critical path.
So uploading textures is still lazy, even if rasterization is eager.

[nical]

Reviewable status: all files reviewed, 37 unresolved discussions (waiting on @kvark, @nical, @gw3583, and @Darkspirit)

---

examples/blob.rs, line 147 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why is the variable called _services?

Because it was already called _services in a few parts of the code and copy-pasting ensued. I'll change it to _resources.

---

examples/blob.rs, line 168 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

similarly, why is requests an array of XxxParams?

I'm not sure I understand the question.
In a previous version of this work the rasterizer object was a request object which knew what blobs to rasterize. In the latest iteration it is now an object that represents a snapshot of all blob images in order to be able to keep it around after scene building and lazily rasterize missing blobs if any. So the information about which blobs to render during scene building is passed as a separate array, now.

---

webrender/doc/blob.md, line 4 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

wouldn't this link die after a pull request is closed (and the branch is deleted)?
also, I don't think it makes sense to base the explanation of the "previous BlobImageRenderer". It'd be cleaner to just say what this is now.

I guess so? It probably makes most sense to remove this doc from the PR and do it later when we have actual stable urls to link to.

---

webrender/doc/blob.md, line 17 at r3 (raw file):

Previously, Darkspirit (Jan Andre Ikenmeyer) wrote…

added {before the transaction] -> added [before the transaction]

Done.

---

webrender/src/image.rs, line 239 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

how can we make sure this case doesn't overflow?

We could use more bits to store tile offsets, but since this is in number of tiles, 16 bits covers quite a bit of area already, and it wouldn't make sense to change it here without changing it everywhere so a lot of hash keys would get bigger.

---

webrender/src/image.rs, line 255 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

looks like TileRangeshould just impl Iterator

Sure, that's sugar, though. I'd rather not spend too much time in the details of small things that we can change as followups.

---

webrender/src/render_backend.rs, line 1251 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

match *update is preferred style

Done.

---

webrender/src/render_backend.rs, line 229 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could this be &[ImageKey] instead?

Done.

---

webrender/src/render_backend.rs, line 1015 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

hold on - aren't we rasterizing them on the scene building thread now?

This comment doesn't make sense anymore. Removed it.

---

webrender/src/render_backend.rs, line 1248 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could also be impl Iterator<Item = ImageKey>, so that the caller can re-use the storage

Sounds reasonable. Let's make it a followup.

---

webrender/src/resource_cache.rs, line 429 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

what if we go though the updates twice - first, iterating mutably, so that we can do all the things we need, including moving out the font variants (by replacing with Vec::new()), and second - with retain() to only filter things without doing any work. That should let us avoid allocating the new_updates entirely

I'm not sure I follow. To move out the native handle in the font stuff I need to consume the vector. Since the vector is consumed there needs to be another allocation. In order to avoid the allocation we'd need to be able to not move out the font variants without consuming the vector.

At some point I tried to add a AddFont::Empty variant in order to be able to take the native handle and leave that in place but then it can with a bunch of downsides like having this variant we should never receive from the API and only have in between the pre and post scene building phase was a bit hacky and surprising, so I decided to put that on the side. Easier to come back and polish this type of thing when the big changes are landed and stable.

---

webrender/src/resource_cache.rs, line 758 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Can use expect() here

Done.

---

webrender/src/resource_cache.rs, line 1089 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

match *tile

Done.

---

webrender/src/resource_cache.rs, line 101 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can it have both the tiled and untiled results? if not, it would be cleaner to make this an enum:

enum RasterizedBloobImage {
  Single(BlobImageResult),
  Tiled(FastHashMap<TileOffset, BlobImageResult>),
}

I agree the type would look nicer but this currently maps directly to how the things are used, so it would make the code that manipulates rasterized blob images more branchy. I don't mind it too much but let's make it a followup thing if you want to do that.

---

webrender/src/resource_cache.rs, line 762 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could this be logic be shared with add_blob_image?

Done.

---

webrender/src/resource_cache.rs, line 941 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I think by this point we'd long panic if the handler is None. Perhaps, we should check for keys.is_empty() instead?

If keys is empty we still want to forward the rasterizer to the scene builder so that frame building gets an up to date rasterizer to deal with missing blobs if any.

---

webrender/src/resource_cache.rs, line 983 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

we could also have the tiles as None here in case of no viewport_tiles specified, just taking the dirty_tiles straight

If we do that we need to add code below to handle tiles being None if there is no dirty rect, so I think it's simpler this way.

---

webrender/src/resource_cache.rs, line 985 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

what happens if they don't intersect? seems like we are missing that code path

Good question. In theory, viewport tiles cover the entire display list so changes to something outside the are of DL shouldn't get into the blob image, but it doesn't hurt to play safe. I'll make it not request any tile.

---

webrender/src/resource_cache.rs, line 1052 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

hmm, seems not obvious that create_xxx_requests ends up mutating it's state. Should probably be documented that calling the function twice will not yield the same results.

It's similar to how requesting texture uploads mutates the dirty rect of the image template: this state represents the amount of invalid content we haven't requested yet, so in my opinion where we request rasterization is the right place to keep track of this information.

---

webrender/src/resource_cache.rs, line 1055 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

could this just return the rasterizer right away?

We also need to prepare resources when rasterizing missing blobs at which point the rasterizer already exists.

---

webrender/src/resource_cache.rs, line 1090 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's shout out accordingly - at least with error!

Done.

---

webrender/src/resource_cache.rs, line 1344 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

does it make sense to still prepare resources if the blob rasterizer is None?

At this point if we have missing blobs but no blob handler I'm pretty sure we have panicked long ago.

---

webrender/src/resource_cache.rs, line 1373 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

you shouldn't need to do &xxxoption.as_ref(). Let's just do match xxxoption right away

Done.

---

webrender/src/resource_cache.rs, line 1384 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: let's print out the request
also, I think error! would be more appropriate - to see this in release builds

Sure I can make it an error!. It's best to avoid debug formatters in release builds, see https://groups.google.com/forum/#!topic/mozilla.dev.servo/_gllnLmN_VU

---

webrender/src/resource_cache.rs, line 1684 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

does it have to be heap-allocated?

Good catch. I removed the allocation.

---

webrender/src/scene_builder.rs, line 153 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Could probably be blob_rasterizer.map_or() or similar.

Done.

---

webrender_api/src/image.rs, line 191 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's comment the hell out of these trait methods, please

Done.

---

webrender_api/src/units.rs, line 119 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

uh, this is tricky. I think of normazilation as a property of an existing space rather than a space of it's own (e.g. normalized device coordinates)

Physically speaking the normalization is unit-less in the sense that it is a ratio. It's both in DevicePixels divided by DivicePixels, and in LayoutPixels divided by LayoutPixels.

We generate it by dividing layout coordinates by layout coordinates and use it by multiplying it to device pixels.

---

wrench/src/blob.rs, line 189 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can just do let (color, tile_size) = self.image_cmds[item.request.key]?

Done.

---

wrench/src/rawtest.rs, line 201 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

isn't this number deterministic? could use a stronger check

It's sensible to any heuristic we change in the code that decides to what to rasterize. Right now this code is deterministic but I want to followup with more involved tweaks to compensate the extra work we do due to eager rasterization. This check is there to verify that we don't render thousands of tiles so there isn't much value in making it more precise.

[nical]

interesting. Could you explain why it has to go into euclid?

TileRange is a TypedRect which is in euclid even if one of type parameter is defined in webrender (I tried to be sure).

There is a green try push from a week ago but there was a few rather bumpy rebases in between so I do want do another one before landing (I tried on Friday but got stuck with non-trivial wr update)

[nical]

Latest try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a167b8873fba6c7b9a6802b88b6046d6580686c3 (so far the failures are unexpected passes)

[kvark]

Looks good, thank you!
@bors-servo r+

[bors-servo]

📌 Commit 6d750a2 has been approved by kvark

[bors-servo]

⌛ Testing commit 6d750a2 with merge ae171f7...

[staktrace]

Is bors stuck? The taskcluster jobs on the merge commit passed but it didn't finish the merge...

[gw3583]

@bors-servo retry

[bors-servo]

⌛ Testing commit 6d750a2 with merge 4704cf2...

[gw3583]

@staktrace it looks like bors was stuck. I issued a retry - if that doesn't work, we can manually merge, or if @nical pushes an amended commit hash, that is generally enough to fix up the state that bors is in.

[bors-servo]

💔 Test failed - status-appveyor

[kvark]

spurious network error (2 tries remaining): [28] Timeout was reached (Operation too slow. Less than 10 bytes/sec transferred the last 30 seconds)

@bors-servo retry

[bors-servo]

⌛ Testing commit 6d750a2 with merge c28c8bd...

[bors-servo]

💔 Test failed - status-appveyor

[kvark]

@bors-servo retry

[bors-servo]

⌛ Testing commit 6d750a2 with merge 719bb23...

[bors-servo]

💔 Test failed - status-appveyor

[nical]

Rebased and squashed. @bors-servo r=kvark

[bors-servo]

📌 Commit 6b29ecc has been approved by kvark

[bors-servo]

⌛ Testing commit 6b29ecc with merge 3baeb23...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 3baeb23 to master...