Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(271)

Issue 2122573003: media: replace LUMINANCE_F16 by RG_88 for 9/10-bit h264 videos

Created:
4 years, 5 months ago by dshwang
Modified:
3 years, 8 months ago
CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, cc-bugs_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, feature-media-reviews_chromium.org, jam, kalyank, ozone-reviews_chromium.org, piman+watch_chromium.org, posciak+watch_chromium.org, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org, enne. hubbe_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

media: replace LUMINANCE_F16 by RG_88 for 9/10-bit h264 videos LUMINANCE_F16 has following issues 1. GL_LUMINANCE (as well as GL_ALPHA) is deprecated. 2. GpuMemoryBuffer cannot support LUMINANCE_F16. 3. LUMINANCE_F16 requires cpu int-to-float conversion. This CL replaces LUMINANCE_F16 by RG_88 to fix all above issues. Rationale to choose RG_88, instead of R_16 1. GpuMemoryBuffer cannot support R_16. (e.g. Mesa supports R8 and GR88 dmabuf) 2. GL_EXT_texture_rg is more widely used than GL_EXT_color_buffer_half_float (e.g. Mesa v11.2) This CL introduces kUseRGTexture feature to use RG_88, instead of LUMINANCE_F16. When we make sure RG_88 is better on all platforms, LUMINANCE_F16 will be removed. BUG=445071, 624436, 622133 TEST=existing cc_unittests. e.g. VideoGLRendererPixelHiLoTest, VideoResourceUpdaterTestWithRG media_unittests --gtest_filter=PipelineIntegrationTest.BasicPlaybackHi10P content_browsertests --gtest_filter=*VideoBearHighBitDepthMp4* chrome file:///<chromium>/src/media/test/data/bear-320x180-hi10p.mp4 chrome file:///<chromium>/src/media/test/data/bear-320x180-hi10p.mp4 --enable-features=video-color-management chrome file:///<chromium>/src/media/test/data/bear-320x180-hi10p.mp4 --enable-features=use-rg-texture chrome file:///<chromium>/src/media/test/data/bear-320x180-hi10p.mp4 --enable-features=video-color-management,use-rg-texture CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_precise_blink_rel

Patch Set 1 : build fix #

Total comments: 6

Patch Set 2 : add custom bilinear filter and pass all unittests #

Patch Set 3 : before flag-controlled for RG8 #

Patch Set 4 : introduce --disable-half-float-conversion-texture flag #

Total comments: 26

Patch Set 5 : resolve hubbe's concerns except for test. separate the CLs. #

Total comments: 9

Patch Set 6 : add pixel tests for RG88 and resolve hubbe's comments #

Total comments: 6

Patch Set 7 : add more pixel test, and resolve concerns #

Total comments: 4

Patch Set 8 : rebase to ToT #

