Bug 63847

Summary: [chromium] Externalize layer visibility calculation
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: enne, kbr, nduca, senorblanco, vangelis
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch senorblanco: review+

Description James Robinson 2011-07-01 14:20:22 PDT
[chromium] Externalize layer visibility calculation
Comment 1 James Robinson 2011-07-01 14:47:57 PDT
Created attachment 99526 [details]
Patch
Comment 2 Adrienne Walker 2011-07-01 15:36:23 PDT
Comment on attachment 99526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review

A few small questions.  In general, I really like where this is going.  :)

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:151
>  TransformationMatrix ContentLayerChromium::tilingTransform()

Can this get pushed into LayerChromium? It'd help with your ImageLayer/ContentLayer split.  It's also not really a tiling transform anymore.  It's now the scale that needs to be applied to the contents, so maybe it needs a slightly different name.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:202
> -void ContentLayerChromium::draw(const IntRect& targetSurfaceRect)
> +void ContentLayerChromium::draw()
>  {
> -    const TransformationMatrix transform = tilingTransform();
> -    IntRect layerRect = visibleLayerRect(targetSurfaceRect);
> +    const IntRect& layerRect = visibleLayerRect();
>      if (!layerRect.isEmpty())
> -        m_tiler->draw(layerRect, transform, ccLayerImpl()->drawOpacity(), m_textureUpdater.get());
> +        m_tiler->draw(layerRect, tilingTransform(), ccLayerImpl()->drawOpacity(), m_textureUpdater.get());
>  }

Maybe I'm just not following this, but tilingTransform() appears to longer include the translation of half the layer bounds so that the origin is the top left corner.  How does this continue to work the same way as before this patch without any changes to LayerTilerChromium?
Comment 3 James Robinson 2011-07-01 15:58:04 PDT
Comment on attachment 99526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:202
>>  }
> 
> Maybe I'm just not following this, but tilingTransform() appears to longer include the translation of half the layer bounds so that the origin is the top left corner.  How does this continue to work the same way as before this patch without any changes to LayerTilerChromium?

there's the translate in tilingTransform() line 159 in this patch. is that what you mean?
Comment 4 Adrienne Walker 2011-07-01 16:02:35 PDT
(In reply to comment #3)
> (From update of attachment 99526 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:202
> >>  }
> > 
> > Maybe I'm just not following this, but tilingTransform() appears to longer include the translation of half the layer bounds so that the origin is the top left corner.  How does this continue to work the same way as before this patch without any changes to LayerTilerChromium?
> 
> there's the translate in tilingTransform() line 159 in this patch. is that what you mean?

Oh, yes.  I was just misreading the diff.  :)
Comment 5 James Robinson 2011-07-01 16:33:25 PDT
(In reply to comment #2)
> (From update of attachment 99526 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review
> 
> A few small questions.  In general, I really like where this is going.  :)
> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:151
> >  TransformationMatrix ContentLayerChromium::tilingTransform()
> 
> Can this get pushed into LayerChromium? It'd help with your ImageLayer/ContentLayer split.  It's also not really a tiling transform anymore.  It's now the scale that needs to be applied to the contents, so maybe it needs a slightly different name.
> 

That's a good point.  The main reason I left it on ContentLayerChromium is I'm still not entirely sure how the tiling transform and the draw transform are supposed to interact.  My initial instinct was to just do all of this in the draw transform, but I couldn't get the math worked out properly for sublayers, and maybe they are separate concepts after all.

Anyway leaving it down on ContentLayerChromium is a slightly smaller delta from the status quo (which is nice) and it actually merged really well with the content/image layer split (I've merged together all the branches locally to make sure).
Comment 6 Adrienne Walker 2011-07-01 17:37:46 PDT
(In reply to comment #5)
> (In reply to comment #2)
> > (From update of attachment 99526 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review
> > 
> > A few small questions.  In general, I really like where this is going.  :)
> > 
> > > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:151
> > >  TransformationMatrix ContentLayerChromium::tilingTransform()
> > 
> > Can this get pushed into LayerChromium? It'd help with your ImageLayer/ContentLayer split.  It's also not really a tiling transform anymore.  It's now the scale that needs to be applied to the contents, so maybe it needs a slightly different name.
> > 
> 
> That's a good point.  The main reason I left it on ContentLayerChromium is I'm still not entirely sure how the tiling transform and the draw transform are supposed to interact.  My initial instinct was to just do all of this in the draw transform, but I couldn't get the math worked out properly for sublayers, and maybe they are separate concepts after all.

We could certainly clean up that distinction in a separate patch.

> Anyway leaving it down on ContentLayerChromium is a slightly smaller delta from the status quo (which is nice) and it actually merged really well with the content/image layer split (I've merged together all the branches locally to make sure).

In that case, that sounds fine to me.

Unofficially, it looks good to me.
Comment 7 James Robinson 2011-07-01 18:53:27 PDT
Cool, thanks!  Ken or Stephen - would you mind giving this an official review?  Thanks.
Comment 8 Stephen White 2011-07-04 09:00:06 PDT
Comment on attachment 99526 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review

I leave it up to you to decide if my comments are relevant here.

Looks good.  r=me.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:156
> +    transform.scaleNonUniform(bounds().width() / static_cast<double>(contentBounds().width()),
> +                              bounds().height() / static_cast<double>(contentBounds().height()));

This could be a div-zero if contentBounds() has zero width or height.  Dunno if that's an issue here.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:370
> +    transform.scaleNonUniform(bounds.width() / static_cast<double>(contentBounds.width()),
> +                              bounds.height() / static_cast<double>(contentBounds.height()));

Same as above; potential div-zero.

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:449
> +                    maskLayer->setVisibleLayerRect(IntRect(IntPoint(), maskLayer->contentBounds()));
> +                    maskLayer->paintContentsIfDirty();

This function-pair is called four times here; could potentially be refactored.
Comment 9 James Robinson 2011-07-06 13:24:11 PDT
(In reply to comment #8)
> (From update of attachment 99526 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=99526&action=review
> 
> I leave it up to you to decide if my comments are relevant here.
> 
> Looks good.  r=me.

Thanks!

> 
> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:156
> > +    transform.scaleNonUniform(bounds().width() / static_cast<double>(contentBounds().width()),
> > +                              bounds().height() / static_cast<double>(contentBounds().height()));
> 
> This could be a div-zero if contentBounds() has zero width or height.  Dunno if that's an issue here.
> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:370
> > +    transform.scaleNonUniform(bounds.width() / static_cast<double>(contentBounds.width()),
> > +                              bounds.height() / static_cast<double>(contentBounds.height()));
> 
> Same as above; potential div-zero.

Good point on both counts, I'll add some early-outs for empty contentBounds().

> 
> > Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:449
> > +                    maskLayer->setVisibleLayerRect(IntRect(IntPoint(), maskLayer->contentBounds()));
> > +                    maskLayer->paintContentsIfDirty();
> 
> This function-pair is called four times here; could potentially be refactored.

I'll add a helper.
Comment 10 James Robinson 2011-07-06 13:56:38 PDT
Committed r90492: <http://trac.webkit.org/changeset/90492>