Fix HTML5 video blurry

Fix HTML5 video blurry

HW decoded video is blurry, because crbug.com/575587 shinked visual rect by 1
pixel, if coded rect is larger than visual rect. This fixed bleeding texture,
but caused blurry issue.

Meanwhile, crbug.com/429640 fixed bleeding texture for software YUV video in
the elegant way. It clamps all texture coordinates to this maximum value in the
fragment shader.

This CL removes the 1 pixel hack and applies the way to TextureDrawQuad and
StreamVideoDrawQuad (i.e. HW decoded video path).

BUG=615325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/b38566a76c4af61c0d34df7d8b906e7936b8e524
Cr-Commit-Position: refs/heads/master@{#428707}

[dshwang, 2016-10-14 17:02:23]

Description was changed from

==========
Fix HTML5 video blurry

HW decoded video is blurry, because crbug.com/575587 shinked visual rect by 1
pixel, if coded rect is larger than visual rect. This fixed bleeding texture,
but caused blurry issue.

Meanwhile, crbug.com/429640 fixed bleeding texture for software YUV video in
the elegant way. It clamps all texture coordinates to this maximum value in the
fragment shader.

This CL removes the 1 pixel hack and applies the way to TextureDrawQuad and
StreamVideoDrawQuad (i.e. HW decoded video path).

BUG=615325
==========

to

==========
Fix HTML5 video blurry

HW decoded video is blurry, because crbug.com/575587 shinked visual rect by 1
pixel, if coded rect is larger than visual rect. This fixed bleeding texture,
but caused blurry issue.

Meanwhile, crbug.com/429640 fixed bleeding texture for software YUV video in
the elegant way. It clamps all texture coordinates to this maximum value in the
fragment shader.

This CL removes the 1 pixel hack and applies the way to TextureDrawQuad and
StreamVideoDrawQuad (i.e. HW decoded video path).

BUG=615325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[inactive_dshwang_plz_cc_intel, 2016-10-14 17:02:30]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 17:02:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2016-10-14 17:03:33]

Description was changed from

==========
Fix HTML5 video blurry

HW decoded video is blurry, because crbug.com/575587 shinked visual rect by 1
pixel, if coded rect is larger than visual rect. This fixed bleeding texture,
but caused blurry issue.

Meanwhile, crbug.com/429640 fixed bleeding texture for software YUV video in
the elegant way. It clamps all texture coordinates to this maximum value in the
fragment shader.

This CL removes the 1 pixel hack and applies the way to TextureDrawQuad and
StreamVideoDrawQuad (i.e. HW decoded video path).

BUG=615325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix HTML5 video blurry

HW decoded video is blurry, because crbug.com/575587 shinked visual rect by 1
pixel, if coded rect is larger than visual rect. This fixed bleeding texture,
but caused blurry issue.

Meanwhile, crbug.com/429640 fixed bleeding texture for software YUV video in
the elegant way. It clamps all texture coordinates to this maximum value in the
fragment shader.

This CL removes the 1 pixel hack and applies the way to TextureDrawQuad and
StreamVideoDrawQuad (i.e. HW decoded video path).

BUG=615325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[dshwang, 2016-10-14 17:05:15]

dongseong.hwang@intel.com changed reviewers:
+ enne@chromium.org

[dshwang, 2016-10-14 17:05:15]

enne@: could you review? It's because this CL is inspired by your CL.
https://codereview.chromium.org/881963002

[commit-bot: I haz the power, 2016-10-14 17:12:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 17:12:55]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[inactive_dshwang_plz_cc_intel, 2016-10-14 17:29:04]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-14 17:29:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[inactive_dshwang_plz_cc_intel, 2016-10-14 17:31:54]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[dshwang, 2016-10-14 17:31:57]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2016-10-14 17:32:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[enne (OOO), 2016-10-14 17:54:42]

On 2016/10/14 at 17:05:15, dongseong.hwang wrote:
> enne@: could you review? It's because this CL is inspired by your CL.
https://codereview.chromium.org/881963002

This all looks reasonable.  Could you add a pixel test that includes texture2d
and texture rectangle?

