[webkit-reviews] review denied: [Bug 45069] VideoLayerChromium uses GPU YUV to RGB color conversion : [Attachment 68968] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Victoria Kirst
<vrk at google.com>'s request for review:
Bug 45069: VideoLayerChromium uses GPU YUV to RGB color conversion
https://bugs.webkit.org/show_bug.cgi?id=45069

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

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

r- for nitpicks but overall looks good

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:44
> +const float VideoLayerChromium::cYUV2RGB[9] = {

add a comment here indicating what these values are. if they are just PFM then
note that

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:201
> +    VideoFrameChromium* frame = 0;

Why not just declare this in the initialization 3 lines below?

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:202
> +    bool needsAllocate = false;

this variable looks unused

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:270
> +	   if (planeTextureSize != IntSize() && planeTextureSize !=
m_textureSizes[plane]) {

There are .isEmpty() and .isZero() functions on IntSize, please use one of
those

> WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:294
> +    } else
> +	   m_skipsDraw = true;

Can you leave a FIXME or notImplemented() here or something?  mapTexSubImage2D
can fail in the normal course of operation and we should handle it.

> WebCore/platform/graphics/chromium/VideoLayerChromium.h:89
> +    explicit VideoLayerChromium(GraphicsLayerChromium* owner,
VideoFrameProvider* provider);

nit: no explicit for a two-argument constructor, don't name the provider
argument