Video Color Managament

Video Color Managament

Creates a new class media::VideoColorSpace which can represent all h264 color spaces, we encode this into the metadata for a video frame and forward it all the way to gl_renderer.cc, where we use it to render the video correctly.

BUG=622133
Committed: https://crrev.com/af57af3079ddbb21be992e839023ff6e45ec6ef4
Cr-Commit-Position: refs/heads/master@{#411297}

[hubbe, 2016-06-22 22:46:54]

Description was changed from

==========
Video Color Managament

Attaches an SkColorSpace to video frames and video quads and uses a 3d LUT to
correct colors on output.
==========

to

==========
Video Color Managament

Attaches an SkColorSpace to video frames and video quads and uses a 3d LUT to
correct colors on output.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[hubbe, 2016-07-20 23:23:44]

Description was changed from

==========
Video Color Managament

Attaches an SkColorSpace to video frames and video quads and uses a 3d LUT to
correct colors on output.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Video Color Managament

Attaches an SkColorSpace to video frames and video quads and uses a 3d LUT to
correct colors on output.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

BUG=622133
==========

[hubbe, 2016-07-20 23:23:52]

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

[commit-bot: I haz the power, 2016-07-20 23:24:22]

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-07-20 23:24:24]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.blink
   For more details, see http://crbug.com/617627.

[hubbe, 2016-07-20 23:25:07]

Description was changed from

==========
Video Color Managament

Attaches an SkColorSpace to video frames and video quads and uses a 3d LUT to
correct colors on output.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

BUG=622133
==========

to

==========
Video Color Managament

Creates a new class media::VideoColorSpace which can represent all h264 color
spaces, we encode this into the metadata for a video frame and forward it all
the way to gl_renderer.cc, where we use it to render the video correctly.

BUG=622133
==========

[hubbe, 2016-07-20 23:25:22]

hubbe@chromium.org changed reviewers:
+ ccameron@chromium.org

[commit-bot: I haz the power, 2016-07-20 23:38:45]

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

[commit-bot: I haz the power, 2016-07-20 23:38:46]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[hubbe, 2016-07-21 17:45:33]

Description was changed from

==========
Video Color Managament

Creates a new class media::VideoColorSpace which can represent all h264 color
spaces, we encode this into the metadata for a video frame and forward it all
the way to gl_renderer.cc, where we use it to render the video correctly.

BUG=622133
==========

to

==========
Video Color Managament

Creates a new class media::VideoColorSpace which can represent all h264 color
spaces, we encode this into the metadata for a video frame and forward it all
the way to gl_renderer.cc, where we use it to render the video correctly.

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

[hubbe, 2016-07-21 17:45:45]

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

[commit-bot: I haz the power, 2016-07-21 17:46:03]

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-07-21 17:53:45]

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

[commit-bot: I haz the power, 2016-07-21 17:53:45]

Dry run: Try jobs failed on following builders:
  win8_chromium_gyp_rel on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gyp...)

[ccameron, 2016-07-21 18:38:13]

The meat of this looks good. Couple comments on the plumbing

https://codereview.chromium.org/2088273003/diff/140001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2088273003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2108: resource_provider_, quad->u_plane_resource_id(),
GL_TEXTURE2, GL_LINEAR);
Here we'd get the gfx::ColorSpace from the y_plane_lock.

In the case of YUV multi-planar textures (like here), we'd just ant to DHECK
that Y/UV/U/V all have the same color space. IOSurfaces are a bit smarter in
that they allow multi-planar textures, which share a single color space object.

https://codereview.chromium.org/2088273003/diff/140001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/2088273003/diff/140001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:444: }
At this point we would call

plane_resource.gpu_memory_buffer->SetColorSpace(/* the color space from the
frame */)

That would go all the way down to the IOSurface for CALayer overlays.

[hubbe, 2016-07-21 19:05:31]

https://codereview.chromium.org/2088273003/diff/140001/cc/output/gl_renderer.cc
File cc/output/gl_renderer.cc (right):

https://codereview.chromium.org/2088273003/diff/140001/cc/output/gl_renderer....
cc/output/gl_renderer.cc:2108: resource_provider_, quad->u_plane_resource_id(),
GL_TEXTURE2, GL_LINEAR);
On 2016/07/21 18:38:13, ccameron wrote:
> Here we'd get the gfx::ColorSpace from the y_plane_lock.
> 
> In the case of YUV multi-planar textures (like here), we'd just ant to DHECK
> that Y/UV/U/V all have the same color space. IOSurfaces are a bit smarter in
> that they allow multi-planar textures, which share a single color space
object.

While entirely possible, it doesn't make any actual sense (to me). The textures
are grayscale, so it's not possible for the textures to be BT709. Only the Quad
actually knows that these textures belong together.

