From c434cf219d2371ba33acf595b3d099be0aedb7da Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Sun, 23 Sep 2018 08:21:59 +1000 Subject: [PATCH 1/4] Revert "address reviews" This reverts commit 0cf32bf350024c4b70fda004a1b02e5ce3327dc9. --- webrender/src/resource_cache.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/webrender/src/resource_cache.rs b/webrender/src/resource_cache.rs index 9094e1aa6a..8c5a53f994 100644 --- a/webrender/src/resource_cache.rs +++ b/webrender/src/resource_cache.rs @@ -614,8 +614,6 @@ impl ResourceCache { } }; - // First make sure we have an entry for this key (using a placeholder - // if need be). let image = self.rasterized_blob_images.entry(request.key).or_insert_with( || { RasterizedBlob::Tiled(FastHashMap::default()) } ); @@ -1132,7 +1130,7 @@ impl ResourceCache { // // We do the latter here but it's not ideal and might want to revisit and do // the former instead. - match self.rasterized_blob_images.get(&key) { + match self.rasterized_blob_images.get_mut(&key) { Some(RasterizedBlob::NonTiled(ref queue)) => { if queue.len() > 2 { needs_upload = true; @@ -1485,7 +1483,7 @@ impl ResourceCache { (RasterizedBlob::NonTiled(ref mut queue), None) => { for img in queue.drain(..) { updates.push(( - ImageData::Raw(img.data), + ImageData::Raw(Arc::clone(&img.data)), Some(img.rasterized_rect) )); } From 296125549e163e6dad85c7952c1a21a63ad91daf Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Sun, 23 Sep 2018 08:22:04 +1000 Subject: [PATCH 2/4] Revert "Maintain a queue of rasterized blob updates per non-tiled image." This reverts commit 6f7ab2a848570089b4668cbb5f72fadc2a6dec3c. --- webrender/src/resource_cache.rs | 233 ++++++++++++++------------------ 1 file changed, 99 insertions(+), 134 deletions(-) diff --git a/webrender/src/resource_cache.rs b/webrender/src/resource_cache.rs index 8c5a53f994..316888ac5c 100644 --- a/webrender/src/resource_cache.rs +++ b/webrender/src/resource_cache.rs @@ -33,7 +33,6 @@ use profiler::{ResourceProfileCounters, TextureCacheProfileCounters}; use render_backend::FrameId; use render_task::{RenderTaskCache, RenderTaskCacheKey, RenderTaskId}; use render_task::{RenderTaskCacheEntry, RenderTaskCacheEntryHandle, RenderTaskTree}; -use smallvec::SmallVec; use std::collections::hash_map::Entry::{self, Occupied, Vacant}; use std::collections::hash_map::IterMut; use std::{cmp, mem}; @@ -103,7 +102,7 @@ enum State { /// Post scene building state. enum RasterizedBlob { Tiled(FastHashMap), - NonTiled(Vec), + NonTiled(RasterizedBlobImage), } /// Pre scene building state. @@ -627,16 +626,7 @@ impl ResourceCache { tiles.insert(tile, data); } } else { - if let RasterizedBlob::NonTiled(ref mut queue) = *image { - // If our new rasterized rect overwrites items in the queue, discard them. - queue.retain(|img| { - !data.rasterized_rect.contains_rect(&img.rasterized_rect) - }); - - queue.push(data); - } else { - *image = RasterizedBlob::NonTiled(vec![data]); - } + *image = RasterizedBlob::NonTiled(data); } } } @@ -1113,32 +1103,13 @@ impl ResourceCache { ); }); } else { - let mut needs_upload = match self.cached_images.try_get(&key) { + let needs_upload = match self.cached_images.try_get(&key) { Some(&ImageResult::UntiledAuto(ref entry)) => { self.texture_cache.needs_upload(&entry.texture_cache_handle) } _ => true, }; - // If the queue of ratserized updates is growing it probably means that - // the texture is not getting uploaded because the display item is off-screen. - // In that case we are better off - // - Either not kicking rasterization for that image (avoid wasted cpu work - // but will jank next time the item is visible because of lazy rasterization. - // - Clobber the update queue by pushing an update with a larger dirty rect - // to prevent it from accumulating. - // - // We do the latter here but it's not ideal and might want to revisit and do - // the former instead. - match self.rasterized_blob_images.get_mut(&key) { - Some(RasterizedBlob::NonTiled(ref queue)) => { - if queue.len() > 2 { - needs_upload = true; - } - } - _ => {}, - }; - let dirty_rect = if needs_upload { // The texture cache entry has been evicted, treat it as all dirty. None @@ -1461,32 +1432,28 @@ impl ResourceCache { let image_template = self.resources.image_templates.get_mut(request.key).unwrap(); debug_assert!(image_template.data.uses_texture_cache()); - let mut updates: SmallVec<[(ImageData, Option); 1]> = SmallVec::new(); - - match image_template.data { + let mut blob_rasterized_rect = None; + let image_data = match image_template.data { ImageData::Raw(..) | ImageData::External(..) => { // Safe to clone here since the Raw image data is an // Arc, and the external image data is small. - updates.push((image_template.data.clone(), None)); + image_template.data.clone() } ImageData::Blob(..) => { - let blob_image = self.rasterized_blob_images.get_mut(&request.key).unwrap(); + + let blob_image = self.rasterized_blob_images.get(&request.key).unwrap(); match (blob_image, request.tile) { (RasterizedBlob::Tiled(ref tiles), Some(tile)) => { let img = &tiles[&tile]; - updates.push(( - ImageData::Raw(Arc::clone(&img.data)), - Some(img.rasterized_rect) - )); + blob_rasterized_rect = Some(img.rasterized_rect); + + ImageData::Raw(Arc::clone(&img.data)) } - (RasterizedBlob::NonTiled(ref mut queue), None) => { - for img in queue.drain(..) { - updates.push(( - ImageData::Raw(Arc::clone(&img.data)), - Some(img.rasterized_rect) - )); - } + (RasterizedBlob::NonTiled(ref img), None) => { + blob_rasterized_rect = Some(img.rasterized_rect); + + ImageData::Raw(Arc::clone(&img.data)) } _ => { debug_assert!(false, "invalid blob image request during frame building"); @@ -1496,102 +1463,100 @@ impl ResourceCache { } }; - for (image_data, blob_rasterized_rect) in updates { - let entry = match *self.cached_images.get_mut(&request.key) { - ImageResult::UntiledAuto(ref mut entry) => entry, - ImageResult::Multi(ref mut entries) => entries.get_mut(&request.into()), - ImageResult::Err(_) => panic!("Update requested for invalid entry") - }; - - let mut descriptor = image_template.descriptor.clone(); - let mut local_dirty_rect; + let entry = match *self.cached_images.get_mut(&request.key) { + ImageResult::UntiledAuto(ref mut entry) => entry, + ImageResult::Multi(ref mut entries) => entries.get_mut(&request.into()), + ImageResult::Err(_) => panic!("Update requested for invalid entry") + }; - if let Some(tile) = request.tile { - let tile_size = image_template.tiling.unwrap(); - let clipped_tile_size = compute_tile_size(&descriptor, tile_size, tile); + match (blob_rasterized_rect, entry.dirty_rect) { + (Some(rasterized), Some(dirty)) => { + debug_assert!(request.tile.is_some() || rasterized.contains_rect(&dirty)); + } + _ => {} + } - local_dirty_rect = if let Some(rect) = entry.dirty_rect.take() { - // We should either have a dirty rect, or we are re-uploading where the dirty - // rect is ignored anyway. - let intersection = intersect_for_tile(rect, clipped_tile_size, tile_size, tile); - debug_assert!(intersection.is_some() || - self.texture_cache.needs_upload(&entry.texture_cache_handle)); - intersection - } else { - None - }; + let mut descriptor = image_template.descriptor.clone(); + let local_dirty_rect; - // The tiled image could be stored on the CPU as one large image or be - // already broken up into tiles. This affects the way we compute the stride - // and offset. - let tiled_on_cpu = image_template.data.is_blob(); - if !tiled_on_cpu { - let bpp = descriptor.format.bytes_per_pixel(); - let stride = descriptor.compute_stride(); - descriptor.stride = Some(stride); - descriptor.offset += - tile.y as u32 * tile_size as u32 * stride + - tile.x as u32 * tile_size as u32 * bpp; - } - - descriptor.size = clipped_tile_size; + if let Some(tile) = request.tile { + let tile_size = image_template.tiling.unwrap(); + let clipped_tile_size = compute_tile_size(&descriptor, tile_size, tile); + + local_dirty_rect = if let Some(rect) = entry.dirty_rect.take() { + // We should either have a dirty rect, or we are re-uploading where the dirty + // rect is ignored anyway. + let intersection = intersect_for_tile(rect, clipped_tile_size, tile_size, tile); + debug_assert!(intersection.is_some() || + self.texture_cache.needs_upload(&entry.texture_cache_handle)); + intersection } else { - local_dirty_rect = entry.dirty_rect.take(); - } + None + }; - // If we are uploading the dirty region of a blob image we might have several - // rects to upload so we use each of these rasterized rects rather than the - // overall dirty rect of the image. - if blob_rasterized_rect.is_some() { - local_dirty_rect = blob_rasterized_rect; + // The tiled image could be stored on the CPU as one large image or be + // already broken up into tiles. This affects the way we compute the stride + // and offset. + let tiled_on_cpu = image_template.data.is_blob(); + if !tiled_on_cpu { + let bpp = descriptor.format.bytes_per_pixel(); + let stride = descriptor.compute_stride(); + descriptor.stride = Some(stride); + descriptor.offset += + tile.y as u32 * tile_size as u32 * stride + + tile.x as u32 * tile_size as u32 * bpp; } - let filter = match request.rendering { - ImageRendering::Pixelated => { - TextureFilter::Nearest - } - ImageRendering::Auto | ImageRendering::CrispEdges => { - // If the texture uses linear filtering, enable mipmaps and - // trilinear filtering, for better image quality. We only - // support this for now on textures that are not placed - // into the shared cache. This accounts for any image - // that is > 512 in either dimension, so it should cover - // the most important use cases. We may want to support - // mip-maps on shared cache items in the future. - if descriptor.allow_mipmaps && - descriptor.size.width > 512 && - descriptor.size.height > 512 && - !self.texture_cache.is_allowed_in_shared_cache( - TextureFilter::Linear, - &descriptor, - ) { - TextureFilter::Trilinear - } else { - TextureFilter::Linear - } + descriptor.size = clipped_tile_size; + } else { + local_dirty_rect = entry.dirty_rect.take(); + } + + let filter = match request.rendering { + ImageRendering::Pixelated => { + TextureFilter::Nearest + } + ImageRendering::Auto | ImageRendering::CrispEdges => { + // If the texture uses linear filtering, enable mipmaps and + // trilinear filtering, for better image quality. We only + // support this for now on textures that are not placed + // into the shared cache. This accounts for any image + // that is > 512 in either dimension, so it should cover + // the most important use cases. We may want to support + // mip-maps on shared cache items in the future. + if descriptor.allow_mipmaps && + descriptor.size.width > 512 && + descriptor.size.height > 512 && + !self.texture_cache.is_allowed_in_shared_cache( + TextureFilter::Linear, + &descriptor, + ) { + TextureFilter::Trilinear + } else { + TextureFilter::Linear } - }; + } + }; - let eviction = if image_template.data.is_blob() { - Eviction::Manual - } else { - Eviction::Auto - }; + let eviction = if image_template.data.is_blob() { + Eviction::Manual + } else { + Eviction::Auto + }; - //Note: at this point, the dirty rectangle is local to the descriptor space - self.texture_cache.update( - &mut entry.texture_cache_handle, - descriptor, - filter, - Some(image_data), - [0.0; 3], - local_dirty_rect, - gpu_cache, - None, - UvRectKind::Rect, - eviction, - ); - } + //Note: at this point, the dirty rectangle is local to the descriptor space + self.texture_cache.update( + &mut entry.texture_cache_handle, + descriptor, + filter, + Some(image_data), + [0.0; 3], + local_dirty_rect, + gpu_cache, + None, + UvRectKind::Rect, + eviction, + ); } } From 4229c7c7b685a0ce9516556655cd852579ea74ec Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Sun, 23 Sep 2018 08:22:09 +1000 Subject: [PATCH 3/4] Revert "Store tiled and non-tiled rasterized blobs differently." This reverts commit 713ef1cd4d7e60bd2b312262db9cb2bd4705265a. --- webrender/src/resource_cache.rs | 84 ++++++++++++++------------------- 1 file changed, 35 insertions(+), 49 deletions(-) diff --git a/webrender/src/resource_cache.rs b/webrender/src/resource_cache.rs index 316888ac5c..b968fab785 100644 --- a/webrender/src/resource_cache.rs +++ b/webrender/src/resource_cache.rs @@ -3,7 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use api::{AddFont, BlobImageResources, AsyncBlobImageRasterizer, ResourceUpdate}; -use api::{BlobImageDescriptor, BlobImageHandler, BlobImageRequest, RasterizedBlobImage}; +use api::{BlobImageDescriptor, BlobImageHandler, BlobImageRequest}; use api::{ClearCache, ColorF, DevicePoint, DeviceUintPoint, DeviceUintRect, DeviceUintSize}; use api::{FontInstanceKey, FontKey, FontTemplate, GlyphIndex}; use api::{ExternalImageData, ExternalImageType, BlobImageResult, BlobImageParams}; @@ -100,9 +100,8 @@ enum State { } /// Post scene building state. -enum RasterizedBlob { - Tiled(FastHashMap), - NonTiled(RasterizedBlobImage), +struct RasterizedBlobImage { + data: FastHashMap, BlobImageResult>, } /// Pre scene building state. @@ -406,7 +405,7 @@ pub struct ResourceCache { pending_image_requests: FastHashSet, blob_image_handler: Option>, - rasterized_blob_images: FastHashMap, + rasterized_blob_images: FastHashMap, blob_image_templates: FastHashMap, // If while building a frame we encounter blobs that we didn't already @@ -605,29 +604,10 @@ impl ResourceCache { pub fn add_rasterized_blob_images(&mut self, images: Vec<(BlobImageRequest, BlobImageResult)>) { for (request, result) in images { - let data = match result { - Ok(data) => data, - Err(..) => { - warn!("Failed to rasterize a blob image"); - continue; - } - }; - let image = self.rasterized_blob_images.entry(request.key).or_insert_with( - || { RasterizedBlob::Tiled(FastHashMap::default()) } + || { RasterizedBlobImage { data: FastHashMap::default() } } ); - - if let Some(tile) = request.tile { - if let RasterizedBlob::NonTiled(..) = *image { - *image = RasterizedBlob::Tiled(FastHashMap::default()); - } - - if let RasterizedBlob::Tiled(ref mut tiles) = *image { - tiles.insert(tile, data); - } - } else { - *image = RasterizedBlob::NonTiled(data); - } + image.data.insert(request.tile, result); } } @@ -965,10 +945,9 @@ impl ResourceCache { if template.data.is_blob() { let request: BlobImageRequest = request.into(); - let missing = match (self.rasterized_blob_images.get(&request.key), request.tile) { - (Some(RasterizedBlob::Tiled(tiles)), Some(tile)) => !tiles.contains_key(&tile), - (Some(RasterizedBlob::NonTiled(..)), None) => false, - _ => true, + let missing = match self.rasterized_blob_images.get(&request.key) { + Some(img) => !img.data.contains_key(&request.tile), + None => true, }; // For some reason the blob image is missing. We'll fall back to @@ -1155,19 +1134,28 @@ impl ResourceCache { Some(size) => size, None => { return; } }; - - let tiles = match self.rasterized_blob_images.get_mut(&key) { - Some(RasterizedBlob::Tiled(tiles)) => tiles, - _ => { return; } + let image = match self.rasterized_blob_images.get_mut(&key) { + Some(image) => image, + None => { + //println!("Missing rasterized blob (key={:?})!", key); + return; + } }; - let tile_range = compute_tile_range( &area, &template.descriptor.size, tile_size, ); - - tiles.retain(|tile, _| { tile_range.contains(tile) }); + image.data.retain(|tile, _| { + match *tile { + Some(offset) => tile_range.contains(&offset), + // This would be a bug. If we get here the blob should be tiled. + None => { + error!("Blob image template and image data tiling don't match."); + false + } + } + }); } pub fn request_glyphs( @@ -1440,22 +1428,20 @@ impl ResourceCache { image_template.data.clone() } ImageData::Blob(..) => { - - let blob_image = self.rasterized_blob_images.get(&request.key).unwrap(); - match (blob_image, request.tile) { - (RasterizedBlob::Tiled(ref tiles), Some(tile)) => { - let img = &tiles[&tile]; - blob_rasterized_rect = Some(img.rasterized_rect); + match blob_image.data.get(&request.tile) { + Some(result) => { + let result = result + .as_ref() + .expect("Failed to render a blob image"); - ImageData::Raw(Arc::clone(&img.data)) - } - (RasterizedBlob::NonTiled(ref img), None) => { - blob_rasterized_rect = Some(img.rasterized_rect); + // TODO: we may want to not panic and show a placeholder instead. + + blob_rasterized_rect = Some(result.rasterized_rect); - ImageData::Raw(Arc::clone(&img.data)) + ImageData::Raw(Arc::clone(&result.data)) } - _ => { + None => { debug_assert!(false, "invalid blob image request during frame building"); continue; } From 02a06071506e63bd39bbf379bb17f727fc3ea8e9 Mon Sep 17 00:00:00 2001 From: Glenn Watson Date: Sun, 23 Sep 2018 08:26:19 +1000 Subject: [PATCH 4/4] Disable blob memory reporters temporarily. --- webrender/src/resource_cache.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/webrender/src/resource_cache.rs b/webrender/src/resource_cache.rs index b968fab785..d9f863c025 100644 --- a/webrender/src/resource_cache.rs +++ b/webrender/src/resource_cache.rs @@ -1620,6 +1620,9 @@ impl ResourceCache { } // Mesure rasterized blobs. + // TODO(gw): Temporarily disabled while we roll back a crash. We can re-enable + // these when that crash is fixed. + /* for (_, image) in self.rasterized_blob_images.iter() { let mut accumulate = |b: &RasterizedBlobImage| { report.rasterized_blobs += unsafe { op(b.data.as_ptr() as *const c_void) }; @@ -1629,6 +1632,7 @@ impl ResourceCache { RasterizedBlob::NonTiled(vec) => vec.iter().for_each(&mut accumulate), }; } + */ report }