[commit-bot: I haz the power, 2016-10-14 18:36:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-14 18:36:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dshwang, 2016-10-18 17:33:14]

Patchset #4 (id:80001) has been deleted

[dshwang, 2016-10-18 17:34:41]

Patchset #4 (id:100001) has been deleted

[dshwang, 2016-10-18 17:34:48]

Patchset #3 (id:60001) has been deleted

[inactive_dshwang_plz_cc_intel, 2016-10-18 19:05:44]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-18 19:05:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2016-10-18 19:14:37]

> This all looks reasonable.  Could you add a pixel test that includes texture2d
> and texture rectangle?

enne, Done. could you review agian?

The added tests reveal two existing hidden bugs, so this CL fixes them.

https://codereview.chromium.org/2418173002/diff/120001/cc/layers/video_layer_...
File cc/layers/video_layer_impl.cc (left):

https://codereview.chromium.org/2418173002/diff/120001/cc/layers/video_layer_...
cc/layers/video_layer_impl.cc:198:
visible_sample_rect.set_height(visible_rect.height() - 1);
it requires new baseline of pixetests

https://codereview.chromium.org/2418173002/diff/120001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2418173002/diff/120001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:222:
video_resource_updater->CreateExternalResourcesFromVideoFrame(
It produces RGBA Texture resource if the GL context don't support one component
texture. Pixel test uses this path to make sure TextureQuad handle bleeding edge
issue.

The path is tested first time and reveals two hidden bugs, so this CL fixes
them.

https://codereview.chromium.org/2418173002/diff/120001/cc/resources/video_res...
File cc/resources/video_resource_updater.cc (right):

https://codereview.chromium.org/2418173002/diff/120001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:312: ResourceFormat
VideoResourceUpdater::YuvResourceFormat(int bits) const {
Move this logic from ResourceProvider because
1. this is more right place
2. to control output format by unittest

https://codereview.chromium.org/2418173002/diff/120001/cc/resources/video_res...
cc/resources/video_resource_updater.cc:501: plane_resource.resource_size(),
false, false);
This hidden bug is covered by VideoGLRendererPixelHiLoTest.* and
VideoGLRendererPixelTest.TextureQuadEdgeBleed

https://codereview.chromium.org/2418173002/diff/120001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2418173002/diff/120001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:559: if (CheckColorSpace(video_frame,
COLOR_SPACE_JPEG)) {
SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels() always use visual rect,
while coded rect can be bigger.
New VideoGLRendererPixelHiLoTest.SimpleYUVARect/1|3 reveals this existing bug.
VideoGLRendererPixelHiLoTest covers this change.

[commit-bot: I haz the power, 2016-10-18 20:20:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-18 20:20:16]

Dry run: Exceeded global retry quota

[inactive_dshwang_plz_cc_intel, 2016-10-19 12:28:18]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 12:28:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 14:03:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 14:03:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[inactive_dshwang_plz_cc_intel, 2016-10-19 16:33:25]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 16:33:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-19 19:01:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-19 19:01:13]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[enne (OOO), 2016-10-19 19:04:03]

What was wrong with canvas and the plugin test?

