commit d385716b1faebe6b5a3187a5eec2a4b0e263f014
parent bd246bc20b5d2cb76e75464d310247acc9d0abc1
Author: Nicolas Silva <nical@fastmail.com>
Date: Tue, 9 Dec 2025 08:19:04 +0000
Bug 1996818 - Avoid re-generating uv rect addresses for images multiple times. r=gw
This is important because with external images we are sometimes producing normalized UVs, and replacing them with the non-normalized UVs in the common path then leads to incorrect behavior.
Differential Revision: https://phabricator.services.mozilla.com/D275049
Diffstat:
3 files changed, 54 insertions(+), 56 deletions(-)
diff --git a/gfx/wr/webrender/src/render_task.rs b/gfx/wr/webrender/src/render_task.rs
@@ -15,7 +15,7 @@ use crate::profiler::{add_text_marker};
use crate::spatial_tree::SpatialNodeIndex;
use crate::filterdata::SFilterData;
use crate::frame_builder::FrameBuilderConfig;
-use crate::gpu_types::{BorderInstance, ImageSource, UvRectKind, TransformPaletteId, BlurEdgeMode};
+use crate::gpu_types::{BorderInstance, UvRectKind, TransformPaletteId, BlurEdgeMode};
use crate::internal_types::{CacheTextureId, FastHashMap, FilterGraphNode, FilterGraphOp, FilterGraphPictureReference, SVGFE_CONVOLVE_VALUES_LIMIT, TextureSource, Swizzle};
use crate::picture::{ResolvedSurfaceTexture, MAX_SURFACE_SIZE};
use crate::prim_store::ClipData;
@@ -1040,7 +1040,7 @@ pub struct RenderTask {
/// manages the handle.
pub uv_rect_handle: GpuBufferAddress,
pub cache_handle: Option<RenderTaskCacheEntryHandle>,
- uv_rect_kind: UvRectKind,
+ pub uv_rect_kind: UvRectKind,
}
impl RenderTask {
@@ -2755,39 +2755,6 @@ impl RenderTask {
self.kind.target_kind()
}
- pub fn write_gpu_blocks(
- &mut self,
- target_rect: DeviceIntRect,
- gpu_buffer: &mut GpuBufferBuilder,
- ) {
- profile_scope!("write_gpu_blocks");
-
- self.kind.write_gpu_blocks(gpu_buffer);
-
- // Render tasks are re-created each frame so we can safely check that we aren't
- // producing their uv address multiple times. This is important because some
- // code paths may produce a different uv rect than here, for example normalized
- // uv rects coming from external images. In these cases we rely on the uv address
- // being et earlier in the frame.
- // Cases where we expect to already have a uv rect handle include:
- // - cached render tasks,
- // - Image requests (in some cases but not all).
- if self.uv_rect_handle.is_valid() {
- return;
- }
-
- let p0 = target_rect.min.to_f32();
- let p1 = target_rect.max.to_f32();
- let image_source = ImageSource {
- p0,
- p1,
- user_data: [0.0; 4],
- uv_rect_kind: self.uv_rect_kind,
- };
-
- self.uv_rect_handle = image_source.write_gpu_blocks(&mut gpu_buffer.f32).address_unchecked();
- }
-
/// Called by the render task cache.
///
/// Tells the render task that it is cached (which means its gpu cache
diff --git a/gfx/wr/webrender/src/render_task_graph.rs b/gfx/wr/webrender/src/render_task_graph.rs
@@ -9,6 +9,7 @@
use api::units::*;
use api::ImageFormat;
+use crate::gpu_types::ImageSource;
use crate::internal_types::{TextureSource, CacheTextureId, FastHashMap, FastHashSet, FrameId};
use crate::internal_types::size_of_frame_vec;
use crate::render_task::{StaticRenderTaskSurface, RenderTaskLocation, RenderTask};
@@ -619,9 +620,9 @@ impl RenderTaskGraphBuilder {
// considered to be immutable for the rest of the frame building process.
for task in &mut graph.tasks {
- // First check whether the render task texture and uv rects are managed
- // externally. This is the case for image tasks and cached tasks. In both
- // cases it results in a finding the information in the texture cache.
+ // Check whether the render task texture and uv rects are managed externally.
+ // This is the case for image tasks and cached tasks. In both cases it
+ // results in a finding the information in the texture cache.
let cache_item = if let Some(ref cache_handle) = task.cache_handle {
Some(resolve_cached_render_task(
cache_handle,
@@ -640,12 +641,13 @@ impl RenderTaskGraphBuilder {
None
};
- if let Some(cache_item) = cache_item {
+ if let Some(cache_item) = &cache_item {
+ task.uv_rect_handle = gpu_buffers.f32.resolve_handle(cache_item.uv_rect_handle);
+
// Update the render task even if the item is invalid.
// We'll handle it later and it's easier to not have to
// deal with unexpected location variants like
// RenderTaskLocation::CacheRequest when we do.
- task.uv_rect_handle = gpu_buffers.f32.resolve_handle(cache_item.uv_rect_handle);
if let RenderTaskLocation::CacheRequest { .. } = &task.location {
let source = cache_item.texture_id;
task.location = RenderTaskLocation::Static {
@@ -655,14 +657,25 @@ impl RenderTaskGraphBuilder {
}
}
- // Give the render task an opportunity to add any
- // information to the GPU cache, if appropriate.
+ // This has to be done after we do the task location fixup above.
let target_rect = task.get_target_rect();
- task.write_gpu_blocks(
- target_rect,
- gpu_buffers,
- );
+ // If the uv rect is not managed externally, generate it now.
+ if cache_item.is_none() {
+ let image_source = ImageSource {
+ p0: target_rect.min.to_f32(),
+ p1: target_rect.max.to_f32(),
+ user_data: [0.0; 4],
+ uv_rect_kind: task.uv_rect_kind,
+ };
+
+ let uv_rect_handle = image_source.write_gpu_blocks(&mut gpu_buffers.f32);
+ task.uv_rect_handle = gpu_buffers.f32.resolve_handle(uv_rect_handle);
+ }
+
+ // Give the render task an opportunity to add any
+ // information to the GPU cache, if appropriate.
+ task.kind.write_gpu_blocks(gpu_buffers);
graph.task_data.push(
task.kind.write_task_data(target_rect)
@@ -740,6 +753,7 @@ impl RenderTaskGraph {
}
let uv_address = task.get_texture_address();
+ assert!(uv_address.is_valid());
Some((uv_address, texture_source))
}
diff --git a/gfx/wr/webrender/src/texture_cache.rs b/gfx/wr/webrender/src/texture_cache.rs
@@ -98,11 +98,14 @@ pub struct CacheEntry {
/// Arbitrary user data associated with this item.
pub user_data: [f32; 4],
/// The last frame this item was requested for rendering.
- // TODO(gw): This stamp is only used for picture cache tiles, and some checks
- // in the glyph cache eviction code. We could probably remove it
- // entirely in future (or move to PictureCacheEntry).
pub last_access: FrameStamp,
/// Address of the resource rect in the GPU cache.
+ ///
+ /// The handle is stored in the cache entry to avoid duplicates when an
+ /// item is used multiple times per frame, but greate care must be taken
+ /// to not reuse a handle that was created in a previous frame.
+ /// TODO: For now the validity of the handle can be checked by comparing
+ /// last_access with the current FrameStamp, but this is error prone.
pub uv_rect_handle: GpuBufferHandle,
/// Image format of the data that the entry expects.
pub input_format: ImageFormat,
@@ -581,7 +584,7 @@ pub struct TextureCache {
pub pending_updates: TextureUpdateList,
/// The current `FrameStamp`. Used for cache eviction policies.
- now: FrameStamp,
+ pub now: FrameStamp,
/// Cache of texture cache handles with automatic lifetime management, evicted
/// in a least-recently-used order.
@@ -846,10 +849,12 @@ impl TextureCache {
},
};
entry.map_or(true, |entry| {
- // If an image is requested that is already in the cache,
- // refresh the GPU buffer data associated with this item.
- entry.last_access = now;
- entry.write_gpu_blocks(gpu_buffer);
+ if entry.last_access != now {
+ // If an image is requested that is already in the cache,
+ // refresh the GPU buffer data associated with this item.
+ entry.last_access = now;
+ entry.write_gpu_blocks(gpu_buffer);
+ }
false
})
}
@@ -941,6 +946,7 @@ impl TextureCache {
dirty_rect = DirtyRect::All;
}
+ let now = self.now;
let entry = self.get_entry_opt_mut(handle)
.expect("BUG: There must be an entry at this handle now");
@@ -948,8 +954,13 @@ impl TextureCache {
entry.eviction_notice = eviction_notice.cloned();
entry.uv_rect_kind = uv_rect_kind;
- // Upload the resource rect and texture array layer.
- entry.write_gpu_blocks(gpu_buffer);
+ // If we just allocated the entry, its framestamp is up to date but it does
+ // not uset have up-to-date gpu blocks.
+ if entry.last_access != now || realloc {
+ entry.last_access = now;
+ // Upload the resource rect and texture array layer.
+ entry.write_gpu_blocks(gpu_buffer);
+ }
// Create an update command, which the render thread processes
// to upload the new image data into the correct location
@@ -1025,6 +1036,12 @@ impl TextureCache {
) -> Option<(CacheTextureId, DeviceIntRect, Swizzle, GpuBufferHandle, [f32; 4])> {
let entry = self.get_entry_opt(handle)?;
let origin = entry.details.describe();
+ if entry.last_access != self.now {
+ // On rare occasions we may have an image request that does not materialize
+ // into up to date data in the cache. For example if we failed to produce a
+ // stacking context snapshot.
+ return None;
+ }
Some((
entry.texture_id,
DeviceIntRect::from_origin_and_size(origin, entry.size),
