Bug 40376 - [Qt] Sometimes I see pixel dust when the transform is changing rapidly with Qt graphics layers
Summary: [Qt] Sometimes I see pixel dust when the transform is changing rapidly with Q...
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt, QtTriaged
Depends on:
Blocks: 38744
  Show dependency treegraph
 
Reported: 2010-06-09 11:38 PDT by Sam Magnuson
Modified: 2010-08-14 08:21 PDT (History)
4 users (show)

See Also:


Attachments
Adding a handleGeometryChange() to correct this problem. (6.21 KB, patch)
2010-06-09 12:33 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
Proposed patch with proper coding style (6.40 KB, patch)
2010-06-12 10:20 PDT, Sam Magnuson
jedrzej.nowacki: review-
jedrzej.nowacki: commit-queue-
Details | Formatted Diff | Diff
Proposed patch with proper coding style and commented on the test case in the changelog (7.31 KB, patch)
2010-06-15 11:58 PDT, Sam Magnuson
no flags Details | Formatted Diff | Diff
Rediff against trunk (4.50 KB, patch)
2010-06-22 16:08 PDT, Sam Magnuson
kenneth: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Magnuson 2010-06-09 11:38:21 PDT
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.
Comment 1 Sam Magnuson 2010-06-09 12:33:57 PDT
Created attachment 58273 [details]
Adding a handleGeometryChange() to correct this problem.
Comment 2 Sam Magnuson 2010-06-12 10:20:24 PDT
Created attachment 58555 [details]
Proposed patch with proper coding style
Comment 3 Jędrzej Nowacki 2010-06-15 01:16:44 PDT
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.
Comment 4 Jędrzej Nowacki 2010-06-15 01:19:00 PDT
(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 :-)
Comment 5 Jędrzej Nowacki 2010-06-15 05:43:19 PDT
Please provide a test case fro the bug
Comment 6 Sam Magnuson 2010-06-15 08:50:43 PDT
(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.
Comment 7 Sam Magnuson 2010-06-15 11:58:37 PDT
Created attachment 58802 [details]
Proposed patch with proper coding style and commented on the test case in the changelog
Comment 8 Jędrzej Nowacki 2010-06-16 03:40:47 PDT
(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 :-)
Comment 9 Sam Magnuson 2010-06-22 16:08:49 PDT
Created attachment 59431 [details]
Rediff against trunk
Comment 10 Kenneth Rohde Christiansen 2010-08-14 07:33:00 PDT
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()
Comment 11 Noam Rosenthal 2010-08-14 08:11:57 PDT
We're rewriting AC in a cross-platform manner. New features won't be added to GraphicsLayerQt.
Comment 12 Kenneth Rohde Christiansen 2010-08-14 08:16:24 PDT
(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?
Comment 13 Noam Rosenthal 2010-08-14 08:19:34 PDT
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).
Comment 14 Kenneth Rohde Christiansen 2010-08-14 08:21:23 PDT
(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.