https://codereview.chromium.org/2418173002/diff/120001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2418173002/diff/120001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:222:
video_resource_updater->CreateExternalResourcesFromVideoFrame(
On 2016/10/18 at 19:14:36, dshwang wrote:
> It produces RGBA Texture resource if the GL context don't support one
component texture. Pixel test uses this path to make sure TextureQuad handle
bleeding edge issue.
> 
> The path is tested first time and reveals two hidden bugs, so this CL fixes
them.

What were the bugs?

https://codereview.chromium.org/2418173002/diff/160001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2418173002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2505: // Prevent texture bleeding issue.
comment nit: I don't think it's helpful to have this "Prevent texture bleeding
issue" comment anywhere that there's a clamping rect.  I think a comment here
would be useful to say "calculate a half pixel offset from the texture edges for
the shader to use in clamping texture coordinates in order to prevent texture
bleed", since that covers both the "what" and the "why" of this block of code
(which maybe doesn't need to be in a {} block either).

I think the rest of the "prevent texture bleeding issue" comments below can just
be removed.

Also, is there any way to make this block of code reuse the UVClampRect
function?

https://codereview.chromium.org/2418173002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2751:
draw_cache_.tex_clamp_rect_data.push_back(tex_clamp_rect);
This is a bit odd.  I think that draw_cache_'s vectors are intended to be
parallel vectors, and if you had another texture quad before this without a
clamp rect, then this would get coalesced incorrectly.

For what it's worth, the texture draw quad queue is an optimization to allow
coalescing of texture quads that share the same texture (and other things).  I
don't think we need to support batching for this case, so maybe just always
flush the cache if there's a tex clamp rect and only store one of them?

Does that make sense?

https://codereview.chromium.org/2418173002/diff/160001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2418173002/diff/160001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:292: default:
style nit: Don't use "default: NOTREACHED" here.  Just handle all the cases of
the enum.

https://codereview.chromium.org/2418173002/diff/160001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1124: enum class HighbitTexture {
What does HighbitTexture mean? I guess I don't understand the "high bit" part of
that name.

https://codereview.chromium.org/2418173002/diff/160001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1279: // LUMINANCE_F16 requires
--use-gpu-in-tests
Do we have --use-gpu-in-tests running anywhere? Can you include this but early
out of this configuration if --use-gpu-in-tests isn't running or if this case
isn't supported?

https://codereview.chromium.org/2418173002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2418173002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:432:
*outMailbox = cc::TextureMailbox(mailbox, syncToken, GL_TEXTURE_2D, m_size,
Why did this need to change?

[inactive_dshwang_plz_cc_intel, 2016-10-20 18:57:13]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 18:57:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2016-10-20 18:57:56]

enne@, thank you for reviewing. I resolved or replied your comments. Could you
review again?

https://codereview.chromium.org/2418173002/diff/120001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2418173002/diff/120001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:222:
video_resource_updater->CreateExternalResourcesFromVideoFrame(
On 2016/10/19 19:04:03, enne wrote:
> On 2016/10/18 at 19:14:36, dshwang wrote:
> > It produces RGBA Texture resource if the GL context don't support one
> component texture. Pixel test uses this path to make sure TextureQuad handle
> bleeding edge issue.
> > 
> > The path is tested first time and reveals two hidden bugs, so this CL fixes
> them.
> 
> What were the bugs?

1. texture size can be zero in GLRenderer::EnqueueTextureQuad because some
clients doesn't set size.
2. SkCanvasVideoRenderer::ConvertVideoFrameToRGBPixels() doesn't convert video
frame correctly when coded size is bigger than visual size.

See below comments for more details.

https://codereview.chromium.org/2418173002/diff/160001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2418173002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2505: // Prevent texture bleeding issue.
On 2016/10/19 19:04:03, enne wrote:
> comment nit: I don't think it's helpful to have this "Prevent texture bleeding
> issue" comment anywhere that there's a clamping rect.  I think a comment here
> would be useful to say "calculate a half pixel offset from the texture edges
for
> the shader to use in clamping texture coordinates in order to prevent texture
> bleed", since that covers both the "what" and the "why" of this block of code
> (which maybe doesn't need to be in a {} block either).
> 
> I think the rest of the "prevent texture bleeding issue" comments below can
just
> be removed.
> 
> Also, is there any way to make this block of code reuse the UVClampRect
> function?

Done.

https://codereview.chromium.org/2418173002/diff/160001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2751:
draw_cache_.tex_clamp_rect_data.push_back(tex_clamp_rect);
On 2016/10/19 19:04:03, enne wrote:
> This is a bit odd.  I think that draw_cache_'s vectors are intended to be
> parallel vectors, and if you had another texture quad before this without a
> clamp rect, then this would get coalesced incorrectly.
> 
> For what it's worth, the texture draw quad queue is an optimization to allow
> coalescing of texture quads that share the same texture (and other things).  I
> don't think we need to support batching for this case, so maybe just always
> flush the cache if there's a tex clamp rect and only store one of them?
> 
> Does that make sense?

I don't understand why you think this would get coalesced incorrectly?

|tex_clamp_rect| is used in fragment shader to pick sampling point up for
bilinear filter. It juse affects edge of each tex quad slightly. As we can
coalesce several quads, we can also coalesce several quads with slight boarder
change.

In another words, draw_cache_.tex_clamp_rect_data is similar type of data to
draw_cache_.uv_xform_data as both affect glsl attributes per quad.

In addition, the same video texture (i.e. VideoFrame) can be rendered multiple
times. e.g. reflect filter, WebRTC has one stream but binds it to multiple
<video> elements.

Please advise me if I'm wrong.

https://codereview.chromium.org/2418173002/diff/160001/cc/output/renderer_pix...
File cc/output/renderer_pixeltest.cc (right):

https://codereview.chromium.org/2418173002/diff/160001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:292: default:
On 2016/10/19 19:04:03, enne wrote:
> style nit: Don't use "default: NOTREACHED" here.  Just handle all the cases of
> the enum.

Done.

https://codereview.chromium.org/2418173002/diff/160001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1124: enum class HighbitTexture {
On 2016/10/19 19:04:03, enne wrote:
> What does HighbitTexture mean? I guess I don't understand the "high bit" part
of
> that name.

I follow this existing naming. e.g. 1113   bool highbit = GetParam();

highbit means one channel value is more than 8 bit.
e.g. YUV420P10 video's each channel is 10bit.

Y8 uses LUMINACE8 or R8 after converting 10bit value to 8bit value.
RGBA_8888 contains RGBA data after converting YUV to RGBA.
LUMINANCE_F16 contains intact 10bit value.

https://codereview.chromium.org/2418173002/diff/160001/cc/output/renderer_pix...
cc/output/renderer_pixeltest.cc:1279: // LUMINANCE_F16 requires
--use-gpu-in-tests
On 2016/10/19 19:04:03, enne wrote:
> Do we have --use-gpu-in-tests running anywhere? Can you include this but early
> out of this configuration if --use-gpu-in-tests isn't running or if this case
> isn't supported?

good idea. Done.

https://codereview.chromium.org/2418173002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2418173002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:432:
*outMailbox = cc::TextureMailbox(mailbox, syncToken, GL_TEXTURE_2D, m_size,
On 2016/10/19 19:04:03, enne wrote:
> Why did this need to change?

It's the bug I mentioned above. without this change,
GLRenderer::EnqueueTextureQuad() don't know texture size. It's ok so far,
because the method doesn't use texture size. However, now we need texture size
to calculate one unit texel.

The reason is that ResourceProvider::CreateResourceFromTextureMailbox() set size
to Resource using size in TextureMailbox.
https://cs.chromium.org/chromium/src/cc/resources/resource_provider.cc?q=Crea...

I have a plan to fix it in next CL by replacing cc::TextureMailbox(3 params) to
cc::TextureMailbox(6 params) in all place.

[enne (OOO), 2016-10-20 20:29:51]

https://codereview.chromium.org/2418173002/diff/180001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2418173002/diff/180001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2733: if (draw_cache_.tex_clamp_rect_location != -1) {
Here's the issue.  tex_clamp_rect is meant to be a parallel array.  If you only
conditionally append to that array, then it won't index correctly.  If you have
one texture quad without a tex clamp rect and then one with a tex clamp rect,
the array will be of size 1, which isn't correct for two quads batched together.

I don't think coalescing these is an important use case for this batching
optimization, which I consider an edge case.  I would prefer if you just make
tex clamp rect a float4 uniform instead of an array and just not batch anything
with a texture clamp rect.  It will make the code simpler, in my opinion.

[commit-bot: I haz the power, 2016-10-20 21:00:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-20 21:00:11]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dshwang, 2016-10-21 13:26:21]

Hi enne, I replied your comment. It takes time due to different time zone. (I'm
in Finland)

https://codereview.chromium.org/2418173002/diff/180001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2418173002/diff/180001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2733: if (draw_cache_.tex_clamp_rect_location != -1) {
On 2016/10/20 20:29:51, enne wrote:
> Here's the issue.  tex_clamp_rect is meant to be a parallel array.  If you
only
> conditionally append to that array, then it won't index correctly.  If you
have
> one texture quad without a tex clamp rect and then one with a tex clamp rect,
> the array will be of size 1, which isn't correct for two quads batched
together.

In one batch, it is not conditional. This CL implement clamp rect when only
background color is SK_ColorTRANSPARENT, which means subset of shaders implement
it. It's because video_layer_impl.cc produces only TextureQuad with
SK_ColorTRANSPARENT background.
https://cs.chromium.org/chromium/src/cc/layers/video_layer_impl.cc?q=VideoFra...

If this CL implement clamp rect for background color shaders also, this
condition is not needed.

> I don't think coalescing these is an important use case for this batching
> optimization, which I consider an edge case.  I would prefer if you just make
> tex clamp rect a float4 uniform instead of an array and just not batch
anything
> with a texture clamp rect.  It will make the code simpler, in my opinion.

Your explanation makes sense. However, there is few caveats.
1. TextureQuad doesn't know content rect in the texture. Now clamp logic depends
on texel quad->uv_top_left and quad->uv_bottom_right, which can be smaller than
actual content rect. To make it uniform, it's needed to plumb actual content
rect in all TextureQuad's client code. It may result in more complicated code,
because most client has same size of coded_size and visual_rect.
2. TextureQuad has capability to deal with Texture Atlas (many images on a
texture). uniform breaks the capability. Is it desirable? Texture Atlas may be
used to optimize browser UI in the future?

https://codereview.chromium.org/2418173002/diff/180001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/2418173002/diff/180001/cc/output/shader.cc#ne...
cc/output/shader.cc:1225: int FragmentTexQuadBase::tex_clamp_rect_location()
const {
When TextureQuad has background color, this location is always -1, because
FragmentTexBackgroundBinding doesn't implement it.

[inactive_dshwang_plz_cc_intel, 2016-10-21 14:32:30]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 14:32:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-21 16:51:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-21 16:51:22]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[enne (OOO), 2016-10-21 18:53:05]

Oh, right, sure.  The program id would be different and they wouldn't batch
together anyway.  Sounds good, sorry for my confusion!  lgtm

[dshwang, 2016-10-25 21:11:02]

dongseong.hwang@intel.com changed reviewers:
+ ccameron@chromium.org, dalecurtis@chromium.org, dpranke@chromium.org,
junov@chromium.org

[dshwang, 2016-10-25 21:11:03]

junov@, could you review WebKit/Canvas ?

ccameron@, could you review content/browser ? 

dpranke@, could you review test_runner/ ?

dalecurtis@, could you review media/ ?

[Justin Novosad, 2016-10-25 21:28:17]

https://codereview.chromium.org/2418173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2418173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:436: false,
false);
These "false" literals are bad for code readability.  Please use temporary
variables.
Ex: 
bool isOverlayCandidate = false;
bool secureOutputOnly = false;
*outMailbox = cc::TextureMailbox( ... , isOverlayCandidate, secureOutputOnly);

[DaleCurtis, 2016-10-25 23:53:38]

media/ lgtm % nits.

https://codereview.chromium.org/2418173002/diff/200001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2418173002/diff/200001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:528: if (size_type ==
SkCanvasVideoRenderer::ConvertingSize::VISUAL) {
Drop {} from single life if.

https://codereview.chromium.org/2418173002/diff/200001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:531: DCHECK(size_type ==
SkCanvasVideoRenderer::ConvertingSize::CODED);
DCHECK_EQ?

[inactive_dshwang_plz_cc_intel, 2016-10-26 15:25:56]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 15:26:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2016-10-26 15:26:28]

junov@, could you review WebKit/Canvas ?
ccameron@, could you review content/browser ? 
dpranke@, could you review test_runner/ ?

https://codereview.chromium.org/2418173002/diff/200001/media/renderers/skcanv...
File media/renderers/skcanvas_video_renderer.cc (right):

https://codereview.chromium.org/2418173002/diff/200001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:528: if (size_type ==
SkCanvasVideoRenderer::ConvertingSize::VISUAL) {
On 2016/10/25 23:53:37, DaleCurtis wrote:
> Drop {} from single life if.

Done.

https://codereview.chromium.org/2418173002/diff/200001/media/renderers/skcanv...
media/renderers/skcanvas_video_renderer.cc:531: DCHECK(size_type ==
SkCanvasVideoRenderer::ConvertingSize::CODED);
On 2016/10/25 23:53:37, DaleCurtis wrote:
> DCHECK_EQ?

DCHECK_EQ doesn't support enum class, unfortunately.

https://codereview.chromium.org/2418173002/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp
(right):

https://codereview.chromium.org/2418173002/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/graphics/Canvas2DLayerBridge.cpp:436: false,
false);
On 2016/10/25 21:28:16, Justin Novosad wrote:
> These "false" literals are bad for code readability.  Please use temporary
> variables.
> Ex: 
> bool isOverlayCandidate = false;
> bool secureOutputOnly = false;
> *outMailbox = cc::TextureMailbox( ... , isOverlayCandidate, secureOutputOnly);