https://codereview.chromium.org/2088273003/diff/140001/media/video/gpu_memory...
File media/video/gpu_memory_buffer_video_frame_pool.cc (right):

https://codereview.chromium.org/2088273003/diff/140001/media/video/gpu_memory...
media/video/gpu_memory_buffer_video_frame_pool.cc:444: }
On 2016/07/21 18:38:13, ccameron wrote:
> At this point we would call
> 
> plane_resource.gpu_memory_buffer->SetColorSpace(/* the color space from the
> frame */)
> 
> That would go all the way down to the IOSurface for CALayer overlays.

Are you suggesting that we don't set the color space on the video frame? If so,
what happens in those cases when we can't use a gpu memory buffer?

[hubbe, 2016-08-05 23:10:50]

Now uses gfx::ColorSpace.
Probably still needs some other clean up though.

PTAL.

[hubbe, 2016-08-05 23:11:14]

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

[commit-bot: I haz the power, 2016-08-05 23:11:42]

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-08-05 23:11:42]

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

[commit-bot: I haz the power, 2016-08-05 23:11:43]

Dry run: Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[ccameron, 2016-08-08 18:45:07]

lg in general -- the only thing that needs fixing IMO is the
GL_TEXTURE_2D->GL_TEXTURE_3D bit.

https://codereview.chromium.org/2088273003/diff/160001/cc/layers/video_layer_...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/2088273003/diff/160001/cc/layers/video_layer_...
cc/layers/video_layer_impl.cc:222:
render_pass->CreateAndAppendDrawQuad<TextureDrawQuad>();
FYI, this is the path that Mac takes to draw most video (when we fail to use
CoreAnimation), and it pre-translates YUV->RGB in a buffer (groan WRT precision
loss), in case you have issues debugging on Mac.

https://codereview.chromium.org/2088273003/diff/160001/cc/output/color_lut_ca...
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2088273003/diff/160001/cc/output/color_lut_ca...
cc/output/color_lut_cache.cc:81: lut.data());
We should be using GL_TEXTURE_3D for the lut -- then we can sample using a vec3,
and we don't have to do any work to clamp the R coordinate.

https://codereview.chromium.org/2088273003/diff/160001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/2088273003/diff/160001/cc/output/shader.cc#ne...
cc/output/shader.cc:2138: vec4 LUT(sampler2D sampler, vec3 pos, float size) {
With sampler3D, this just becomes texture3D(sampler, pos), and we can drop all
of the uniforms for size, etc.

https://codereview.chromium.org/2088273003/diff/160001/cc/quads/yuv_video_dra...
File cc/quads/yuv_video_draw_quad.h (right):

https://codereview.chromium.org/2088273003/diff/160001/cc/quads/yuv_video_dra...
cc/quads/yuv_video_draw_quad.h:92: gfx::ColorSpace video_color_space;
I'd rather pull this out of the ResourceProvider::ScopedSamplerGL, since we'll
need to be setting the cc::ResourceProvider's parameters for all resources
anyway.

That plumbing isn't present, so I'm fine leaving this as-is, and unifying it
when the ResourceProvider work is in place.

[hubbe, 2016-08-08 19:45:37]

hubbe@chromium.org changed reviewers:
+ nasko@chromium.org

[hubbe, 2016-08-08 19:45:38]

+nasko for
    cc/ipc/cc_param_traits.cc
    cc/ipc/cc_param_traits_unittest.cc

https://codereview.chromium.org/2088273003/diff/160001/cc/layers/video_layer_...
File cc/layers/video_layer_impl.cc (right):

https://codereview.chromium.org/2088273003/diff/160001/cc/layers/video_layer_...
cc/layers/video_layer_impl.cc:222:
render_pass->CreateAndAppendDrawQuad<TextureDrawQuad>();
On 2016/08/08 18:45:07, ccameron wrote:
> FYI, this is the path that Mac takes to draw most video (when we fail to use
> CoreAnimation), and it pre-translates YUV->RGB in a buffer (groan WRT
precision
> loss), in case you have issues debugging on Mac.

Really? Why doesn't it planar yuv resources when it fails to use CoreAnimation?

https://codereview.chromium.org/2088273003/diff/160001/cc/output/color_lut_ca...
File cc/output/color_lut_cache.cc (right):

https://codereview.chromium.org/2088273003/diff/160001/cc/output/color_lut_ca...
cc/output/color_lut_cache.cc:81: lut.data());
On 2016/08/08 18:45:07, ccameron wrote:
> We should be using GL_TEXTURE_3D for the lut -- then we can sample using a
vec3,
> and we don't have to do any work to clamp the R coordinate.

No we should not, GL_TEXTURE_3D has absolutely awful performance on some
platforms, which is why skia doesn't use it in it's 3d-lut implementation
either.

