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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=40376





--- Comment #6 from Sam Magnuson <smagnuson at netflix.com>  2010-06-15 08:50:43 PST ---
(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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.