Done. Could you review again?

In addition, do you know how to run
Pixel_OffscreenCanvasUnaccelerated2DGPUCompositing test?
It should belong to gpu test, but Linux chromium doesn't support it..
content/test/gpu/gpu_tests/pixel_test_pages.py:      base_name +
'_OffscreenCanvasUnaccelerated2DGPUCompositing',

> ./content/test/gpu/run_gpu_test.py  --browser=release
Available benchmarks for release are:
  cloud_storage_test_base.CloudStorageTestBase 
  device_trace_test                            Tests GPU Device traces show up
on devices that support it.
  gpu_process                                  Tests that accelerated content
triggers the creation of a GPU process
  gpu_rasterization                            Tests that GPU rasterization
produces valid content
  gpu_test_base.TestBase                       Base classes for all GPU tests in
this directory. Implements
  gpu_test_base_unittest.FakeTest              
  hardware_accelerated_feature                 Tests GPU acceleration is
reported as active for various features
  maps                                         Google Maps pixel tests.
  screenshot_sync                              Tests that screenhots are
properly synchronized with the frame one which
  trace_test                                   Tests GPU traces are plumbed
through properly.
  trace_test.TraceTestBase                     Base class for the trace tests.

[commit-bot: I haz the power, 2016-10-26 16:43:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-26 16:43:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Dirk Pranke, 2016-10-26 17:51:19]

