Bug 72964 - [chromium] Set opaque flag for ImageLayerChromium
Summary: [chromium] Set opaque flag for ImageLayerChromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dana Jansens
URL:
Keywords:
Depends on: 73898
Blocks: 72980
  Show dependency treegraph
 
Reported: 2011-11-22 09:50 PST by Dana Jansens
Modified: 2011-12-06 13:44 PST (History)
14 users (show)

See Also:


Attachments
Patch (1.72 KB, patch)
2011-11-22 09:52 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (4.12 KB, patch)
2011-11-22 11:47 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (27.48 KB, patch)
2011-12-01 16:03 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (29.24 KB, patch)
2011-12-02 08:01 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (35.00 KB, patch)
2011-12-02 12:07 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (10.11 KB, patch)
2011-12-05 12:06 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (10.62 KB, patch)
2011-12-06 07:02 PST, Dana Jansens
no flags Details | Formatted Diff | Diff
Patch (12.14 KB, patch)
2011-12-06 12:53 PST, Dana Jansens
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Jansens 2011-11-22 09:50:29 PST
This is similar to code required in RenderImage::foregroundContentsAreaIsOpaqueInRect() in bug #70634
Comment 1 Dana Jansens 2011-11-22 09:52:00 PST
Created attachment 116237 [details]
Patch
Comment 2 Dana Jansens 2011-11-22 11:47:24 PST
Created attachment 116257 [details]
Patch
Comment 3 Dana Jansens 2011-11-22 11:50:01 PST
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.
Comment 4 Dana Jansens 2011-12-01 15:58:07 PST
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.)
Comment 5 James Robinson 2011-12-01 16:00:44 PST
(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?
Comment 6 Dana Jansens 2011-12-01 16:03:41 PST
Created attachment 117510 [details]
Patch
Comment 7 Dana Jansens 2011-12-01 16:04:30 PST
(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.
Comment 8 James Robinson 2011-12-01 16:13:08 PST
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
Comment 9 Dana Jansens 2011-12-01 16:17:48 PST
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?
Comment 10 David Reveman 2011-12-01 16:22:49 PST
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?
Comment 11 James Robinson 2011-12-01 16:23:37 PST
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.
Comment 12 James Robinson 2011-12-01 16:24:18 PST
Victoria, Scherkus - can you answer the question from comment #9 about which video formats are opaque?
Comment 13 David Reveman 2011-12-01 16:25:37 PST
(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.
Comment 14 James Robinson 2011-12-01 16:32:00 PST
(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.
Comment 15 Dana Jansens 2011-12-01 16:36:19 PST
(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?
Comment 16 Andrew Scherkus 2011-12-01 16:37:41 PST
(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...)
Comment 17 Dana Jansens 2011-12-01 16:48:20 PST
(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...)
Comment 18 Andrew Scherkus 2011-12-01 16:50:53 PST
(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...)
Comment 19 Dana Jansens 2011-12-01 16:53:23 PST
(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.
Comment 20 Antoine Labour 2011-12-01 17:08:50 PST
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?
Comment 21 David Reveman 2011-12-01 17:11:25 PST
(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.
Comment 22 David Reveman 2011-12-01 17:15:56 PST
(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.
Comment 23 Nat Duca 2011-12-01 17:30:11 PST
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?
Comment 24 David Reveman 2011-12-01 22:23:07 PST
(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().
Comment 25 Dana Jansens 2011-12-02 08:01:47 PST
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.
Comment 26 Dana Jansens 2011-12-02 08:22:44 PST
(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.
Comment 27 Dana Jansens 2011-12-02 09:59:38 PST
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.
Comment 28 David Reveman 2011-12-02 10:24:40 PST
(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.
Comment 29 Dana Jansens 2011-12-02 12:07:36 PST
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().
Comment 30 WebKit Review Bot 2011-12-02 12:10:19 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 31 James Robinson 2011-12-02 12:44:04 PST
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.
Comment 32 Antoine Labour 2011-12-02 12:49:44 PST
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.
Comment 33 James Robinson 2011-12-02 12:57:09 PST
(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.
Comment 34 David Reveman 2011-12-02 12:57:37 PST
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?
Comment 35 James Robinson 2011-12-02 12:59:43 PST
(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.
Comment 36 Dana Jansens 2011-12-02 13:00:57 PST
(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?
Comment 37 James Robinson 2011-12-02 13:02:24 PST
(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 :(
Comment 38 Dana Jansens 2011-12-02 13:45:54 PST
(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.
Comment 39 James Robinson 2011-12-02 13:55:58 PST
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.
Comment 40 James Robinson 2011-12-02 14:01:10 PST
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.
Comment 41 James Robinson 2011-12-02 14:01:15 PST
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.
Comment 42 David Reveman 2011-12-02 22:25:52 PST
(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..
Comment 43 James Robinson 2011-12-04 22:29:30 PST
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.
Comment 44 Dana Jansens 2011-12-05 12:06:02 PST
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.
Comment 45 James Robinson 2011-12-05 12:12:22 PST
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
Comment 46 WebKit Review Bot 2011-12-05 12:52:32 PST
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
Comment 47 Dana Jansens 2011-12-05 12:56:16 PST
Test passes for me. :/
Comment 48 James Robinson 2011-12-05 13:32:27 PST
That test is failing on every patch - looks like it's just broken in the tree. Let's try again...
Comment 49 James Robinson 2011-12-05 13:32:48 PST
Comment on attachment 117911 [details]
Patch

Flipping cq+ again. if this fails I'll land it by hand.
Comment 50 WebKit Review Bot 2011-12-05 13:56:11 PST
Comment on attachment 117911 [details]
Patch

Clearing flags on attachment: 117911

Committed r102043: <http://trac.webkit.org/changeset/102043>
Comment 51 WebKit Review Bot 2011-12-05 13:56:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 52 Vsevolod Vlasov 2011-12-06 02:39:49 PST
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.
Comment 53 Dana Jansens 2011-12-06 07:02:32 PST
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.
Comment 54 James Robinson 2011-12-06 11:27:44 PST
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?
Comment 55 Dana Jansens 2011-12-06 12:34:58 PST
(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.
Comment 56 Dana Jansens 2011-12-06 12:53:12 PST
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.
Comment 57 James Robinson 2011-12-06 13:20:10 PST
Comment on attachment 118097 [details]
Patch

Yay! Can you keep an eye on DragImageTest after this lands, just to be sure?
Comment 58 Dana Jansens 2011-12-06 13:34:14 PST
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.
Comment 59 WebKit Review Bot 2011-12-06 13:37:45 PST
Comment on attachment 118097 [details]
Patch

Clearing flags on attachment: 118097

Committed r102174: <http://trac.webkit.org/changeset/102174>
Comment 60 WebKit Review Bot 2011-12-06 13:37:53 PST
All reviewed patches have been landed.  Closing bug.
Comment 61 Vsevolod Vlasov 2011-12-06 13:43:28 PST
(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
Comment 62 Dana Jansens 2011-12-06 13:44:54 PST
Awesome, thanks!