Bug 72964 – [chromium] Set opaque flag for ImageLayerChromium

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

This is similar to code required in RenderImage::foregroundContentsAreaIsOpaqueInRect() in bug #70634

[Dana Jansens]

Created attachment 116237 [details]
Patch

[Dana Jansens]

Created attachment 116257 [details]
Patch

[Dana Jansens]

Setting the opaque flag on the GraphicsLayer allows us to test which layers are opaque, and have a consistent view of the status.

I'd like to make a LayoutTest for this but it requires printing the contentsOpaque flag which is coming in bug #70634.

I will make a new bug that depends on this with a LayoutTest.

[Dana Jansens]

Had insightful conversations with @reveman and @enne, and changing this patch accordingly.

No longer pushing the opaque stuff back into GraphicsLayer, as maintaining consistency with GraphicsLayer is not deemed correct. Instead, each layer can explicitly control its opaque() flag independently of the GraphicsLayer::contentsOpaque value via a new virtual function LayerChromium::updateOpaque().

Then set opaque on Image, Video and Canvas layers appropriately. Unit tests for Image and Video included. (Is the Canvas one worth testing? it's pretty simple.)

[James Robinson]

(In reply to comment #4)
> Had insightful conversations with @reveman and @enne, and changing this patch accordingly.
> 
> No longer pushing the opaque stuff back into GraphicsLayer, as maintaining consistency with GraphicsLayer is not deemed correct. Instead, each layer can explicitly control its opaque() flag independently of the GraphicsLayer::contentsOpaque value via a new virtual function LayerChromium::updateOpaque().
> 
> Then set opaque on Image, Video and Canvas layers appropriately. Unit tests for Image and Video included. (Is the Canvas one worth testing? it's pretty simple.)

Canvases are typically *not* opaque. I'm not sure how you would easily test if a canvas were opaque, that's definitely non-trivial.

For video I'm less sure.

I only see logic here for image - did you intend to upload something for canvas/video?

[Dana Jansens]

Created attachment 117510 [details]
Patch

[Dana Jansens]

(In reply to comment #5)
> Canvases are typically *not* opaque. I'm not sure how you would easily test if a canvas were opaque, that's definitely non-trivial.
> 
> For video I'm less sure.
> 
> I only see logic here for image - did you intend to upload something for canvas/video?

Yeh see new patch, sorry it took a sec to follow.

[James Robinson]

Comment on attachment 117510 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.h:56
> +    void setHasAlpha(bool hasAlpha) { m_hasAlpha = hasAlpha; updateOpaque(); }

all 2d canvases have alpha:

data:text/html;charset=utf-8,%3C!DOCTYPE%20html%3E%0A%3Cdiv%20style%3D%22background-color%3A%20blue%3B%20width%3A50px%3B%20height%3A50px%22%3E%3C%2Fdiv%3E%0A%3Ccanvas%20style%3D%22position%3Aabsolute%3B%20top%3A0px%3B%20left%3A0px%22%20width%3D40%20height%3D40%3E%3C%2Fcanvas%3E%0A%3Cscript%3E%0Avar%20c%3Ddocument.querySelector(%22canvas%22).getContext(%222d%22)%3B%0Ac.fillStyle%3D%22rgba(0%2C128%2C0%2C0.5)%22%3B%0Ac.fillRect(0%2C%200%2C%2040%2C%2040)%3B%0A%3C%2Fscript%3E

We could do some complex logic to try to detect if every pixel of the canvas was actually opaque but that's a bit involved. I think this should go on WebGLLayerChromium since that actually has an 'has alpha' bit.

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:156
> +    bool frameOpaque = m_frameFormat != VideoFrameChromium::RGBA && m_frameFormat != VideoFrameChromium::Invalid && m_frameFormat != VideoFrameChromium::Empty;

are you sure that every video format except for RGBA is fully opaque, always? even videos decoded straight to a texture? i'm not familiar enough with our video stack and video formats to know if this is true

[Dana Jansens]

Comment on attachment 117510 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.h:56
>> +    void setHasAlpha(bool hasAlpha) { m_hasAlpha = hasAlpha; updateOpaque(); }
> 
> all 2d canvases have alpha:
> 
> data:text/html;charset=utf-8,%3C!DOCTYPE%20html%3E%0A%3Cdiv%20style%3D%22background-color%3A%20blue%3B%20width%3A50px%3B%20height%3A50px%22%3E%3C%2Fdiv%3E%0A%3Ccanvas%20style%3D%22position%3Aabsolute%3B%20top%3A0px%3B%20left%3A0px%22%20width%3D40%20height%3D40%3E%3C%2Fcanvas%3E%0A%3Cscript%3E%0Avar%20c%3Ddocument.querySelector(%22canvas%22).getContext(%222d%22)%3B%0Ac.fillStyle%3D%22rgba(0%2C128%2C0%2C0.5)%22%3B%0Ac.fillRect(0%2C%200%2C%2040%2C%2040)%3B%0A%3C%2Fscript%3E
> 
> We could do some complex logic to try to detect if every pixel of the canvas was actually opaque but that's a bit involved. I think this should go on WebGLLayerChromium since that actually has an 'has alpha' bit.

Sounds good. I will move the m_hasAlpha bit down to the WebGL layer.

>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:156
>> +    bool frameOpaque = m_frameFormat != VideoFrameChromium::RGBA && m_frameFormat != VideoFrameChromium::Invalid && m_frameFormat != VideoFrameChromium::Empty;
> 
> are you sure that every video format except for RGBA is fully opaque, always? even videos decoded straight to a texture? i'm not familiar enough with our video stack and video formats to know if this is true

I was actually hoping for verification of this. I was told the YUV formats for example are always opaque, but I'm not sure which others. Who can I ask this?

[David Reveman]

Comment on attachment 117510 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:224
> +    virtual void updateOpaque() { setOpaque(m_contentsOpaque); }

could we remove setOpaque() and make LayerChromium::opaque() virtual instead of adding updateOpaque()?

>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:156
>> +    bool frameOpaque = m_frameFormat != VideoFrameChromium::RGBA && m_frameFormat != VideoFrameChromium::Invalid && m_frameFormat != VideoFrameChromium::Empty;
> 
> are you sure that every video format except for RGBA is fully opaque, always? even videos decoded straight to a texture? i'm not familiar enough with our video stack and video formats to know if this is true

Maybe make this "frameOpaque = m_frameFormat == VideoFrameChromium::YV12 || m_frameFormat == VideoFrameChromium::YV16" instead?

[James Robinson]

One thing that sucks about this whole mess is the ambiguity about GraphicsLayer::*Contents*. Normally GraphicsLayer functions that talk about 'contents' are talking about the GraphicsLayer::m_contentsLayer. For example for an image layer the GraphicsLayer has two PlatformLayers - GraphicsLayer::m_layer has the borders, etc, for the <img> or has drawsContent=false if it's a simple container layer. For chromium GraphicsLayer::setContentsToImage() creates an ImageLayerChromium, in the CoreAnimation implementation that function (indirectly) creates a CALayer called GraphicsLayer::m_contentsLayer and sets the content of that layer to the image.  Most GraphicsLayer::*Content* functions are about the m_contentsLayer. GraphicsLayer::setContentsOpaque is about the GraphicsLayer::m_layer which makes things all confusing as fuck.

I think we should make GraphicsLayer::setContentsOpaque() refer to the m_contentsLayer and have some other function for saying 'the stuff in this GraphicsLayer::m_layer is the thing that is opaque'. If I can catch smfr I'll run this by him.

If we sort that out then I think cross-platform WebKit code can take care of setting the opaque bit for the GraphicsLayer::m_contentsLayer, at least for image and webgl layers. That would be better IMO.

[James Robinson]

Victoria, Scherkus - can you answer the question from comment #9 about which video formats are opaque?

[David Reveman]

(In reply to comment #9)
> (From update of attachment 117510 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117510&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.h:56
> >> +    void setHasAlpha(bool hasAlpha) { m_hasAlpha = hasAlpha; updateOpaque(); }
> > 
> > all 2d canvases have alpha:
> > 
> > data:text/html;charset=utf-8,%3C!DOCTYPE%20html%3E%0A%3Cdiv%20style%3D%22background-color%3A%20blue%3B%20width%3A50px%3B%20height%3A50px%22%3E%3C%2Fdiv%3E%0A%3Ccanvas%20style%3D%22position%3Aabsolute%3B%20top%3A0px%3B%20left%3A0px%22%20width%3D40%20height%3D40%3E%3C%2Fcanvas%3E%0A%3Cscript%3E%0Avar%20c%3Ddocument.querySelector(%22canvas%22).getContext(%222d%22)%3B%0Ac.fillStyle%3D%22rgba(0%2C128%2C0%2C0.5)%22%3B%0Ac.fillRect(0%2C%200%2C%2040%2C%2040)%3B%0A%3C%2Fscript%3E
> > 
> > We could do some complex logic to try to detect if every pixel of the canvas was actually opaque but that's a bit involved. I think this should go on WebGLLayerChromium since that actually has an 'has alpha' bit.
> 
> Sounds good. I will move the m_hasAlpha bit down to the WebGL layer.
> 
> >> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:156
> >> +    bool frameOpaque = m_frameFormat != VideoFrameChromium::RGBA && m_frameFormat != VideoFrameChromium::Invalid && m_frameFormat != VideoFrameChromium::Empty;
> > 
> > are you sure that every video format except for RGBA is fully opaque, always? even videos decoded straight to a texture? i'm not familiar enough with our video stack and video formats to know if this is true
> 
> I was actually hoping for verification of this. I was told the YUV formats for example are always opaque, but I'm not sure which others. Who can I ask this?

We only need to care about the formats we can actually render. Right now that is YV12 and YV16.

[James Robinson]

(In reply to comment #13)
> (In reply to comment #9)
> > (From update of attachment 117510 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117510&action=review
> > 
> > >> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.h:56
> > >> +    void setHasAlpha(bool hasAlpha) { m_hasAlpha = hasAlpha; updateOpaque(); }
> > > 
> > > all 2d canvases have alpha:
> > > 
> > > data:text/html;charset=utf-8,%3C!DOCTYPE%20html%3E%0A%3Cdiv%20style%3D%22background-color%3A%20blue%3B%20width%3A50px%3B%20height%3A50px%22%3E%3C%2Fdiv%3E%0A%3Ccanvas%20style%3D%22position%3Aabsolute%3B%20top%3A0px%3B%20left%3A0px%22%20width%3D40%20height%3D40%3E%3C%2Fcanvas%3E%0A%3Cscript%3E%0Avar%20c%3Ddocument.querySelector(%22canvas%22).getContext(%222d%22)%3B%0Ac.fillStyle%3D%22rgba(0%2C128%2C0%2C0.5)%22%3B%0Ac.fillRect(0%2C%200%2C%2040%2C%2040)%3B%0A%3C%2Fscript%3E
> > > 
> > > We could do some complex logic to try to detect if every pixel of the canvas was actually opaque but that's a bit involved. I think this should go on WebGLLayerChromium since that actually has an 'has alpha' bit.
> > 
> > Sounds good. I will move the m_hasAlpha bit down to the WebGL layer.
> > 
> > >> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:156
> > >> +    bool frameOpaque = m_frameFormat != VideoFrameChromium::RGBA && m_frameFormat != VideoFrameChromium::Invalid && m_frameFormat != VideoFrameChromium::Empty;
> > > 
> > > are you sure that every video format except for RGBA is fully opaque, always? even videos decoded straight to a texture? i'm not familiar enough with our video stack and video formats to know if this is true
> > 
> > I was actually hoping for verification of this. I was told the YUV formats for example are always opaque, but I'm not sure which others. Who can I ask this?
> 
> We only need to care about the formats we can actually render. Right now that is YV12 and YV16.

That seems short-sighted.  For example, earlier this week Ami landed support for externally decoded texture video rendering.

[Dana Jansens]

(In reply to comment #10)
> (From update of attachment 117510 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117510&action=review
> 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:224
> > +    virtual void updateOpaque() { setOpaque(m_contentsOpaque); }
> 
> could we remove setOpaque() and make LayerChromium::opaque() virtual instead of adding updateOpaque()?

I chose to do it this way because deciding if something is opaque could require some amount of work, so it seemed worth it to just save it in a bool on update.  For instance an image currentFrameHasAlpha() function seems completely out of our control and don't want to put it in a code path it doesn't need to be in. What do you think of this?

[Andrew Scherkus]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #9)
> > > (From update of attachment 117510 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=117510&action=review
> > > 
> > > >> Source/WebCore/platform/graphics/chromium/CanvasLayerChromium.h:56
> > > >> +    void setHasAlpha(bool hasAlpha) { m_hasAlpha = hasAlpha; updateOpaque(); }
> > > > 
> > > > all 2d canvases have alpha:
> > > > 
> > > > data:text/html;charset=utf-8,%3C!DOCTYPE%20html%3E%0A%3Cdiv%20style%3D%22background-color%3A%20blue%3B%20width%3A50px%3B%20height%3A50px%22%3E%3C%2Fdiv%3E%0A%3Ccanvas%20style%3D%22position%3Aabsolute%3B%20top%3A0px%3B%20left%3A0px%22%20width%3D40%20height%3D40%3E%3C%2Fcanvas%3E%0A%3Cscript%3E%0Avar%20c%3Ddocument.querySelector(%22canvas%22).getContext(%222d%22)%3B%0Ac.fillStyle%3D%22rgba(0%2C128%2C0%2C0.5)%22%3B%0Ac.fillRect(0%2C%200%2C%2040%2C%2040)%3B%0A%3C%2Fscript%3E
> > > > 
> > > > We could do some complex logic to try to detect if every pixel of the canvas was actually opaque but that's a bit involved. I think this should go on WebGLLayerChromium since that actually has an 'has alpha' bit.
> > > 
> > > Sounds good. I will move the m_hasAlpha bit down to the WebGL layer.
> > > 
> > > >> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:156
> > > >> +    bool frameOpaque = m_frameFormat != VideoFrameChromium::RGBA && m_frameFormat != VideoFrameChromium::Invalid && m_frameFormat != VideoFrameChromium::Empty;
> > > > 
> > > > are you sure that every video format except for RGBA is fully opaque, always? even videos decoded straight to a texture? i'm not familiar enough with our video stack and video formats to know if this is true
> > > 
> > > I was actually hoping for verification of this. I was told the YUV formats for example are always opaque, but I'm not sure which others. Who can I ask this?
> > 
> > We only need to care about the formats we can actually render. Right now that is YV12 and YV16.
> 
> That seems short-sighted.  For example, earlier this week Ami landed support for externally decoded texture video rendering.

Flash shipped with VP6 that had alpha channel support but other than that video codecs are opaque.

There's also a handful of other opaque YUV formats that should be on our radar (YV18 for YUV444 support, camera formats, etc...)

[Dana Jansens]

(In reply to comment #16)
> Flash shipped with VP6 that had alpha channel support but other than that video codecs are opaque.

And I don't see VP6 in VideoFrameChromium.h

> There's also a handful of other opaque YUV formats that should be on our radar (YV18 for YUV444 support, camera formats, etc...)

[Andrew Scherkus]

(In reply to comment #17)
> (In reply to comment #16)
> > Flash shipped with VP6 that had alpha channel support but other than that video codecs are opaque.
> 
> And I don't see VP6 in VideoFrameChromium.h

Right, so lets not worry about alpha video formats for now.  When they do come there'll be many more things that I'll have to fix anyway :)
 
> > There's also a handful of other opaque YUV formats that should be on our radar (YV18 for YUV444 support, camera formats, etc...)

[Dana Jansens]

(In reply to comment #11)
> One thing that sucks about this whole mess is the ambiguity about GraphicsLayer::*Contents*. Normally GraphicsLayer functions that talk about 'contents' are talking about the GraphicsLayer::m_contentsLayer. For example for an image layer the GraphicsLayer has two PlatformLayers - GraphicsLayer::m_layer has the borders, etc, for the <img> or has drawsContent=false if it's a simple container layer. For chromium GraphicsLayer::setContentsToImage() creates an ImageLayerChromium, in the CoreAnimation implementation that function (indirectly) creates a CALayer called GraphicsLayer::m_contentsLayer and sets the content of that layer to the image.  Most GraphicsLayer::*Content* functions are about the m_contentsLayer. GraphicsLayer::setContentsOpaque is about the GraphicsLayer::m_layer which makes things all confusing as fuck.

In my code so far it's being used to represent the box decorations and the contents box. Which I guess would be some part of both layers together when both are present (are they always or is m_contentsLayer just for accelerated compositing?).

> I think we should make GraphicsLayer::setContentsOpaque() refer to the m_contentsLayer and have some other function for saying 'the stuff in this GraphicsLayer::m_layer is the thing that is opaque'. If I can catch smfr I'll run this by him.
> 
> If we sort that out then I think cross-platform WebKit code can take care of setting the opaque bit for the GraphicsLayer::m_contentsLayer, at least for image and webgl layers. That would be better IMO.

That does sound better. But also sounds like it could be ways out. Would you like to land something that'll work in the meantime and then fix it up later? I don't want to just use the opaque() flag in tot, because GraphicsLayer::setContentsOpaque would conflict with it.

[Antoine Labour]

Comment on attachment 117510 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/LayerChromium.h:224
>>> +    virtual void updateOpaque() { setOpaque(m_contentsOpaque); }
>> 
>> could we remove setOpaque() and make LayerChromium::opaque() virtual instead of adding updateOpaque()?
> 
> I chose to do it this way because deciding if something is opaque could require some amount of work, so it seemed worth it to just save it in a bool on update.  For instance an image currentFrameHasAlpha() function seems completely out of our control and don't want to put it in a code path it doesn't need to be in. What do you think of this?

One way or another I would like a single source of authority.
Today you can call both setContentsOpaque and setOpaque on a layer, and they clobber each other's state.
WebLayer and friends still call setOpaque - they should be fixed.

Note: setContentsOpaque sounds like it doesn't belong to LayerChromium, and should probably be in ContentLayerChromium. Unfortunately GLC doesn't keep track of which type of LayerChromium it's operating on, so it can't do a safe downcast (it's not the only case, setNeedsDisplay comes to mind)... I hate to introduce more like this, but at the same time, I don't have a great idea about how to fix this short of refactoring GLC...

>>>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:156
>>>> +    bool frameOpaque = m_frameFormat != VideoFrameChromium::RGBA && m_frameFormat != VideoFrameChromium::Invalid && m_frameFormat != VideoFrameChromium::Empty;
>>> 
>>> are you sure that every video format except for RGBA is fully opaque, always? even videos decoded straight to a texture? i'm not familiar enough with our video stack and video formats to know if this is true
>> 
>> I was actually hoping for verification of this. I was told the YUV formats for example are always opaque, but I'm not sure which others. Who can I ask this?
> 
> Maybe make this "frameOpaque = m_frameFormat == VideoFrameChromium::YV12 || m_frameFormat == VideoFrameChromium::YV16" instead?

How about pushing this to a VideoFrameChromium::isFormatOpaque so that it's easier to keep in sync with the enum there?

[David Reveman]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Flash shipped with VP6 that had alpha channel support but other than that video codecs are opaque.
> > 
> > And I don't see VP6 in VideoFrameChromium.h
> 
> Right, so lets not worry about alpha video formats for now.  When they do come there'll be many more things that I'll have to fix anyway :)

I'd prefer this to be exclusive rather than inclusive so that if someone forgets to update the opaque() value when adding a new format, we might just get a bit less efficient occlusion culling instead of incorrect rendering.

[David Reveman]

(In reply to comment #20)
> (From update of attachment 117510 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=117510&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/LayerChromium.h:224
> >>> +    virtual void updateOpaque() { setOpaque(m_contentsOpaque); }
> >> 
> >> could we remove setOpaque() and make LayerChromium::opaque() virtual instead of adding updateOpaque()?
> > 
> > I chose to do it this way because deciding if something is opaque could require some amount of work, so it seemed worth it to just save it in a bool on update.  For instance an image currentFrameHasAlpha() function seems completely out of our control and don't want to put it in a code path it doesn't need to be in. What do you think of this?
> 
> One way or another I would like a single source of authority.
> Today you can call both setContentsOpaque and setOpaque on a layer, and they clobber each other's state.
> WebLayer and friends still call setOpaque - they should be fixed.
> 
> Note: setContentsOpaque sounds like it doesn't belong to LayerChromium, and should probably be in ContentLayerChromium. Unfortunately GLC doesn't keep track of which type of LayerChromium it's operating on, so it can't do a safe downcast (it's not the only case, setNeedsDisplay comes to mind)... I hate to introduce more like this, but at the same time, I don't have a great idea about how to fix this short of refactoring GLC...
> 
> >>>> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:156
> >>>> +    bool frameOpaque = m_frameFormat != VideoFrameChromium::RGBA && m_frameFormat != VideoFrameChromium::Invalid && m_frameFormat != VideoFrameChromium::Empty;
> >>> 
> >>> are you sure that every video format except for RGBA is fully opaque, always? even videos decoded straight to a texture? i'm not familiar enough with our video stack and video formats to know if this is true
> >> 
> >> I was actually hoping for verification of this. I was told the YUV formats for example are always opaque, but I'm not sure which others. Who can I ask this?
> > 
> > Maybe make this "frameOpaque = m_frameFormat == VideoFrameChromium::YV12 || m_frameFormat == VideoFrameChromium::YV16" instead?
> 
> How about pushing this to a VideoFrameChromium::isFormatOpaque so that it's easier to keep in sync with the enum there?

That sounds good.

Another concern of mine right now is that we don't seem to know what the format used by the external decoder is. It might happen to always be opaque formats right now but I think it's incorrect to assume that until we're actually aware of the format used from within VideoLayerChromium.

[Nat Duca]

Isn't contents opacity a property of the layer delegate? The layer delegate is the one we go to paint, so why not just ask it whether the thing it just painted was opaque or not?

[David Reveman]

(In reply to comment #15)
> (In reply to comment #10)
> > (From update of attachment 117510 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=117510&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/LayerChromium.h:224
> > > +    virtual void updateOpaque() { setOpaque(m_contentsOpaque); }
> > 
> > could we remove setOpaque() and make LayerChromium::opaque() virtual instead of adding updateOpaque()?
> 
> I chose to do it this way because deciding if something is opaque could require some amount of work, so it seemed worth it to just save it in a bool on update.  For instance an image currentFrameHasAlpha() function seems completely out of our control and don't want to put it in a code path it doesn't need to be in. What do you think of this?

I think that a simple "virtual bool LayerChromium::isOpaque()" is preferred. If it happens to be too expensive for some layer type to compute this every time isOpaque() is called, that could instead be handled by that specific layer implementation.

There are a few layer functions that are not returning valid values for the current update until prepareToPaintIfDirty has been called. I think isOpaque should fall into this same category of functions, which means prepareToPaintIfDirty is a good time to compute a new value that will be returned by isOpaque().

[Dana Jansens]

Created attachment 117623 [details]
Patch

(In reply to comment #9)
> > We could do some complex logic to try to detect if every pixel of the canvas was actually opaque but that's a bit involved. I think this should go on WebGLLayerChromium since that actually has an 'has alpha' bit.
> 
> Sounds good. I will move the m_hasAlpha bit down to the WebGL layer.

The reason the m_hasAlpha bit is on the CanvasLayerChromium is because it is also on the CCCanvasLayerChromium, even though the WebGL layer is the only subtype to actually set the flag.

This is one way to move the alpha bits to the WebGL layer, but it leaves it out of the pushPropertiesTo call entirely in the Canvas layer. Is that okay?  An alternate means could be to have a virtual hasAlpha() function on the Canvas layer and push that, and have the WebGL layer override and return its m_hasAlpha.

Lots of good new feeback to go through also, thanks.

[Dana Jansens]

(In reply to comment #23)
> Isn't contents opacity a property of the layer delegate? The layer delegate is the one we go to paint, so why not just ask it whether the thing it just painted was opaque or not?

If I understand correctly, we paint from bottom to top. Waiting to decide if something is opaque until it was painted would exclude is from occlusion culling below an opaque. I've been specifically avoiding putting the contentsOpaque computation into the paint pass for this reason.

[Dana Jansens]

Nat actually brought up an amazing point, and had a long discussion with @reveman re the feasibility of this.

We could traverse layers top to bottom, painting each in webkit, then checking the opaque bit via delegate, and using that to decide occlusion on further layers below.

The best part of this is that if we push this off to the delegate, then I can compute contentsOpaque in webkit during the painting process, which would be a huge win for that CL.

I'm going to drop the contentsOpaque() from the LC here, and push it up to the delegate class. I think this means storing the contentsOpaque bit from GraphicsLayer in the GraphicsLayerChromium instead.

@jamesr can you verify that you want the m_hasAlpha flag moved down to WebGLLayerChromium? It doesn't feel right to me since it's copied to CCCanvasLayerChromium. I think with the virtual bool opaque() function, it's a lot cleaner, and doesn't require adding the hasAlpha()/setHasAlpha() functions to CanvasLayerChromium. So maybe the desire to move it would be less then.

[David Reveman]

(In reply to comment #27)
> Nat actually brought up an amazing point, and had a long discussion with @reveman re the feasibility of this.
> 
> We could traverse layers top to bottom, painting each in webkit, then checking the opaque bit via delegate, and using that to decide occlusion on further layers below.

My parallel paint/upload and shared memory painting patches requires us to accurately allocate compositor resources prior to painting. It would be bad to allocate shared memory for a tile that we end up not painting. So I'll have to make some minor adjustments to these patches. There's currently no reason for why we can't do the layer paint call prior to resource allocation. It's just the per-tile painting that needs to happen after resource allocation so this should work fine.

[Dana Jansens]

Created attachment 117665 [details]
Patch

This pushes the idea of painted contents being opaque off to the layer delegate, and makes the LayerChromium opaque bits independent from that, where layers set their opaque flag intelligently based on internal knowledge about themselves.

- Removed hasAlpha from CanvasLayer and CCCanvasLayer (they can use opaque() for this).
- Removed GLC::setContentsOpaque/updateContentsOpaque. The contentsOpaque is going to be something to be queried off the CCLayerDelegate after paintContents().
- Added contentsOpaque to various CCLayerDelegates in the WebLayer hierarchy.
- LC subclasses just setOpaque() directly.
- Added VideoFrameChromium::isFrameOpaque().

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[James Robinson]

Comment on attachment 117665 [details]
Patch

GraphicsLayer::contentsOpaque is a useful bit and has nothing to do with the opaque-ness of the image/webgl/canvas/video content inside a layer.

[Antoine Labour]

Comment on attachment 117665 [details]
Patch

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

I would really like a single source of authority... I'm still confused about which layer calls contentsOpaque vs expects setOpaque to be called etc. Even more confusing that it seems different between the Web*Layers and the *LayerChromium

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:65
> +    virtual bool contentsOpaque() const = 0;

Where is that called?

Also, shouldn't LayerChromium::setOpaque go?

> Source/WebKit/chromium/public/WebContentLayerClient.h:42
> +    virtual bool contentsOpaque() const = 0;

This conflicts with WebLayer::setOpaque - it needs to go.

> Source/WebKit/chromium/src/WebExternalTextureLayerImpl.cpp:65
> +    return false;

NAK, we need opaque WebExternalTextureLayers. You could add setOpaque back onto WebExternalTextureLayer, leaving it as a local state on the WebExternalTextureLayerImpl.
But we also need to get PluginLayerChromium to query this.

[James Robinson]

(In reply to comment #23)
> Isn't contents opacity a property of the layer delegate? The layer delegate is the one we go to paint, so why not just ask it whether the thing it just painted was opaque or not?

The problem here is we don't "paint" any of these layer types.  In fact I think we should probably break up the delegate types out a bit to reflect this - the only *LayerChromium type for which the paintContents() call is used is ContentLayerChromium.

The types of layers we're talking about here all map to the GraphicsLayer's m_contentsLayer.

I'll put a diagram up somewhere to make this easier to talk about.

[David Reveman]

Comment on attachment 117665 [details]
Patch

Did you consider using the return value of paintContents for opaqueness? contentsOpaque() will only be valid after you called paintContents so it would nice to just have it part of the paintContents function call.

Also could we make m_opaque protected instead of private and get rid of the LayerChromium::setOpaque function? or make setOpaque protected?

[James Robinson]

(In reply to comment #34)
> (From update of attachment 117665 [details])
> Did you consider using the return value of paintContents for opaqueness? contentsOpaque() will only be valid after you called paintContents so it would nice to just have it part of the paintContents function call.
> 
> Also could we make m_opaque protected instead of private and get rid of the LayerChromium::setOpaque function? or make setOpaque protected?

Again there is no paint for image, video, webgl, etc.

[Dana Jansens]

(In reply to comment #33)
> (In reply to comment #23)
> > Isn't contents opacity a property of the layer delegate? The layer delegate is the one we go to paint, so why not just ask it whether the thing it just painted was opaque or not?
> 
> The problem here is we don't "paint" any of these layer types.  In fact I think we should probably break up the delegate types out a bit to reflect this - the only *LayerChromium type for which the paintContents() call is used is ContentLayerChromium.

Maybe this is confusing because I did the delegate thing in this patch. I was basically attempting to disconnect GL::contentsOpaque from LC::opaque, and then have LC subclasses set LC::opaque appropriately.

I didn't want to leave GL::contentsOpaque inaccessible from LC, as we'd like to use it for paint culling, so I stuck it in the delegate here, even though it is not used anywhere right now. Maybe I should just take that part out and do it in a separate CL?

[James Robinson]

(In reply to comment #36)
> (In reply to comment #33)
> > (In reply to comment #23)
> > > Isn't contents opacity a property of the layer delegate? The layer delegate is the one we go to paint, so why not just ask it whether the thing it just painted was opaque or not?
> > 
> > The problem here is we don't "paint" any of these layer types.  In fact I think we should probably break up the delegate types out a bit to reflect this - the only *LayerChromium type for which the paintContents() call is used is ContentLayerChromium.
> 
> Maybe this is confusing because I did the delegate thing in this patch. I was basically attempting to disconnect GL::contentsOpaque from LC::opaque, and then have LC subclasses set LC::opaque appropriately.
> 
> I didn't want to leave GL::contentsOpaque inaccessible from LC, as we'd like to use it for paint culling, so I stuck it in the delegate here, even though it is not used anywhere right now. Maybe I should just take that part out and do it in a separate CL?

I think this is confusing because we have many layer trees that are not isomorphic and that do different things. Pretty much every comment here is talking about a different layer type with different behavior and it all makes me very sad :(

[Dana Jansens]

(In reply to comment #34)
> (From update of attachment 117665 [details])
> Also could we make m_opaque protected instead of private and get rid of the LayerChromium::setOpaque function? or make setOpaque protected?

Making setOpaque protected is a good call since it's meant only for its subclasses, thanks.

[James Robinson]

Here's a simplified example of what happens for a composited <img src="cat.png">:

http://www.diagrammr.com/edit?key=dR35BSNm9w4

If WebCore calls GraphicsLayer::setContentsOpaque(), it is referring to the ContentsLayerChromium pointed to by GraphicsLayer::m_layer.  This ContentsLayerChromium is responsible for painting the <img>'s styles, background, and any other CSS painting things except for the image itself.  The ImageLayerChromium, which is set up on the GraphicsLayer via setContentsToImage(), it responsible only for painting cat.png.

Note that the CCLayerDelegate for the m_layer and m_contentsLayer are actually both the same thing - the GraphicsLayerChromium - even though the opaque-ness of the two will often be different!  So I don't think going to the CCLayerDelegate to ask for opaque-ness would work.

[James Robinson]

I think the general way to go here is:
*) opaqueness for the GraphicsLayer::m_layer should be set by WebCore somehow - either via the Render* code pushing an opaque bit or pulling it or generating it at paint time and then setting it via the LayerChromium interface

*) opaqueness for the GraphicsLayer::m_contentsLayer should be determined on a per-layer basis since the logic will differ for each type. This could be done inside (Image|Video|WebGL)LayerChromium code or in the code that creates these objects but I don't think it should be on CCLayerDelegate.

[James Robinson]

Comment on attachment 117665 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:-76
> -    virtual void setContentsOpaque(bool);

I think this is misguided. This is a useful bit to have that we should be using. Please see comments on the bug.

[David Reveman]

(In reply to comment #40)
> I think the general way to go here is:
> *) opaqueness for the GraphicsLayer::m_layer should be set by WebCore somehow - either via the Render* code pushing an opaque bit or pulling it or generating it at paint time and then setting it via the LayerChromium interface
> 
> *) opaqueness for the GraphicsLayer::m_contentsLayer should be determined on a per-layer basis since the logic will differ for each type. This could be done inside (Image|Video|WebGL)LayerChromium code or in the code that creates these objects but I don't think it should be on CCLayerDelegate.

Thanks for the diagram.

I was thinking that we could have the delegate's paint call return opaqueness for what it just painted:

class CCLayerDelegate {
    // Returns true if paint operations resulted in |clip| being filled with opaque contents.
    virtual bool paintContents(GraphicsContext&, const IntRect& clip) = 0;
};

It doesn't matter if multiple layers use the same delegate in this case.

If determining opacity during paint is convenient, wouldn't this be a nice way to handle layers that "paint" contents? Layers that don't paint contents will need a different way to determine what value to return from LayerChromium::isOpaque(). As the opaqueness for non-paint layers is specific to the layer type, using a layer type specific way to pass this information the layer implementation doesn't seem inappropriate to me. Or maybe there's something about these non-paint layers that I still don't understand..

[James Robinson]

Knowing if a certain clip rect is fully opaque isn't nearly as useful as knowing if the entire later is opaque, which we can tell sometimes. But perhaps we'll want both eventually.


I think we should do the easy case first, which is non-painting layers.

[Dana Jansens]

Created attachment 117911 [details]
Patch

Round three, with more understanding of the layer rainbow. This sets opaque() appropriately on the ImageLayerChromium with a unit test.

Video and WebGL will follow in their own CLs once this is agreed on.

[James Robinson]

Comment on attachment 117911 [details]
Patch

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

Great! R=me

> Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:56
> +    explicit TestImage(const IntSize& size, bool opaque)

nit: for the future, you don't need explicit for 2-arg c'tors. it doesn't really hurt to have it in test code, though

[WebKit Review Bot]

Comment on attachment 117911 [details]
Patch

Attachment 117911 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10729810

New failing tests:
svg/custom/linking-uri-01-b.svg

[Dana Jansens]

Test passes for me. :/

[James Robinson]

That test is failing on every patch - looks like it's just broken in the tree. Let's try again...

[James Robinson]

Comment on attachment 117911 [details]
Patch

Flipping cq+ again. if this fails I'll land it by hand.

[WebKit Review Bot]

Comment on attachment 117911 [details]
Patch

Clearing flags on attachment: 117911

Committed r102043: <http://trac.webkit.org/changeset/102043>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Vsevolod Vlasov]

This patch broke chromium mac-cg compilation with many errors like that one:

/src/third_party/WebKit/Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:60:23: error: assigning to 'NativeImagePtr' (aka 'CGImage *') from incompatible type 'WebCore::NativeImageSkia *'
        m_nativeImage = new NativeImageSkia();

                      ^ ~~~~~~~~~~~~~~~~~~~~~
Builder link:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac%20Builder%20%28CG%29/builds/3668

Rolled out: <http://trac.webkit.org/changeset/102112>

Reopening.

[Dana Jansens]

Created attachment 118043 [details]
Patch

Turned off the test on Mac, as it won't compile there. I had copied the TestImage class from DragImageTest, without noticing it is disabled on Mac.

[James Robinson]

Comment on attachment 118043 [details]
Patch

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

I think this will still fail on mac-cg the same way as the last patch did.

> Source/WebKit/chromium/WebKit.gypi:108
> +                    # FIXME: Port DragImageTest/ImageLayerChromiumTest to Mac.
>                      'tests/DragImageTest.cpp',
> +                    'tests/ImageLayerChromiumTest.cpp',

i'm not sure i understand what this does - it looks like it's adding an entry to webkit_unit_tests on windows, but it's already in the list so i don't think this has any impact. we also want to run this test on linux and on mac when using skia

> Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:60
> +        m_nativeImage = new NativeImageSkia();

I think this will still fail on MAC-CG

Could you instead guard this test with #if USE(SKIA) ?

> Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:69
> +        delete m_nativeImage;

can we use an OwnPtr<NativeImagePtr> member variable instead of doing this?

[Dana Jansens]

(In reply to comment #54)
> (From update of attachment 118043 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=118043&action=review
> 
> I think this will still fail on mac-cg the same way as the last patch did.

Oh gosh, sorry. Total Gyp fail.

> > Source/WebKit/chromium/WebKit.gypi:108
> > +                    # FIXME: Port DragImageTest/ImageLayerChromiumTest to Mac.
> >                      'tests/DragImageTest.cpp',
> > +                    'tests/ImageLayerChromiumTest.cpp',
> 
> i'm not sure i understand what this does - it looks like it's adding an entry to webkit_unit_tests on windows, but it's already in the list so i don't think this has any impact. we also want to run this test on linux and on mac when using skia
> 
> I think this will still fail on MAC-CG
> 
> Could you instead guard this test with #if USE(SKIA) ?

Sure, I will do that in the gyp for this and the DragImageTest.

> > Source/WebKit/chromium/tests/ImageLayerChromiumTest.cpp:69
> > +        delete m_nativeImage;
> 
> can we use an OwnPtr<NativeImagePtr> member variable instead of doing this?

Yep! Will change in DragImageTest too.

[Dana Jansens]

Created attachment 118097 [details]
Patch

I don't see anything in DragImageTest that would break on Mac other than the skia use, so I put it behind use_skia also. And I swapped to using OwnPtr for the TestImage class in both tests.

[James Robinson]

Comment on attachment 118097 [details]
Patch

Yay! Can you keep an eye on DragImageTest after this lands, just to be sure?

[Dana Jansens]

Thanks, I will. Am watching this, please let me know if it's the wrong place to do so.
http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29

The chromium mac bots seem down so no testing SKIA on mac I guess right now, if I am getting the bot builds right.

[WebKit Review Bot]

Comment on attachment 118097 [details]
Patch

Clearing flags on attachment: 118097

Committed r102174: <http://trac.webkit.org/changeset/102174>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Vsevolod Vlasov]

(In reply to comment #58)
> Thanks, I will. Am watching this, please let me know if it's the wrong place to do so.
> http://build.webkit.org/builders/SnowLeopard%20Intel%20Debug%20%28Tests%29
> 
> The chromium mac bots seem down so no testing SKIA on mac I guess right now, if I am getting the bot builds right.

I think you should watch 
http://build.chromium.org/p/chromium.webkit/waterfall?branch=&builder=Webkit+Mac+Builder&builder=Webkit+Mac10.5&builder=Webkit+Mac10.6&builder=Webkit+Mac+Builder+%28CG%29&builder=Webkit+Mac10.5+%28CG%29&builder=Webkit+Mac10.6+%28CG%29

[Dana Jansens]

Awesome, thanks!