lgtm

[ccameron, 2016-10-26 18:30:16]

On 2016/10/26 17:51:19, Dirk Pranke (slow) wrote:
> lgtm

content/browser lgtm

[Justin Novosad, 2016-10-26 19:38:31]

lgtm

[inactive_dshwang_plz_cc_intel, 2016-10-27 15:37:46]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 15:38:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[inactive_dshwang_plz_cc_intel, 2016-10-27 15:47:10]

The CQ bit was checked by dongseong.hwang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 15:47:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dshwang, 2016-10-27 18:22:56]

Patchset #10 (id:260001) has been deleted

[dshwang, 2016-10-27 18:23:07]

Patchset #9 (id:240001) has been deleted

[dshwang, 2016-10-27 18:24:44]

Thank you for review!

I'll land it after https://codereview.chromium.org/2454153002/ is landed.
mac_chromium_rel_ng fails due to offscreen canvas hidden bug, which this cl
revealed. I'm fixing the bug in the CL.

[commit-bot: I haz the power, 2016-10-27 22:01:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-27 22:01:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dshwang, 2016-10-31 13:15:24]

The CQ bit was checked by dongseong.hwang@intel.com

[dshwang, 2016-10-31 13:15:24]

The patchset sent to the CQ was uploaded after l-g-t-m from enne@chromium.org,
dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/2418173002/#ps220001
(title: "fix nits")

