601888 - Teach ThebesLayerOGL about resolution-scaled drawing

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

They need to be able to request scaling during repaint (bug 586683) and draw scaled content to screen, possibly created by a remote layer (bug 598864).  I'm more interested in the latter so may stop there.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Teach ThebesLayerOGL to draw backing buffers drawn with a resolution other than 1.0

This allows gets us to properly draw shadow BasicThebesLayers painted with a non-1.0 resolution.  We still need a part 2 that allows ThebesLayerOGL to request non-1.0 resolution during painting.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This blocks turning on GL compositing for fennec.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 481020 [details] [diff] [review]
part 1: Teach ThebesLayerOGL to draw backing buffers drawn with a resolution other than 1.0

Looks good to me. The comment confused me slightly but I think I didn't read the second sentence properly.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 481020 [details] [diff] [review]
part 1: Teach ThebesLayerOGL to draw backing buffers drawn with a resolution other than 1.0

r- based on IRC discussion -- the wrong thing is being scaled here, leading to rendering artifacts.  I believe we concluded that the right thing to scale is the quadRect argument given to BindAndDrawQuadWithTextureRect.  With a shadow layer buffer/resolution, mTexImage is no longer a 1:1 representation of this layer's visible rect.  So, we have to select the right rectangle from it to draw into the screen-space quadRect -- which is quadRect scaled by the resolution.  The texture's size has to stay fixed, it's only passed in so that the function can correctly calculate texture coordinates for drawing a quadRect region of the texture to the screen.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Teach ThebesLayerOGL to draw backing buffers drawn with a resolution other than 1.0

Scale quadRect instead of anti-scaling texture size.  Also fix an annoying compiler warning.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: part 1: Teach ThebesLayerOGL to draw backing buffers drawn with a resolution other than 1.0 , v2.1

Forgot RoundOut().

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 481137 [details] [diff] [review]
part 1: Teach ThebesLayerOGL to draw backing buffers drawn with a resolution other than 1.0 , v2.1

this makes a lot more sense :-)

Comment 8

[Brian Crowder]

cjones:  Making you the owner of this for now, disown it if you feel that's inappropriate.  (mostly 'cause I'm looking for unowned blocker bugs to work on)

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Matt, please steal this from me when your part 2 patch is ready to go.

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/76cd71c36c9b

Can leave this open for part 2.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

The first part of this bug already landed.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Part 2 of this doesn't block fennec b3, because that's only really relevant on desktop.

Comment 13

[Joe Drew (not getting mail)]

I don't think this blocks at all, then, unless I'm missing something?

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #12)
> Part 2 of this doesn't block fennec b3, because that's only really relevant on
> desktop.

perhaps parts 1 and 2 should be separate bugs?

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

This is the GL part of bug 586683, which is currently blocking-final.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Let ThebesLayerOGL render content using custom resolutions

This appears to work for the most part (forcing press shell to set weird resolutions on the layer manager), but I haven't run it through tests yet.

I'm worried that scaling the region to pass to BeginUpdate and then reverse scaling the result will sometime have rounding issues since we snap to the outside rectangle both times.

WIP Followup to solve this by making TextureImage handle resolution changes.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2b: Refactor Resolution handling into TextureImage

I'm less sure if this is a good way to handle things, but does appear to make the code cleaner.

Known issues:
- I think this will break IPC since the layers resolution won't match that of the TextureImage.
- Black squares when scrolling.
- EGL implementation of TextureImage needs fixing

Still very much a WIP, will fix the issues if decided to be a good path to take.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 491743 [details] [diff] [review]
Part 2b: Refactor Resolution handling into TextureImage

I don't think it makes sense to push resolution handling down into TextureImage.  That's an abstraction that's only supposed to deal with platform-specific, efficient uploading of image data to GPU-accessible memory.  I would prefer handling resolution in a higher level; I don't see a fundamental reason we need to move resolution into TextureImage.

But, I've been out of this code for a while, and the use of TextureImage may have evolved in the meantime, so pinging vlad for feedback.

Comment 19

[Jeff Muizelaar [:jrmuizel]]

(In reply to comment #18)
> Comment on attachment 491743 [details] [diff] [review]
> Part 2b: Refactor Resolution handling into TextureImage
> 
> I don't think it makes sense to push resolution handling down into
> TextureImage.  That's an abstraction that's only supposed to deal with
> platform-specific, efficient uploading of image data to GPU-accessible memory. 
> I would prefer handling resolution in a higher level; I don't see a fundamental
> reason we need to move resolution into TextureImage.

I agree with this. I'd rather we not complicate TextureImage further.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Fixed by bug 586683.