Issue 6049046: fix error when mask filter is used with drawBitmapRect (Closed)
Descriptionfix error when mask filter is used with drawBitmapRect
BUG=
TEST=
Patch Set 1 #Patch Set 2 : new fix #Patch Set 3 : fixup #
MessagesTotal messages: 12
Hunting the bug for one day, and finally I got some clues: the first texture's matrix should be preconcated the correct inverse matrix, therefore when in GLSL shader, we can get the correct texture coordinate. However, in this case, context's matrix is not the correct one. We have to use SkDraw's matrix. As this is the core part of GPU code, please help review and hopefully I'm not misunderstood here. :)
Sign in to reply to this message.
gm change lgtm
Sign in to reply to this message.
On 2012/04/17 12:34:39, reed1 wrote: > gm change lgtm Guanqun, do you have a repro case for the bug. I'd like to understand why the context's matrix is not the correct matrix to invert. Is it the case when drawBitmap is passed a non-identity additional matrix param?
Sign in to reply to this message.
On 2012/04/17 13:03:03, bsalomon wrote: > On 2012/04/17 12:34:39, reed1 wrote: > > gm change lgtm > > Guanqun, do you have a repro case for the bug. I'd like to understand why the > context's matrix is not the correct matrix to invert. Is it the case when > drawBitmap is passed a non-identity additional matrix param? The gm test case I added in this CL can be reproduced to show the bug. Yes, at that moment, the matrix param passed is not an identity matrix. For the gm case I added, here's info: matrix param: [ 21.3333 0.0000 485.0000][ 0.0000 21.3333 480.0000][ 0.0000 0.0000 1.0000] contex matrix: [ 1.0000 0.0000 485.0000][ 0.0000 1.0000 480.0000][ 0.0000 0.0000 1.0000]
Sign in to reply to this message.
Adding junov who added that part of code in SkGpuDevice::drawBitmap() According to my understanding, GrContext's matrix should be the same with SkDraw's matrix, so in this case, when a new SkDraw is used with a new matrix, it should do the same thing to GrContext's matrix. (My previous fix is to use SkDraw's matrix in drawWithMaskFilter() which I think is not elegant enough.) The added gm case shows that without this patch, it could result in wrong rendering result. Please review. Thanks!
Sign in to reply to this message.
I don't think this is the right fix for this problem. I think the problem is
that the context's matrix is getting used when it shouldn't . I tried to find
where the context matrix was being used. I think the problem is in function
drawMaskWithFilter:
GrMatrix ivm = context->getMatrix();
(...)
if (grp->hasTextureOrMask() && ivm.invert(&ivm)) {
I think it should be:
GrMatrix ivm;
if (grp->hasTextureOrMask() && matrix.invert(&ivm)) {
This would be consistent with the behavior of drawWithGPUMaskFilter
On 2012/04/24 04:50:04, guanqun wrote:
> Adding junov who added that part of code in SkGpuDevice::drawBitmap()
>
> According to my understanding, GrContext's matrix should be the same with
> SkDraw's matrix, so in this case, when a new SkDraw is used with a new matrix,
> it should do the same thing to GrContext's matrix. (My previous fix is to use
> SkDraw's matrix in drawWithMaskFilter() which I think is not elegant enough.)
>
> The added gm case shows that without this patch, it could result in wrong
> rendering result.
>
> Please review. Thanks!
Sign in to reply to this message.
On Tue, Apr 24, 2012 at 10:03 PM, <junov@chromium.org> wrote: > GrMatrix ivm = context->getMatrix(); Thanks for your comments, so could you please take a look at my first patchset in this CL, do you think it's the right fix? I changed the above line in drawWithMaskFilter to the following: GrMatrix ivm = matrix; So, I understand that the matrix of SkDraw and the matrix of GrContext could differ AT THE TIME SkGpuDevice starts to call GrContext scope, is it right? -- Guanqun
Sign in to reply to this message.
On 2012/04/24 14:20:04, guanqun.lu_gmail.com wrote: > On Tue, Apr 24, 2012 at 10:03 PM, <mailto:junov@chromium.org> wrote: > > GrMatrix ivm = context->getMatrix(); > > Thanks for your comments, so could you please take a look at my first > patchset in this CL, do you think it's the right fix? I changed the > above line in drawWithMaskFilter to the following: > GrMatrix ivm = matrix; > > So, I understand that the matrix of SkDraw and the matrix of GrContext > could differ AT THE TIME SkGpuDevice starts to call GrContext scope, > is it right? > > -- > Guanqun I think the lastest patch is a better fix. I also think we should get rid of the matrix params to the two mask drawing functions and take your subsequent patch that adds an assertion about matrix. It seems we are redundantly specifying the matrix. I will try this and see if we pass GM assert-free.
Sign in to reply to this message.
On 2012/04/24 14:20:04, guanqun.lu_gmail.com wrote: > On Tue, Apr 24, 2012 at 10:03 PM, <mailto:junov@chromium.org> wrote: > > GrMatrix ivm = context->getMatrix(); > > Thanks for your comments, so could you please take a look at my first > patchset in this CL, do you think it's the right fix? I changed the > above line in drawWithMaskFilter to the following: > GrMatrix ivm = matrix; > > So, I understand that the matrix of SkDraw and the matrix of GrContext > could differ AT THE TIME SkGpuDevice starts to call GrContext scope, > is it right? > > -- > Guanqun Yes, I think your first patch was a better solution. In general, context objects should store persistent state, while temporary objects like the SkDraw in this case are more appropriate for storing temporary state adjustments. In the fisrt patch, I would move the declaration of ivm closer to where the variable is used, and it is unnecessary to initialize ivm if you re-write the inversion as: matrix.invert(&ivm)
Sign in to reply to this message.
On 2012/04/24 14:50:46, junov1 wrote: > Yes, I think your first patch was a better solution. In general, context > objects should store persistent state, while temporary objects like the SkDraw > in this case are more appropriate for storing temporary state adjustments. Wow, it's interesting to see you and Brian have different opinions on this one. :) Personally, I like my latest patchset, because it's kind of more harmony "both have the same matrix"... Quoting: "context objects should store persistent state" I have a question about your statement, I've seen lots of places where we set the matrix in GrContext to identity and do the drawing afterwards. Is this behaviour contrary to persistent philosophy?
Sign in to reply to this message.
On 2012/04/24 15:06:12, guanqun wrote: > On 2012/04/24 14:50:46, junov1 wrote: > > Yes, I think your first patch was a better solution. In general, context > > objects should store persistent state, while temporary objects like the SkDraw > > in this case are more appropriate for storing temporary state adjustments. > > Wow, it's interesting to see you and Brian have different opinions on this one. > :) > > Personally, I like my latest patchset, because it's kind of more harmony "both > have the same matrix"... > > Quoting: > "context objects should store persistent state" > > I have a question about your statement, I've seen lots of places where we set > the matrix in GrContext to identity and do the drawing afterwards. Is this > behaviour contrary to persistent philosophy? Yeah, I was discussing this with Brian earlier. There are some deeper design flaws that need to be addressed. He will come back to you with a more sound proposal. It's confusing that we have two different places where we store the view matrix. Even more confusing when the two are not synchronized. Right now, the SkDraw is the vehicle used by SkCanvas to communicate drawing state tweaks to the SkDevice when we are using certain types of effects (like SkBlurMaskFilter). Therefore, the state expressed in the SkDraw must override the state stored in GrContext. That begs the question: why do we even store a matrix in GrContext then? I take back what I said about GrContext storing persistent state. SkCanvas is the place where persistent matrix and clipping state are kept. For what it's worth I think the latest patch set fails to fix the same problem with drawRect and drawPath, while the first patch fixed all cases.
Sign in to reply to this message.
On 2012/04/24 19:26:56, junov1 wrote: > On 2012/04/24 15:06:12, guanqun wrote: > > On 2012/04/24 14:50:46, junov1 wrote: > > > Yes, I think your first patch was a better solution. In general, context > > > objects should store persistent state, while temporary objects like the > SkDraw > > > in this case are more appropriate for storing temporary state adjustments. > > > > Wow, it's interesting to see you and Brian have different opinions on this > one. > > :) > > > > Personally, I like my latest patchset, because it's kind of more harmony "both > > have the same matrix"... > > > > Quoting: > > "context objects should store persistent state" > > > > I have a question about your statement, I've seen lots of places where we set > > the matrix in GrContext to identity and do the drawing afterwards. Is this > > behaviour contrary to persistent philosophy? > > Yeah, I was discussing this with Brian earlier. There are some deeper design > flaws that need to be addressed. He will come back to you with a more sound > proposal. > > It's confusing that we have two different places where we store the view matrix. > Even more confusing when the two are not synchronized. Right now, the SkDraw > is the vehicle used by SkCanvas to communicate drawing state tweaks to the > SkDevice when we are using certain types of effects (like SkBlurMaskFilter). > Therefore, the state expressed in the SkDraw must override the state stored in > GrContext. That begs the question: why do we even store a matrix in GrContext > then? > > I take back what I said about GrContext storing persistent state. SkCanvas is > the place where persistent matrix and clipping state are kept. > > For what it's worth I think the latest patch set fails to fix the same problem > with drawRect and drawPath, while the first patch fixed all cases. I landed the first patch and filed a bug about the deeper problem: http://code.google.com/p/skia/issues/detail?id=586.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
