Bug 73266 – [Chromium] Avoid ASSERT_NOT_REACHED() from creating FBO with content texture of size 0

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Daniel Sievers]

[Chromium] Avoid ASSERT_NOT_REACHED() from creating FBO with content texture of size 0

[Daniel Sievers]

Created attachment 116849 [details]
Patch

[Daniel Sievers]

Hrm on second though, we might want to have that check also before this early return, right?

    // If neither this layer nor any of its children were added, early out.
    if (sortingStartIndex == descendants.size())
        return;

Do we need to duplicate the logic instead where we call renderSurface->clearLayerList()?

(In reply to comment #1)
> Created an attachment (id=116849) [details]
> Patch

[Adrienne Walker]

Comment on attachment 116849 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:383
>      // If neither this layer nor any of its children were added, early out.
>      if (sortingStartIndex == descendants.size())
>          return;

This little patch of code should move too, because descendants.size() could be zero, causing a problem for descendants.at(0) in the sort at the end.

[Vangelis Kokkevis]

Comment on attachment 116849 [details]
Patch

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

Don't you also need to clear the layer's RS at the drawOpacity == 0 early-out?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:382
>      if (sortingStartIndex == descendants.size())

You'll also want to clear the RS here. Maybe it would be best to split the cleanup into a separate function that calls:

if (layer->renderSurface() && layer != rootLayer)
 renderSurfaceList.remove(layer->renderSurface());
layer->clearRenderSurface();

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:446
> +            renderSurfaceLayerList.removeLast();

You can nest this inside the previous 
if (layer->renderSurface() && layer != rootLayer) {} clause .

[Daniel Sievers]

Created attachment 116863 [details]
Patch

[Daniel Sievers]

(In reply to comment #3)
> (From update of attachment 116849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116849&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:383
> >      // If neither this layer nor any of its children were added, early out.
> >      if (sortingStartIndex == descendants.size())
> >          return;
> 
> This little patch of code should move too, because descendants.size() could be zero, causing a problem for descendants.at(0) in the sort at the end.

Done.

[Daniel Sievers]

(In reply to comment #4)
> (From update of attachment 116849 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116849&action=review
> 
> Don't you also need to clear the layer's RS at the drawOpacity == 0 early-out?
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:382
> >      if (sortingStartIndex == descendants.size())
> 
> You'll also want to clear the RS here. Maybe it would be best to split the cleanup into a separate function that calls:
> 


I'll deal with resetting the render surfaces for the early returns separately in https://bugs.webkit.org/show_bug.cgi?id=73270. I uploaded a different test there that exemplifies the problem.


> if (layer->renderSurface() && layer != rootLayer)
>  renderSurfaceList.remove(layer->renderSurface());
> layer->clearRenderSurface();
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.cpp:446
> > +            renderSurfaceLayerList.removeLast();
> 
> You can nest this inside the previous 
> if (layer->renderSurface() && layer != rootLayer) {} clause .


Done.

[James Robinson]

Comment on attachment 116863 [details]
Patch

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

Vangelis or Enne, could you give this another look?

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:480
> +    renderSurface1->setOpacity(0.9f);

drop the trailing 'f' - see http://www.webkit.org/coding/coding-style.html "Floating point literals"

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:483
> +    setLayerPropertiesForTesting(renderSurface1.get(), identityMatrix, identityMatrix, FloatPoint(0.0f, 0.0f), FloatPoint(0.0f, 0.0f), IntSize(10, 10), false);

the 0.0f isn't needed here. FloatPoint() or FloatPoint::zero() would be even better

[Adrienne Walker]

Comment on attachment 116863 [details]
Patch

Looks good to me.

[Vangelis Kokkevis]

Comment on attachment 116863 [details]
Patch

Looks good

[James Robinson]

Comment on attachment 116863 [details]
Patch

R=me in that case

[Daniel Sievers]

Created attachment 116996 [details]
Patch

[Daniel Sievers]

James, could you cq+ for me in case your comments are addressed? thanks!

(In reply to comment #12)
> Created an attachment (id=116996) [details]
> Patch

[WebKit Review Bot]

Comment on attachment 116996 [details]
Patch

Clearing flags on attachment: 116996

Committed r101472: <http://trac.webkit.org/changeset/101472>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.