Bug 54559 – [chromium] Fix green pixels at edge of certain GPU-accelerated videos

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Victoria Kirst]

[chromium] Fix green pixels at edge of certain GPU-accelerated videos

[Victoria Kirst]

Created attachment 82643 [details]
Patch

[Andrew Scherkus]

Comment on attachment 82643 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:316
> +            float deadPixelsPerRow = planeTextureSize.width() - frameWidth;

does this need to be float?  I think planeTextureSize and frameWidth are both ints

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:394
> +    // Artibitrarily take the u sizes because u and v dimensions are identical.

Artibitrarily -> Arbitrarily

[Victoria Kirst]

Created attachment 82687 [details]
Patch

[Victoria Kirst]

Updated patch to reflect Andrew's suggested changes, and subtracts 2 from Y plane's width instead of 1 in YV12 case.

Also moved some of the logic to VideoFrameChromium and did some refactoring there for a cleaner solution.

[Kenneth Russell]

Comment on attachment 82687 [details]
Patch

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

I'll be happy to r+ this once a member of the video stack team signs off on it. One minor stylistic comment.

> Source/WebCore/platform/graphics/chromium/VideoLayerChromium.cpp:313
> +                frameWidth--;

Prefer predecrement in WebKit. (i.e., --frameWidth)

[Andrew Scherkus]

I ran some tests using a static YUV test image and this patch is good to go!

On width==stride videos we have pixel-accurate scaling as expected.

On width<stride videos we _do_ end up scaling the video horizontally by one pixel (i.e., right-most pixel line is gone!) but Y and UV planes are still in sync which is the important part.

[Victoria Kirst]

Created attachment 82719 [details]
Patch

[Victoria Kirst]

Thanks for all the help with the bug and for writing a way to test it, Andrew!

kbr: Thanks for reviewing! I fixed the nit in my latest uploaded patch. (Also made a minor adjustment to the CL description; otherwise kept the same.)

[Kenneth Russell]

Comment on attachment 82719 [details]
Patch

Sounds good. r=me

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 82719 [details]:

http/tests/websocket/tests/multiple-connections.html bug 53825 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 82719 [details]
Patch

Clearing flags on attachment: 82719

Committed r78787: <http://trac.webkit.org/changeset/78787>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.