[commit-bot: I haz the power, 2016-10-31 13:15:35]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-31 14:42:08]

Description was changed from

==========
Fix HTML5 video blurry

HW decoded video is blurry, because crbug.com/575587 shinked visual rect by 1
pixel, if coded rect is larger than visual rect. This fixed bleeding texture,
but caused blurry issue.

Meanwhile, crbug.com/429640 fixed bleeding texture for software YUV video in
the elegant way. It clamps all texture coordinates to this maximum value in the
fragment shader.

This CL removes the 1 pixel hack and applies the way to TextureDrawQuad and
StreamVideoDrawQuad (i.e. HW decoded video path).

BUG=615325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix HTML5 video blurry

HW decoded video is blurry, because crbug.com/575587 shinked visual rect by 1
pixel, if coded rect is larger than visual rect. This fixed bleeding texture,
but caused blurry issue.

Meanwhile, crbug.com/429640 fixed bleeding texture for software YUV video in
the elegant way. It clamps all texture coordinates to this maximum value in the
fragment shader.

This CL removes the 1 pixel hack and applies the way to TextureDrawQuad and
StreamVideoDrawQuad (i.e. HW decoded video path).

BUG=615325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

[commit-bot: I haz the power, 2016-10-31 14:42:09]

Committed patchset #8 (id:220001)

