[TexMap][QT] TexMap does not draw the border of media and webgl elements.

[Dongseong Hwang]

TexMap does not draw the border of media and webgl elements.

[Dongseong Hwang]

Created attachment 117865 [details]
patch

[Noam Rosenthal]

Comment on attachment 117865 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +        GraphicsContext3D only draws the content, not CSS box border and TexMap only
> +        composited the platformLayer of media and webgl elements.
> +        It causes that TexMap did not render the border of following layout test.
> +        LayoutTests/compositing/webgl/webgl-reflection.html

Let's rewrite this...
"GraphicsContext3D only draws the content of the WebGL canvas, not the additional CSS such as the borders. TextureMapper should render the content of a media/webgl layer before drawing the actual canvas."
This makes LayoutTests/compositing/webgl/webgl-reflection.html work.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:318
> +        if (m_currentContent.contentType == MediaContentType && !needsReset)
> +            continue;

Doesn't seem correct. This means that we ignore partial-updates for WebGL/media layers.

[Noam Rosenthal]

> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:318
> > +        if (m_currentContent.contentType == MediaContentType && !needsReset)
> > +            continue;
> 

I think the real problem this tries to solve is that we treat setNeedsDisplay and setContentsNeedsDisplay differently. We should change that.

[Dongseong Hwang]

Created attachment 117955 [details]
patch

[Dongseong Hwang]

I was afraid about ignoring partial-updates for WebGL/media layers, but I did not know how to solve.
Your feedback is what I need.

[Dongseong Hwang]

Created attachment 117958 [details]
patch

[Noam Rosenthal]

Comment on attachment 117955 [details]
patch

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

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:72
> +    m_pendingContent.needsDisplayContents = true;

contentsNeedDisplay instead of needsDisplayContents

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:303
> +    }

This is incorrect.
If content type is not media, we shouldn't use contentsNeedsDisplay at all!

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:123
> +        bool needsDisplayContents;

contentsNeedDisplay

[Noam Rosenthal]

Comment on attachment 117958 [details]
patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:307
> +    if (m_currentContent.contentType == MediaContentType && !m_currentContent.needsDisplay) {
> +        m_currentContent.needsDisplayContents = false;
>          return;
> +    }
> +    if (m_currentContent.contentType != MediaContentType) {
> +        m_currentContent.needsDisplay = m_currentContent.needsDisplayContents;
> +        m_currentContent.needsDisplayContents = false;
> +    }

This is very hard to read. I think you want a temporary variable here instead of modifying m_currentContent.needsDisplay.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.h:123
> +        bool needsDisplayContents;

contentsNeedsDisplay

[Dongseong Hwang]

Created attachment 117964 [details]
patch

[Dongseong Hwang]

Thanks for this declaration.
"If content type is not media, we shouldn't use contentsNeedsDisplay at all!"

[Noam Rosenthal]

Comment on attachment 117964 [details]
patch

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

You can probably get the same result without redundant updates by getting rid of some code :)

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:73
> +        m_pendingContent.needsDisplay = true;

You can probably remove these two lines; otherwise you're causing unnecessary updates.

> Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:301
> -    if (m_currentContent.contentType == MediaContentType)
> +    if (m_currentContent.contentType == MediaContentType && !m_currentContent.needsDisplay)
>          return;

The code would probably work just the same if you remove these lines from the code altogether.

[Dongseong Hwang]

Created attachment 117974 [details]
patch

[Noam Rosenthal]

(In reply to comment #11)
> (From update of attachment 117964 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117964&action=review
> 
> You can probably get the same result without redundant updates by getting rid of some code :)
> 
> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:73
> > +        m_pendingContent.needsDisplay = true;
> 
> You can probably remove these two lines; otherwise you're causing unnecessary updates.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperNode.cpp:301
> > -    if (m_currentContent.contentType == MediaContentType)
> > +    if (m_currentContent.contentType == MediaContentType && !m_currentContent.needsDisplay)
> >          return;
> 
> The code would probably work just the same if you remove these lines from the code altogether.

Correction: You do need the first one, because setContentsNeedsDisplay is also called for images. So, a blend between the two last patches should work (include the stuff around line 73, remove the stuff around 301).

[Dongseong Hwang]

Created attachment 117980 [details]
patch

[Dongseong Hwang]

I thought image and 2d canvas would have problem, so I tested 2d canvas.
However, 2d canvas was rendered properly, so I thought there is something that you know but I don't know. and patched it.

After this patch, I tested 2d canvas again, and there is setContentsToCanvas call every frame, and it cause 2d canvas is rendered every frame.

[WebKit Review Bot]

Comment on attachment 117980 [details]
patch

Clearing flags on attachment: 117980

Committed r102113: <http://trac.webkit.org/changeset/102113>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.