Description was changed from ========== Use 'int' content quad coordinates all the way. (not for ...
4 years, 5 months ago
(2016-07-01 02:49:19 UTC)
#1
Description was changed from
==========
Use 'int' content quad coordinates all the way. (not for commit)
BUG=623198
==========
to
==========
Use 'int' content quad coordinates all the way. (not for commit)
BUG=623198
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========
vmiura
Yet another way to improve rendering on Issue 623198 http://jason-kenny.com/, which fixes the bugs on ...
4 years, 5 months ago
(2016-07-01 03:02:52 UTC)
#2
Yet another way to improve rendering on Issue 623198 http://jason-kenny.com/,
which fixes the bugs on desktop but not on Android.
It's possible they're separate bugs that manifest in the same way.
I'm not totally familiar with our GLRenderer, but it seems to me we have many
ways of computing the transformed quad coordinates; sometimes we bake it in the
transform matrix, sometimes we pass the original quad coordinates in a uniform
like the code modified in this CL.
IMO this could use a rewrite, appending the quad vertex properties to a vertex
buffer, with consistent int -> float conversions, and referencing that. This
could also simplify the uniform indexing in VertexShaderPosTexTransform, which
may also be more performant.
enne (OOO)
Yeah, at some point in time GLRenderer only did it the shared geometry buffer way ...
4 years, 5 months ago
(2016-07-01 17:05:08 UTC)
#3
Yeah, at some point in time GLRenderer only did it the shared geometry buffer
way and kept the same vertex buffer bound the whole time. Antialiased quads and
then bsp trees changed that, and I don't think this has ever quite been cleaned
up entirely. Sorry for the mess.
I don't think this is ultimately going to fix the issue though, as you go right
back into floating point here and so you have to cancel out the draw transform
and the offset in a floating point space. I think cancelling out the large
translation in integer space back in PictureLayerImpl is going to be a more
numerically reliable solution.
vmiura
On 2016/07/01 17:05:08, enne wrote: > Yeah, at some point in time GLRenderer only did ...
4 years, 5 months ago
(2016-07-01 17:52:27 UTC)
#4
On 2016/07/01 17:05:08, enne wrote:
> Yeah, at some point in time GLRenderer only did it the shared geometry buffer
> way and kept the same vertex buffer bound the whole time. Antialiased quads
and
> then bsp trees changed that, and I don't think this has ever quite been
cleaned
> up entirely. Sorry for the mess.
>
> I don't think this is ultimately going to fix the issue though, as you go
right
> back into floating point here and so you have to cancel out the draw transform
> and the offset in a floating point space.
If the transform matrix is based only on shared quad state, and the quad
coordinates truncated to fp32 consistently, then I think the geometry can be
water tight even with large offsets. Water tight but maybe jittery...
>I think cancelling out the large
> translation in integer space back in PictureLayerImpl is going to be a more
> numerically reliable solution.
Yeah, I agree ultimately we should do this. There could still be numerical
differences in the renderer, if we have a funky scale for example, but in
practice that seems to not occur.
enne (OOO)
I don't see this bug on Android on ToT, at least in content shell. What ...
4 years, 5 months ago
(2016-07-18 22:39:48 UTC)
#5
I don't see this bug on Android on ToT, at least in content shell. What are you
doing to reproduce it there?
sohanjg
On 2016/07/18 22:39:48, enne wrote: > I don't see this bug on Android on ToT, ...
4 years, 5 months ago
(2016-07-19 09:07:25 UTC)
#6
On 2016/07/18 22:39:48, enne wrote:
> I don't see this bug on Android on ToT, at least in content shell. What are
you
> doing to reproduce it there?
I have added a repro video in the bug, on Chrome public apk.
enne (OOO)
I've come around to thinking that maybe this is the best way to fix this. ...
4 years, 4 months ago
(2016-07-27 20:33:01 UTC)
#7
I've come around to thinking that maybe this is the best way to fix this.
Adjusting PictureLayerImpl geometry rects started looking complicated. lgtm if
you want to land this with a comment about why you're calculating the
coordinates that way.
vmiura
On 2016/07/27 20:33:01, enne wrote: > I've come around to thinking that maybe this is ...
4 years, 4 months ago
(2016-07-29 23:27:37 UTC)
#8
On 2016/07/27 20:33:01, enne wrote:
> I've come around to thinking that maybe this is the best way to fix this.
> Adjusting PictureLayerImpl geometry rects started looking complicated. lgtm
if
> you want to land this with a comment about why you're calculating the
> coordinates that way.
I can go ahead and land this, but in my mind adjusting rects could catch more
cases. What was tricky about applying an offset?
enne (OOO)
On 2016/07/29 at 23:27:37, vmiura wrote: > On 2016/07/27 20:33:01, enne wrote: > > I've ...
4 years, 4 months ago
(2016-08-08 18:46:40 UTC)
#9
On 2016/07/29 at 23:27:37, vmiura wrote:
> On 2016/07/27 20:33:01, enne wrote:
> > I've come around to thinking that maybe this is the best way to fix this.
> > Adjusting PictureLayerImpl geometry rects started looking complicated. lgtm
if
> > you want to land this with a comment about why you're calculating the
> > coordinates that way.
>
> I can go ahead and land this, but in my mind adjusting rects could catch more
cases. What was tricky about applying an offset?
Maybe I'm just not thinking about the math right, but it wasn't clear to me what
offset to apply in all cases other than just a large translation. Your solution
also seemed much simpler than adding extra logic into the already long
AppendQuads function or worse, into the CoverageIterator.
Issue 2109333005: Use 'int' content quad coordinates all the way. (not for commit)
Created 4 years, 5 months ago by vmiura
Modified 4 years, 4 months ago
Reviewers:
Base URL: https://chromium.googlesource.com/chromium/src.git@master
Comments: 0