Bug 40376 – [Qt] Sometimes I see pixel dust when the transform is changing rapidly with Qt graphics layers

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKit Qt

[Sam Magnuson]

It appears that graphicsview isn't taking into account the transform (since that is where the position lives in the webgraphicsview) when handleGeometryChange happens. I think this is what the quick fix in there was seeing as well.

[Sam Magnuson]

Created attachment 58273 [details]
Adding a handleGeometryChange() to correct this problem.

[Sam Magnuson]

Created attachment 58555 [details]
Proposed patch with proper coding style

[Jędrzej Nowacki]

Comment on attachment 58555 [details]
Proposed patch with proper coding style

I don't know about problem, but this patch won't be accepted because it doesn't apply (cq-) and have multiple coding style issues (r-). You can use WebKitTools/Scripts/check-coding-style script to find some coding style problems. 

Actually I should close this bug report as invalid, as it is not possible to reproduce the problem, please provide us a test case :-). Good luck!

> +        No new tests. (OOPS!)
It is possible to add a new autotest?

> +            if (QGraphicsScene *s = scene()) {
'*' is in a wrong place.

> -    m_backingStoreKey = QPixmapCache::insert(pixmap);    
> +    m_backingStoreKey = QPixmapCache::insert(pixmap);
Spaces at the end of the line

> @@ -398,7 +410,10 @@ void GraphicsLayerQtImpl::updateTransform()
>      if (!inverseOk)
>          return;
>  
> -    setTransform(transform2D);
> +        if ( transform2D != transform() ) {
> +                handleGeometryChange();
> +                setTransform(transform2D);
> +        }
Wrong intendation

> @@ -537,20 +552,23 @@ void GraphicsLayerQtImpl::flushChanges(bool recursive, bool forceUpdateTransform
>  
>      if (m_changeMask & SizeChange) {
>          if (m_layer->size() != m_state.size) {
> -            prepareGeometryChange();
> +                        handleGeometryChange();
Wrong indentation

> +#if 0
>      // FIXME: this is a hack, due to a probable QGraphicsScene bug when rapidly modifying the perspective
>      // but without this line we get graphic artifacts
>      if ((m_changeMask & ChildrenTransformChange) && m_state.childrenTransform != m_layer->childrenTransform())
>          if (scene())
>              scene()->update();
> +#endif
If you believe that this code in unneeded you should remove it.

> +        // handleGeometryChange();
It is not a proper comment.

>      if (!recursive)
> -        return;    
> +        return;
Spaces at the end of line.

> -        }        
> +        }
Spaces at the end of line.

> +#if 1
>          // FIXME: this is a hack, due to a probable QGraphicsScene bug.
>          // Without this the opacity change doesn't always have immediate effect.
>          if (m_layer.data()->scene() && !m_layer.data()->opacity() && opacity)
>              m_layer.data()->scene()->update();
> +#endif
#if 1 is not pointless as 1 == true, so this code will be always included.

[Jędrzej Nowacki]

(In reply to comment #3)
> #if 1 is not pointless as 1 == true, so this code will be always included.
Oops, I mean that you can remove the #if 1 :-)

[Jędrzej Nowacki]

Please provide a test case fro the bug

[Sam Magnuson]

(In reply to comment #3)
> (From update of attachment 58555 [details])
> I don't know about problem, but this patch won't be accepted because it doesn't apply (cq-) and have multiple coding style issues (r-). You can use WebKitTools/Scripts/check-coding-style script to find some coding style problems. 
> 
> Actually I should close this bug report as invalid, as it is not possible to reproduce the problem, please provide us a test case :-). Good luck!
> 
> > +        No new tests. (OOPS!)
> It is possible to add a new autotest?
> 

I don't have an autotest, it was clearly a corner case that had the pixels left uncovered. I very much doubt I'll get a case to cover it - but can try later. Can you point me to a autotest that does pixel comparisons?

> > +            if (QGraphicsScene *s = scene()) {
> '*' is in a wrong place.
> 

For whatever its worth check-webkit-style doesn't flag it, thus I didn't spot it. Fixed.

> > -    m_backingStoreKey = QPixmapCache::insert(pixmap);    
> > +    m_backingStoreKey = QPixmapCache::insert(pixmap);
> Spaces at the end of the line
> 

Why do you want spaces at the end of the line?

> > @@ -398,7 +410,10 @@ void GraphicsLayerQtImpl::updateTransform()
> >      if (!inverseOk)
> >          return;
> >  
> > -    setTransform(transform2D);
> > +        if ( transform2D != transform() ) {
> > +                handleGeometryChange();
> > +                setTransform(transform2D);
> > +        }
> Wrong intendation
> 

Fixed, check-webkit-style didn't find this either. 

> > @@ -537,20 +552,23 @@ void GraphicsLayerQtImpl::flushChanges(bool recursive, bool forceUpdateTransform
> >  
> >      if (m_changeMask & SizeChange) {
> >          if (m_layer->size() != m_state.size) {
> > -            prepareGeometryChange();
> > +                        handleGeometryChange();
> Wrong indentation
> 

Thanks.

> > +#if 0
> >      // FIXME: this is a hack, due to a probable QGraphicsScene bug when rapidly modifying the perspective
> >      // but without this line we get graphic artifacts
> >      if ((m_changeMask & ChildrenTransformChange) && m_state.childrenTransform != m_layer->childrenTransform())
> >          if (scene())
> >              scene()->update();
> > +#endif
> If you believe that this code in unneeded you should remove it.
> 

This was for No'am to experiment with as I think my fix removes the need for the above "hack" as they are solving a similar problem. 

> > +        // handleGeometryChange();
> It is not a proper comment.
> 

In this case it isn't needed, but the review seems scattershot. I can't use #if 0 to leave in code that will still need to be experimented with, I can't use c comments to leave in code that warrants further examination. I see other places in the code base where a line of code is commented out, this doesn't seem to be a real issue.

> >      if (!recursive)
> > -        return;    
> > +        return;
> Spaces at the end of line.
> 
> > -        }        
> > +        }
> Spaces at the end of line.
> 

Again, I don't see why you want the spaces at the end of the line.

> > +#if 1
> >          // FIXME: this is a hack, due to a probable QGraphicsScene bug.
> >          // Without this the opacity change doesn't always have immediate effect.
> >          if (m_layer.data()->scene() && !m_layer.data()->opacity() && opacity)
> >              m_layer.data()->scene()->update();
> > +#endif
> #if 1 is not pointless as 1 == true, so this code will be always included.

Agree the #if 1 here is unnecessary - its a piece of code I never triggered so I left in No'am's "hack" to work around the issue since I couldn't confirm whether it was still necessary. My hope was that No'am would test if it was still necessary in the test that required it and possibly remove it.

[Sam Magnuson]

Created attachment 58802 [details]
Proposed patch with proper coding style and commented on the test case in the changelog

[Jędrzej Nowacki]

(In reply to comment #6)
> (In reply to comment #3)
> > (From update of attachment 58555 [details] [details])
> > Actually I should close this bug report as invalid, as it is not possible to reproduce the problem, please provide us a test case :-). Good luck!
> >
> > > +        No new tests. (OOPS!)
> > It is possible to add a new autotest?
> I don't have an autotest, it was clearly a corner case that had the pixels left uncovered. I very much doubt I'll get a case to cover it - but can try later. Can you point me to a autotest that does pixel comparisons?
I can't as you didn't provide a test case, I can't reproduce it. I'm not sure what you want to fix, but maybe you should look at graphicsview autotest (part of Qt).

> > Spaces at the end of the line
> Why do you want spaces at the end of the line?
Sorry, my message wasn't clear. I don't want spaces at the end of line, but you shouldn't change it, as it is not related to the pixel dust problem. Actually the rule is more general a patch should touch only relevant code and only fix one issue.

> > > +#if 0
> > If you believe that this code in unneeded you should remove it.
> This was for Noam to experiment with as I think my fix removes the need for the above "hack" as they are solving a similar problem.

> > > +#if 1
> Agree the #if 1 here is unnecessary - its a piece of code I never triggered so I left in Noam's "hack" to work around the issue since I couldn't confirm whether it was still necessary. My hope was that Noam would test if it was still necessary in the test that required it and possibly remove it.
Great, so we need Noam's expertise :-)
Preferably, we avoid #if 0/1. If you feel that a code could be removed then you can remove it, if code have to stay then you shouldn't touch it. If you need to share some experimental code, try to do it in other branch unless it is something really big. WebKit code base is huge and it is difficult to maintain it so lets try to keep it simple :-)

[Sam Magnuson]

Created attachment 59431 [details]
Rediff against trunk

[Kenneth Rohde Christiansen]

Comment on attachment 59431 [details]
Rediff against trunk

Almost there, just a few nits

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:171
 +      void    handleGeometryChange()
wrong coding style... no need to add more than one space after void.

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:174
 +              // I do this here because it seems QGraphicsView doesn't seem to take into account the translated bounding rect when prepareGeometryChange happens
Add a dot at the end. Personally I would wrap it at the middle

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:176
 +                  const QRectF g = mapRectToScene(boundingRect());
g ? geometry?

WebCore/platform/graphics/qt/GraphicsLayerQt.cpp:453
 +      if ( transform2D != transform() ) {
Remove space after transform2D and after transform()

[Noam Rosenthal]

We're rewriting AC in a cross-platform manner. New features won't be added to GraphicsLayerQt.

[Kenneth Rohde Christiansen]

(In reply to comment #11)
> We're rewriting AC in a cross-platform manner. New features won't be added to GraphicsLayerQt.

So this is not interesting for the qtwebkit2.0.x branch?

[Noam Rosenthal]

It is, but it's a corner case and in low priority to getting the major ones work faster (which would happen after 2.0/2.1).

[Kenneth Rohde Christiansen]

(In reply to comment #13)
> It is, but it's a corner case and in low priority to getting the major ones work faster (which would happen after 2.0/2.1).

OK, I will let that up to you then. Please take a look at AC patches on webkit.org/pending-review then to WONTFIX those needed.