[commit-bot: I haz the power, 2016-10-31 14:43:40]

Description was changed from

==========
Fix HTML5 video blurry

HW decoded video is blurry, because crbug.com/575587 shinked visual rect by 1
pixel, if coded rect is larger than visual rect. This fixed bleeding texture,
but caused blurry issue.

Meanwhile, crbug.com/429640 fixed bleeding texture for software YUV video in
the elegant way. It clamps all texture coordinates to this maximum value in the
fragment shader.

This CL removes the 1 pixel hack and applies the way to TextureDrawQuad and
StreamVideoDrawQuad (i.e. HW decoded video path).

BUG=615325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel
==========

to

==========
Fix HTML5 video blurry

HW decoded video is blurry, because crbug.com/575587 shinked visual rect by 1
pixel, if coded rect is larger than visual rect. This fixed bleeding texture,
but caused blurry issue.

Meanwhile, crbug.com/429640 fixed bleeding texture for software YUV video in
the elegant way. It clamps all texture coordinates to this maximum value in the
fragment shader.

This CL removes the 1 pixel hack and applies the way to TextureDrawQuad and
StreamVideoDrawQuad (i.e. HW decoded video path).

BUG=615325
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Committed: https://crrev.com/b38566a76c4af61c0d34df7d8b906e7936b8e524
Cr-Commit-Position: refs/heads/master@{#428707}
==========

[commit-bot: I haz the power, 2016-10-31 14:43:42]

Patchset 8 (id:??) landed as
https://crrev.com/b38566a76c4af61c0d34df7d8b906e7936b8e524
Cr-Commit-Position: refs/heads/master@{#428707}

[xidachen, 2016-10-31 16:26:07]

A revert of this CL (patchset #8 id:220001) has been created in
https://codereview.chromium.org/2463103002/ by xidachen@chromium.org.

The reason for reverting is: Suspect causing cc_unittests failure:
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....

[dshwang, 2016-10-31 17:47:29]

On 2016/10/31 16:26:07, xidachen wrote:
> A revert of this CL (patchset #8 id:220001) has been created in
> https://codereview.chromium.org/2463103002/ by mailto:xidachen@chromium.org.
> 
> The reason for reverting is: Suspect causing cc_unittests failure:
>
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....

Thx for reverting. I'm figuring out whether it causes cc_unittests failure. If
other CL causes the issue, please let me know.

[ynovikov, 2016-10-31 17:54:04]

On 2016/10/31 17:47:29, dshwang wrote:
> On 2016/10/31 16:26:07, xidachen wrote:
> > A revert of this CL (patchset #8 id:220001) has been created in
> > https://codereview.chromium.org/2463103002/ by mailto:xidachen@chromium.org.
> > 
> > The reason for reverting is: Suspect causing cc_unittests failure:
> >
>
https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2....
> 
> Thx for reverting. I'm figuring out whether it causes cc_unittests failure. If
> other CL causes the issue, please let me know.

This also causes pixel_test failures on Android GPU.FYI bots. Please update
expectations (reference images) when you reland this.