[webkit-reviews] review denied: [Bug 88363] [chromium] Separate CCVideoDrawQuad and from the layer tree and video provider by removing ManagedTexture and WebVideoFrame pointers from the quad : [Attachment 145889] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Dana Jansens
<danakj at chromium.org>'s request for review:
Bug 88363: [chromium] Separate CCVideoDrawQuad and from the layer tree and
video provider by removing ManagedTexture and WebVideoFrame pointers from the
quad
https://bugs.webkit.org/show_bug.cgi?id=88363

Attachment 145889: Patch
https://bugs.webkit.org/attachment.cgi?id=145889&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=145889&action=review


The division of responsibilities here doesn't seem quite right (see inline
comments).  Moving GC3D-aware stuff up to CC*LayerImpls is the wrong direction
to be moving in.  We're syncing quads, not layers - right?  When we go to
serialize the quads for a given video layer, we can take care of any uploads
that need to happen at that point.

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:82
> +    ASSERT(MaxPlanes == WebKit::WebVideoFrame::maxPlanes);

Both of these values are compile-time assertions, so this should also be a
compile-time assertion (and probably not in this file).

Why do we even need a different enum - can't we just use
WebKit::WebVideoFrame::maxPlanes directly?

> Source/WebCore/platform/graphics/chromium/cc/CCVideoLayerImpl.cpp:264
> +void CCVideoLayerImpl::copyPlaneToTexture(GraphicsContext3D* context3d,
const void* planeTextureData, int toPlaneIndex)

Having the texture copies on the CCVideoLayerImpl seems a lot worse than having
it in LRC.  The layer impls shouldn't care what the context implementation is,
that's a concern for the CCRenderer implementation (or something it holds on
to, like a texture uploader class) to figure out.