https://codereview.chromium.org/2088273003/diff/160001/cc/output/shader.cc
File cc/output/shader.cc (right):

https://codereview.chromium.org/2088273003/diff/160001/cc/output/shader.cc#ne...
cc/output/shader.cc:2138: vec4 LUT(sampler2D sampler, vec3 pos, float size) {
On 2016/08/08 18:45:07, ccameron wrote:
> With sampler3D, this just becomes texture3D(sampler, pos), and we can drop all
> of the uniforms for size, etc.

Agreed, but it's a performance minefield...
texture3D is probably fine on windows though, and may be more efficient than
this code, so it's probably worth investigating, but I don't think it's right
for an initial implementation.

https://codereview.chromium.org/2088273003/diff/160001/cc/quads/yuv_video_dra...
File cc/quads/yuv_video_draw_quad.h (right):

https://codereview.chromium.org/2088273003/diff/160001/cc/quads/yuv_video_dra...
cc/quads/yuv_video_draw_quad.h:92: gfx::ColorSpace video_color_space;
On 2016/08/08 18:45:07, ccameron wrote:
> I'd rather pull this out of the ResourceProvider::ScopedSamplerGL, since we'll
> need to be setting the cc::ResourceProvider's parameters for all resources
> anyway.
> 
> That plumbing isn't present, so I'm fine leaving this as-is, and unifying it
> when the ResourceProvider work is in place.

I'll add a TODO.

[nasko, 2016-08-08 21:15:22]

https://codereview.chromium.org/2088273003/diff/180001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2088273003/diff/180001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:939: l->append(", ");
Shouldn't we also be logging the new member here?

[hubbe, 2016-08-10 17:03:25]

https://codereview.chromium.org/2088273003/diff/180001/cc/ipc/cc_param_traits.cc
File cc/ipc/cc_param_traits.cc (right):

https://codereview.chromium.org/2088273003/diff/180001/cc/ipc/cc_param_traits...
cc/ipc/cc_param_traits.cc:939: l->append(", ");
On 2016/08/08 21:15:21, nasko wrote:
> Shouldn't we also be logging the new member here?

Done.

[nasko, 2016-08-10 20:30:05]

IPC LGTM

[ccameron, 2016-08-11 00:25:56]

lgtm as well

I took a look back at the chrome-color threads, and found where 3D versus
2D-faking-3D lut performance is discussed. Surprising (which was what I said
then, too, but didn't remember).

[hubbe, 2016-08-11 00:26:55]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2016-08-11 00:27:30]

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-08-11 00:27:32]

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

[commit-bot: I haz the power, 2016-08-11 00:27:33]

Your CL was about to rely on recently removed CQ feature(s):
 * master.tryserver.blink builder linux_blink_rel was renamed to
linux_precise_blink_rel (http://crbug.com/590036)

[hubbe, 2016-08-11 06:58:23]

Description was changed from

==========
Video Color Managament

Creates a new class media::VideoColorSpace which can represent all h264 color
spaces, we encode this into the metadata for a video frame and forward it all
the way to gl_renderer.cc, where we use it to render the video correctly.

BUG=622133
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_blink_rel
==========

to

==========
Video Color Managament

Creates a new class media::VideoColorSpace which can represent all h264 color
spaces, we encode this into the metadata for a video frame and forward it all
the way to gl_renderer.cc, where we use it to render the video correctly.

BUG=622133
==========

[hubbe, 2016-08-11 06:58:31]

The CQ bit was checked by hubbe@chromium.org

[commit-bot: I haz the power, 2016-08-11 06:58:40]

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-08-11 10:42:49]

Description was changed from

==========
Video Color Managament

Creates a new class media::VideoColorSpace which can represent all h264 color
spaces, we encode this into the metadata for a video frame and forward it all
the way to gl_renderer.cc, where we use it to render the video correctly.

BUG=622133
==========

to

==========
Video Color Managament

Creates a new class media::VideoColorSpace which can represent all h264 color
spaces, we encode this into the metadata for a video frame and forward it all
the way to gl_renderer.cc, where we use it to render the video correctly.

BUG=622133
==========

[commit-bot: I haz the power, 2016-08-11 10:42:50]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2016-08-11 10:44:17]

Description was changed from

==========
Video Color Managament

Creates a new class media::VideoColorSpace which can represent all h264 color
spaces, we encode this into the metadata for a video frame and forward it all
the way to gl_renderer.cc, where we use it to render the video correctly.

BUG=622133
==========

to

==========
Video Color Managament

Creates a new class media::VideoColorSpace which can represent all h264 color
spaces, we encode this into the metadata for a video frame and forward it all
the way to gl_renderer.cc, where we use it to render the video correctly.

BUG=622133

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

[commit-bot: I haz the power, 2016-08-11 10:44:19]

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