Unified diffs Side-by-side diffs Delta from patch set Stats (+625 lines, -249 lines) Patch
M cc/layers/video_layer_impl.cc View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M cc/output/gl_renderer.h View 1 2 3 4 5 6 7 2 chunks +11 lines, -7 lines 0 comments Download
M cc/output/gl_renderer.cc View 1 2 3 4 5 6 7 9 chunks +139 lines, -81 lines 0 comments Download
M cc/output/gl_renderer_unittest.cc View 1 2 3 4 5 6 7 1 chunk +6 lines, -2 lines 0 comments Download
M cc/output/renderer_pixeltest.cc View 1 2 3 4 5 6 7 12 chunks +171 lines, -73 lines 0 comments Download
M cc/output/shader.h View 1 2 3 4 5 4 chunks +16 lines, -1 line 0 comments Download
M cc/output/shader.cc View 1 2 3 4 5 6 9 chunks +98 lines, -20 lines 0 comments Download
M cc/quads/yuv_video_draw_quad.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M cc/raster/raster_buffer_provider.cc View 1 2 3 4 5 6 7 3 chunks +3 lines, -0 lines 0 comments Download
M cc/resources/platform_color.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M cc/resources/platform_color_unittest.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M cc/resources/resource_format.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M cc/resources/resource_format.cc View 1 2 3 7 chunks +25 lines, -2 lines 0 comments Download
M cc/resources/resource_format_utils.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M cc/resources/resource_provider.h View 1 2 3 4 5 6 7 4 chunks +7 lines, -3 lines 0 comments Download
M cc/resources/resource_provider.cc View 1 2 3 4 5 6 7 11 chunks +22 lines, -23 lines 0 comments Download
M cc/resources/video_resource_updater.h View 1 2 3 4 5 6 3 chunks +8 lines, -0 lines 0 comments Download
M cc/resources/video_resource_updater.cc View 1 2 3 4 5 6 11 chunks +73 lines, -34 lines 0 comments Download
M cc/resources/video_resource_updater_unittest.cc View 1 2 3 4 5 6 2 chunks +18 lines, -1 line 0 comments Download
A cc/test/data/yuv_stripes_clipped_rg88.png View 1 2 3 4 5 6 Binary file 0 comments Download
A cc/test/data/yuv_stripes_clipped_rgba.png View 1 2 3 4 5 6 Binary file 0 comments Download
A cc/test/data/yuv_stripes_rg88.png View 1 2 3 4 5 6 Binary file 0 comments Download
A cc/test/data/yuv_stripes_rgba.png View 1 2 3 4 5 6 Binary file 0 comments Download
M cc/test/test_in_process_context_provider.h View 1 2 3 4 5 6 2 chunks +10 lines, -0 lines 0 comments Download
M cc/test/test_in_process_context_provider.cc View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M cc/test/test_web_graphics_context_3d.h View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M media/base/media_switches.h View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M media/base/media_switches.cc View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 75 (38 generated)
dshwang
dnicoara, dcastagna, hubbe, could you review? hubbe suggested Y16 (or Z16) use LUMINANCE_F16 in https://bugs.chromium.org/p/chromium/issues/detail?id=624436#c4 ...
4 years, 5 months ago (2016-07-04 11:27:15 UTC) #4
Daniele Castagna
Hi DS! Could you please split this CL? You could start just adding and testing ...
4 years, 5 months ago (2016-07-06 17:38:21 UTC) #6
dshwang
On 2016/07/06 17:38:21, Daniele Castagna wrote: > Hi DS! > Could you please split this ...
4 years, 5 months ago (2016-07-06 17:45:54 UTC) #7
hubbe
https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc#newcode2110 cc/output/shader.cc:2110: return ((rg.g * 65536.0) + (rg.r * 256.0)) / ...
4 years, 5 months ago (2016-07-06 17:54:17 UTC) #8
dshwang
Thank you for feedback. https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc#newcode2110 cc/output/shader.cc:2110: return ((rg.g * 65536.0) + ...
4 years, 5 months ago (2016-07-06 18:12:20 UTC) #9
fbarchard1
Both Arm and Intel provide half float SIMD conversions, if performance is a concern.
4 years, 5 months ago (2016-07-06 18:19:19 UTC) #10
Ken Russell (switch to Gerrit)
https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc#newcode2110 cc/output/shader.cc:2110: return ((rg.g * 65536.0) + (rg.r * 256.0)) / ...
4 years, 5 months ago (2016-07-06 18:20:21 UTC) #11
hubbe
On 2016/07/06 18:12:20, dshwang wrote: > Thank you for feedback. > > https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc > File ...
4 years, 5 months ago (2016-07-06 18:44:59 UTC) #12
dshwang
Hi, Thank you for your feedback. I took time to investigate and write my result ...
4 years, 5 months ago (2016-07-07 15:49:48 UTC) #13
hubbe
https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc#newcode2110 cc/output/shader.cc:2110: return ((rg.g * 65536.0) + (rg.r * 256.0)) / ...
4 years, 5 months ago (2016-07-07 17:28:09 UTC) #14
dshwang
https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc#newcode2110 cc/output/shader.cc:2110: return ((rg.g * 65536.0) + (rg.r * 256.0)) / ...
4 years, 5 months ago (2016-07-07 17:48:59 UTC) #15
hubbe
On 2016/07/07 17:48:59, dshwang wrote: > https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc > File cc/output/shader.cc (right): > > https://codereview.chromium.org/2122573003/diff/60001/cc/output/shader.cc#newcode2110 > ...
4 years, 5 months ago (2016-07-07 18:19:01 UTC) #16
fbarchard
> > CPU conversion is very fast and does not require a lot of power. ...
4 years, 5 months ago (2016-07-07 23:20:56 UTC) #17
dshwang
hi, long time no see I push forward this CL, because 1. alecksandar shows rg8 ...
4 years, 3 months ago (2016-09-20 15:04:01 UTC) #19
chromium-reviews
Just make sure that the RG8 change is flag-controlled so we can test it out ...
4 years, 3 months ago (2016-09-20 17:08:16 UTC) #20
dshwang
hubbe@, could you review again? I introduce --disable-half-float-conversion-texture to use RG_88, instead of LUMINANCE_F16. When ...
4 years, 2 months ago (2016-09-27 18:57:24 UTC) #31
hubbe
THis CL is rather large, is it possible to split it into smaller CLs? Maybe ...
4 years, 2 months ago (2016-09-27 20:49:52 UTC) #34
dshwang
> This CL is rather large, is it possible to split it into smaller CLs? ...
4 years, 2 months ago (2016-09-28 10:33:27 UTC) #35
hubbe
https://codereview.chromium.org/2122573003/diff/120001/cc/base/switches.cc File cc/base/switches.cc (right): https://codereview.chromium.org/2122573003/diff/120001/cc/base/switches.cc#newcode48 cc/base/switches.cc:48: const char kDisableHalfFloatConversionTexture[] = On 2016/09/28 10:33:27, dshwang wrote: ...
4 years, 2 months ago (2016-09-28 17:57:44 UTC) #36
dshwang
Thank you for reviewing. I resolved all your concern except for additional test. I'm working ...
4 years, 2 months ago (2016-09-29 18:52:28 UTC) #39
hubbe
https://codereview.chromium.org/2122573003/diff/120001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/2122573003/diff/120001/cc/output/shader.cc#newcode2133 cc/output/shader.cc:2133: min(clamp_rect.zw, tex_coord + vec2(unit_texel.x, 0.))); On 2016/09/29 18:52:28, dshwang ...
4 years, 2 months ago (2016-09-29 23:36:51 UTC) #40
dshwang
hubbe@, I resolved all your comments and add new pixel test for RG88 code. could ...
4 years, 2 months ago (2016-10-05 15:13:42 UTC) #45
hubbe
I think we're getting pretty close, but I'm an owner of this code... https://codereview.chromium.org/2122573003/diff/160001/cc/output/renderer_pixeltest.cc File ...
4 years, 2 months ago (2016-10-05 18:03:25 UTC) #48
enne (OOO)
https://codereview.chromium.org/2122573003/diff/140001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/2122573003/diff/140001/cc/output/gl_renderer.cc#newcode2370 cc/output/gl_renderer.cc:2370: if (use_color_lut) { On 2016/10/05 at 15:13:39, dshwang wrote: ...
4 years, 2 months ago (2016-10-05 18:59:44 UTC) #50
dshwang
hubbe, I took investigation to explain why the pixel results are different. I think RG ...
4 years, 2 months ago (2016-10-07 12:35:01 UTC) #55
dshwang
https://codereview.chromium.org/2122573003/diff/180001/cc/output/renderer_pixeltest.cc File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2122573003/diff/180001/cc/output/renderer_pixeltest.cc#newcode1188 cc/output/renderer_pixeltest.cc:1188: // crbug.com/622133 On 2016/10/07 12:35:01, dshwang wrote: > unfortunately, ...
4 years, 2 months ago (2016-10-07 13:00:32 UTC) #56
dshwang
https://codereview.chromium.org/2122573003/diff/180001/cc/output/renderer_pixeltest.cc File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/2122573003/diff/180001/cc/output/renderer_pixeltest.cc#newcode1188 cc/output/renderer_pixeltest.cc:1188: // crbug.com/622133 On 2016/10/07 13:00:32, dshwang wrote: > On ...
4 years, 2 months ago (2016-10-07 17:55:17 UTC) #59
hubbe
On 2016/10/07 17:55:17, dshwang wrote: > https://codereview.chromium.org/2122573003/diff/180001/cc/output/renderer_pixeltest.cc > File cc/output/renderer_pixeltest.cc (right): > > https://codereview.chromium.org/2122573003/diff/180001/cc/output/renderer_pixeltest.cc#newcode1188 > ...
4 years, 2 months ago (2016-10-07 18:13:03 UTC) #60
hubbe
On 2016/10/07 18:13:03, hubbe wrote: > On 2016/10/07 17:55:17, dshwang wrote: > > > https://codereview.chromium.org/2122573003/diff/180001/cc/output/renderer_pixeltest.cc ...
4 years, 2 months ago (2016-10-12 21:59:16 UTC) #61
dshwang
On 2016/10/12 21:59:16, hubbe wrote: > I'd like to run this CL through our power ...
4 years, 2 months ago (2016-10-13 12:16:04 UTC) #64
ccameron
This approach does not lgtm. We should not simulate 1=channel images using 2-channel images. It ...
4 years, 2 months ago (2016-10-17 22:28:31 UTC) #67
dshwang
On 2016/10/17 22:28:31, ccameron wrote: > This approach does not lgtm. We should not simulate ...
4 years, 2 months ago (2016-10-18 12:03:16 UTC) #69
aleksandar.stojiljkovic
On 2016/10/17 22:28:31, ccameron wrote: > This approach does not lgtm. We should not simulate ...
4 years, 2 months ago (2016-10-18 19:24:57 UTC) #70
hubbe
On 2016/10/18 19:24:57, aleksandar.stojiljkovic wrote: > On 2016/10/17 22:28:31, ccameron wrote: > > This approach ...
4 years, 2 months ago (2016-10-18 23:47:06 UTC) #71
Ken Russell (switch to Gerrit)
On 2016/10/17 22:28:31, ccameron wrote: > This approach does not lgtm. We should not simulate ...
4 years, 2 months ago (2016-10-20 00:39:44 UTC) #72
marcheu
On 2016/10/18 12:03:16, dshwang wrote: > On 2016/10/17 22:28:31, ccameron wrote: > > This approach ...
3 years, 8 months ago (2017-04-01 00:37:47 UTC) #74
aleksandar.stojiljkovic
3 years, 8 months ago (2017-04-02 08:56:39 UTC) #75
On 2017/04/01 00:37:47, marcheu wrote:
> On 2016/10/18 12:03:16, dshwang wrote:
> > On 2016/10/17 22:28:31, ccameron wrote:
> > > This approach does not lgtm. We should not simulate 1=channel images using
> > > 2-channel images.
> > > 
> > > It is possible to create R16 unorm GMBs. We should use these. I verified
> this
> > on
> > > Mac with IOSurfaces, and I can send you a test application which
> demonstrates
> > > this.
> > 
> > IOSurfaces has R16 format but linux kernel (i.e. chromeos) doesn't support
R16
> > format for native DMA-BUF
> >
>
https://github.com/torvalds/linux/blob/master/include/uapi/drm/drm_fourcc.h#L45
> 
> DMA-BUF doesn't care about the format. It's a way to share buffers (a list of
> memory pages) and as such the format that is being transported is completely
> irrelevant. So you argument doesn't make sense.
> 
> > 
> > I'm not sure that linux community will agree on R16 introduction. If so, I'm
> not
> > sure how long will it take.

As DS mentioned, it is format agnostic.
Good to revisit this.

Meanwhile, DRM_FORMAT_R16 got added - interestingly, to the same line 45 [1].

Another patch [2], is introducing the usage of GL_R16_EXT (available through
core desktop and EXT_texture_norm16 extension) so we could rebase this patch to
use GL_R16_EXT.

[1] DRM_FORMAT_R16
https://github.com/torvalds/linux/commit/fd056f05b9fcba35b77ec5b20e553cc48884...

[2]
https://crrev.com/2767063002/

> > 
> > Note: Mesa allocates format agnostic DMA-BUF for texture, which means R16
> > dma-buf is not very needed in current linux graphics stack.
> >
>
https://cs.chromium.org/chromium/src/third_party/mesa/src/src/mesa/drivers/dr...
> > 
> > > Alternatively, unless we prove that the short<->half-float conversion is
the
> > > critical bottleneck for a critical use-case, then we should consider using
> > R16F
> > > (half-float single channel).
> > 
> > R16F requires half-float conversion data also. TexImage2D(R16F) doesn't
> convert
> > uint16 to half-float automatically, while TexImage2D(R8) converts uint8 to
> > half-float automatically.

Powered by Google App Engine
This is Rietveld 408576698