The tiling used for large image surfaces is broken. This means images with a dimension larger than 8192 pixels are broken. I've attached a high resolution image to test.

This patch fixes the tiling code. While working on this I've discovered another problem though, our tiling strategy does not work 100%. When a transform is applied that causes subpixel displacement of the surface being rendered, seams can occur at the edges of the tiles. I have no good solution for this except maybe to fallback rather than tile for large surfaces.

This patch adds proper support for large surfaces. By first of all looking which part of the image is actually needed. And if that is still too big downscaling it to the screen diagonal (the maximum amount of pixels that could actually be needed) in whichever direction does not fit. This patch seems to produce very acceptable results image quality wise.

With proper large surface support in place, we don't need multiple run tiling. This patch removes all the code supporting that.

I wasn't handling all cases properly, this fixes some issues. We fallback in one edge-case but it should be -very- rare.

I took a look at what IE 9 is doing here. It currently takes the large images bilinearly scales them down to the maximum texture size and then uses that as a replacement for the image. This means that viewing large images at full size causes them to be upscaled. Their approach doesn't seem that good at all.

(In reply to comment #5) > I took a look at what IE 9 is doing here. It currently takes the large images > bilinearly scales them down to the maximum texture size and then uses that as a > replacement for the image. This means that viewing large images at full size > causes them to be upscaled. Their approach doesn't seem that good at all. Indeed, I think technically my approach is better, although obviously slightly more complicated!

Problems with large surface handling cause http://www.kevs3d.co.uk/dev/asteroids/ to use loads of memory and eventually crash. This is fixed with the new code.

Same patch, rebased to latest trunk, lots of code in this area had changed.

Rebased as well, we should get this in soon! It cleans up the D2D code a lot and fixes a whole bunch of different issues that can occur if a page contains (non-obvious) large images.

Previous version contained two bugs from rebasing mistakes that showed after a bit more testing. Also updated to use RefPtr where possible.

Comment on attachment 441391 [details] [diff] [review] Part 1: Add proper support for large surfaces v4 Overall this function is pretty hairy. I wonder if we could break it out into some separate functions. >diff --git a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp >--- a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp >+++ b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp >@@ -777,21 +777,23 @@ _cairo_d2d_create_brush_for_pattern(cair > } else if (pattern->extend == CAIRO_EXTEND_REPEAT) { > extendMode = D2D1_EXTEND_MODE_WRAP; > } else if (pattern->extend == CAIRO_EXTEND_REFLECT) { > extendMode = D2D1_EXTEND_MODE_MIRROR; > } else { > extendMode = D2D1_EXTEND_MODE_CLAMP; > } > RefPtr<ID2D1Bitmap> sourceBitmap; >- bool tiled = false; >+ bool partial = false; > unsigned int xoffset = 0; > unsigned int yoffset = 0; > unsigned int width; > unsigned int height; >+ unsigned char *data = NULL; >+ unsigned int stride = 0; > *remaining_runs = 0; > if (surfacePattern->surface->type == CAIRO_SURFACE_TYPE_D2D) { > /** > * \todo We need to somehow get a rectangular transparent > * border here too!! > */ > cairo_d2d_surface_t *srcSurf = > reinterpret_cast<cairo_d2d_surface_t*>(surfacePattern->surface); >@@ -806,16 +808,24 @@ _cairo_d2d_create_brush_for_pattern(cair > D2D1_ALPHA_MODE alpha; > if (srcSurf->format == CAIRO_FORMAT_ARGB32 || > srcSurf->format == CAIRO_FORMAT_A8) { > alpha = D2D1_ALPHA_MODE_PREMULTIPLIED; > } else { > alpha = D2D1_ALPHA_MODE_IGNORE; > } > >+ data = srcSurf->data; >+ stride = srcSurf->stride; >+ >+ /** >+ * This can be used as a temporary surface for resampling surfaces larget than maxSize. >+ */ This style of comment seems a bit heavy weight. /* [text] */ is probably sufficient. Also, the comment should probably be "This is used as a temporary..." >+ pixman_image_t *pix_image = NULL; >+ > DXGI_FORMAT format; > unsigned int Bpp; > if (srcSurf->format == CAIRO_FORMAT_ARGB32) { > format = DXGI_FORMAT_B8G8R8A8_UNORM; > Bpp = 4; > } else if (srcSurf->format == CAIRO_FORMAT_RGB24) { > format = DXGI_FORMAT_B8G8R8A8_UNORM; > Bpp = 4; >@@ -825,63 +835,132 @@ _cairo_d2d_create_brush_for_pattern(cair > } else { > return NULL; > } > > /** Leave room for extend_none space, 2 pixels */ > UINT32 maxSize = d2dsurf->rt->GetMaximumBitmapSize() - 2; > > if ((UINT32)srcSurf->width > maxSize || (UINT32)srcSurf->height > maxSize) { >- tiled = true; >- UINT32 horiz_tiles = (UINT32)ceil((float)srcSurf->width / maxSize); >- UINT32 vert_tiles = (UINT32)ceil((float)srcSurf->height / maxSize); >- UINT32 current_vert_tile = last_run / horiz_tiles; >- UINT32 current_horiz_tile = last_run % horiz_tiles; >- xoffset = current_horiz_tile * maxSize; >- yoffset = current_vert_tile * maxSize; >- *remaining_runs = horiz_tiles * vert_tiles - last_run - 1; >- width = min(maxSize, srcSurf->width - maxSize * current_horiz_tile); >- height = min(maxSize, srcSurf->height - maxSize * current_vert_tile); >- // Move the image to the right spot. >- cairo_matrix_translate(&mat, xoffset, yoffset); >- if (true) { >- RefPtr<ID2D1RectangleGeometry> clipRect; >- D2DSurfFactory::Instance()->CreateRectangleGeometry(D2D1::RectF(0, 0, (float)width, (float)height), >- &clipRect); >+ /** >+ * We cannot fit this image directly into a texture, start doing tricks to >+ * draw correctly anyway. >+ */ >+ partial = true; >+ /** >+ * First we check which part of the image is inside the viewable area. >+ */ Same comment style problem. >+ >+ /** Transformation this surface to image surface space */ s/Transformation/Transform/ >+ cairo_matrix_translate(&mat, xoffset, yoffset); >+ >+ if (width > maxSize || height > maxSize) { >+ /** >+ * We cannot upload the required part of the surface directly, we're going to create >+ * a version which is downsampled to a smaller size by pixman and then uploaded. >+ * >+ * We need to size it to atleast the diagonal size of this surface, in order to prevent ever "atleast" typo >+ * upsampling this again when drawing it to the surface. We want the resized surface >+ * to be as small as possible to limit pixman required fill rate. >+ */ >+ unsigned int minSize = (unsigned int)sqrt(pow((float)desc.Width, 2) + pow((float)desc.Height, 2)); I don't really like this guessing approach to finding a scale factor. Let me see if I can come up with something better... > >- d2dsurf->rt->PushLayer(D2D1::LayerParameters(D2D1::InfiniteRect(), >- clipRect, >- D2D1_ANTIALIAS_MODE_PER_PRIMITIVE, >- _cairo_d2d_matrix_from_matrix(&mat)), >- d2dsurf->helperLayer); >- *pushed_clip = true; >- } >+ unsigned int newWidth = MIN(minSize, MIN(width, maxSize)); >+ unsigned int newHeight = MIN(minSize, MIN(height, maxSize)); >+ double xRatio = (double)width / newWidth; >+ double yRatio = (double)height / newHeight; >+ >+ if (newWidth > maxSize || newHeight > maxSize) { >+ /** >+ * Okay, the diagonal of our surface is big enough to require a sampling larger >+ * than the maximum texture size. This is where we give up. >+ */ >+ return NULL; We should in theory be able to handle these sitations, if we have a different approach to getting the scale factor. >+ } >+ >+ /** Create a temporary surface to hold the downsampled image */ A single asterix is sufficient. >+ pix_image = pixman_image_create_bits(srcSurf->pixman_format, >+ newWidth, >+ newHeight, >+ NULL, >+ -1); >+ >+ /** Set the transformation to downsample and call pixman_image_composite to downsample */ >+ pixman_transform_t transform; >+ pixman_transform_init_scale(&transform, pixman_double_to_fixed(xRatio), pixman_double_to_fixed(yRatio)); >+ pixman_transform_translate(&transform, NULL, pixman_double_to_fixed(xoffset), pixman_double_to_fixed(yoffset)); xoffset and yoffset are ints not doubles. Perhaps they can be integrated into the pixman_image_composite call? >+ >+ pixman_image_set_transform(srcSurf->pixman_image, &transform); >+ pixman_image_composite(PIXMAN_OP_SRC, srcSurf->pixman_image, NULL, pix_image, 0, 0, 0, 0, 0, 0, newWidth, newHeight); >+ >+ /** Adjust the pattern transform to the used temporary surface */ >+ cairo_matrix_scale(&mat, xRatio, yRatio); >+ >+ data = (unsigned char*)pixman_image_get_data(pix_image); >+ stride = pixman_image_get_stride(pix_image); >+ >+ /** Into this image we actually have no offset */ >+ xoffset = 0; >+ yoffset = 0; >+ width = newWidth; >+ height = newHeight; >+ } > } else { > width = srcSurf->width; > height = srcSurf->height; > } > >@@ -896,18 +975,18 @@ _cairo_d2d_create_brush_for_pattern(cair > _cairo_surface_attach_snapshot(surfacePattern->surface, > nullSurf, > _d2d_snapshot_detached); > } > } else { > cached_bitmap *cachebitmap = new cached_bitmap; Should we be allocating this if (partial) ? >@@ -917,45 +996,51 @@ _cairo_d2d_create_brush_for_pattern(cair >+ } >+ if (pix_image) { >+ pixman_image_unref(pix_image); >+ } Does if (pix_image) always have the same result as if (!partial)? >@@ -969,16 +1054,19 @@ _cairo_d2d_create_brush_for_pattern(cair >+ if (partial) { >+ sourceBitmap->Release(); >+ } Why do we need to manually release here?

(In reply to comment #11) > (From update of attachment 441391 [details] [diff] [review]) > > We should in theory be able to handle these sitations, if we have a different > approach to getting the scale factor. A couple more than we can handle currently, but still not all. > > >+ pix_image = pixman_image_create_bits(srcSurf->pixman_format, > >+ newWidth, > >+ newHeight, > >+ NULL, > >+ -1); > >+ > >+ /** Set the transformation to downsample and call pixman_image_composite to downsample */ > >+ pixman_transform_t transform; > >+ pixman_transform_init_scale(&transform, pixman_double_to_fixed(xRatio), pixman_double_to_fixed(yRatio)); > >+ pixman_transform_translate(&transform, NULL, pixman_double_to_fixed(xoffset), pixman_double_to_fixed(yoffset)); > > xoffset and yoffset are ints not doubles. Perhaps they can be integrated into > the pixman_image_composite call? I experimented with this but it didn't always work well. If you have a suggestion I'm open to trying it! > > Should we be allocating this if (partial) ? We should move it down into the existing if (!partial) if clause. > > >@@ -917,45 +996,51 @@ _cairo_d2d_create_brush_for_pattern(cair > >+ } > >+ if (pix_image) { > >+ pixman_image_unref(pix_image); > >+ } > > Does if (pix_image) always have the same result as if (!partial)? It does, but I thought reusing pix_image as a bool to check if we were doing partial drawing was a bit ugly, so I used partial. > > >@@ -969,16 +1054,19 @@ _cairo_d2d_create_brush_for_pattern(cair > >+ if (partial) { > >+ sourceBitmap->Release(); > >+ } > > Why do we need to manually release here? We don't, rebasing mistake.

(In reply to comment #11) > > >+ * upsampling this again when drawing it to the surface. We want the resized surface > >+ * to be as small as possible to limit pixman required fill rate. > >+ */ > >+ unsigned int minSize = (unsigned int)sqrt(pow((float)desc.Width, 2) + pow((float)desc.Height, 2)); > > I don't really like this guessing approach to finding a scale factor. Let me > see if I can come up with something better... This is some code I wrote a while a go for getting the scaled size of an image from a matrix. http://pastebin.mozilla.org/718968

(In reply to comment #13) > (In reply to comment #11) > > > > >+ * upsampling this again when drawing it to the surface. We want the resized surface > > >+ * to be as small as possible to limit pixman required fill rate. > > >+ */ > > >+ unsigned int minSize = (unsigned int)sqrt(pow((float)desc.Width, 2) + pow((float)desc.Height, 2)); > > > > I don't really like this guessing approach to finding a scale factor. Let me > > see if I can come up with something better... > > This is some code I wrote a while a go for getting the scaled size of an image > from a matrix. > > http://pastebin.mozilla.org/718968 The problem here, as far as I can see, is that this is not exactly accurate, imagine you have a 2 by 2 pixel grid, and a 3x2 image, containing a line of black pixels, white pixels, and black pixels. If you rotate this by 45 degrees and scale it to a size of 2x2 pixels and then put it on there. You're correct that you're going to transform the origin surface to 2x2, however to do the most accurate sampling to the destination, you'd still want that third row of pixels. Since all 3 columns of pixels will affect the correct sampling onto the 2x2 destination.

Is this something we should be testing with grafx bot, and if so, is there a reftest that could be constructed to test it?

(In reply to comment #14) > (In reply to comment #13) > > (In reply to comment #11) > > > > > > >+ * upsampling this again when drawing it to the surface. We want the resized surface > > > >+ * to be as small as possible to limit pixman required fill rate. > > > >+ */ > > > >+ unsigned int minSize = (unsigned int)sqrt(pow((float)desc.Width, 2) + pow((float)desc.Height, 2)); > > > > > > I don't really like this guessing approach to finding a scale factor. Let me > > > see if I can come up with something better... > > > > This is some code I wrote a while a go for getting the scaled size of an image > > from a matrix. > > > > http://pastebin.mozilla.org/718968 > > The problem here, as far as I can see, is that this is not exactly accurate, > imagine you have a 2 by 2 pixel grid, and a 3x2 image, containing a line of > black pixels, white pixels, and black pixels. If you rotate this by 45 degrees > and scale it to a size of 2x2 pixels and then put it on there. You're correct > that you're going to transform the origin surface to 2x2, however to do the > most accurate sampling to the destination, you'd still want that third row of > pixels. Since all 3 columns of pixels will affect the correct sampling onto the > 2x2 destination. True. We basically have a quality/performance tradeoff here. As an aside, how hard is it do this correctly by using tiles?

(In reply to comment #16) > (In reply to comment #14) > > (In reply to comment #13) > > > (In reply to comment #11) > > > > > > > > >+ * upsampling this again when drawing it to the surface. We want the resized surface > > > > >+ * to be as small as possible to limit pixman required fill rate. > > > > >+ */ > > > > >+ unsigned int minSize = (unsigned int)sqrt(pow((float)desc.Width, 2) + pow((float)desc.Height, 2)); > > > > > > > > I don't really like this guessing approach to finding a scale factor. Let me > > > > see if I can come up with something better... > > > > > > This is some code I wrote a while a go for getting the scaled size of an image > > > from a matrix. > > > > > > http://pastebin.mozilla.org/718968 > > > > The problem here, as far as I can see, is that this is not exactly accurate, > > imagine you have a 2 by 2 pixel grid, and a 3x2 image, containing a line of > > black pixels, white pixels, and black pixels. If you rotate this by 45 degrees > > and scale it to a size of 2x2 pixels and then put it on there. You're correct > > that you're going to transform the origin surface to 2x2, however to do the > > most accurate sampling to the destination, you'd still want that third row of > > pixels. Since all 3 columns of pixels will affect the correct sampling onto the > > 2x2 destination. > > True. We basically have a quality/performance tradeoff here. As an aside, how > hard is it do this correctly by using tiles? I think it would be fairly hard actually, preventing any seeming from AA on the different tiles and such it pretty tricky. Also I noticed performance of the new method is significantly better than any attempts at tiling that I tried. I chose the diagonal as a sort of heuristic that should always get pretty good quality (hey! if we're rotating anyway, there's going to be sampling artifacts anyway). But I'm also open to different approaches there (largest of width/height * 2, or something like that). The problem with largest we can accommodate (i.e. scale to MaxTextureSize, was that it was pretty slow, on my machine the CPU potentially had to fill an 8192x8192 pixel target surface, that's expensive!). I think the quality we get from the diagonal approach seems to be pretty good, on the test image on this bug the quality of the line was as good as on a normal trunk build. But ofcourse a 2 times approach could be used to prevent the square root.

Not sure but www.aol.com also consumes memory like its water...

Confirmed, and still happening on newest nightly. Tried with all extensions off/only ABP on. With D2D, AOL.com causes a massive memory leak. With D2D off, it works fine.

Comment on attachment 441391 [details] [diff] [review] Part 1: Add proper support for large surfaces v4 Here are some more review comments. General comment: lots of casts. >+ * First we check which part of the image is inside the viewable area. >+ */ >+ >+ /** Transformation this surface to image surface space */ >+ cairo_matrix_t invMat = mat; >+ cairo_matrix_invert(&invMat); What happens if the matrix isn't invertible? > >- if (!d2dsurf->helperLayer) { >- d2dsurf->rt->CreateLayer(&d2dsurf->helperLayer); >- } >+ RefPtr<IDXGISurface> surf; >+ d2dsurf->surface->QueryInterface(&surf); >+ DXGI_SURFACE_DESC desc; >+ surf->GetDesc(&desc); >+ >+ cairo_point_double_t topLeft; >+ cairo_point_double_t topRight; >+ cairo_point_double_t bottomLeft; >+ cairo_point_double_t bottomRight; >+ bottomLeft.x = topRight.y = topLeft.x = topLeft.y = 0; >+ topRight.x = bottomRight.x = desc.Width; >+ bottomRight.y = bottomLeft.y = desc.Height; This might be more clear if you used initializers. e.g. cairo_point_double_t topLeft = { 0, 0}; cairo_point_double_t topRight = {desc.Width, 0}; cairo_point_double_t bottomLeft = { 0, desc.Height}; cairo_point_double_t bottomRight = {desc.Width, desc.Height}; >+ >+ /** Transform this surface's corners to image surface space */ >+ cairo_matrix_transform_point(&invMat, &topLeft.x, &topLeft.y); >+ cairo_matrix_transform_point(&invMat, &topRight.x, &topRight.y); >+ cairo_matrix_transform_point(&invMat, &bottomLeft.x, &bottomLeft.y); >+ cairo_matrix_transform_point(&invMat, &bottomRight.x, &bottomRight.y); >+ >+ /** Get the bounding axis aligned rectangle, adjust by one to prevent rounding errors */ >+ double leftMost = MIN(topLeft.x, MIN(topRight.x, MIN(bottomLeft.x, bottomRight.x))) - 1; >+ double rightMost = MAX(topLeft.x, MAX(topRight.x, MAX(bottomLeft.x, bottomRight.x))) - 1; >+ double topMost = MIN(topLeft.y, MIN(topRight.y, MIN(bottomLeft.y, bottomRight.y))) + 1; >+ double bottomMost = MAX(topLeft.y, MAX(topRight.y, MAX(bottomLeft.y, bottomRight.y))) + 1; >+ Getting the axis aligned bounding box seems like it could be a helper method. Perhaps something like this already exists in cairo? >+ /** Calculate the offsets into the source image and the width of the part required */ >+ xoffset = (unsigned int)MAX(0, floor(leftMost)); >+ yoffset = (unsigned int)MAX(0, floor(topMost)); >+ width = (unsigned int)MIN(MAX(0, ceil(rightMost - xoffset)), srcSurf->width - xoffset); >+ height = (unsigned int)MIN(MAX(0, ceil(bottomMost - yoffset)), srcSurf->height - yoffset); >+ >+ cairo_matrix_translate(&mat, xoffset, yoffset); >+ >+ if (width > maxSize || height > maxSize) { >+ /** >+ * We cannot upload the required part of the surface directly, we're going to create >+ * a version which is downsampled to a smaller size by pixman and then uploaded. >+ * >+ * We need to size it to atleast the diagonal size of this surface, in order to prevent ever >+ * upsampling this again when drawing it to the surface. We want the resized surface >+ * to be as small as possible to limit pixman required fill rate. This comment should include something like your explanation about sampling from comment 14. . >+ */ >+ unsigned int minSize = (unsigned int)sqrt(pow((float)desc.Width, 2) + pow((float)desc.Height, 2)); Should this be rounded up instead of truncated?

(In reply to comment #20) > (From update of attachment 441391 [details] [diff] [review]) > >+ * First we check which part of the image is inside the viewable area. > >+ */ > >+ > >+ /** Transformation this surface to image surface space */ > >+ cairo_matrix_t invMat = mat; > >+ cairo_matrix_invert(&invMat); > > What happens if the matrix isn't invertible? From looking at the code, the only matrix that is non-invertible that can ever occur is [ 0 0 ; 0 0 ] since every function inside cairo (including set_matrix) appears to return an error is a non-invertible matrix is passed. But please let me know if I am mistaking!

Updated to address comments.

Comment on attachment 444229 [details] [diff] [review] Part 1: Add proper support for large surfaces v5 >diff --git a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp >--- a/gfx/cairo/cairo/src/cairo-d2d-surface.cpp >+++ b/gfx/cairo/cairo/src/cairo-d2d-surface.cpp > > /** Leave room for extend_none space, 2 pixels */ This still has a bunch of "**" comments, please switch them to "*" > UINT32 maxSize = d2dsurf->rt->GetMaximumBitmapSize() - 2; > >+ if (!partial) { >+ cached_bitmap *cachebitmap = new cached_bitmap; >+ /** We can cache it if it isn't a partial bitmap */ >+ cachebitmap->dirty = false; >+ cachebitmap->bitmap = sourceBitmap; >+ cachebitmap->refs = 2; The '2' should have an explanation. > if (surfacePattern->base.filter == CAIRO_FILTER_NEAREST) { > bitProps = D2D1::BitmapBrushProperties(extendMode, >@@ -972,16 +1050,17 @@ _cairo_d2d_create_brush_for_pattern(cair > if (unique) { > RefPtr<ID2D1BitmapBrush> bitBrush; > D2D1_BRUSH_PROPERTIES brushProps = > D2D1::BrushProperties(1.0, _cairo_d2d_matrix_from_matrix(&mat)); > d2dsurf->rt->CreateBitmapBrush(sourceBitmap, > &bitProps, > &brushProps, > &bitBrush); >+ extra whitespace?

Pushed http://hg.mozilla.org/mozilla-central/rev/71be7adf2850. Pushed http://hg.mozilla.org/mozilla-central/rev/f10540947f10.