Teach ThebesLayerOGL about resolution-scaled drawing

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: cjones, Assigned: mattwoodrow)

Tracking

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.
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.
Attachment #481020 - Flags: review?(vladimir)
Attachment #481020 - Flags: review?(matt.woodrow+bugzilla)
This blocks turning on GL compositing for fennec.
tracking-fennec: --- → ?
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.
Attachment #481020 - Flags: review?(matt.woodrow+bugzilla) → review+
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.
Attachment #481020 - Flags: review?(vladimir) → review-
Scale quadRect instead of anti-scaling texture size.  Also fix an annoying compiler warning.
Attachment #481020 - Attachment is obsolete: true
Attachment #481136 - Flags: review?(vladimir)
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 :-)
Attachment #481137 - Flags: review?(vladimir) → review+
tracking-fennec: ? → 2.0b2+
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)
Assignee: nobody → jones.chris.g
Matt, please steal this from me when your part 2 patch is ready to go.
tracking-fennec: 2.0b2+ → 2.0b3+
Whiteboard: [fennec-checkin-postb2][has-patch]
The first part of this bug already landed.
Whiteboard: [fennec-checkin-postb2][has-patch]
Part 2 of this doesn't block fennec b3, because that's only really relevant on desktop.
Assignee: jones.chris.g → matt.woodrow+bugzilla
blocking2.0: --- → ?
tracking-fennec: 2.0b3+ → ---
I don't think this blocks at all, then, unless I'm missing something?
blocking2.0: ? → -
(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?
This is the GL part of bug 586683, which is currently blocking-final.
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.
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.
Attachment #491743 - Flags: feedback?(jones.chris.g)
Attachment #491742 - Flags: review?(vladimir)
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.
Attachment #491743 - Flags: feedback?(vladimir)
Attachment #491743 - Flags: feedback?(jones.chris.g)
Attachment #491743 - Flags: feedback-
(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.
Fixed by bug 586683.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Attachment #491742 - Flags: review?(vladimir)
Attachment #491743 - Flags: feedback?(vladimir)
You need to log in before you can comment on or make changes to this bug.