Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12157002:
Adding YUVA support for enabling Alpha Playback (Closed)
|
Created:
7 years, 10 months ago by vignesh Modified:
7 years, 6 months ago Reviewers:
jzern, fgalligan1, vigneshv, jamesr, piman, jschuh, Tom Finegan, enne (OOO), danakj, brettw CC:
chromium-reviews, joi+watch-content_chromium.org, feature-media-reviews_chromium.org, cc-bugs_chromium.org, jam, darin-cc_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdding YUVA support for enabling Alpha Playback
BUG=147355
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204508
Patch Set 1 #Patch Set 2 : Moving VP8 Alpha Playback behind its own flag #
Total comments: 91
Patch Set 3 : remove files unrelated to cc/ #
Total comments: 15
Patch Set 4 : Removing YUVAVideoDrawQuad class #Patch Set 5 : minor fixes and changes. #Patch Set 6 : rebase #
Total comments: 6
Patch Set 7 : Merging DrawYUVAVideoQuad with DrawYUVVideoQuad #
Total comments: 29
Patch Set 8 : addressing comments on patchset 7 #
Total comments: 4
Patch Set 9 : addressing comments #
Total comments: 4
Patch Set 10 : adding unittests #Patch Set 11 : addressing comments #
Total comments: 6
Patch Set 12 : rebase and pixel tests for YUV and YUVA #
Total comments: 14
Patch Set 13 : rebase on test file landing and nit fixes #Patch Set 14 : rebase and calming down the try bots #
Total comments: 1
Messages
Total messages: 62 (0 generated)
Frank is doing a full review, but I'll let my two nits and one general comment stand. General comment: The code in src/cc/ seems to be written to a coding standard different than everywhere else, and appears to be self contained from a quick look. I think that should be the first piece of this that gets broken off and handled in a separate CL. Nits follow... nothing serious. https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode984 cc/gl_renderer.cc:984: //GLC(context(), context()->uniform1f(program->vertexShader().uvWidthScaleFactorLocation(), uvWidthScaleFactor)); Remove commented out code? https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode999 cc/gl_renderer.cc:999: Delete extra line.
You'll need to do content/common/cc_messages.h at the same time as cc/ https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.h File cc/yuva_video_draw_quad.h (right): https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.h#... cc/yuva_video_draw_quad.h:46: VideoLayerImpl::FramePlane a_plane; Can you just add this plane to the YUVDrawQuad class and a flag to say if it's being used?
https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.h File cc/yuva_video_draw_quad.h (right): https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.h#... cc/yuva_video_draw_quad.h:46: VideoLayerImpl::FramePlane a_plane; On 2013/02/12 00:53:26, danakj wrote: > Can you just add this plane to the YUVDrawQuad class and a flag to say if it's > being used? Can i extend this class from YUVVideoDrawQuad instead of adding a flag to track its usage. I feel that's a little cleaner than adding a flag and adding if checks in almost every function of the class.
https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.h File cc/yuva_video_draw_quad.h (right): https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.h#... cc/yuva_video_draw_quad.h:46: VideoLayerImpl::FramePlane a_plane; On 2013/02/12 01:13:50, vignesh wrote: > On 2013/02/12 00:53:26, danakj wrote: > > Can you just add this plane to the YUVDrawQuad class and a flag to say if it's > > being used? > > Can i extend this class from YUVVideoDrawQuad instead of adding a flag to track > its usage. I feel that's a little cleaner than adding a flag and adding if > checks in almost every function of the class. Why do you need so many ifs? The FramePlane is a pretty simple struct, it could just contain 0's if it's unusued. Maybe you don't even need a bool (thought it might be nice for the GLRenderer code though).
https://codereview.chromium.org/12157002/diff/3001/cc/draw_quad.h File cc/draw_quad.h (right): https://codereview.chromium.org/12157002/diff/3001/cc/draw_quad.h#newcode1 cc/draw_quad.h:1: // Copyright 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode67 cc/gl_renderer.cc:67: float yuv2RGB[9] = { Move these values into the anonymous namespace. https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode980 cc/gl_renderer.cc:980: float yWidthScaleFactor = static_cast<float>(yPlane.size.width()) / yPlane.size.width(); This scale factor is always 1. Also looks like it is not used. Remove. https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode982 cc/gl_renderer.cc:982: float uvWidthScaleFactor = static_cast<float>(uPlane.size.width()) / uPlane.size.width(); Ditto https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode983 cc/gl_renderer.cc:983: //GLC(context(), context()->uniform1f(program->vertexShader().yWidthScaleFactorLocation(), yWidthScaleFactor)); Remove unused code. https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode984 cc/gl_renderer.cc:984: //GLC(context(), context()->uniform1f(program->vertexShader().uvWidthScaleFactorLocation(), uvWidthScaleFactor)); Ditto https://codereview.chromium.org/12157002/diff/3001/cc/shader.h File cc/shader.h (right): https://codereview.chromium.org/12157002/diff/3001/cc/shader.h#newcode373 cc/shader.h:373: class FragmentShaderYUVAVideo { You could make this inherit from FragmentShaderYUVVideo. https://codereview.chromium.org/12157002/diff/3001/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://codereview.chromium.org/12157002/diff/3001/cc/video_layer_impl.cc#new... cc/video_layer_impl.cc:227: const FramePlane& yPlane = m_framePlanes[media::VideoFrame::kYPlane]; You can move yPlane, uPlave, vPlane, and texScale up one scope. https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.cc File cc/yuva_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.cc... cc/yuva_video_draw_quad.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.cc... cc/yuva_video_draw_quad.cc:27: bool needs_blending = true; Just call SetAll(shared_quad_state, rect, opaque_rect, visible_rect, needs_blending, tex_sacle, y_plane, u_plane, v_plane, a_plane); And delete the rest of the code in the function. https://codereview.chromium.org/12157002/diff/3001/media/base/decoder_buffer.cc File media/base/decoder_buffer.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/base/decoder_buffer.... media/base/decoder_buffer.cc:13: : size_(size) { Initialize side_data_size_ to 0. https://codereview.chromium.org/12157002/diff/3001/media/base/decoder_buffer.... media/base/decoder_buffer.cc:18: : size_(size) { Initialize side_data_size_ to 0. https://codereview.chromium.org/12157002/diff/3001/media/base/decoder_buffer.... media/base/decoder_buffer.cc:51: base::AlignedAlloc(side_data_size_ + kPaddingSize, kAlignmentSize))); nit: 4 space indent https://codereview.chromium.org/12157002/diff/3001/media/base/simd/convert_yu... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:101: for (int x = 0; x < width; x = 2) { I think it should be x += 2. https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc#... media/base/video_frame.cc:168: || format_ == VideoFrame::YV12A); Logical operators are typically at the end of the previous line. Style in this file is at EOL. https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc#... media/base/video_frame.cc:190: || format_ == VideoFrame::YV12A) ? Logical operators are typically at the end of the previous line. Style in this file is at EOL. https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc#... media/base/video_frame.cc:191: y_height / 2 : y_height; indent 4 https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc#... media/base/video_frame.cc:212: if(format_ == YV12A) { nit: space between if and ( https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc#... media/base/video_frame.cc:250: plane == kAPlane; nit: 4 space indent https://codereview.chromium.org/12157002/diff/3001/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/ffmpeg/ffmpeg_common... media/ffmpeg/ffmpeg_common.cc:390: AVDictionaryEntry *webm_alpha = nit: Asterisk adjacent to the type per style in this file. https://codereview.chromium.org/12157002/diff/3001/media/ffmpeg/ffmpeg_common... media/ffmpeg/ffmpeg_common.cc:391: av_dict_get(stream->metadata, "alpha_mode", NULL, 0); 4 space indent http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla... https://codereview.chromium.org/12157002/diff/3001/media/filters/ffmpeg_demux... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:78: uint8_t *side_data; nit: asterisk adjacent to type. https://codereview.chromium.org/12157002/diff/3001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:80: //int i; remove https://codereview.chromium.org/12157002/diff/3001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:81: AVPacket *pkt; nit: asterisk adjacent to type. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:30: media::VideoFrame::Format format) { nit: 4 space indent. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:83: video_frame->format() == media::VideoFrame::YV12A Line up. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:84: ? media::YV12 : media::YV16; ? should be on the previous line. Then the rest of the ternary operator should be indented 4 spaces. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:87: video_frame->format() == media::VideoFrame::YV12A) { Line up. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:188: << video_frame->format(); nit: 4 space indent. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:191: video_frame->stride(media::VideoFrame::kVPlane)); Line up. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:216: video_frame->format() == media::VideoFrame::YV12A) { Line up. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:299: !IsEitherYV12OrYV12AOrYV16OrNative(video_frame->format())) { Line up. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:108: config.codec() == kCodecVP9) { Line up. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:112: config.codec() == kCodecVP8 && format_ == VideoFrame::YV12A) { Line up. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:122: nit: Remove line https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:129: vpx_codec_vp9_dx() : nit: 4 space indent. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:341: void *user_priv_alpha = reinterpret_cast<void*>(×tamp_alpha); nit: asterisk with type. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:342: uint64_t side_data_id = *((uint64_t*)buffer->GetSideData()); Use c++ cast. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:343: if(side_data_id == 1) { Space between if and ( https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:364: reinterpret_cast<void*>(×tamp_alpha)) { Line up. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:425: * vpx_image->d_h; I would put vpx_image->stride on the next line indent 4. Or you could put * on previous line and line up vpx_image->d_h https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:426: unsigned char *alpha_plane = (unsigned char*)malloc(alpha_plane_size); nit: asterisk with type. Use C++ cast.
https://codereview.chromium.org/12157002/diff/3001/cc/draw_quad.h File cc/draw_quad.h (right): https://codereview.chromium.org/12157002/diff/3001/cc/draw_quad.h#newcode1 cc/draw_quad.h:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2013/02/12 01:20:58, fgalligan1 wrote: > 2013 NACK! We don't do this. https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8p4JKV76kig/dis...
https://codereview.chromium.org/12157002/diff/3001/cc/draw_quad.h File cc/draw_quad.h (right): https://codereview.chromium.org/12157002/diff/3001/cc/draw_quad.h#newcode1 cc/draw_quad.h:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2013/02/12 01:23:15, jamesr wrote: > On 2013/02/12 01:20:58, fgalligan1 wrote: > > 2013 > > NACK! We don't do this. > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/8p4JKV76kig/dis... OK Don't change it. https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.cc File cc/yuva_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.cc... cc/yuva_video_draw_quad.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. On 2013/02/12 01:20:58, fgalligan1 wrote: > 2013 nm. Don't change it.
structure of these new classes/structs I'll leave to people more familiar with this code. it might be worth mentioning that on the image side we deal with premultiplied alpha... https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc#newcode65 cc/gl_renderer.cc:65: // These values are magic numbers that are used in the transformation from YUV to RGB color values. these are actually ITU-R BT.601 https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc#newcode67 cc/gl_renderer.cc:67: float yuv2RGB[9] = { these should be moved into the anonymous namespace, they're not used elsewhere right? https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc#newcode943 cc/gl_renderer.cc:943: drop this line https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc#newcode983 cc/gl_renderer.cc:983: //GLC(context(), context()->uniform1f(program->vertexShader().yWidthScaleFactorLocation(), yWidthScaleFactor)); dead code https://codereview.chromium.org/12157002/diff/6002/cc/shader.cc File cc/shader.cc (right): https://codereview.chromium.org/12157002/diff/6002/cc/shader.cc#newcode953 cc/shader.cc:953: vec3 yuv = vec3(y_raw, u_unsigned, v_unsigned) yuv_adj; '+' yuv_adj ? https://codereview.chromium.org/12157002/diff/6002/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://codereview.chromium.org/12157002/diff/6002/cc/video_layer_impl.cc#new... cc/video_layer_impl.cc:225: if(m_frame->HasAlpha()) { add a space after the if https://codereview.chromium.org/12157002/diff/6002/cc/yuva_video_draw_quad.cc File cc/yuva_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/6002/cc/yuva_video_draw_quad.cc... cc/yuva_video_draw_quad.cc:26: gfx::Rect visible_rect = rect; 4 space indent is the local norm https://codereview.chromium.org/12157002/diff/6002/cc/yuva_video_draw_quad.cc... cc/yuva_video_draw_quad.cc:57: const DrawQuad* quad) { could be 1 line
The structure seems OK, but you have a lot of repetitive code that does nearly the same thing 4 times. Can you please tidy them up? https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc#newcode975 cc/gl_renderer.cc:975: GLC(context(), context()->bindTexture(GL_TEXTURE_2D, aPlaneLock.textureId())); it looks like you could use some for loops here. https://codereview.chromium.org/12157002/diff/6002/cc/yuva_video_draw_quad.cc File cc/yuva_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/6002/cc/yuva_video_draw_quad.cc... cc/yuva_video_draw_quad.cc:1: // Copyright 2012 The Chromium Authors. All rights reserved. 2013 https://codereview.chromium.org/12157002/diff/6002/cc/yuva_video_draw_quad.h File cc/yuva_video_draw_quad.h (right): https://codereview.chromium.org/12157002/diff/6002/cc/yuva_video_draw_quad.h#... cc/yuva_video_draw_quad.h:1: // Copyright 2012 The Chromium Authors. All rights reserved. 2013
On 2013/02/14 01:55:19, jamesr wrote: > The structure seems OK, but you have a lot of repetitive code that does nearly > the same thing 4 times. Can you please tidy them up? Also, did you consider using one quad type for YUV and YUVA video data with an empty A channel where there isn't one? Having such near-duplicate types is going to be a headache.
Marking Frank's comments on non cc/ changes as done. I will address them in this CL: https://codereview.chromium.org/12263013/ . For now just marking them as done, will upload the actual patch set that fixes these shortly. https://codereview.chromium.org/12157002/diff/3001/media/base/decoder_buffer.cc File media/base/decoder_buffer.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/base/decoder_buffer.... media/base/decoder_buffer.cc:13: : size_(size) { On 2013/02/12 01:20:58, fgalligan1 wrote: > Initialize side_data_size_ to 0. Done. https://codereview.chromium.org/12157002/diff/3001/media/base/decoder_buffer.... media/base/decoder_buffer.cc:18: : size_(size) { On 2013/02/12 01:20:58, fgalligan1 wrote: > Initialize side_data_size_ to 0. Done. https://codereview.chromium.org/12157002/diff/3001/media/base/decoder_buffer.... media/base/decoder_buffer.cc:51: base::AlignedAlloc(side_data_size_ + kPaddingSize, kAlignmentSize))); On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: 4 space indent Done. https://codereview.chromium.org/12157002/diff/3001/media/base/simd/convert_yu... File media/base/simd/convert_yuv_to_rgb_c.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/base/simd/convert_yu... media/base/simd/convert_yuv_to_rgb_c.cc:101: for (int x = 0; x < width; x = 2) { On 2013/02/12 01:20:58, fgalligan1 wrote: > I think it should be x += 2. Done. https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc File media/base/video_frame.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc#... media/base/video_frame.cc:168: || format_ == VideoFrame::YV12A); On 2013/02/12 01:20:58, fgalligan1 wrote: > Logical operators are typically at the end of the previous line. Style in this > file is at EOL. Done. https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc#... media/base/video_frame.cc:190: || format_ == VideoFrame::YV12A) ? On 2013/02/12 01:20:58, fgalligan1 wrote: > Logical operators are typically at the end of the previous line. Style in this > file is at EOL. Done. https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc#... media/base/video_frame.cc:191: y_height / 2 : y_height; On 2013/02/12 01:20:58, fgalligan1 wrote: > indent 4 Done. https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc#... media/base/video_frame.cc:212: if(format_ == YV12A) { On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: space between if and ( Done. https://codereview.chromium.org/12157002/diff/3001/media/base/video_frame.cc#... media/base/video_frame.cc:250: plane == kAPlane; On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: 4 space indent Done. https://codereview.chromium.org/12157002/diff/3001/media/ffmpeg/ffmpeg_common.cc File media/ffmpeg/ffmpeg_common.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/ffmpeg/ffmpeg_common... media/ffmpeg/ffmpeg_common.cc:390: AVDictionaryEntry *webm_alpha = On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: Asterisk adjacent to the type per style in this file. Done. https://codereview.chromium.org/12157002/diff/3001/media/ffmpeg/ffmpeg_common... media/ffmpeg/ffmpeg_common.cc:391: av_dict_get(stream->metadata, "alpha_mode", NULL, 0); On 2013/02/12 01:20:58, fgalligan1 wrote: > 4 space indent > http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla... Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/ffmpeg_demux... File media/filters/ffmpeg_demuxer.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:78: uint8_t *side_data; On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: asterisk adjacent to type. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:80: //int i; On 2013/02/12 01:20:58, fgalligan1 wrote: > remove Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/ffmpeg_demux... media/filters/ffmpeg_demuxer.cc:81: AVPacket *pkt; On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: asterisk adjacent to type. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... File media/filters/skcanvas_video_renderer.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:30: media::VideoFrame::Format format) { On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: 4 space indent. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:83: video_frame->format() == media::VideoFrame::YV12A On 2013/02/12 01:20:58, fgalligan1 wrote: > Line up. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:84: ? media::YV12 : media::YV16; On 2013/02/12 01:20:58, fgalligan1 wrote: > ? should be on the previous line. Then the rest of the ternary operator should > be indented 4 spaces. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:87: video_frame->format() == media::VideoFrame::YV12A) { On 2013/02/12 01:20:58, fgalligan1 wrote: > Line up. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:188: << video_frame->format(); On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: 4 space indent. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:191: video_frame->stride(media::VideoFrame::kVPlane)); On 2013/02/12 01:20:58, fgalligan1 wrote: > Line up. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:216: video_frame->format() == media::VideoFrame::YV12A) { On 2013/02/12 01:20:58, fgalligan1 wrote: > Line up. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/skcanvas_vid... media/filters/skcanvas_video_renderer.cc:299: !IsEitherYV12OrYV12AOrYV16OrNative(video_frame->format())) { On 2013/02/12 01:20:58, fgalligan1 wrote: > Line up. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... File media/filters/vpx_video_decoder.cc (right): https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:108: config.codec() == kCodecVP9) { On 2013/02/12 01:20:58, fgalligan1 wrote: > Line up. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:112: config.codec() == kCodecVP8 && format_ == VideoFrame::YV12A) { On 2013/02/12 01:20:58, fgalligan1 wrote: > Line up. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:122: On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: Remove line Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:129: vpx_codec_vp9_dx() : On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: 4 space indent. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:341: void *user_priv_alpha = reinterpret_cast<void*>(×tamp_alpha); On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: asterisk with type. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:342: uint64_t side_data_id = *((uint64_t*)buffer->GetSideData()); On 2013/02/12 01:20:58, fgalligan1 wrote: > Use c++ cast. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:343: if(side_data_id == 1) { On 2013/02/12 01:20:58, fgalligan1 wrote: > Space between if and ( Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:364: reinterpret_cast<void*>(×tamp_alpha)) { On 2013/02/12 01:20:58, fgalligan1 wrote: > Line up. Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:425: * vpx_image->d_h; On 2013/02/12 01:20:58, fgalligan1 wrote: > I would put vpx_image->stride on the next line indent 4. Or you could put * on > previous line and line up vpx_image->d_h Done. https://codereview.chromium.org/12157002/diff/3001/media/filters/vpx_video_de... media/filters/vpx_video_decoder.cc:426: unsigned char *alpha_plane = (unsigned char*)malloc(alpha_plane_size); On 2013/02/12 01:20:58, fgalligan1 wrote: > nit: asterisk with type. Use C++ cast. Done.
> Also, did you consider using one quad type for YUV and YUVA video data with an > empty A channel where there isn't one? Having such near-duplicate types is > going to be a headache. Yes, as danakj@ suggested on a comment in the earlier patchset, i will be adding the a_plane field to YUVVideoDrawQuad class. Will address your other comments in the meantime as well.
Patch Set 4 Have addressed most of the comments on the first three patchsets (whatever has been addressed has been marked done individually). Removed the YUVAVideoDrawQuad class to merge it with the YUVVideoDrawQuad as suggested by danakj@. https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode67 cc/gl_renderer.cc:67: float yuv2RGB[9] = { On 2013/02/12 01:20:58, fgalligan1 wrote: > Move these values into the anonymous namespace. Done. https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode980 cc/gl_renderer.cc:980: float yWidthScaleFactor = static_cast<float>(yPlane.size.width()) / yPlane.size.width(); On 2013/02/12 01:20:58, fgalligan1 wrote: > This scale factor is always 1. Also looks like it is not used. Remove. Done. https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode982 cc/gl_renderer.cc:982: float uvWidthScaleFactor = static_cast<float>(uPlane.size.width()) / uPlane.size.width(); On 2013/02/12 01:20:58, fgalligan1 wrote: > Ditto Done. https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode983 cc/gl_renderer.cc:983: //GLC(context(), context()->uniform1f(program->vertexShader().yWidthScaleFactorLocation(), yWidthScaleFactor)); On 2013/02/12 01:20:58, fgalligan1 wrote: > Remove unused code. Done. https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode984 cc/gl_renderer.cc:984: //GLC(context(), context()->uniform1f(program->vertexShader().uvWidthScaleFactorLocation(), uvWidthScaleFactor)); On 2013/02/12 01:20:58, fgalligan1 wrote: > Ditto Done. https://codereview.chromium.org/12157002/diff/3001/cc/gl_renderer.cc#newcode999 cc/gl_renderer.cc:999: On 2013/02/12 00:48:34, Tom Finegan wrote: > Delete extra line. Done. https://codereview.chromium.org/12157002/diff/3001/cc/video_layer_impl.cc File cc/video_layer_impl.cc (right): https://codereview.chromium.org/12157002/diff/3001/cc/video_layer_impl.cc#new... cc/video_layer_impl.cc:227: const FramePlane& yPlane = m_framePlanes[media::VideoFrame::kYPlane]; On 2013/02/12 01:20:58, fgalligan1 wrote: > You can move yPlane, uPlave, vPlane, and texScale up one scope. Done. https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.cc File cc/yuva_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.cc... cc/yuva_video_draw_quad.cc:27: bool needs_blending = true; On 2013/02/12 01:20:58, fgalligan1 wrote: > Just call SetAll(shared_quad_state, rect, opaque_rect, visible_rect, > needs_blending, tex_sacle, y_plane, u_plane, v_plane, a_plane); > > And delete the rest of the code in the function. Done. https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.h File cc/yuva_video_draw_quad.h (right): https://codereview.chromium.org/12157002/diff/3001/cc/yuva_video_draw_quad.h#... cc/yuva_video_draw_quad.h:46: VideoLayerImpl::FramePlane a_plane; On 2013/02/12 01:18:09, danakj wrote: > On 2013/02/12 01:13:50, vignesh wrote: > > On 2013/02/12 00:53:26, danakj wrote: > > > Can you just add this plane to the YUVDrawQuad class and a flag to say if > it's > > > being used? > > > > Can i extend this class from YUVVideoDrawQuad instead of adding a flag to > track > > its usage. I feel that's a little cleaner than adding a flag and adding if > > checks in almost every function of the class. > > Why do you need so many ifs? The FramePlane is a pretty simple struct, it could > just contain 0's if it's unusued. Maybe you don't even need a bool (thought it > might be nice for the GLRenderer code though). Done. https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc File cc/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc#newcode67 cc/gl_renderer.cc:67: float yuv2RGB[9] = { On 2013/02/13 19:56:14, jzern wrote: > these should be moved into the anonymous namespace, they're not used elsewhere > right? Done. https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc#newcode943 cc/gl_renderer.cc:943: On 2013/02/13 19:56:14, jzern wrote: > drop this line Done. https://codereview.chromium.org/12157002/diff/6002/cc/gl_renderer.cc#newcode983 cc/gl_renderer.cc:983: //GLC(context(), context()->uniform1f(program->vertexShader().yWidthScaleFactorLocation(), yWidthScaleFactor)); On 2013/02/13 19:56:14, jzern wrote: > dead code Done. https://codereview.chromium.org/12157002/diff/6002/cc/shader.cc File cc/shader.cc (right): https://codereview.chromium.org/12157002/diff/6002/cc/shader.cc#newcode953 cc/shader.cc:953: vec3 yuv = vec3(y_raw, u_unsigned, v_unsigned) yuv_adj; On 2013/02/13 19:56:14, jzern wrote: > '+' yuv_adj ? Done.
Doing a rebase. Have addressed all the previous comments. CL ready for a thorough review.
https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1266: void GLRenderer::DrawYUVAVideoQuad(const DrawingFrame* frame, Can you merge this function with DrawYUVVideoQuad? Would you need a separate enum value for it then? How about just leaving the resource id == 0 in the alpha channel if there's no "A" present in the YUV.
https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1266: void GLRenderer::DrawYUVAVideoQuad(const DrawingFrame* frame, On 2013/03/28 20:59:24, danakj wrote: > Can you merge this function with DrawYUVVideoQuad? Would you need a separate > enum value for it then? How about just leaving the resource id == 0 in the alpha > channel if there's no "A" present in the YUV. Which enum value are you referring to ? Also, what happens to VideoYUVAProgram typedef if i need to merge this function into DrawYUVVideoQuad ?
https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1266: void GLRenderer::DrawYUVAVideoQuad(const DrawingFrame* frame, On 2013/04/02 20:56:15, vignesh wrote: > On 2013/03/28 20:59:24, danakj wrote: > > Can you merge this function with DrawYUVVideoQuad? Would you need a separate > > enum value for it then? How about just leaving the resource id == 0 in the > alpha > > channel if there's no "A" present in the YUV. > > Which enum value are you referring to ? Also, what happens to VideoYUVAProgram > typedef if i need to merge this function into DrawYUVVideoQuad ? Sorry, I am referring to the DrawQuad::Material enum. You've done a great job eliminating the extra quad type, but I wonder if we can go further and eliminate the extra enum value as well. It is fine to have a single "DrawFooQuad" method use different programs depending on the content of the quad. See the DrawRenderPassQuad() for an example, it chooses between something like 16 programs depending on the content of the quad. You could choose to use a program with alpha depending if there was an alpha resource present in the YUV quad.
https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.h File cc/output/gl_renderer.h (right): https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.h#n... cc/output/gl_renderer.h:284: const VideoYUVAProgram* GetVideoYUVAProgram(); By the way, when adding a new program, please add it to the shader unit tests in gl_renderer_unittest.cc as well.
https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1266: void GLRenderer::DrawYUVAVideoQuad(const DrawingFrame* frame, I have eliminated the enum type and included both YUV and YUVA in a single DrawYUVVideoQuad function depending on presence of Alpha channel. Please see if this implementation is okay. Thanks :) On 2013/04/03 00:41:12, danakj wrote: > On 2013/04/02 20:56:15, vignesh wrote: > > On 2013/03/28 20:59:24, danakj wrote: > > > Can you merge this function with DrawYUVVideoQuad? Would you need a separate > > > enum value for it then? How about just leaving the resource id == 0 in the > > alpha > > > channel if there's no "A" present in the YUV. > > > > Which enum value are you referring to ? Also, what happens to VideoYUVAProgram > > typedef if i need to merge this function into DrawYUVVideoQuad ? > > Sorry, I am referring to the DrawQuad::Material enum. > > You've done a great job eliminating the extra quad type, but I wonder if we can > go further and eliminate the extra enum value as well. > > It is fine to have a single "DrawFooQuad" method use different programs > depending on the content of the quad. See the DrawRenderPassQuad() for an > example, it chooses between something like 16 programs depending on the content > of the quad. You could choose to use a program with alpha depending if there was > an alpha resource present in the YUV quad. https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.h File cc/output/gl_renderer.h (right): https://codereview.chromium.org/12157002/diff/33001/cc/output/gl_renderer.h#n... cc/output/gl_renderer.h:284: const VideoYUVAProgram* GetVideoYUVAProgram(); On 2013/04/03 00:42:04, danakj wrote: > By the way, when adding a new program, please add it to the shader unit tests in > gl_renderer_unittest.cc as well. Done.
Looks good overall, thanks for the changes. Someone with some more shader knowledge should take a look at the shader bits. enne? https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:82: // These values are magic numbers that are used in the transformation from YUV to RGB color values. nit: 80 columns upload should complain about this if you're synced to ToT. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1232: if (a_plane.resource_id) { how about "bool use_alpha_plane = a_plane.resource_id != 0;" up near the top of this method, then use that everywhere instead of always comparing to the resource_id. I think that would help reading this method. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1233: ResourceProvider::ScopedSamplerGL a_plane_lock( this seems like it works at the moment, but its name implies that it is valid while it is in scope, which doesn't last the whole function. You could solve this by storing it in a scoped_ptr and allocating it on the heap with "new" instead. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1243: int yuv_matrix_location; nothing seems to set this. can you initialize all of these variables to -1 here. then, set the ones you intend to use to other values below. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1255: a_texture_location = program->fragment_shader().yuv_matrix_location(); why is this yuv_matrix_location and not just -1? https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1267: a_texture_location = program->fragment_shader().yuv_matrix_location(); why is this yuv_matrix_location and not a_texture_location? https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.h File cc/output/gl_renderer.h (right): https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.h#n... cc/output/gl_renderer.h:147: void DrawYUVAVideoQuad(const DrawingFrame* frame, remove this https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.h File cc/output/shader.h (right): https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.h#newcod... cc/output/shader.h:417: public: nit: 1 space before public/private: does the presubmit not mention this on upload? https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.h#newcod... cc/output/shader.h:421: void Init(WebKit::WebGraphicsContext3D*, nit: give all arguments variable names https://codereview.chromium.org/12157002/diff/41001/cc/quads/yuv_video_draw_q... File cc/quads/yuv_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/41001/cc/quads/yuv_video_draw_q... cc/quads/yuv_video_draw_quad.cc:86: if(a_plane.resource_id) { nit: no {} for single-line body https://codereview.chromium.org/12157002/diff/41001/cc/quads/yuv_video_draw_q... File cc/quads/yuv_video_draw_quad.h (right): https://codereview.chromium.org/12157002/diff/41001/cc/quads/yuv_video_draw_q... cc/quads/yuv_video_draw_quad.h:30: void SetNew(const SharedQuadState* shared_quad_state, There should be only one SetNew() or SetAll() here. Since planes are optional, how about changing them to be const* instead of const&, then we can pass in a NULL a_plane if there's none present?
Please update the CL subject and description: It's confusing having two CLs to review that both have the subject, "Adding VP8 Alpha playback to Chrome." This CL is really adding YUVA support, right?
Addressing comments on the previous patchset. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:82: // These values are magic numbers that are used in the transformation from YUV to RGB color values. On 2013/04/04 15:01:55, danakj wrote: > nit: 80 columns > > upload should complain about this if you're synced to ToT. Done. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1232: if (a_plane.resource_id) { On 2013/04/04 15:01:55, danakj wrote: > how about "bool use_alpha_plane = a_plane.resource_id != 0;" up near the top of > this method, then use that everywhere instead of always comparing to the > resource_id. I think that would help reading this method. Done. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1233: ResourceProvider::ScopedSamplerGL a_plane_lock( On 2013/04/04 15:01:55, danakj wrote: > this seems like it works at the moment, but its name implies that it is valid > while it is in scope, which doesn't last the whole function. > > You could solve this by storing it in a scoped_ptr and allocating it on the heap > with "new" instead. Done. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1243: int yuv_matrix_location; On 2013/04/04 15:01:55, danakj wrote: > nothing seems to set this. > > can you initialize all of these variables to -1 here. then, set the ones you > intend to use to other values below. Done. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1255: a_texture_location = program->fragment_shader().yuv_matrix_location(); On 2013/04/04 15:01:55, danakj wrote: > why is this yuv_matrix_location and not just -1? Done. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1267: a_texture_location = program->fragment_shader().yuv_matrix_location(); On 2013/04/04 15:01:55, danakj wrote: > why is this yuv_matrix_location and not a_texture_location? Done. https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.h File cc/output/gl_renderer.h (right): https://codereview.chromium.org/12157002/diff/41001/cc/output/gl_renderer.h#n... cc/output/gl_renderer.h:147: void DrawYUVAVideoQuad(const DrawingFrame* frame, On 2013/04/04 15:01:55, danakj wrote: > remove this Done. https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.h File cc/output/shader.h (right): https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.h#newcod... cc/output/shader.h:417: public: On 2013/04/04 15:01:55, danakj wrote: > nit: 1 space before public/private: > > does the presubmit not mention this on upload? not sure why, presubmit did not mention this. Done. https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.h#newcod... cc/output/shader.h:421: void Init(WebKit::WebGraphicsContext3D*, On 2013/04/04 15:01:55, danakj wrote: > nit: give all arguments variable names Done. https://codereview.chromium.org/12157002/diff/41001/cc/quads/yuv_video_draw_q... File cc/quads/yuv_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/41001/cc/quads/yuv_video_draw_q... cc/quads/yuv_video_draw_quad.cc:86: if(a_plane.resource_id) { On 2013/04/04 15:01:55, danakj wrote: > nit: no {} for single-line body Done. https://codereview.chromium.org/12157002/diff/41001/cc/quads/yuv_video_draw_q... File cc/quads/yuv_video_draw_quad.h (right): https://codereview.chromium.org/12157002/diff/41001/cc/quads/yuv_video_draw_q... cc/quads/yuv_video_draw_quad.h:30: void SetNew(const SharedQuadState* shared_quad_state, On 2013/04/04 15:01:55, danakj wrote: > There should be only one SetNew() or SetAll() here. > > Since planes are optional, how about changing them to be const* instead of > const&, then we can pass in a NULL a_plane if there's none present? Done.
https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.cc#newco... cc/output/shader.cc:917: : y_texture_location_(-1) style nit: commas don't go at the beginning of lines. https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.cc#newco... cc/output/shader.cc:931: { style nit: { goes on the previous line. How did this pass presubmit? https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.cc#newco... cc/output/shader.cc:932: static const char* shader_uniforms[] = { style nit: don't use a 4 space indent https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.cc#newco... cc/output/shader.cc:972: varying vec2 uv_texCoord; What does this second varying parameter get hooked up to? Doesn't the vertex shader only emit one varying parameter?
https://codereview.chromium.org/12157002/diff/53001/cc/quads/yuv_video_draw_q... File cc/quads/yuv_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/53001/cc/quads/yuv_video_draw_q... cc/quads/yuv_video_draw_quad.cc:27: bool needs_blending = true; This is true only if there's an a_plane present, right? https://codereview.chromium.org/12157002/diff/53001/cc/quads/yuv_video_draw_q... cc/quads/yuv_video_draw_quad.cc:57: if(a_plane.resource_id) nit: space before (
https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.cc#newco... cc/output/shader.cc:917: : y_texture_location_(-1) On 2013/04/04 22:08:44, enne wrote: > style nit: commas don't go at the beginning of lines. Done. https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.cc#newco... cc/output/shader.cc:931: { On 2013/04/04 22:08:44, enne wrote: > style nit: { goes on the previous line. How did this pass presubmit? Sorry, but i did not get any presubmit warnings. Not sure why. Done. https://codereview.chromium.org/12157002/diff/41001/cc/output/shader.cc#newco... cc/output/shader.cc:932: static const char* shader_uniforms[] = { On 2013/04/04 22:08:44, enne wrote: > style nit: don't use a 4 space indent Done. https://codereview.chromium.org/12157002/diff/53001/cc/quads/yuv_video_draw_q... File cc/quads/yuv_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/53001/cc/quads/yuv_video_draw_q... cc/quads/yuv_video_draw_quad.cc:27: bool needs_blending = true; On 2013/04/04 22:18:45, danakj wrote: > This is true only if there's an a_plane present, right? Right. Sorry, my bad. Done. https://codereview.chromium.org/12157002/diff/53001/cc/quads/yuv_video_draw_q... cc/quads/yuv_video_draw_quad.cc:57: if(a_plane.resource_id) On 2013/04/04 22:18:45, danakj wrote: > nit: space before ( Done.
Hmm, it does appear like presubmit is no longer linting files. I filed http://crbug.com/227196 for that. https://codereview.chromium.org/12157002/diff/60001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/12157002/diff/60001/cc/output/shader.cc#newco... cc/output/shader.cc:969: varying vec2 uv_texCoord; I'm still a little confused about how this varying parameter gets hooked up since there's only one varying parameter in the vertex shader. Am I missing something?
https://codereview.chromium.org/12157002/diff/60001/cc/quads/yuv_video_draw_q... File cc/quads/yuv_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/60001/cc/quads/yuv_video_draw_q... cc/quads/yuv_video_draw_quad.cc:27: bool needs_blending = !!a_plane; Actually, I think you can just leave this false still. Take a look at the opaque_rect given to this function. It's an empty rect if the layer doesn't have contents_opaque() set to true on it. The video system shouldn't be setting contents_opaque() true if the alpha plane isn't opaque, so you should get the right behaviour from the compositor if the video system is setting the opaque-ness on the video layer correctly. The needs_blending=true flag is really just there to override and turn on blending even though the contents are opaque.
Have added/modified unit tests. Will address enne and danakj's pending comments shortly.
I have addressed all standing comments. https://codereview.chromium.org/12157002/diff/60001/cc/output/shader.cc File cc/output/shader.cc (right): https://codereview.chromium.org/12157002/diff/60001/cc/output/shader.cc#newco... cc/output/shader.cc:969: varying vec2 uv_texCoord; On 2013/04/05 23:06:53, enne wrote: > I'm still a little confused about how this varying parameter gets hooked up > since there's only one varying parameter in the vertex shader. Am I missing > something? You are right, the shader emits only one varying parameter. Have changed it. https://codereview.chromium.org/12157002/diff/60001/cc/quads/yuv_video_draw_q... File cc/quads/yuv_video_draw_quad.cc (right): https://codereview.chromium.org/12157002/diff/60001/cc/quads/yuv_video_draw_q... cc/quads/yuv_video_draw_quad.cc:27: bool needs_blending = !!a_plane; On 2013/04/07 19:08:46, danakj wrote: > Actually, I think you can just leave this false still. > > Take a look at the opaque_rect given to this function. It's an empty rect if the > layer doesn't have contents_opaque() set to true on it. The video system > shouldn't be setting contents_opaque() true if the alpha plane isn't opaque, so > you should get the right behaviour from the compositor if the video system is > setting the opaque-ness on the video layer correctly. > > The needs_blending=true flag is really just there to override and turn on > blending even though the contents are opaque. Done.
I assume there's going to be some other patch that follows this that will actually have the VideoLayerImpl generate these quads? Since this code seems unreachable, is there any way to test that this shader and the renderer changes behave correctly, such as via gl_renderer_pixeltest?
On 2013/04/09 21:22:20, enne wrote: > I assume there's going to be some other patch that follows this that will > actually have the VideoLayerImpl generate these quads? Yes, this CL: https://codereview.chromium.org/12263013/ Actually, i was meaning to ask, could you please review the cc/ file in that CL as well? > > Since this code seems unreachable, is there any way to test that this shader and > the renderer changes behave correctly, such as via gl_renderer_pixeltest? I am not sure what test exactly you mean here. Can you please help me out?
On 2013/04/09 21:28:15, vignesh wrote: > > Since this code seems unreachable, is there any way to test that this shader > and > > the renderer changes behave correctly, such as via gl_renderer_pixeltest? > > I am not sure what test exactly you mean here. Can you please help me out? For example, something like https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende...
On 2013/04/09 21:32:55, enne wrote: > On 2013/04/09 21:28:15, vignesh wrote: > > > Since this code seems unreachable, is there any way to test that this shader > > and > > > the renderer changes behave correctly, such as via gl_renderer_pixeltest? > > > > I am not sure what test exactly you mean here. Can you please help me out? > > For example, something like > https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende... I don't see an equivalent such test for existing YUV (without alpha). so is the test for YUVA really necessary ? also, the media files CL (https://codereview.chromium.org/12263013/) is going to test this part of code anyway (there has already been local tests with both CL's applied which tests the whole workflow).
https://codereview.chromium.org/12157002/diff/70001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/70001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:82: // These values are magic numbers that are used in the transformation from YUV Seems like these can stay local to the DrawYUVQuad function now since they are only used there? https://codereview.chromium.org/12157002/diff/70001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1252: if (use_alpha_plane) { If (use_alpha_plane) is true, you are using the YUV program instead of the YUVA program. I think you might want to test this to make sure it's working as expected. https://codereview.chromium.org/12157002/diff/70001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1287: GLC(Context(), Context()->uniform1i(a_texture_location, 4)); nit: no {} for 1 line body
On 2013/04/11 21:03:59, vignesh wrote: > On 2013/04/09 21:32:55, enne wrote: > > On 2013/04/09 21:28:15, vignesh wrote: > > > > Since this code seems unreachable, is there any way to test that this > shader > > > and > > > > the renderer changes behave correctly, such as via gl_renderer_pixeltest? > > > > > > I am not sure what test exactly you mean here. Can you please help me out? > > > > For example, something like > > > https://code.google.com/p/chromium/codesearch#chromium/src/cc/output/gl_rende... > > I don't see an equivalent such test for existing YUV (without alpha). so is the > test for YUVA really necessary ? also, the media files CL > (https://codereview.chromium.org/12263013/) is going to test this part of code > anyway (there has already been local tests with both CL's applied which tests > the whole workflow). Composited layout tests in Blink arguably test YUV videos on an ongoing basis, although it'd be nice to have a test for the video quad inside of cc. There are no such tests for YUVA video quads and your new shaders, so we could break this path and nobody would ever know. What part of your media files CL tests the shaders?
Sorry about the delay in this. I am completely new to OpenGL and shader stuff, so it took some time to understand things and write these tests. PTAL now. Thanks. https://codereview.chromium.org/12157002/diff/70001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/70001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:82: // These values are magic numbers that are used in the transformation from YUV On 2013/04/11 21:10:00, danakj wrote: > Seems like these can stay local to the DrawYUVQuad function now since they are > only used there? Done. https://codereview.chromium.org/12157002/diff/70001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1252: if (use_alpha_plane) { On 2013/04/11 21:10:00, danakj wrote: > If (use_alpha_plane) is true, you are using the YUV program instead of the YUVA > program. > > I think you might want to test this to make sure it's working as expected. Done. https://codereview.chromium.org/12157002/diff/70001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1287: GLC(Context(), Context()->uniform1i(a_texture_location, 4)); On 2013/04/11 21:10:00, danakj wrote: > nit: no {} for 1 line body Done.
have put up a CL which has the test file for YUVA: https://codereview.chromium.org/15853015/
Thanks for the pixel test! This is looking great. A few nits: https://codereview.chromium.org/12157002/diff/77001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/77001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1402: scoped_ptr<ResourceProvider::ScopedSamplerGL> a_plane_lock; I suspect you might need to move this scoped_ptr outside of the if statement. https://codereview.chromium.org/12157002/diff/77001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2777: scoped_ptr<VideoYUVAProgram> &program = style nit: "& " not " &". https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... cc/output/renderer_pixeltest.cc:221: cc::PixelTest::resource_provider_->CreateResource( cc::PixelTest::resource_provider_ => this->resource_provider_? https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... cc/output/renderer_pixeltest.cc:238: uint8_t* y_plane = new uint8_t[w * h]; Please use a scoped_ptr for any memory ownership: scoped_ptr<uint8_t[]> y_plane(new uint8_t[w * h]); https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... cc/output/renderer_pixeltest.cc:239: uint8_t* u_plane = new uint8_t[(w / 2) * (h / 2)]; Can you put this repeated expression in a temporary constant, like y_plane_size and uv_plane_size? https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... cc/output/renderer_pixeltest.cc:254: yuv_quad->SetNew(shared_state.get(), rect, opaque_rect, gfx::Size(0, 0), style nit: gfx::Size(), here and elsewhere https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... cc/output/renderer_pixeltest.cc:282: ResourceProvider::ResourceId y_resource = There's a lot of repeated code between these two tests. What about a helper VideoGLRendererPixelTest class that could do a lot of this repeated setup in one place?
Have addressed all the comments. https://codereview.chromium.org/12157002/diff/77001/cc/output/gl_renderer.cc File cc/output/gl_renderer.cc (right): https://codereview.chromium.org/12157002/diff/77001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:1402: scoped_ptr<ResourceProvider::ScopedSamplerGL> a_plane_lock; On 2013/05/30 23:52:54, enne wrote: > I suspect you might need to move this scoped_ptr outside of the if statement. Done. https://codereview.chromium.org/12157002/diff/77001/cc/output/gl_renderer.cc#... cc/output/gl_renderer.cc:2777: scoped_ptr<VideoYUVAProgram> &program = On 2013/05/30 23:52:54, enne wrote: > style nit: "& " not " &". Done. https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... File cc/output/renderer_pixeltest.cc (right): https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... cc/output/renderer_pixeltest.cc:221: cc::PixelTest::resource_provider_->CreateResource( On 2013/05/30 23:52:54, enne wrote: > cc::PixelTest::resource_provider_ => this->resource_provider_? Done. https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... cc/output/renderer_pixeltest.cc:238: uint8_t* y_plane = new uint8_t[w * h]; On 2013/05/30 23:52:54, enne wrote: > Please use a scoped_ptr for any memory ownership: > > scoped_ptr<uint8_t[]> y_plane(new uint8_t[w * h]); Done. https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... cc/output/renderer_pixeltest.cc:239: uint8_t* u_plane = new uint8_t[(w / 2) * (h / 2)]; On 2013/05/30 23:52:54, enne wrote: > Can you put this repeated expression in a temporary constant, like y_plane_size > and uv_plane_size? Done. https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... cc/output/renderer_pixeltest.cc:254: yuv_quad->SetNew(shared_state.get(), rect, opaque_rect, gfx::Size(0, 0), On 2013/05/30 23:52:54, enne wrote: > style nit: gfx::Size(), here and elsewhere Done. https://codereview.chromium.org/12157002/diff/77001/cc/output/renderer_pixelt... cc/output/renderer_pixeltest.cc:282: ResourceProvider::ResourceId y_resource = On 2013/05/30 23:52:54, enne wrote: > There's a lot of repeated code between these two tests. What about a helper > VideoGLRendererPixelTest class that could do a lot of this repeated setup in one > place? Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/12157002/85001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
jschuh@ could you please take a look at into content/common/* stuff in this CL. Thanks!
jschuh/cevans, this is just a one line change and is probably quick-LGTM'able. Please take a look when you find time. Thanks.
doesn't seem to change any attack surface, so lgtm for ipc security.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/12157002/85001
cevans@/jln@, could you please review content/common/cc_messages_unittest.cc ? it's a straightforward change and should be quick LGTM'able. Thanks!
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
Sorry, you need a content/ owner for content/common/cc_messages_unittest.cc (cevans and myself are only there for sandbox changes).
+piman for cc_messages_unittest.cc On Tue, Jun 4, 2013 at 5:21 PM, <jln@chromium.org> wrote: > Sorry, you need a content/ owner for content/common/cc_messages_** > unittest.cc > (cevans and myself are only there for sandbox changes). > > https://codereview.chromium.**org/12157002/<https://codereview.chromium.org/1... >
piman/brettw, could you please review content/common/cc_messages_unittest.cc ? it's a fairly small change and should be quick LGTM'able.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/12157002/85001
Sorry for I got bad news for ya. Compile failed with a clobber build on linux_rel. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&... Your code is likely broken or HEAD is junk. Please ensure your code is not broken then alert the build sheriffs. Look at the try server FAQ for more details.
- Fixing a compilation warning. - Fixing deletegating_renderer_unittest.cc to accomodate the change made in AppendOneOfEveryQuadType() function.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/12157002/119001
Retried try job too often on win_rel for step(s) sync_integration_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/vigneshv@chromium.org/12157002/119001
https://chromiumcodereview.appspot.com/12157002/diff/119001/cc/output/delegat... File cc/output/delegating_renderer_unittest.cc (right): https://chromiumcodereview.appspot.com/12157002/diff/119001/cc/output/delegat... cc/output/delegating_renderer_unittest.cc:130: // Each render pass has 9 resources in it. And the root render pass has a Mind updating the comment as well?
Message was sent while issue was closed.
Change committed as 204508 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
