1156454 - Make sure we're using sub image sampling when drawing with Direct2D/Cairo

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

We'll need to be able to do this to get rid of CreateSamplingRestrictedDrawable. This involves using the mSamplingRect of SourceSurface properly.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Success means sub non-tiled (background) should be all black on http://people.mozilla.org/~jmuizelaar/implementation-tests/background-interpolation/test.html

Comment 2

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mason Chang [:mchang] from comment #1)
> Success means sub non-tiled (background) should be all black on
> http://people.mozilla.org/~jmuizelaar/implementation-tests/background-
> interpolation/test.html

Just to clarify, we currently show all black and we need to make sure we stay that way when not using CSR.

Comment 3

[Mason Chang [Inactive] [:mchang]]

I think on direct 2d 11 and cairo, this is already happening. I will check on direct 2d 10 if I'm actually on the right path. Sorry that this may be long winded, but I'd like to make sure I understand correctly. 

The source image (grid.png) has one white spot surrounded by black. Sub image sampling means that we want to create a surface out of a sub image which doesn't include the white, so when we sample the image, we should get all black. Then when we render this image and tile it out, it will be repeating only the black image. Is this the correct understanding?

When I first tested this by deleting CreateSamplingRestrictedDrawable on Windows, the image was already black as if nothing had changed. Chrome renders like we do but IE samples the white area, which presumably means they do not do sub image sampling.

The point of this bug is that when we create a SurfacePattern, we use a sampling rect which says what the subimage is to sample from (https://dxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxDrawable.cpp?from=gfxSurfaceDrawable::DrawInternal&case=true#94). Then when the DrawTarget actually fills the rect, we need to make sure that a subimage is created from this SurfacePattern:mSamplingRect. Is this correct?

When I forced a cairo backend, we use the created pattern.mSamplingRect (in this case (0,0) at width = 1px, height = 1px), during conversion to a cairo pattern (https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp?from=DrawTargetCairo.cpp&case=true#874), we explicitly pass in the sampling rect to create the cairo surface (https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp?from=DrawTargetCairo.cpp&case=true#487), which explicitly creates a subimage (https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.cpp#337). Therefore, we are using sub image sampling with cairo. Is this logic correct?

When using a D2D11 backend, we take the same pattern and create sampling bounds based on the gfx SurfacePattern (https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetD2D1.cpp?from=DrawTargetD2D1.cpp&case=true#1293). When we create the ID2D1Image, it doesn't actually use the sampling bounds, so we get an image that's the original size. Then we create an ImageBrush with the sampling rect (https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetD2D1.cpp?from=DrawTargetD2D1.cpp&case=true#1314). I think in this case, we don't explicitly create a subimage surface like in Cairo but tell d2d to only sample from the specific subimage sampling rect. Does that make sense?

OK, so overall I think we don't have to do anything for Cairo / D2D 11 since we're already using subimage sampling. Or am I way off? Thanks!

Comment 4

[Matt Woodrow (:mattwoodrow)]

Yes, mSamplingRect should be supported for all backends, otherwise [1] would be very broken.

The restriction is that mSamplingRect must be contained within the bounds of the original source, whereas we often use CreateSamplingRestrictedDrawable where the sampling rect extends beyond the source and we expect the source to be tiled within that area.

I'm not sure what we want to do for the tiled source case, Bas is quite opposed to supporting it with mSamplingRect (because the way we implement it for direct2d 1.1 doesn't support it)

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUtils.cpp#645

Comment 5

[Mason Chang [Inactive] [:mchang]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Yes, mSamplingRect should be supported for all backends, otherwise [1] would
> be very broken.
> 
> The restriction is that mSamplingRect must be contained within the bounds of
> the original source, whereas we often use CreateSamplingRestrictedDrawable
> where the sampling rect extends beyond the source and we expect the source
> to be tiled within that area.
> 
> I'm not sure what we want to do for the tiled source case, Bas is quite
> opposed to supporting it with mSamplingRect (because the way we implement it
> for direct2d 1.1 doesn't support it)
> 
> [1] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUtils.cpp#645

Could we manually create a larger tiled source to fit the size of the sampling rect, then go down the regular sampling rect path or does that make no sense?

Comment 6

[Matt Woodrow (:mattwoodrow)]

(In reply to Mason Chang [:mchang] from comment #5)
 
> Could we manually create a larger tiled source to fit the size of the
> sampling rect, then go down the regular sampling rect path or does that make
> no sense?

That's exactly what CSRD does :)

Except that it doesn't specify a sampling rect as such, because after that work sampling rect == source rect.

It has really poor performing edge cases, like when downscaling a repeated image (the tscrollx has an example of this, thank Jeff). In this scenario, the sampling rect (in source image space) can be many times larger than the viewport and it's a lot of work to allocate, draw and then downsample from that source.

I believe Jeff was a fan of just not bothering with sampling restrictions in that case, and living with the artifacts.

There's still the question of whether we want to handle the case where the sampling rect size is <= the image size, but isn't positioned at 0,0 such that it overlaps an edge. That's essentially the 'rotated buffer' case, and may be something we want to treat different to 'normal' tiling. Or not, I'm not sure.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mason Chang [:mchang] from comment #3)
> When I forced a cairo backend, we use the created pattern.mSamplingRect (in
> this case (0,0) at width = 1px, height = 1px), during conversion to a cairo
> pattern
> (https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.
> cpp?from=DrawTargetCairo.cpp&case=true#874), we explicitly pass in the
> sampling rect to create the cairo surface
> (https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.
> cpp?from=DrawTargetCairo.cpp&case=true#487), which explicitly creates a
> subimage
> (https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetCairo.
> cpp#337). Therefore, we are using sub image sampling with cairo. Is this
> logic correct?

Looks to be.

> When using a D2D11 backend, we take the same pattern and create sampling
> bounds based on the gfx SurfacePattern
> (https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetD2D1.
> cpp?from=DrawTargetD2D1.cpp&case=true#1293). When we create the ID2D1Image,
> it doesn't actually use the sampling bounds, so we get an image that's the
> original size. Then we create an ImageBrush with the sampling rect
> (https://dxr.mozilla.org/mozilla-central/source/gfx/2d/DrawTargetD2D1.
> cpp?from=DrawTargetD2D1.cpp&case=true#1314). I think in this case, we don't
> explicitly create a subimage surface like in Cairo but tell d2d to only
> sample from the specific subimage sampling rect. Does that make sense?

Yep.

> 
> OK, so overall I think we don't have to do anything for Cairo / D2D 11 since
> we're already using subimage sampling. Or am I way off? Thanks!j

That looks to be the case.

(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> 
> I believe Jeff was a fan of just not bothering with sampling restrictions in
> that case, and living with the artifacts.

Correct.

> There's still the question of whether we want to handle the case where the
> sampling rect size is <= the image size, but isn't positioned at 0,0 such
> that it overlaps an edge. That's essentially the 'rotated buffer' case, and
> may be something we want to treat different to 'normal' tiling. Or not, I'm
> not sure.

I don't think it's worth trying to deal with this case. IE doesn't.