1212664 - Compositor surface dumping code performs y-inversion incorrectly

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Botond Ballo [:botond]]

The transform set in YInvertImageSurface() is:

    Matrix::Scaling(1.0, -1.0) *
    Matrix::Translation(0.0, aSurf->GetSize().height)

That's ends up being

    1  0     0
    0 -1  -height

which is incorrect because after the -1 y-scale flips the y-coordinate, translating by -height moves the pixel further off-surface. The matrix we want is

    1  0     0
    0 -1   height

which is obtained as:

    Matrix::Translation(0.0, aSurf->GetSize().height) *
    Matrix::Scaling(1.0, -1.0)

Comment 1

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1212664 - Perform y-inversion correctly when dumping compositor surface. r=jrmuizel

Bug 1212664 - Perform y-inversion correctly when dumping compositor surface. r=jrmuizel

Comment 2

[Botond Ballo [:botond]]

https://reviewboard.mozilla.org/r/21531/#review19357

jrmuizel gave verbal r+

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8671097 [details]
MozReview Request: Bug 1212664 - Perform y-inversion correctly when dumping compositor surface. r=jrmuizel

Jeff gave me verbal r+.

Comment 4

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
> The transform set in YInvertImageSurface() is:
> 
>     Matrix::Scaling(1.0, -1.0) *
>     Matrix::Translation(0.0, aSurf->GetSize().height)
> 
> That's ends up being
> 
>     1  0     0
>     0 -1  -height
> 
> which is incorrect because after the -1 y-scale flips the y-coordinate,
> translating by -height moves the pixel further off-surface. The matrix we
> want is
> 
>     1  0     0
>     0 -1   height
> 
> which is obtained as:
> 
>     Matrix::Translation(0.0, aSurf->GetSize().height) *
>     Matrix::Scaling(1.0, -1.0)

Sorry I mixed those up.

     Matrix::Translation(0.0, aSurf->GetSize().height) *
     Matrix::Scaling(1.0, -1.0)

is the one we had, which gives the incorrect matrix

     1  0     0
     0 -1  -height

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9a501cd1b1d

Comment 6

[Botond Ballo [:botond]]

I should also mention that the Part 7 patch in bug 709490 (which recently landed and got backed out) contains this fix, though it's not clear whether that's intentional or it just snuck into the patch.

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d9a501cd1b1d